Should we accept # comment marks on fixes and parse them out?

[Daniel Vetter]

On Wed, Sep 30, 2020 at 11:01 PM Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>
> On Wed, Sep 30, 2020 at 12:28:21PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 29, 2020 at 07:37:13PM +0000, Vivi, Rodrigo wrote:
> > > Apparently #x86-32 as comment is breaking dim on the fixes flow.
> > >
> > >
> > > $ tdim push drm-intel-next-fixes
> > > dim: d6ec212e4a0d ("drm/i915/gem: Avoid implicit vmap for highmem on x86-32"): Subject in fixes line doesn't match referenced commit:
> > > dim:     fb8621d3bee8 ("drm/i915: Avoid allocating a vmap arena for a single page") #x86-32
> > > dim: ERROR: issues in commits detected, aborting
> > >
> > > Should we parse the comment out?
> > >
> > > if yes:
> > >
> > > a simple
> > >
> > >  fixes_subject=$(echo "${BASH_REMATCH[2]}" | cut -d# -f1
> > >
> > > didn't work out with current comparison flow..
> > > any suggestion for a clean parse and comparison?
> >
> > Adjusting the regex to ignore an optional # suffix at the end should work
> > better and more cleanly, something like the below. Totally untested.
> >
> > diff --git a/dim b/dim
> > index c3a048db8956..a379bcbc897b 100755
> > --- a/dim
> > +++ b/dim
> > @@ -824,7 +824,8 @@ function checkpatch_fixes_tag
> >               [[ "$fline" =~ ^[[:space:]]*[Ff][Ii][Xx][Ee][Ss]:[[:space:]]*(.*)$ ]]
> >               fline="${BASH_REMATCH[1]}"
> >
> > -             if [[ ! "$fline" =~ ^[[:space:]]*([[:xdigit:]]{5,})[[:space:]]*(.*)$ ]]; then
> > +             if [[ ! "$fline" =~
> > +                     ^[[:space:]]*([[:xdigit:]]{5,})[[:space:]]*(.*)([[:space:]]*#.*)?$ ]]; then
>
> not sure... this is just telling that we might match or not the spaces and #...
> so it pass them all anyway without actually stripping....

We pick the submatches, and my change should exclude the # from the
2nd submatch. No need to have another regex afterwards to strip that
out again, we can just match the right stuff from the start. If you
want to strip out the optional end, there's bash built-in string match
functions for that (search for %%. %. #, and ## in bash docs).
-Daniel

> I believe that for strip we should have something like (#|$) and include the
> possible spaces, right?! But I couldn't get any good version working here....
>
> for now the local version I have to bypass this is this:
>
> +               if [[ "$fixes_subject" =~ ^(.*)([[:space:]])\# ]]; then
> +                       fixes_subject="${BASH_REMATCH[1]}"
> +               fi
> +
>
>
> once I face more cases I will try to refine this and try
> again to include on the main parse
>
> >                       echoerr "$cite: Malformed fixes line:"
> >                       echoerr "    $fline"
> >                       rv=1
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dim-tools mailing list
> dim-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch