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

From: Elijah Newren <newren@gmail.com>
Date: Jan 4, 2018, 21:10 UTC
Message-ID: <CABPp-BH66p1MU0Ry0LRcSpr7yGZymqwrnBmMh239xO3rgShdcw@mail.gmail.com>
In-Reply-To: <20180104005223.13566-1-szeder.dev@gmail.com>

On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Show 8 quoted lines

>> +             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?

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

Show 11 quoted lines

> I noticed that this patch series adds several similar
>
>   test $(git hash-object this) = $(git rev-parse that)
>
> conditions.  Well, for that we don't have a test helper
> function.  Similar 'hash-object = rev-parse' comparisons are already
> present in two other test scripts, so perhaps it's worth adding a
> helper function.  Or you could perhaps
>
>   git cat-file -p that >out &&
>   test_cmp this out

Yeah, I switched these over to the same format above; e.g.
   git hash-object this >actual &&
   git rev-parse that > expect &&
   test_cmp expect actual

That does have the advantage of re-using a similar style.

Show 8 quoted lines

>> +             test "very" = "$(cat y/c)" &&
>> +
>> +             test "important" = "$(cat y/d)" &&
>
> The 'verbose' helper could make conditions like these more, well,
> verbose about their failure:
>
>   verbose test "very" = "$(cat y/c)" &&

Also good to know. Is there any reason the "verbose" helper isn't documented in t/README?

I also switched these over yesterday to write to files and then use test_cmp, but

>> +             test "important" != "$(git rev-parse :3:y/d)" &&
>
> I'm not sure what this condition is supposed to check.

Well that's embarrassing. That was supposed to be 'cat-file -p', not rev-parse. And since I'm already testing that y/d on disk does have "important" as its contents and that :3:y/d matches O:z/c (i.e. has contents of "c"), the check was rather unnecessary, and too easy to false pass. I think I should just remove it (and two others like it).