Re: [PATCH] diff: Add diff.orderfile configuration variable

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 21, 2013, 18:40 UTC
Message-ID: <20131021184040.GX9464@google.com>
In-Reply-To: <CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com>

Hi,

Anders Waldenborg wrote:

> diff.orderfile acts as a default for the -O command line option.
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>

Thanks.

[...]

Show 5 quoted lines

> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -98,6 +98,10 @@ diff.mnemonicprefix::
>  diff.noprefix::
>   If set, 'git diff' does not show any source or destination prefix.

It looks like your mailer is corrupting tabs and converting them into spaces. See the "Discussion" section of git-format-patch(1) for hints on checking a patch by mailing it to yourself and applying with git-am(1).

> +diff.orderfile::
> + Path to file to use for ordering the files in the diff, each line
> + is a shell glob pattern; equivalent to the 'git diff' option '-O'.

Nits:

 * "Path to" could be left out, since a path is the only way to specify a
   file :)
 * Comma splice.
 * What happens if both [diff] orderfile and the -O option are used?

How about something like the following?

	diff.orderfile::
		File indicating how to order files within a diff, using
		one shell glob pattern per line.
		Can be overridden by the '-O' option to linkgit:git-diff[1].

Should the git-diff(1) manpage get a note about this setting as well (perhaps in a new CONFIGURATION section)?

[...]

Show 26 quoted lines

> --- a/diff.c
> +++ b/diff.c
> @@ -30,6 +30,7 @@ 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;
> +static const char *diff_order_file_cfg;
>  int diff_auto_refresh_index = 1;
>  static int diff_mnemonic_prefix;
>  static int diff_no_prefix;
> @@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char
> *value, void *cb)
>   return git_config_string(&external_diff_cmd_cfg, var, value);
>   if (!strcmp(var, "diff.wordregex"))
>   return git_config_string(&diff_word_regex_cfg, var, value);
> + if (!strcmp(var, "diff.orderfile"))
> + return git_config_string(&diff_order_file_cfg, var, value);
> 
>   if (!strcmp(var, "diff.ignoresubmodules"))
>   handle_ignore_submodules_arg(&default_diff_options, value);
> @@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
>   options->detect_rename = diff_detect_rename_default;
>   options->xdl_opts |= diff_algorithm;
> 
> + options->orderfile = diff_order_file_cfg;
> +

Should Documentation/technical/api-diff.txt be tweaked to mention that the options set by diff_setup() depend on configuration now?

If a caller wants to parse diff config and also wants to make a diff without using the config (the example I'm imagining is an alternative implemention fo "git log -p --cherry-pick"), can they do that? It's tempting to move handling of configuration into a separate function. (Perhaps it's not worth worrying about that until someone needs the flexibility, though.)

Show 10 quoted lines

> --- /dev/null
> +++ b/t/t4056-diff-order.sh
> @@ -0,0 +1,74 @@
> +#!/bin/sh
> +
> +test_description='diff order'
> +
> +. ./test-lib.sh
> +
> +_test_create_files () {

Why the leading underscore?

[...]

> +test_expect_success "setup" '_test_create_files 1 && _test_create_files 2'

Usual style is to put each command on its own line:

	test_expect_success 'setup' '
		_test_create_files 1 &&
		_test_create_files 2
	'

> +
> +test_expect_success "no order (=tree object order)" '
> + git diff HEAD^..HEAD | grep ^diff >actual_diff_headers &&

This loses the exit code from "git diff", which loses a chance to notice if "git diff" starts to segfault now and then. How about:

	git diff HEAD^..HEAD >patch &&
	grep ^diff patch >actual_diff_headers
	test_cmp expect_diff_headers_non actual_diff_headers

> + test_debug actual_diff_headers

test_debug runs its argument as a command, which is not what I think you want here. :) Probably you wanted to write the diff header out when testing with "--verbose" so if it fails it is clear how it failed?

> + test_cmp expect_diff_headers_none actual_diff_headers'

Luckily test_cmp already takes care of that, by printing a diff.

Hope that helps, Jonathan