Re: [PATCH] diff: diff.context configuration gives default to -U

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

Jeff Muizelaar <jmuizelaar@mozilla.com> writes:

> +	if (!strcmp(var, "diff.context")) {
> +		diff_context_default = git_config_int(var, value);
> +		if (diff_context_default < 0)
> +			return -1;
> +		return 0;

I am somewhat torn on this part. This fails the entire command when
diff.context is set to non integer or negative integer, which means
trouble for a user of a future version of git that accepts such a
value to do something intelligent we do not anticipate today. The
useful configuration value cannot be given unless the user is
certain that .gitconfig file will never be read by older version of
git.

Perhaps it is OK, at least for now.  We'd have the same worry for
what is given to -U<n> anyway.

> +	}
>  	if (!strcmp(var, "diff.renames")) {
>  		diff_detect_rename_default = git_config_rename(var, value);
>  		return 0;
> @@ -3170,7 +3177,7 @@ void diff_setup(struct diff_options *options)
>  	options->break_opt = -1;
>  	options->rename_limit = -1;
>  	options->dirstat_permille = diff_dirstat_permille_default;
> -	options->context = 3;
> +	options->context = diff_context_default;
>  	DIFF_OPT_SET(options, RENAME_EMPTY);
>  
>  	options->change = diff_change;
> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> new file mode 100755
> index 0000000..8a31448
> --- /dev/null
> +++ b/t/t4055-diff-context.sh
> @@ -0,0 +1,128 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Mozilla Foundation
> +#
> +
> +test_description='diff.context configuration'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	cat >x <<-EOF &&
> +firstline
> +b
> +...
> +n
> +EOF
> +	git update-index --add x &&
> +	git commit -m initial &&
> +	cat >x <<-\EOF &&
> +firstline
> +b

The dash after here-document redirection (i.e. "<<-") allows you to
indent the meat of the here-document and the end-of-here-document
marker.  Please take advantage of the facility to make the result
more readable.

Also unless there is something that needs interpolation by the
shell, we prefer to use quoted here-document (i.e. "<<-\EOF") to
reduce the mental burden by the readers of the code.  So

	test_expect_success setup '
		cat >x <<-\EOF &&
		firstline
		b
		...
		n
		EOF
		use x in the test &&
		check the result
	'

This comment applies to all other uses of <<- in this patch.

> +test_expect_success 'diff.context affects log' '
> +	git log -1 -p >output &&
> +	! grep firstline output &&
> +	git config diff.context 8 &&
> +	git log -1 -p >output &&
> +	grep firstline output
> +'

Is there a reason to favor "log -1 -p" over something a lot simpler
like "show"?  Not requesting to change anything, but just being
curious.

> +test_expect_success 'different -U value' '
> +	git config diff.context 8 &&
> +	git log -U4 -1 >output &&
> +	! grep firstline output
> +'

OK, so -U4 overrides configured diff.context setting and you make
sure by asking -U4 that is too small to show firstline (but if 8
were used, the line would have been shown).

> +test_expect_success 'diff.context affects diff' '
> +	git config diff.context 8 &&
> +	git diff >output &&
> +	grep firstline output
> +'

Don't we want to make sure that without diff.context the output will
give you the default 3 lines of context?  It is a common mistake
shared by people who want to demonstrate their shiny new toys to
test only the positive cases while forgetting to test non-regression.

> +test_expect_success 'plumbing not affected' '
> +	git config diff.context 8 &&
> +	git diff-files -p > output &&

Style.  No SP between redirection and the filename, i.e.

	git diff-files -p >output &&

> +	! grep firstline output
> +'

A blank line between tests.

> +test_expect_success 'non-integer config parsing' '
> +	cat > .git/config <<-\EOF &&

> +[diff]
> +	context = no
> +EOF
> +	! git diff 2>output &&
> +	grep "bad config value" output
> +'

We are not in the business of debugging "grep", so writing "! grep ..."
to make sure grep does not find something is perfectly fine, but
when expecting an error from "git" command, please write it like

	test_must_fail git diff 2>error &&
        test_i18ngrep "bad config value" error

instead.  The error messages meant for human consumption could be
localized, and use of test_i18ngrep allows use to ignore its error
condition when the test is run under "funny" locale.

> +test_expect_success 'negative integer config parsing' '
> +	cat >.git/config <<-\EOF &&
> +[diff]
> +	context = -1
> +EOF
> +	! git diff 2>output &&
> +	grep "bad config file" output
> +'
> +
> +test_expect_success '0 config parsing' '
> +	cat > .git/config <<-\EOF &&
> +[diff]
> +	context = 0
> +EOF
> +	git diff >output &&
> +	grep preline output
> +'

Hrm, is this correct?  Your test vectors change a single line that
is surrounded by "preline" and "postline", so -U0 patch should show
only the change for that single line, without showing "preline" or
"postline".

I think the "grep" pattern you used is too loose to be useful.  You
would probably want to be grepping for something like

	git diff >output &&
        grep "^-1" output &&
        grep "^+2" output

instead.

> +test_expect_success 'config parsing' '
> +	cat >.git/config <<-\EOF &&
> +[diff]
> +	context = 8
> +EOF
> +	git diff >output &&
> +	grep postline output
> +'

I do not think this last test adds any value; you already checked it
with "git config diff.context 8" much earlier, no?  This script is
not about parsing of the handcrafted configuration files.

> +test_done

To summarize...

 t/t4055-diff-context.sh | 135 ++++++++++++++++++------------------------------
 1 file changed, 50 insertions(+), 85 deletions(-)

diff --git c/t/t4055-diff-context.sh w/t/t4055-diff-context.sh
index 8a31448..3527686 100755
--- c/t/t4055-diff-context.sh
+++ w/t/t4055-diff-context.sh
@@ -8,121 +8,86 @@ test_description='diff.context configuration'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	cat >x <<-EOF &&
-firstline
-b
-c
-d
-e
-f
-preline
-postline
-i
-j
-k
-l
-m
-n
-EOF
+	cat >x <<-\EOF &&
+	firstline
+	b
+	c
+	d
+	e
+	f
+	preline
+	postline
+	i
+	j
+	k
+	l
+	m
+	n
+	EOF
 	git update-index --add x &&
 	git commit -m initial &&
-	cat >x <<-\EOF &&
-firstline
-b
-c
-d
-e
-f
-preline
-1
-postline
-i
-j
-k
-l
-m
-n
-EOF
+
+	git cat-file blob HEAD:x |
+	sed "/preline/a\
+	ADDED" >x &&
 	git update-index --add x &&
 	git commit -m next &&
-cat >x <<-\EOF
-firstline
-b
-c
-d
-e
-f
-preline
-2
-postline
-i
-j
-k
-l
-m
-n
-EOF
+
+	git cat-file blob HEAD:x |
+	sed s/ADDED/MODIFIED/ >x
 '
 
-test_expect_success 'diff.context affects log' '
+test_expect_success 'the default number of context lines is 3' '
+	git diff >output &&
+	! grep "^ d" output &&
+	grep "^ e" output &&
+	grep "^ j" output &&
+	! grep "^ k" output
+'
+
+test_expect_success 'diff.context honored by "log"' '
 	git log -1 -p >output &&
 	! grep firstline output &&
 	git config diff.context 8 &&
 	git log -1 -p >output &&
-	grep firstline output
+	grep "^ firstline" output
 '
 
-test_expect_success 'different -U value' '
+test_expect_success 'The -U option overrides diff.context' '
 	git config diff.context 8 &&
 	git log -U4 -1 >output &&
-	! grep firstline output
+	! grep "^ firstline" output
 '
 
-test_expect_success 'diff.context affects diff' '
+test_expect_success 'diff.context honored by "diff"' '
 	git config diff.context 8 &&
 	git diff >output &&
-	grep firstline output
+	grep "^ firstline" output
 '
 
 test_expect_success 'plumbing not affected' '
 	git config diff.context 8 &&
-	git diff-files -p > output &&
-	! grep firstline output
+	git diff-files -p >output &&
+	! grep "^ firstline" output
 '
+
 test_expect_success 'non-integer config parsing' '
-	cat > .git/config <<-\EOF &&
-[diff]
-	context = no
-EOF
-	! git diff 2>output &&
-	grep "bad config value" output
+	git config diff.context no &&
+	test_must_fail git diff 2>output &&
+	test_i18ngrep "bad config value" output
 '
 
 test_expect_success 'negative integer config parsing' '
-	cat >.git/config <<-\EOF &&
-[diff]
-	context = -1
-EOF
-	! git diff 2>output &&
-	grep "bad config file" output
-'
-
-test_expect_success '0 config parsing' '
-	cat > .git/config <<-\EOF &&
-[diff]
-	context = 0
-EOF
-	git diff >output &&
-	grep preline output
+	git config diff.context -1 &&
+	test_must_fail git diff 2>output &&
+	test_i18ngrep "bad config file" output
 '
 
-test_expect_success 'config parsing' '
-	cat >.git/config <<-\EOF &&
-[diff]
-	context = 8
-EOF
+test_expect_success '-U0 is valid, so is diff.context=0' '
+	git config diff.context 0 &&
 	git diff >output &&
-	grep postline output
+	grep "^-ADDED" output &&
+	grep "^+MODIFIED" output
 '
 
 test_done