[Mesa-dev] [PATCH 2/2] bin/get-fixes-pick-list.sh: better identify multiple "fixes:" tags

[Andres Gomez]

On Mon, 2017-05-15 at 11:56 +0100, Emil Velikov wrote:
> On 13 May 2017 at 01:11, Andres Gomez <agomez at igalia.com> wrote:
> > We were not considering as multiple fixes lines with:
> > Fixes: $sha_1, Fixes: $sha_2
> > 
> > Now, we split the lines so we will consider them individually, as in:
> > Fixes: $sha_1,
> > Fixes: $sha_2
> > 
> > Additionally, we try to get the SHA from split lines so:
> > Fixes:
> > $sha_1
> > 
> > Will be considered as:
> > Fixes: $sha_1
> > 
> > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > ---
> >  bin/get-fixes-pick-list.sh | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/bin/get-fixes-pick-list.sh b/bin/get-fixes-pick-list.sh
> > index f9afcc49ce..32d830cda0 100755
> > --- a/bin/get-fixes-pick-list.sh
> > +++ b/bin/get-fixes-pick-list.sh
> > @@ -36,14 +36,18 @@ do
> >                 continue
> >         fi
> > 
> > +       # Place every "fixes:" tag on its own line and join with the next word
> > +       # on its line or a later one.
> > +       fixes=`git show -s $sha | tr -d "\n" | sed -e 's/fixes:/\nfixes:/Ig' | grep "fixes:" | sed -e 's/\(fixes:\)[[:space:]]*\([a-zA-Z0-9]*\).*$/\1\2/g'`
> 
> Do we need the tr -d?

If what we are intending is that each new line will start with the
"fixes:" tag then, yes, I think it is needed.

I tried to see how to do this with sed and found it much more
cumbersome and felt using "tr -d" much easier to do and understand.

> Nit: Let's also handle any trailing whitespace after the tag.
> Something like the following should do it.
> 
> 's/fixes:[[:space:]]*/\nfixes:/Ig'

This is already done by the second sed. I'll move it to the first one
but I don't think we are really gaining anything ...

> >         # For each one try to extract the tag
> > -       fixes_count=`git show -s $sha | grep -i "fixes:" | wc -l`
> > +       fixes_count=`echo "$fixes" | wc -l`
> >         warn=`(test $fixes_count -gt 1 && echo $fixes_count) || echo 0`
> >         while [ $fixes_count -gt 0 ] ; do
> > -               fixes=`git show -s $sha | grep -i "fixes:" | tail -n $fixes_count`
> > +               # Treat only the current line
> > +               fix=`echo "$fixes" | tail -n $fixes_count | head -n 1`
> >                 fixes_count=$(($fixes_count-1))
> > -               # The following sed/cut combination is borrowed from GregKH
> > -               id=`echo ${fixes} | sed -e 's/^[ \t]*//' | cut -f 2 -d ':' | sed -e 's/^[ \t]*//' | cut -f 1 -d ' '`
> > +               id=`echo "$fix" | cut -d : -f 2`
> 
> Nit: fold the fix line in here?

I'll do.

> With the nitpicks the series is
> Reviewed-by: Emil Velikov <emli.velikov at collabora.com>
> 
> -Emil
> 
> P.S. An alternative construct that just hit me. It allows us to drop
> the "tail | head" dance, drop the count variable and make the whole
> thing a bit easier to read.
> I'm quite biased of course, but if you agree I can address it as a
> follow-up patch.
> 
>     # try to fold ill-formed fixes tags
>     fixes=`git show -s $sha ...
>     count=... // obsolete? see below
>     warn=0
>     save_IFS...
>     IFS=\n
>     for i in fixes; do
>         $process
>         if failed; then
>             # warn if we fail to parse any of the tags
>             warn=1
>             continue
>         fi
>     done
>     restore_IFS
> 
>     # only if there's multiple?
>     # with the sed pattern fixed, we well drop the multiple 'requirement'
>     if warn /* && count -gt 1 */; then
>         echo WARNING
>     fi

It looks good and, yes, maybe easier to understand, so feel free to go
ahead ☺

I'll push this by now ...

-- 
Br,

Andres