Re: [PATCH v2] diff: Add diff.orderfile configuration variable
Samuel Bronson naes...@gmail.com writes: If somebody has diff.orderfile configuration that points at a custom ordering, and wants to send out a patch (or show a diff) with the standard order, how would the overriding command line look like? Would it be git diff -O/dev/null? It looks like that works ... and so do files that don't exist. What do you think should happen with -O file-that-does-not-exist, and how do you suppose it should be tested? I think the original code is too loose on diagnosing errors. Perhaps something like this is needed. diff --git a/diffcore-order.c b/diffcore-order.c index 23e9385..dd103e3 100644 --- a/diffcore-order.c +++ b/diffcore-order.c @@ -20,8 +20,11 @@ static void prepare_order(const char *orderfile) return; fd = open(orderfile, O_RDONLY); - if (fd 0) + if (fd 0) { + if (errno != ENOENT || errno != ENOTDIR) + die(_(orderfile '%s' does not exist.)); return; + } if (fstat(fd, st)) { close(fd); return; After having fixed this, will /dev/null still work everywhere, or will we want a new diff flag to unset the option? (I see that git diff /dev/null some-file works fine with msysgit, which doesn't seem to actually be linked with MSYS, but I don't know *why* it works, and I don't know what other non-POSIXoid ports exist.) I *think* it should be OK to use -O/dev/null for that purpose, but the primary thing I was hinting at with the rhetoric question was that it probably needs to be documented there. Also, I'm starting to wonder if I shouldn't split this into two patches: 1. diff: Add tests for -O flag 2. diff: Add diff.orderfile configuration variable (If so, I would obviously want to rewrite the above test to avoid the configuration option.) Surely, and thanks. EOF cat order_file_2 -\EOF I'd kind of prefer to keep a blank line between one EOF and the next cat, if that's okay with you. Alright. Making it easier to spot the grouping, it would make it easier to read. Quoting the EOF like the above will help the readers by signaling them that they do not have to wonder if there is some substitution going on in the here text. Perhaps, but probably only after they've scrutinized their shell manuals to figure out what the - and the \ are for. (I had to check two: dash(1) wasn't clear enough for me about the quoting ...) Yes and no. The no primarily comes from that nobody will stay to be novice forever. -- 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
Re: [PATCH v2] diff: Add diff.orderfile configuration variable
On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano gits...@pobox.com wrote: Samuel Bronson naes...@gmail.com writes: Thanks for reviving a stalled topic. I was asking about such a feature in #git and jrnieder was nice enough to point me at the stalled patch. *I* even verified that the tests do fail properly when the feature is sabotaged. Sabotaged in what way? I commented out the options-orderfile = diff_order_file_cfg; line. @@ -432,6 +432,8 @@ endif::git-format-patch[] -Oorderfile:: Output the patch in the order specified in the orderfile, which has one shell glob pattern per line. + This overrides the `diff.orderfile' configuration variable + ((see linkgit:git-config[1]). Double opening parenthesis? Oops, and it looks like I messed up the quoting on diff.orderfile too ... If somebody has diff.orderfile configuration that points at a custom ordering, and wants to send out a patch (or show a diff) with the standard order, how would the overriding command line look like? Would it be git diff -O/dev/null? It looks like that works ... and so do files that don't exist. What do you think should happen with -O file-that-does-not-exist, and how do you suppose it should be tested? After having fixed this, will /dev/null still work everywhere, or will we want a new diff flag to unset the option? (I see that git diff /dev/null some-file works fine with msysgit, which doesn't seem to actually be linked with MSYS, but I don't know *why* it works, and I don't know what other non-POSIXoid ports exist.) For the moment, I've added this to for loop (after some changes based on some of your other suggestions): # I don't think this should just pretend the orderfile was empty? test_expect_failure override with bogus orderfile ($i) ' test_might_fail git -c diff.orderfile=order_file_$i diff -Obogus_file --name-only HEAD^..HEAD actual_diff_filenames ! test_cmp expect_diff_filenames_none actual_diff_filenames ' Does this look (modulo gmail's stupid indentation) anything like a reasonable approach to testing that? (Of course, you can't actually test it because it depends on other changes I haven't posted yet ...) Also, I'm starting to wonder if I shouldn't split this into two patches: 1. diff: Add tests for -O flag 2. diff: Add diff.orderfile configuration variable (If so, I would obviously want to rewrite the above test to avoid the configuration option.) + return $? +} That return looks somewhat strange. Does it even need to be there? I'm certainly no great expert at shell functions, so I expect it isn't. I'm not really sure what possessed me to think it might be needed. EOF cat order_file_2 -\EOF I'd kind of prefer to keep a blank line between one EOF and the next cat, if that's okay with you. Quoting the EOF like the above will help the readers by signaling them that they do not have to wonder if there is some substitution going on in the here text. Perhaps, but probably only after they've scrutinized their shell manuals to figure out what the - and the \ are for. (I had to check two: dash(1) wasn't clear enough for me about the quoting ...) +test_expect_success no order (=tree object order) ' + git diff HEAD^..HEAD patch + grep ^diff patch actual_diff_headers + test_cmp expect_diff_headers_none actual_diff_headers +' Instead of grepping, git diff --name-only would be far easier to check, no? It certainly makes for less-cluttered expected output. (I guess jrnieder didn't know about that trick when he suggested using the intermediate file?) Points to note: * We eval the scriptlets inside test framework, so using $i as a variable inside the single quotes will have the expected result. You do not have to worry about extra quoting inside dq pair. Hmm. I'm obviously not used to things getting eval'd in the same shell instance as my script ... (Thanks for the review!) -- 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
[PATCH v2] diff: Add diff.orderfile configuration variable
From: Anders Waldenborg and...@0x63.nu diff.orderfile acts as a default for the -O command line option. [sb: fixed testcases revised docs based on Jonathan Nieder's suggestions] Signed-off-by: Anders Waldenborg and...@0x63.nu Thanks-to: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Samuel Bronson naes...@gmail.com --- *I* even verified that the tests do fail properly when the feature is sabotaged. Documentation/diff-config.txt | 5 +++ Documentation/diff-options.txt | 2 ++ diff.c | 5 +++ t/t4056-diff-order.sh | 79 ++ 4 files changed, 91 insertions(+) create mode 100755 t/t4056-diff-order.sh diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index 223b931..f07b451 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -98,6 +98,11 @@ diff.mnemonicprefix:: diff.noprefix:: If set, 'git diff' does not show any source or destination prefix. +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]. + diff.renameLimit:: The number of files to consider when performing the copy/rename detection; equivalent to the 'git diff' option '-l'. diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index bbed2cd..1af5a5e 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -432,6 +432,8 @@ endif::git-format-patch[] -Oorderfile:: Output the patch in the order specified in the orderfile, which has one shell glob pattern per line. + This overrides the `diff.orderfile' configuration variable + ((see linkgit:git-config[1]). ifndef::git-format-patch[] -R:: diff --git a/diff.c b/diff.c index e34bf97..a92b570 100644 --- 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; + if (diff_no_prefix) { options-a_prefix = options-b_prefix = ; } else if (!diff_mnemonic_prefix) { diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh new file mode 100755 index 000..a756b34 --- /dev/null +++ b/t/t4056-diff-order.sh @@ -0,0 +1,79 @@ +#!/bin/sh + +test_description='diff order' + +. ./test-lib.sh + +create_files () { + echo $1 a.h + echo $1 b.c + echo $1 c/Makefile + echo $1 d.txt + git add a.h b.c c/Makefile d.txt + git commit -m$1 + return $? +} + +test_expect_success setup ' + mkdir c + create_files 1 + create_files 2 +' + +cat order_file_1 EOF +*Makefile +*.txt +*.h +* +EOF +cat order_file_2 EOF +*Makefile +*.h +*.c +* +EOF + +cat expect_diff_headers_none EOF +diff --git a/a.h b/a.h +diff --git a/b.c b/b.c +diff --git a/c/Makefile b/c/Makefile +diff --git a/d.txt b/d.txt +EOF + +cat expect_diff_headers_1 EOF +diff --git a/c/Makefile b/c/Makefile +diff --git a/d.txt b/d.txt +diff --git a/a.h b/a.h +diff --git a/b.c b/b.c +EOF + +cat expect_diff_headers_2 EOF +diff --git a/c/Makefile b/c/Makefile +diff --git a/a.h b/a.h +diff --git a/b.c b/b.c +diff --git a/d.txt b/d.txt +EOF + +test_expect_success no order (=tree object order) ' + git diff HEAD^..HEAD patch + grep ^diff patch actual_diff_headers + test_cmp expect_diff_headers_none actual_diff_headers +' + +for i in 1 2; do + test_expect_success orderfile using option ($i) + git diff -Oorder_file_$i HEAD^..HEAD patch + grep ^diff patch actual_diff_headers + test_cmp expect_diff_headers_$i actual_diff_headers + +done + +for i in 1 2; do + test_expect_success orderfile using config ($i) + git -c diff.orderfile=order_file_$i diff HEAD^..HEAD patch + grep ^diff patch actual_diff_headers + test_cmp expect_diff_headers_$i actual_diff_headers + +done + +test_done -- 1.8.4.3 -- To unsubscribe from this list: send the