Jonathan Nieder <[email protected]> writes:
> Junio C Hamano wrote:
>> On Mon, Oct 13, 2014 at 2:31 PM, Jonathan Nieder <[email protected]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
