From: SZEDER Gábor <szeder@ira.uka.de>
Date: Mon, 02 Mar 2015 01:00:09 GMT
Subject: Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output
Message-ID: <20150302020009.Horde.9sATpKnsrWQkGadaRTvxkA3@webmail.informatik.kit.edu>
In-Reply-To: <20150301155818.GA5307@laptop>


Hi,

Quoting Mårten Kongstad <marten.kongstad@gmail.com>:

> On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:
>> On 2015-03-01 08.39, Mårten Kongstad wrote:
>> []
>>> +test_expect_success '--shortstat --dirstat should output only
>> one dirstat' '
>>> +	git diff --shortstat --dirstat=changes HEAD^..HEAD
>>> actual_diff_shortstat_dirstat_changes &&
>>> +	test $(grep -c " dst/copy/changed/$"
>> actual_diff_shortstat_dirstat_changes) = 1 &&
>> How portable is the "grep -c" usage ?
>> (I don't now it either, do we have other opinions ?), but the
>> following seems to be more "Git-style":
>>
>> test_expect_success '--shortstat --dirstat should output only one dirstat' '
>> 	git diff --shortstat --dirstat=changes HEAD^..HEAD
>>> actual_diff_shortstat_dirstat_changes &&
>> 	grep " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes >actual &&
>> 	test_line_count = 1 actual
>>


> Granted I didn't miss anything while trawling the tests for the above
> numbers, it feels like the 'grep -c' option is more in line with the
> existing tests. That said, I don't know if there is an ongoing trend to
> deprecate 'grep -c' in favour of 'test_line_count'.

It's not just 'grep -c' but the 'test' checking its output as well.

If something goes wrong and the line count doesn't match expectations
'test' fails silently leaving the developer clueless as to what went
wrong.

'test_line_count', on the other hand, produces useful output in case
of a failure:

    $ printf 'foo\nbar\n' >actual
    $ test_line_count = 1 actual
    test_line_count: line count for actual != 1
    foo
    bar

Since the name of the file in question is included in the output and
since there are three separate checks in this test, I would also
suggest writing 'grep's output into separate files
'actual_{changes,lines,files}'.

Gábor