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
> > 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.
> 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.
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
