[Mesa-dev] [PATCH 1/2] bin/get-pick-list.sh: rework handing of sha nominations

[Andres Gomez]

On Mon, 2018-12-17 at 16:43 +0000, Emil Velikov wrote:
> Currently our is_sha_nomination does:
>  - folds any whitespace, attempting to extract sha-like information
>  - checks that at least one of the shas has landed
> 
> Split it in two and do sha-like validation first.
> 
> This way, commits with mesa-stable and sha nominations will feature the
> fixes/revert/etc instead of stable (a) or will be omitted if not
> applicable for the respective branch (b).
> 
> Misc examples from 18.3
> 
> (a)
> -[   stable ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> +[    fixes ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> 
> (b)
> -[   stable ] 9a7b3199037 anv/query: flush render target before copying results
> 
> CC: Andres Gomez <agomez at igalia.com>
> CC: Juan A. Suarez <jasuarez at igalia.com>
> CC: Dylan Baker <dylan at pnwbakers.com>
> CC: mesa-stable at lists.freedesktop.org
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Juan I've noticed that you've been experiencing the above annoyance for
> a while. Having less false-positives should ease things up a bit :-)
> ---
>  bin/get-pick-list.sh | 46 +++++++++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/bin/get-pick-list.sh b/bin/get-pick-list.sh
> index 9f9cbc44026..08a783f35a8 100755
> --- a/bin/get-pick-list.sh
> +++ b/bin/get-pick-list.sh
> @@ -21,32 +21,36 @@ is_typod_nomination()
>  	git show --summary "$1" | grep -q -i -o "CC:.*mesa-dev"
>  }
>  
> +fixes=
> +

Splitting in 2 functions for which we need now a global variable is not
very nice. Anyway, let's not make this more complicated than needed.

>  # Helper to handle various mistypos of the fixes tag.
>  # The tag string itself is passed as argument and normalised within.
> +#
> +# Resulting string in the global variable "fixes" and contains entries
> +# in the form "fixes:$sha"
>  is_sha_nomination()
>  {
>  	fixes=`git show --pretty=medium -s $1 | tr -d "\n" | \
>  		sed -e 's/'"$2"'/\nfixes:/Ig' | \
>  		grep -Eo 'fixes:[a-f0-9]{8,40}'`
>  
> -	fixes_count=`echo "$fixes" | wc -l`
> +	fixes_count=`echo "$fixes" | grep "fixes:" | wc -l`

Why is the extra "grep" needed here?

>  	if test $fixes_count -eq 0; then
> -		return 0
> +		return 1

Ugh! Right.

>  	fi
> +	return 0
> +}
> +
> +# Checks if at least one of offending commits, listed in the global
> +# "fixes", is in branch.
> +sha_in_range()
> +{
> +	fixes_count=`echo "$fixes" | grep "fixes:" | wc -l`

Ditto.

>  	while test $fixes_count -gt 0; do
>  		# Treat only the current line
>  		id=`echo "$fixes" | tail -n $fixes_count | head -n 1 | cut -d : -f 2`
>  		fixes_count=$(($fixes_count-1))
>  
> -		# Bail out if we cannot find suitable id.
> -		# Any specific validation the $id is valid and not some junk, is
> -		# implied with the follow up code
> -		if test "x$id" = x; then
> -			continue
> -		fi
> -
> -		#Check if the offending commit is in branch.
> -

Was this never needed in the first place? Feels right to remove it
since $fixes should have some content by now, but I wonder how this got
here in the first place.

>  		# Be that cherry-picked ...
>  		# ... or landed before the branchpoint.
>  		if grep -q ^$id already_picked ||
> @@ -103,20 +107,30 @@ do
>  		continue
>  	fi
>  
> -	if is_stable_nomination "$sha"; then
> -		tag=stable
> -	elif is_typod_nomination "$sha"; then
> -		tag=typod
> -	elif is_fixes_nomination "$sha"; then
> +	if is_fixes_nomination "$sha"; then
>  		tag=fixes
>  	elif is_brokenby_nomination "$sha"; then
>  		tag=brokenby
>  	elif is_revert_nomination "$sha"; then
>  		tag=revert
> +	elif is_stable_nomination "$sha"; then
> +		tag=stable
> +	elif is_typod_nomination "$sha"; then
> +		tag=typod
>  	else
>  		continue
>  	fi
>  
> +	case "$tag" in
> +	fixes | brokenby | revert )
> +		if ! sha_in_range; then
> +			continue
> +		fi
> +		;;
> +	* )
> +		;;
> +	esac
> +
>  	printf "[ %8s ] " "$tag"
>  	git --no-pager show --summary --oneline $sha
>  done

I'm not sure we are gaining something with the splitting.

Maybe I'm not understanding correctly the patch but it seems to me that
every successful "is_sha_nomination" is followed by a "sha_in_range"
call. Hence, we are only trading splitting into 2 functions by having a
global variable (!)

Additionally, in the second patch of the series we are adding a warning
that, in case of having a single function, could be placed in the same
while loop, instead of having now 2 loops (?)

-- 
Br,

Andres