On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@xxxxxxxxx> wrote:
> Since folks like to notice other problems with t7406 while reading my
> patches, here's a challenge:
>
> Find something *else* wrong with t7406 that neither I nor any of the
> reviewers so far have caught that could be fixed.
Well, I'd hate to be that guy... but since those who already
commented on previous rounds are not explicitly excluded from the
challenge, let's see.
- There are still a few command substitutions running git commands,
where the exit status of that command is ignored; just look for the
'[^=]$(' pattern in the test script.
(Is not noticing those cases considered as "flubbing"?)
- The 'compare_head' helper function defined in this test script looks
very similar to the generally available 'test_cmp_rev' function,
which has the benefit to provide some visible output on failure
(though, IMO, not a particularly useful output, because the diff of
two OIDs is not very informative, but at least it's something as
opposed to the silence of 'test $this = $that").
Now, since 'compare_head' always compares the same two revisions,
namely 'master' and HEAD, replacing 'compare_head' with an
appropriate 'test_cmp_rev' call would result in repeating 'master'
and 'HEAD' arguments all over the test script. I'm not sure whether
that's good or bad. Anyway, I think that 'compare_head' could be
turned into a wrapper around 'test_cmp_rev'.
> - You get bonus points if that thing is in the context region for
> one of my five patches.
> - Extra bonus points if the thing needing fixing was on a line I
> changed.
> - You win outright if it's something big enough that I give up and
> request to just have my series merged as-is and punt your
> suggested fixes down the road to someone else.
Well, there's always the indentation of the commands run in subshells,
which doesn't conform to our coding style...
Gah, now you made me that guy ;)
> (Note: If I flubbed something in v3, pointing it out doesn't count
> as finding "something else", but do please still point it out.)
>
> :-)
