Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output

Hi,

Quoting Mårten Kongstad [off-list ref]:

On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:

quoted

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]

quoted

+test_expect_success '--shortstat --dirstat should output only

one dirstat' '

quoted

+	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

quoted

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