On Mon, 2017-05-15 at 11:56 +0100, Emil Velikov wrote: > On 13 May 2017 at 01:11, Andres Gomez <ago...@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 <ago...@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.veli...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev