On Mon, 2018-12-17 at 18:36 +0000, Emil Velikov wrote:
> On Mon, 17 Dec 2018 at 18:14, Andres Gomez <ago...@igalia.com> wrote:
> >
> > 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.
> >
>
> Wasn't too happy about it either. As you said - I've decided to go
> with the simpler solution.
>
> > > # 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?
> >
>
> Commits that include "fixes $some_test" will result in fixes="".
> Thus wc -l will return 1
Argh!
Well, this is more a consequence of wc counting newline characters and
echo adding a new one at the end, even when not printing anything.
Let's grep again, then ☹
> > > 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.
> >
>
> Left-over from the old standalone script. Copied a bit too much :-(
>
> > > # 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 (!)
> >
>
> As-is one gets false-positives, for commits like 9a7b3199037 mentioned above.
> As outlined, is_sha_nomination does two separate things. Thus even
> though 9a7b3199037 contains a valid offending sha, we "ignore" that
> and end up flagging the commit.
>
> To better illustrate what that here is an before/after example for
> mesa-18.2.1 (+ cherry-pick
> 559c32d2412b2ea602bb59aa61da75403d01a872~1..adbdfc6666052d604a97009d736b6dee957908a0)
>
> --- old 2018-12-17 18:31:06.023967483 +0000
> +++ new 2018-12-17 18:30:37.963966341 +0000
> @@ -9,14 +9,14 @@
> [ fixes ] 01fa76b7072 nvc0: Update counter reading shaders to new
> NVC0_CB_AUX_MP_INFO
> [ fixes ] c95dd966c43 docs: Update FAQ with respect to s3tc support
> [ fixes ] b473fcc9a39 nvc0: fix bindless multisampled images on Maxwell+
> -[ stable ] 3e7b5e5db2f radeon/uvd: use bitstream coded number for
> symbols of Huffman tables
> +[ fixes ] 3e7b5e5db2f radeon/uvd: use bitstream coded number for
> symbols of Huffman tables
> [ fixes ] bde3102c0dc vulkan/wsi/display: check if
> wsi_swapchain_init() succeeded
> -[ stable ] ec1fcf92ae7 radv: only emit ZPASS_DONE for timestamp
> queries on gfx queues
> +[ fixes ] ec1fcf92ae7 radv: only emit ZPASS_DONE for timestamp
> queries on gfx queues
> [ stable ] 7ee5e5e239a st/nine: Clamp RCP when 0*inf!=0
> [ stable ] dcfde02bb0f st/nine: Avoid redundant SetCursorPos calls
> [ stable ] 7ae2509ce06 st/nine: Increase maximum number of temp registers
> [ stable ] 0d495bec25b radeonsi: NaN should pass kill_if
> -[ stable ] dd333c66bdc vulkan: Disable randr lease for libxcb < 1.13
> +[ fixes ] dd333c66bdc vulkan: Disable randr lease for libxcb < 1.13
> [ fixes ] 7e7959fcb76 intel/fs: Fix a typo in need_matching_subreg_offset
> [ stable ] bfc89c668e2 nir/cf: Remove phi sources if needed in
> nir_handle_add_jump
> [ fixes ] 00f385e6d45 nir/from_ssa: Don't rewrite derefs
> destinations to registers
> @@ -44,7 +44,7 @@
> [ stable ] d179312b53d radv: add a workaround for a VGT hang with
> prim restart and strips
> [ stable ] d76c2774219 st/va: use provided sizes and coords for
> vlVaGetImage
> [ fixes ] cc33621e3b8 r600/sb: Fix constant-logical-operand warning.
> -[ stable ] ca83d51cfb1 ac/nir: Use context-specific LLVM types
> +[ fixes ] ca83d51cfb1 ac/nir: Use context-specific LLVM types
> [ fixes ] c20ba1be184 loader/dri3: Also wait for front buffer
> fence if we triggered it
> [ stable ] e71a87775e4 radv: fix check for perftest options size
> [ stable ] 06bf56725db radeonsi: Bump number of allowed global buffers to
> 32
> @@ -61,7 +61,6 @@
> [ fixes ] 5bcf479524b intel/blorp: Define the clear value bounds
> for HiZ clears
> [ revert ] d6be0b5556c scons/svga: remove opt from the list of
> valid build types
> [ stable ] b6b2b27809b blorp: Emit a dummy 3DSTATE_WM prior to
> 3DSTATE_WM_HZ_OP
> -[ stable ] aa02d7e8781 Revert "anv/skylake: disable
> ForceThreadDispatchEnable"
> [ fixes ] a61952e7374 freedreno: don't flush when new and old pfb
> is identical
> [ fixes ] 98e7c3e7a72 svga: add missing meson build dependency
> [ fixes ] 1df0c1e8fbd clover: add missing meson build dependency
> @@ -75,15 +74,14 @@
> [ stable ] ae8e81b0e30 intel/tools: include stdarg.h in error2aub
> [ fixes ] 7652931d33b meson: link gallium nine with pthreads
> [ fixes ] 64a9ed8848e r600/sb: Fix constant logical operand in assert.
> -[ stable ] b1b2dd06a7b radv: add missing TFB queries support to
> CmdCopyQueryPoolsResults()
> [ fixes ] d515ded4d95 wsi/wayland: only finish() a successfully
> init()ed display
> [ fixes ] 04298a2f244 st/va: fix incorrect use of resource_destroy
> [ fixes ] b3ade653879 egl/glvnd: correctly report errors when
> vendor cannot be found
> -[ stable ] 55af17ffed2 wayland/egl: Resize EGL surface on update
> buffer for swrast
> +[ fixes ] 55af17ffed2 wayland/egl: Resize EGL surface on update
> buffer for swrast
> [ fixes ] 421fa01d64d anv/android: mark gralloc allocated BOs as external
> [ stable ] 0a0aa2ba6c3 radv: disable conditional rendering for
> vkCmdCopyQueryPoolResults()
> [ stable ] 0dcd99c6870 radv: only expose
> VK_SUBGROUP_FEATURE_ARITHMETIC_BIT for VI+
> -[ stable ] 10598c9667a st/nine: fix stack corruption due to ABI mismatch
> +[ fixes ] 10598c9667a st/nine: fix stack corruption due to ABI mismatch
> [ fixes ] 9dd737bb029 nir: add glsl_type_is_integer() helper
> [ fixes ] a068958692c nir: don't pack varyings ints with floats unless
> flat
> [ fixes ] 9c2a95b2986 meson: Don't set -Wall
> @@ -115,7 +113,7 @@
> [ fixes ] cc0bc76a382 vc4: Make sure we make ro scanout resources
> for create_with_modifiers.
> [ fixes ] 1c56d211563 egl/dri: fix error value with unknown drm format
> [ stable ] 84445a86d19 travis: drop unneeded x11proto-xf86vidmode-dev
> -[ stable ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> +[ fixes ] 5bc509363b6 glx: make xf86vidmode mandatory for direct rendering
> [ fixes ] 982e012b3ac travis: adding missing x11-xcb for meson+vulkan
> [ fixes ] b787dcf57b7 i965/batch: avoid reverting batch buffer if
> saved state is an empty
> [ stable ] ea9f95e2a67 radeonsi: go back to using bottom-of-pipe
> for beginning of TIME_ELAPSED
> @@ -142,13 +140,11 @@
> [ stable ] 4f74580d303 st/xvmc: Add X11 include path.
> [ fixes ] fc0139d2833 nv50,nvc0: Fix gallium nine regression
> regarding sampler bindings
> [ fixes ] 9401a2f2e64 amd/vulkan: meson build - use radv_deps for
> libvulkan_radeon
> -[ stable ] 017199d2d2e mesa: Revert INTEL_fragment_shader_ordering support
> [ fixes ] 51091b3e1f2 radv/android: Mark android WSI image as shareable.
> [ fixes ] 3bf48741e12 radv/android: Use buffer metadata to
> determine scanout compat.
> [ fixes ] 1363a47c9c4 radv: use 3d shader for gfx9 copies if dst is 3d
> [ fixes ] 824cfc1ee5e radv: rework the TC-compat HTILE hardware
> bug with COND_EXEC
> [ fixes ] c1b6cb068c4 radv: Flush before vkCmdWriteTimestamp() if needed
> -[ stable ] 9a7b3199037 anv/query: flush render target before copying
> results
> [ stable ] c0ac038c97b gallium: Constify drisw_loader_funcs struct
> [ fixes ] 63c0916ada7 drisw: Use separate drisw_loader_funcs for shm
> [ fixes ] 3bd73d31a86 v3d: Fix a leak of the transfer helper on
> screen destroy.
>
> As you can see some bogus stable nominations are removed, et al. That
> is not possible without the is_sha_nomination rework.
I see that, but it is a consequence of the reordering of the checks,
not of the rework ... or maybe I'm too blind.
> > 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 (?)
> >
>
> In about ~90% of the case, there's a single fixes tag. So I wouldn't
> worry about that.
That's true.
Anyway, I'm fine with it. This series is:
Acked-by: Andres Gomez <ago...@igalia.com>
--
Br,
Andres
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
