Re: [PATCH] diff: Add diff.orderfile configuration variable
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:59:03
Hi, Anders Waldenborg wrote:
diff.orderfile acts as a default for the -O command line option. Signed-off-by: Anders Waldenborg <redacted>
Thanks. [...]
quoted hunk ↗ jump to hunk
--- 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)? [...]
quoted hunk ↗ jump to hunk
--- 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.)
quoted hunk ↗ jump to hunk
--- /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