Re: [PATCHv3 0/5] Simple fixes to t7406

[Elijah Newren <newren@gmail.com>]

Hi,

On Mon, Aug 13, 2018 at 1:28 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> 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"?)

Hmm, borderline.

> - 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'.

Ooh, that does sound better.

> >     - 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 ;)

I read this on Monday and got a really good laugh.  I meant to fix it
up, but fell asleep too soon the first couple nights...and now this
series is in next anyway and there are a couple other git things that
have my attention.  You have pointed out a couple additional nice
fixups, like you always do, but I think at this point I'm just going
to declare you the winner and label these as #leftoverbits.

Thanks for always thoroughly looking over the testcase patches and
your constant work to improve the testsuite.