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 <ago...@igalia.com>
> CC: Juan A. Suarez <jasua...@igalia.com>
> CC: Dylan Baker <dy...@pnwbakers.com>
> CC: mesa-sta...@lists.freedesktop.org
> Signed-off-by: Emil Velikov <emil.veli...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
