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

Jeff Muizelaar [off-list ref] 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 <redacted>
---
 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?

Thanks; looks sensible.

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.

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

quoted

+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

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

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