threads / patch / 31534

patchAdd diff.context option to specify default context

Subject: [PATCH] Add diff.context option to specify default context

## tl;dr

8 messages between Sep 14, 2012 and Oct 19, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jeff Muizelaar· Sep 14, 2012, 18:16 UTC · lore

This adds a diff.context config option to allow specifying the number of lines of context. This is similar to Mercurial's 'unified' option.

Junio C Hamano· Sep 14, 2012, 21:06 UTC · re: Jeff Muizelaar · lore

Re: [PATCH] Add diff.context option to specify default context

Jeff Muizelaar <jmuizelaar@mozilla.com> writes:

> This adds a diff.context config option to allow specifying
> the number of lines of context. This is similar to Mercurial's
> 'unified' option.

Random thoughts.

* Please refer to Documentation/SubmittingPatches.  Saving your
  message in a mbox and applying it would produce this crap:

    commit ba4c972eacb91058f1317dbcd4ff77b471fa938e
    Author: Jeff Muizelaar <jmuizelaar@mozilla.com>
    Date:   Fri Sep 14 14:16:03 2012 -0400

        Add diff.context option to specify default context

        This adds a diff.context config option to allow specifying
        the number of lines of context. This is similar to Mercurial's
        'unified' option.

        commit 1bd81c75de6824c39852bc8516acd0733737ed83
        Author: Jeff Muizelaar <jmuizelaar@mozilla.com>
        Date:   Fri Sep 14 13:55:02 2012 -0400

            [PATCH] Add diff.context option to specify default context

            This adds a diff.context config option to allow specifying
            the number of lines of context. This is similar to
            Mercurial's
            'unified' option.

  which is not acceptable.

* Sign-off your patch.

* Citing similaritly to options in other systems does not add much
  value for people who read the proposed log message.  In this case,
  I think the first sentence is written clearly enough that it is
  sufficient without such clarification.  If anything, it should
  instead say:

	diff: diff.context configuration gives default to -U

	Introduce a configuration variable diff.context that tells
	Porcelain commands to use a non-default number of context
	lines instead of 3 (the default).  With this variable, users
	do not have to keep repeating "git log -U8" from the command
	line; instead, it becomes sufficient to say "git config
	diff.context 8" just once.

  or something like that to make it clear that it is related to our
  -U option.

* That relationship with the -U option may worth mentioning in the
  documentation, not just in the log message.

* The configuration is read only in diff_ui_config and not in the
  lower-level diff_config.  What the patch does is the right thing.

  It however needs to be documented in the patch to diff-config.txt
  that it affects only the Porcelain commands, and does not break
  plumbing commands.

* Tests?  Minimally, the cases you may want to check are:

  - What happens with various values set to this variable, and does
    the code properly diagnose errors?

    [diff]
	context ;# boolean?
        context = no
        context = 0
        context = -1
        context = 8

  - What happens when the variable is set and the command line gives
    a different value with -U?

    git config diff.context 8
    git log -U4 -1

  - Does it really keep plumbing intact?

    git config diff.context 8
    git diff-files -p

Thanks.

Jeff Muizelaar· Sep 27, 2012, 15:04 UTC · re: Junio C Hamano · lore

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

Introduce a configuration variable diff.context that tells Porcelain commands to use a non-default number of context lines instead of 3 (the default). With this variable, users do not have to keep repeating "git log -U8" from the command line; instead, it becomes sufficient to say "git config diff.context 8" just once.

Signed-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>
---
 Documentation/diff-config.txt |    4 +
 diff.c                        |    9 +++-
 t/t4060-diff-context.sh       |  127 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 1 deletions(-)
 create mode 100755 t/t4060-diff-context.sh

Show changes to 3 files +139 −1

Documentation/diff-config.txt, diff.c, t/t4060-diff-context.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 67a90a8..75ab8a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -56,6 +56,10 @@ diff.statGraphWidth::
 	Limit the width of the graph part in --stat output. If set, applies
 	to all commands generating --stat output except format-patch.
 
+diff.context::
+	Generate diffs with <n> lines of context instead of the default of
+	3. This value is overridden by the -U option.
+
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
diff --git a/diff.c b/diff.c
index 35d3f07..86e5f2a 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,7 @@ static int diff_detect_rename_default;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
+static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.context")) {
+		diff_context_default = git_config_int(var, value);
+		if (diff_context_default < 0)
+			return -1;
+		return 0;
+	}
 	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/t4060-diff-context.sh b/t/t4060-diff-context.sh
new file mode 100755
index 0000000..76fa3c3
--- /dev/null
+++ b/t/t4060-diff-context.sh
@@ -0,0 +1,127 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Mozilla Foundation
+#
+
+test_description='diff.context configuration'
+
+. ./test-lib.sh
+
+cat << EOF > x
+firstline
+b
+c
+d
+e
+f
+preline
+postline
+i
+j
+k
+l
+m
+n
+EOF
+test_expect_success 'initial add' '
+	git update-index --add x &&
+	git commit -m initial
+'
+
+cat << EOF > x
+firstline
+b
+c
+d
+e
+f
+preline
+1
+postline
+i
+j
+k
+l
+m
+n
+EOF
+
+test_expect_success 'next commit' '
+	git update-index --add x &&
+	git commit -m next
+'
+cat << EOF > x
+firstline
+b
+c
+d
+e
+f
+preline
+2
+postline
+i
+j
+k
+l
+m
+n
+EOF
+
+
+
+
+test_expect_success 'diff.context affects log' '
+	git log -1 -p | grep -q -v firstline
+	git config diff.context 8 &&
+	git log -1 -p | grep -q firstline
+'
+test_expect_success 'different -U value' '
+	git config diff.context 8 &&
+	git log -U4 -1 | grep -q -v firstline
+'
+
+test_expect_success 'diff.context affects diff' '
+	git config diff.context 8 &&
+	git diff | grep -q firstline
+'
+
+test_expect_success 'plumbing not affected' '
+	git config diff.context 8 &&
+	git diff-files -p | grep -q -v firstline
+'
+
+cat > .git/config << EOF
+[diff]
+	context = no
+EOF
+test_expect_success 'config parsing' '
+	git diff 2>&1 | grep -q "bad config value"
+'
+
+cat > .git/config << EOF
+[diff]
+	context = 0
+EOF
+test_expect_success 'config parsing' '
+	git diff | grep -v preline
+'
+
+cat > .git/config << EOF
+[diff]
+	context = -1
+EOF
+test_expect_success 'config parsing' '
+	git diff 2>&1 | grep -q "bad config file"
+'
+
+cat > .git/config << EOF
+[diff]
+	context = 8
+EOF
+test_expect_success 'config parsing' '
+	git diff 2>&1 | grep "postline"
+'
+
+
+test_done

-- 
1.7.4.4

Junio C Hamano· Sep 27, 2012, 17:40 UTC · re: Jeff Muizelaar · lore

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

Jeff Muizelaar <jmuizelaar@mozilla.com> writes:

Show 14 quoted lines

> Introduce a configuration variable diff.context that tells
> Porcelain commands to use a non-default number of context
> lines instead of 3 (the default).  With this variable, users
> do not have to keep repeating "git log -U8" from the command
> line; instead, it becomes sufficient to say "git config
> diff.context 8" just once.
>
> Signed-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>
> ---
>  Documentation/diff-config.txt |    4 +
>  diff.c                        |    9 +++-
>  t/t4060-diff-context.sh       |  127 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 139 insertions(+), 1 deletions(-)
>  create mode 100755 t/t4060-diff-context.sh

Sigh, we don't have existing tests to check the number of context lines given with -U option and we need to allocate a new test number for it. What is the gap between 4054 (the last one in the tests for the diff family) and 4060 for?

Show 49 quoted lines

> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 67a90a8..75ab8a5 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -56,6 +56,10 @@ diff.statGraphWidth::
>  	Limit the width of the graph part in --stat output. If set, applies
>  	to all commands generating --stat output except format-patch.
>  
> +diff.context::
> +	Generate diffs with <n> lines of context instead of the default of
> +	3. This value is overridden by the -U option.
> +
>  diff.external::
>  	If this config variable is set, diff generation is not
>  	performed using the internal diff machinery, but using the
> diff --git a/diff.c b/diff.c
> index 35d3f07..86e5f2a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -26,6 +26,7 @@ static int diff_detect_rename_default;
>  static int diff_rename_limit_default = 400;
>  static int diff_suppress_blank_empty;
>  static int diff_use_color_default = -1;
> +static int diff_context_default = 3;
>  static const char *diff_word_regex_cfg;
>  static const char *external_diff_cmd_cfg;
>  int diff_auto_refresh_index = 1;
> @@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>  		diff_use_color_default = git_config_colorbool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "diff.context")) {
> +		diff_context_default = git_config_int(var, value);
> +		if (diff_context_default < 0)
> +			return -1;
> +		return 0;
> +	}
>  	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;

Thanks; looks sensible.

Show 31 quoted lines

> diff --git a/t/t4060-diff-context.sh b/t/t4060-diff-context.sh
> new file mode 100755
> index 0000000..76fa3c3
> --- /dev/null
> +++ b/t/t4060-diff-context.sh
> @@ -0,0 +1,127 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Mozilla Foundation
> +#
> +
> +test_description='diff.context configuration'
> +
> +. ./test-lib.sh
> +
> +cat << EOF > x
> +firstline
> +b
> +c
> +d
> +e
> +f
> +preline
> +postline
> +i
> +j
> +k
> +l
> +m
> +n
> +EOF

I know ancient tests are written like this, but we are slowly trying to migrate them to have these test-vector preparation inside test_expect_success block, e.g.

	test_expect_success setup '
		cat >x <<-\EOF &&
                firstline
                b
                ...
                n
                EOF
		git add x &&
                git commit -m initial
	'

Show 5 quoted lines

> +test_expect_success 'diff.context affects log' '
> +	git log -1 -p | grep -q -v firstline
> +	git config diff.context 8 &&
> +	git log -1 -p | grep -q firstline
> +'

Three points:

 - Please avoid "grep -q", which does not help people who ran tests
   (the output is hidden by default) and hurts people who want to
   debug tests.

 - Your test will ignore breakage from the first "log 1" output and
   goes on running "git config".  Make sure you got your && cascades
   right.

 - Because an error from the command on the upstream side of the
   pipe is ignored, we tend to prefer writing things like this:

	git log -n 1 -p >output &&
        grep -v firstline output &&
	...

Show 7 quoted lines

> +cat > .git/config << EOF
> +[diff]
> +	context = no
> +EOF
> +test_expect_success 'config parsing' '
> +	git diff 2>&1 | grep -q "bad config value"
> +'

How does the "git diff" command exit? That is far more important than the actual error message, but being on the left side of the pipe, the exit status from the command is not being tested.

Jeff King· Sep 27, 2012, 18:43 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 27, 2012 at 10:40:26AM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> > +test_expect_success 'diff.context affects log' '
> > +	git log -1 -p | grep -q -v firstline
> > +	git config diff.context 8 &&
> > +	git log -1 -p | grep -q firstline
> > +'
> 
> Three points:
> 
>  - Please avoid "grep -q", which does not help people who ran tests
>    (the output is hidden by default) and hurts people who want to
>    debug tests.
> 
>  - Your test will ignore breakage from the first "log 1" output and
>    goes on running "git config".  Make sure you got your && cascades
>    right.
> 
>  - Because an error from the command on the upstream side of the
>    pipe is ignored, we tend to prefer writing things like this:
> 
> 	git log -n 1 -p >output &&
>         grep -v firstline output &&

I agree with all of that. But also, is "grep -v" the right thing? I think the intent of the test is "firstline does not appear". But that is not what "grep -v" will tell you. It will tell you whether any line that did not have "firstline" in it was shown (which it would be, since there are a bunch of other lines shown).

I think "! grep firstline" is what is needed here. Or even just explicitly matching the diff that we expect via test_cmp. I like the latter much better anyway, as a failure will show exactly what is wrong. Whereas if the grep ends up not matching, there is no helpful output for somebody reading the test.

We already produce nice messages from things like test_must_fail. Maybe it would be nice to have:

  test_contains () {
          if ! grep "$1" "$2"; then
	          echo >&2 "File '$2' does not contain a line with '$1'"
		  return 1
	  fi
  }

and likewise a "test_not_contains" or something to negate it. That makes both the tests and their failure output readable.

-Peff

Jeff Muizelaar· Sep 27, 2012, 19:12 UTC · re: Junio C Hamano · lore

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

Signed-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>
---

This fixes the tests to behave better and avoids 'grep -v'

 Documentation/diff-config.txt |    4 +
 diff.c                        |    9 +++-
 t/t4055-diff-context.sh       |  128 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletions(-)
 create mode 100755 t/t4055-diff-context.sh

Show changes to 3 files +140 −1

Documentation/diff-config.txt, diff.c, t/t4055-diff-context.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 67a90a8..75ab8a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -56,6 +56,10 @@ diff.statGraphWidth::
 	Limit the width of the graph part in --stat output. If set, applies
 	to all commands generating --stat output except format-patch.
 
+diff.context::
+	Generate diffs with <n> lines of context instead of the default of
+	3. This value is overridden by the -U option.
+
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
diff --git a/diff.c b/diff.c
index 35d3f07..86e5f2a 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,7 @@ static int diff_detect_rename_default;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
+static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.context")) {
+		diff_context_default = git_config_int(var, value);
+		if (diff_context_default < 0)
+			return -1;
+		return 0;
+	}
 	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
+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 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
+'
+
+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
+'
+
+test_expect_success 'different -U value' '
+	git config diff.context 8 &&
+	git log -U4 -1 >output &&
+	! grep firstline output
+'
+
+test_expect_success 'diff.context affects diff' '
+	git config diff.context 8 &&
+	git diff >output &&
+	grep firstline output
+'
+
+test_expect_success 'plumbing not affected' '
+	git config diff.context 8 &&
+	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
+'
+
+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
+'
+
+test_expect_success 'config parsing' '
+	cat >.git/config <<-\EOF &&
+[diff]
+	context = 8
+EOF
+	git diff >output &&
+	grep postline output
+'
+
+test_done

-- 
1.7.4.4

Junio C Hamano· Sep 27, 2012, 22:18 UTC · re: Jeff Muizelaar · lore

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

Jeff Muizelaar <jmuizelaar@mozilla.com> writes:

Show 5 quoted lines

> +	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.

Show 40 quoted lines

> +	}
>  	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.

Show 7 quoted lines

> +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.

Show 5 quoted lines

> +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).

Show 5 quoted lines

> +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 &&

Show 6 quoted lines

> +[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.

Show 17 quoted lines

> +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.

Show 8 quoted lines

> +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(-)

Show changes to diff +50 −85

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

Jeff Muizelaar· Oct 19, 2012, 20:54 UTC · re: Junio C Hamano · lore

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

Signed-off-by: Jeff Muizelaar <jmuizelaar@mozilla.com>
---
On 2012-09-27, at 6:18 PM, Junio C Hamano wrote:

Show 18 quoted lines

> 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.

I left this as is.

Show 11 quoted lines

>> +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.

No. I changed it to "show" to avoid the surprise.

Other than that I included the test changes you suggested. I did however adjust the sed 'a' command. It seems that GNU sed and BSD sed have incompatible behaviour here. Unfortunately this also seems to screw up the indentation.

---
 Documentation/diff-config.txt |    4 ++
 diff.c                        |    9 ++++-
 t/t4055-diff-context.sh       |   94 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 1 deletions(-)
 create mode 100755 t/t4055-diff-context.sh

Show changes to 3 files +106 −1

Documentation/diff-config.txt, diff.c, t/t4055-diff-context.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 67a90a8..75ab8a5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -56,6 +56,10 @@ diff.statGraphWidth::
 	Limit the width of the graph part in --stat output. If set, applies
 	to all commands generating --stat output except format-patch.
 
+diff.context::
+	Generate diffs with <n> lines of context instead of the default of
+	3. This value is overridden by the -U option.
+
 diff.external::
 	If this config variable is set, diff generation is not
 	performed using the internal diff machinery, but using the
diff --git a/diff.c b/diff.c
index 35d3f07..86e5f2a 100644
--- a/diff.c
+++ b/diff.c
@@ -26,6 +26,7 @@ static int diff_detect_rename_default;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
+static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
@@ -141,6 +142,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.context")) {
+		diff_context_default = git_config_int(var, value);
+		if (diff_context_default < 0)
+			return -1;
+		return 0;
+	}
 	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..60348b7
--- /dev/null
+++ b/t/t4055-diff-context.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Mozilla Foundation
+#
+
+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
+	git update-index --add x &&
+	git commit -m initial &&
+
+	git cat-file blob HEAD:x |
+	sed "s/preline/preline\\
+ADDED/" >x &&
+	git update-index --add x &&
+	git commit -m next &&
+
+	git cat-file blob HEAD:x |
+	sed s/ADDED/MODIFIED/ >x
+'
+
+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 "show"' '
+	git show >output &&
+	! grep firstline output &&
+	git config diff.context 8 &&
+	git show >output &&
+	grep "^ firstline" output
+'
+
+test_expect_success 'The -U option overrides diff.context' '
+	git config diff.context 8 &&
+	git log -U4 -1 >output &&
+	! grep "^ firstline" output
+'
+
+test_expect_success 'diff.context honored by "diff"' '
+	git config diff.context 8 &&
+	git diff >output &&
+	grep "^ firstline" output
+'
+
+test_expect_success 'plumbing not affected' '
+	git config diff.context 8 &&
+	git diff-files -p >output &&
+	! grep "^ firstline" output
+'
+
+test_expect_success 'non-integer config parsing' '
+	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' '
+	git config diff.context -1 &&
+	test_must_fail git diff 2>output &&
+	test_i18ngrep "bad config file" output
+'
+
+test_expect_success '-U0 is valid, so is diff.context=0' '
+	git config diff.context 0 &&
+	git diff >output &&
+	grep "^-ADDED" output &&
+	grep "^+MODIFIED" output &&
+	! grep "^ postline" output
+'
+
+test_done

-- 
1.7.4.4