On Tue, 2018-12-18 at 10:47 +0000, Emil Velikov wrote: > On Mon, 17 Dec 2018 at 22:35, Andres Gomez <ago...@igalia.com> wrote: > > > > 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. > > > > I agree looking at the diff alone, is quite odd. Let's take > 9a7b3199037 and consider only a reorder. > - in is_sha_nomination > - offending commit is extracted correctly > - offending commit hasn't landed in before the branchpoint > - .... or after the branchpoint (as cherry-pick) > - hence is_sha_nomination returns false > - fallback to mesa-stable
Ugh! Now I get it. 🤦 > - which leads to the false positive > > > > > 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> > > > > Thank you. > > To be on the safe side I'll double-check against 18.1 (currently I've > tried 18.2 and 18.3) You can scratch the Ack. This series is: Reviewed-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