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

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

[Jeff Muizelaar]

[-- Attachment #1: Type: text/plain, Size: 141 bytes --]

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


[-- Attachment #2: add-context-option --]
[-- Type: application/octet-stream, Size: 2066 bytes --]

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.

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 67a90a8..028ff14 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.
+
 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 e6846ca..b5cd8be 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;
 int diff_use_color_default = -1;
+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,10 @@ 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);
+		return 0;
+	}
 	if (!strcmp(var, "diff.renames")) {
 		diff_detect_rename_default = git_config_rename(var, value);
 		return 0;
@@ -3172,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;

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

[Junio C Hamano]

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.

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

[Jeff Muizelaar]

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

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

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

[Junio C Hamano]

Jeff Muizelaar <jmuizelaar@mozilla.com> writes:

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

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

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

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

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

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

[Jeff King]

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

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

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

[Jeff Muizelaar]

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

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

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

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

[Junio C Hamano]

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

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

[Jeff Muizelaar]

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>
---
On 2012-09-27, at 6:18 PM, Junio C Hamano wrote:

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

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

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