Re: [PATCH 2/3] t5304: use helper to report failure of "test foo = bar"

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> On Mon, Oct 13, 2014 at 2:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> It could segfault after producing the good output, but sure,
>>> count-objects code doesn't change very often.
>>
>> "Doesn't change very often" is not the issue. Here we are not testing
>> if it can count correctly without crashing, which *is* the real reason
>> why it is perfectly fine to use $(git count-objects | sed ...) pattern here.
>>
>> There of course should be a test for count-objects to make sure it
>> counts correctly without crashing.
>
> That also makes sense, but it is a pretty big change from the general
> strategy used in git tests today.

I would have to say that you are mistaken in reading what the
"strategy used today" is.  There is a subtle trade-off involved.

When we test, say, "git add a b", we may want to test these things:

    - "git add" when given addable paths a and b finishes without
      crashing;

    - "git add" will leave these paths in the index as expected.

And we write

	git add a b &&
	test_write_lines a b >expect &&
        git ls-files a b >actual &&
        test_cmp expect actual

NOT because we expect "printf" (which underlies test_write_lines) or
"git ls-files" could somehow misbehave and dump core, but primarily
because compared to an alternative, e.g.

	git add a b || return 1
        test_write_lines a b >expect
        git ls-files a b >actual
        test_cmp expect actual

it is far cleaner and easier to read with a rhythm.  It is just an
added bonus that we may catch errors due to filesystem quota when
writing to "expect" or ls-files crashing.  If the alternative had
enough advantage over the established pattern (and here is where the
trade off comes in---you need a certain taste to judge the
advantage), it is perfectly fine to trade the exit value off and
favor the advantage the alternative offers (e.g. a test that is
easier to read).

Between these two, it is very sensible to take A. over B.

    A.
	git create-garbage &&
	test $(git count-objects | sed ... | wc -l) = 0

    B.
	git create-garbage &&
	test_when_finished "rm -f tmp" &&
        git count-objects >tmp &&
        test $(sed ... tmp | wc -l) = 0

It will shift the trade-off if the more verbose alternative gets
wrapped into a helper that is well constructed, though, because
readability advantage of A over B melts away when we do so.