On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor <szeder....@gmail.com> wrote:
>> + 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...
> 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.
>> + 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).
