Hi,
Anders Waldenborg wrote:
> diff.orderfile acts as a default for the -O command line option.
>
> Signed-off-by: Anders Waldenborg
Thanks.
[...]
> --- 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)?
[...]
> --- 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.)
> --- /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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
