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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 27, 2012, 17:40 UTC
Message-ID: <7vr4pnqs8l.fsf@alter.siamese.dyndns.org>
In-Reply-To: <C4993A4E-E443-4DB4-9DCA-20128CADDC6E@mozilla.com>

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.