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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 13, 2018, 20:28 UTC
Message-ID: <CAM0VKjn6QAp-hQa3Qp07qZ2unNk20SXYoPwwFbpiLfqqx+KV+A@mail.gmail.com>
In-Reply-To: <20180807164905.3859-1-newren@gmail.com>

On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:

Show 5 quoted lines

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

Show 7 quoted lines

>     - 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.)
>
> :-)