Re: [PATCHv5 7/7] Improve error handling when parsing dirstat parameters

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

Johan Herland <johan@herland.net> writes:

> ...
> The patch also adds a couple of tests verifying the correct behavior of
> --dirstat and diff.dirstat in the face of unknown (possibly future) dirstat
> parameters.

Thanks.  Patches 1-6/7 looks much better.

When writing a shiny new feature, people tend to test only the cases they
expect to work, leaving the cases that should error out unspecified,
leading to future confusion.  Negative tests to specify and guard error
behaviour are very important, and I like this 7/7 very much.

Having said that, you might want to add tests for parsing --dirstat/-X
options themselves for the same reason.  I think you had troubles with -X3
(the first round), --dirstat40 (the third round), and possibly -X=3; they
could have been avoided if you had such tests.  They probably should be
added to 1/7.

> diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
> index 8ca1d58..20a59ac 100755
> --- a/t/t4047-diff-dirstat.sh
> +++ b/t/t4047-diff-dirstat.sh
> @@ -932,4 +932,34 @@ test_expect_success 'diff.dirstat=0,lines' '
>  	test_cmp expect_diff_dirstat_CC actual_diff_dirstat_CC
>  '
>  
> +test_expect_success '--dirstat=future_param,lines,0 should fail loudly' '
> +	test_must_fail git diff --dirstat=future_param,lines,0 HEAD^..HEAD >actual_diff_dirstat 2>actual_error &&
> +	test_cmp /dev/null actual_diff_dirstat &&
> +	grep -q "future_param" actual_error &&
> +	grep -q "\--dirstat" actual_error &&
> +	test_must_fail git diff --dirstat=future_param,lines,0 -M HEAD^..HEAD >actual_diff_dirstat_M 2>actual_error &&
> +	test_cmp /dev/null actual_diff_dirstat_M &&
> +	grep -q "future_param" actual_error &&
> +	grep -q "\--dirstat" actual_error &&
> +	test_must_fail git diff --dirstat=future_param,lines,0 -C -C HEAD^..HEAD >actual_diff_dirstat_CC 2>actual_error &&
> +	test_cmp /dev/null actual_diff_dirstat_CC &&
> +	grep -q "future_param" actual_error &&
> +	grep -q "\--dirstat" actual_error
> +'

I am not sure if three combinations (vanilla, -M and -C -C) need to be
tested to produce an empty result.  If so, it would make it easier to read
if they are split into three tests, or at least have a blank line between
them, but I suspect that you would agree that it is not worth to have
three separate test_expect_success for these.

I also wanted to see the error output.  How about adding:

	test_debug "cat actual_error" &&

immediately after invocation of "git diff"?

The error output shows "error:" followed by "warning:", which looked
somewhat questionable.  Perhaps allow a pointer to a structure be passed
in to describe the nature of a breakage to parse_dirstat_params()?

Telling "grep" that the pattern string is not an option by quoting the
first dash (i.e. "\--dirstat") is clever, and it is more portable than
using an explicit "-e" to accomodate ancient implementations of grep.

	Side note: we seem to already use "grep -e" in some other tests
	(2200, 2204 and 5540).  We probably should get rid of -e from
	these places.

> +test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still work' '
> +	git -c diff.dirstat=future_param,0,lines diff --dirstat HEAD^..HEAD >actual_diff_dirstat 2>actual_error &&
> +	test_cmp expect_diff_dirstat actual_diff_dirstat &&
> +	grep -q "future_param" actual_error &&
> +	grep -q "diff.dirstat" actual_error &&

This should avoid matching "." with anything, i.e.

	grep -q "diff\\.dirstat" actual_error &&

 t/t4047-diff-dirstat.sh |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index 20a59ac..0942bdb 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh
@@ -934,32 +934,23 @@ test_expect_success 'diff.dirstat=0,lines' '
 
 test_expect_success '--dirstat=future_param,lines,0 should fail loudly' '
 	test_must_fail git diff --dirstat=future_param,lines,0 HEAD^..HEAD >actual_diff_dirstat 2>actual_error &&
+	test_debug "cat actual_error" &&
 	test_cmp /dev/null actual_diff_dirstat &&
 	grep -q "future_param" actual_error &&
-	grep -q "\--dirstat" actual_error &&
-	test_must_fail git diff --dirstat=future_param,lines,0 -M HEAD^..HEAD >actual_diff_dirstat_M 2>actual_error &&
-	test_cmp /dev/null actual_diff_dirstat_M &&
-	grep -q "future_param" actual_error &&
-	grep -q "\--dirstat" actual_error &&
-	test_must_fail git diff --dirstat=future_param,lines,0 -C -C HEAD^..HEAD >actual_diff_dirstat_CC 2>actual_error &&
-	test_cmp /dev/null actual_diff_dirstat_CC &&
-	grep -q "future_param" actual_error &&
 	grep -q "\--dirstat" actual_error
 '
 
 test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still work' '
 	git -c diff.dirstat=future_param,0,lines diff --dirstat HEAD^..HEAD >actual_diff_dirstat 2>actual_error &&
+	test_debug "cat actual_error" &&
 	test_cmp expect_diff_dirstat actual_diff_dirstat &&
 	grep -q "future_param" actual_error &&
-	grep -q "diff.dirstat" actual_error &&
-	git -c diff.dirstat=future_param,0,lines diff --dirstat -M HEAD^..HEAD >actual_diff_dirstat_M 2>actual_error &&
-	test_cmp expect_diff_dirstat_M actual_diff_dirstat_M &&
-	grep -q "future_param" actual_error &&
-	grep -q "diff.dirstat" actual_error &&
-	git -c diff.dirstat=future_param,0,lines diff --dirstat -C -C HEAD^..HEAD >actual_diff_dirstat_CC 2>actual_error &&
-	test_cmp expect_diff_dirstat_CC actual_diff_dirstat_CC &&
-	grep -q "future_param" actual_error &&
-	grep -q "diff.dirstat" actual_error
+	grep -q "diff\\.dirstat" actual_error
+'
+
+test_expect_success 'various ways to misspell --dirstat' '
+	test_must_fail git show --dirstat10,files &&
+	test_must_fail git show -X=20
 '
 
 test_done