Re: [PATCH v5 13/34] directory rename detection: tests for handling overwriting untracked files

On Thu, Jan 4, 2018 at 10:10 PM, Elijah Newren [off-list ref] wrote:

On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor [off-list ref] wrote:

quoted

quoted

+             test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&

There is a test helper for that :)

  test_cmp_rev :0:y/b O:z/b

Note, that this is not only a matter of useful output on failure, but
also that of correctness and robustness.

Cool, good to know.  Is there any reason test_cmp_rev is not
documented in t/README?

Because of mere oversight, perhaps?  (both for this and for the
'verbose' helper)
I forgot that the test helpers are documented in 't/README' (well,
apparently only some of them), I usually go straight to
't/test-lib-functions.sh' to find test helpers or to learn what they are
doing.

I already changed these yesterday, as part of trying to avoid the use
of plain 'test' as you suggested (I was just waiting another day or so
for more feedback before resubmitting the series).  Since I tended to
have several of these rev-parse comparisons in a single test, I simply
combined them:
    git rev-parse >actual
      :0:y/b :1:x/c :2:x/c :3:x/c
    git rev-parse >expect
      O:z/b O:x/c A:x/c B:x/c
    test_cmp expect actual

That does result in fewer rev-parse invocations, which is probably
good, but the test_cmp_rev does seem slightly more readable.  Hmmm...

Yeah, the significantly more 'git rev-parse' invocations are the reason
why I didn't recommend 'test_cmd_rev' in those cases.  Folks running the
test suite on Windows probably won't be very happy about such a change.
Though I had a bit of a "Huh?!" moment when seeing those combined 'git
rev-parse' pairs for the first time, I don't think converting them to a
list of 'test_cmp_rev' invocations would make it notably easier to
read.
Have you considered vertically aligning the corresponding rev arguments
in those 'git rev-parse' pairs?

Furthermore, on second look, while 'test_cmp_rev' does produce output on
failure, it's not really useful for humans, being only a diff of two
files with a single object name in each.  As it is, I don't think it's
any more useful than the output of those combined 'git
rev-parse'-'test_cmp' combos would be, so no gain there, either.
OTOH, we could easily enhance 'test_cmp_rev' to include the given revs
in its error message...

I leave it up to you.