{"thread":{"id":"35172","subject":"[PATCH] diff: Add diff.orderfile configuration variable","startedAt":"2013-10-21T10:31:59Z","lastAt":"2014-01-10T23:30:11Z","messageCount":35,"participants":["Anders Waldenborg","Jonathan Nieder","Samuel Bronson","Junio C Hamano","Antoine Pelisse"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"229244","messageId":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","threadId":"35172","inReplyTo":null,"subject":"[PATCH] diff: Add diff.orderfile configuration variable","fromName":"Anders Waldenborg","fromEmail":"anders.waldenborg@gmail.com","sentAt":"2013-10-21T10:31:59Z","receivedAt":"2013-10-21T10:31:59Z","isPatch":true,"sender":{"key":"anders.waldenborg@gmail.com","avatar":null},"body":"diff.orderfile acts as a default for the -O command line option.\n\nSigned-off-by: Anders Waldenborg \n---\n\n Documentation/diff-config.txt | 4 +++\n diff.c | 5 +++\n t/t4056-diff-order.sh | 74 +++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 83 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 223b931..51f9190 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -98,6 +98,10 @@ diff.mnemonicprefix::\n diff.noprefix::\n If set, 'git diff' does not show any source or destination prefix.\n\n+diff.orderfile::\n+ Path to file to use for ordering the files in the diff, each line\n+ is a shell glob pattern; equivalent to the 'git diff' option '-O'.\n+\n diff.renameLimit::\n The number of files to consider when performing the copy/rename\n detection; equivalent to the 'git diff' option '-l'.\ndiff --git a/diff.c b/diff.c\nindex a04a34d..e66f031 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char\n*value, void *cb)\n return git_config_string(&external_diff_cmd_cfg, var, value);\n if (!strcmp(var, \"diff.wordregex\"))\n return git_config_string(&diff_word_regex_cfg, var, value);\n+ if (!strcmp(var, \"diff.orderfile\"))\n+ return git_config_string(&diff_order_file_cfg, var, value);\n\n if (!strcmp(var, \"diff.ignoresubmodules\"))\n handle_ignore_submodules_arg(&default_diff_options, value);\n@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n options->detect_rename = diff_detect_rename_default;\n options->xdl_opts |= diff_algorithm;\n\n+ options->orderfile = diff_order_file_cfg;\n+\n if (diff_no_prefix) {\n options->a_prefix = options->b_prefix = \"\";\n } else if (!diff_mnemonic_prefix) {\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..fd005d6\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+\n+test_description='diff order'\n+\n+. ./test-lib.sh\n+\n+_test_create_files () {\n+ mkdir c\n+ echo \"$1\" >a.h\n+ echo \"$1\" >b.c\n+ echo \"$1\" >c/Makefile\n+ echo \"$1\" >d.txt\n+ git add a.h b.c c/Makefile d.txt && \\\n+ git commit -m\"$1\"\n+}\n+\n+cat >order_file_1 <order_file_2 <expect_diff_headers_none <expect_diff_headers_1 <expect_diff_headers_2 <actual_diff_headers &&\n+ test_debug actual_diff_headers\n+ test_cmp expect_diff_headers_none actual_diff_headers'\n+\n+test_expect_success \"orderfile using option\" '\n+ git diff -Oorder_file_1 HEAD^..HEAD | grep ^diff >actual_diff_headers &&\n+ test_debug actual_diff_headers\n+ test_cmp expect_diff_headers_1 actual_diff_headers &&\n+ git diff -Oorder_file_2 HEAD^..HEAD | grep ^diff >actual_diff_headers &&\n+ test_debug actual_diff_headers\n+ test_cmp expect_diff_headers_2 actual_diff_headers'\n+\n+test_expect_success \"orderfile using config\" '\n+ git -c diff.orderfile=order_file_1 diff HEAD^..HEAD | grep ^diff\n>actual_diff_headers &&\n+ test_debug actual_diff_headers\n+ test_cmp expect_diff_headers_1 actual_diff_headers &&\n+ git -c diff.orderfile=order_file_2 diff HEAD^..HEAD | grep ^diff\n>actual_diff_headers &&\n+ test_debug actual_diff_headers\n+ test_cmp expect_diff_headers_2 actual_diff_headers'\n+\n+test_done\n-- \n1.8.4.1.559.gdb9bdfb.dirty\n"},{"id":"229255","messageId":"20131021184040.GX9464@google.com","threadId":"35172","inReplyTo":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","subject":"Re: [PATCH] diff: Add diff.orderfile configuration variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-21T18:40:40Z","receivedAt":"2013-10-21T18:40:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nAnders Waldenborg wrote:\n\n> diff.orderfile acts as a default for the -O command line option.\n>\n> Signed-off-by: Anders Waldenborg \n\nThanks.\n\n[...]\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -98,6 +98,10 @@ diff.mnemonicprefix::\n> diff.noprefix::\n> If set, 'git diff' does not show any source or destination prefix.\n\nIt looks like your mailer is corrupting tabs and converting them into\nspaces. See the \"Discussion\" section of git-format-patch(1) for hints\non checking a patch by mailing it to yourself and applying with\ngit-am(1).\n\n> +diff.orderfile::\n> + Path to file to use for ordering the files in the diff, each line\n> + is a shell glob pattern; equivalent to the 'git diff' option '-O'.\n\nNits:\n\n * \"Path to\" could be left out, since a path is the only way to specify a\n file :)\n * Comma splice.\n * What happens if both [diff] orderfile and the -O option are used?\n\nHow about something like the following?\n\n\tdiff.orderfile::\n\t\tFile indicating how to order files within a diff, using\n\t\tone shell glob pattern per line.\n\t\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n\nShould the git-diff(1) manpage get a note about this setting as\nwell (perhaps in a new CONFIGURATION section)?\n\n[...]\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n> static int diff_context_default = 3;\n> static const char *diff_word_regex_cfg;\n> static const char *external_diff_cmd_cfg;\n> +static const char *diff_order_file_cfg;\n> int diff_auto_refresh_index = 1;\n> static int diff_mnemonic_prefix;\n> static int diff_no_prefix;\n> @@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char\n> *value, void *cb)\n> return git_config_string(&external_diff_cmd_cfg, var, value);\n> if (!strcmp(var, \"diff.wordregex\"))\n> return git_config_string(&diff_word_regex_cfg, var, value);\n> + if (!strcmp(var, \"diff.orderfile\"))\n> + return git_config_string(&diff_order_file_cfg, var, value);\n> \n> if (!strcmp(var, \"diff.ignoresubmodules\"))\n> handle_ignore_submodules_arg(&default_diff_options, value);\n> @@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n> options->detect_rename = diff_detect_rename_default;\n> options->xdl_opts |= diff_algorithm;\n> \n> + options->orderfile = diff_order_file_cfg;\n> +\n\nShould Documentation/technical/api-diff.txt be tweaked to mention that\nthe options set by diff_setup() depend on configuration now?\n\nIf a caller wants to parse diff config and also wants to make a diff\nwithout using the config (the example I'm imagining is an alternative\nimplemention fo \"git log -p --cherry-pick\"), can they do that? It's\ntempting to move handling of configuration into a separate function.\n(Perhaps it's not worth worrying about that until someone needs the\nflexibility, though.)\n> --- /dev/null\n> +++ b/t/t4056-diff-order.sh\n> @@ -0,0 +1,74 @@\n> +#!/bin/sh\n> +\n> +test_description='diff order'\n> +\n> +. ./test-lib.sh\n> +\n> +_test_create_files () {\n\nWhy the leading underscore?\n\n[...]\n> +test_expect_success \"setup\" '_test_create_files 1 && _test_create_files 2'\n\nUsual style is to put each command on its own line:\n\n\ttest_expect_success 'setup' '\n\t\t_test_create_files 1 &&\n\t\t_test_create_files 2\n\t'\n\n> +\n> +test_expect_success \"no order (=tree object order)\" '\n> + git diff HEAD^..HEAD | grep ^diff >actual_diff_headers &&\n\nThis loses the exit code from \"git diff\", which loses a chance to\nnotice if \"git diff\" starts to segfault now and then. How about:\n\n\tgit diff HEAD^..HEAD >patch &&\n\tgrep ^diff patch >actual_diff_headers\n\ttest_cmp expect_diff_headers_non actual_diff_headers\n\n> + test_debug actual_diff_headers\n\ntest_debug runs its argument as a command, which is not what I think\nyou want here. :) Probably you wanted to write the diff header out\nwhen testing with \"--verbose\" so if it fails it is clear how it\nfailed?\n\n> + test_cmp expect_diff_headers_none actual_diff_headers'\n\nLuckily test_cmp already takes care of that, by printing a diff.\n\nHope that helps,\nJonathan\n"},{"id":"229512","messageId":"CADsOX3DvqrR66uKtGZr2MJta9F0R7QmU2MO6mr0XA_kut8mZ-Q@mail.gmail.com","threadId":"35172","inReplyTo":"20131021184040.GX9464@google.com","subject":"Re: [PATCH] diff: Add diff.orderfile configuration variable","fromName":"Anders Waldenborg","fromEmail":"anders.waldenborg@gmail.com","sentAt":"2013-10-25T10:24:42Z","receivedAt":"2013-10-25T10:24:42Z","isPatch":true,"sender":{"key":"anders.waldenborg@gmail.com","avatar":null},"body":"(Jonathan, sorry if you got this multiple times, it seems I forgot to Cc list)\n\nOn Mon, Oct 21, 2013 at 8:40 PM, Jonathan Nieder wrote:\n> Should the git-diff(1) manpage get a note about this setting as\n> well (perhaps in a new CONFIGURATION section)?\n\nI'll add a reference to the documentation for the -O option at least.\nThat is how --check, --color, --dirstat and others do it, I guess that\ncould be moved to a CONFIGURATION section later?\n\n> Should Documentation/technical/api-diff.txt be tweaked to mention that\n> the options set by diff_setup() depend on configuration now?\n\nIt already did, didn't it? At least diff.context, diff.renames and\ndiff.color seems to affect diff_setup(), no?\n\n> If a caller wants to parse diff config and also wants to make a diff\n> without using the config (the example I'm imagining is an alternative\n> implemention fo \"git log -p --cherry-pick\"), can they do that? It's\n> tempting to move handling of configuration into a separate function.\n> (Perhaps it's not worth worrying about that until someone needs the\n> flexibility, though.)\n\nRight, patch-ids are not stable wrt ordering. That might be a problem\nif some tool stores patch-ids. But maybe that even is a separate bug?\nShould patch-id always reorder the files internally? Is it expected\nthat \"git diff -Oorder1 | git patch-id\" and \"git diff -Oorder2 | git\npatch-id\" gives same patch id?\n\nIt gets very interesting in an imaginative \"git log -p --cherry-pick\"\nwhich caches patch-ids on disk, one would want one stable ordering for\ncalculating the patchid, while the displayed patch should respect the\nuser requested order.\n\nI guess that in most cases one would want to respect user configured\nordering. Should diff_setup grow an argument \"ignore_config\"? Or\nshould we maybe add an --no-order-file option that easily be set as a\nflag in diff_options in those cases?\n\n> Hope that helps,\n\nIt does. Thanks! I have updated patch as per your other comments.\n\n anders\n"},{"id":"231650","messageId":"1386312508-7421-1-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","subject":"[PATCH v2] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-06T06:48:28Z","receivedAt":"2013-12-06T06:48:28Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"From: Anders Waldenborg \n\ndiff.orderfile acts as a default for the -O command line option.\n\n[sb: fixed testcases & revised docs based on Jonathan Nieder's suggestions]\n\nSigned-off-by: Anders Waldenborg \nThanks-to: Jonathan Nieder \nSigned-off-by: Samuel Bronson \n---\n*I* even verified that the tests do fail properly when the feature is\nsabotaged.\n\n Documentation/diff-config.txt | 5 +++\n Documentation/diff-options.txt | 2 ++\n diff.c | 5 +++\n t/t4056-diff-order.sh | 79 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 91 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 223b931..f07b451 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -98,6 +98,11 @@ diff.mnemonicprefix::\n diff.noprefix::\n \tIf set, 'git diff' does not show any source or destination prefix.\n \n+diff.orderfile::\n+\tFile indicating how to order files within a diff, using\n+\tone shell glob pattern per line.\n+\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n+\n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n \tdetection; equivalent to the 'git diff' option '-l'.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex bbed2cd..1af5a5e 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -432,6 +432,8 @@ endif::git-format-patch[]\n -O::\n \tOutput the patch in the order specified in the\n \t, which has one shell glob pattern per line.\n+\tThis overrides the `diff.orderfile' configuration variable\n+\t((see linkgit:git-config[1]).\n \n ifndef::git-format-patch[]\n -R::\ndiff --git a/diff.c b/diff.c\nindex e34bf97..a92b570 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n+\tif (!strcmp(var, \"diff.orderfile\"))\n+\t\treturn git_config_string(&diff_order_file_cfg, var, value);\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \n+\toptions->orderfile = diff_order_file_cfg;\n+\n \tif (diff_no_prefix) {\n \t\toptions->a_prefix = options->b_prefix = \"\";\n \t} else if (!diff_mnemonic_prefix) {\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..a756b34\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,79 @@\n+#!/bin/sh\n+\n+test_description='diff order'\n+\n+. ./test-lib.sh\n+\n+create_files () {\n+\techo \"$1\" >a.h &&\n+\techo \"$1\" >b.c &&\n+\techo \"$1\" >c/Makefile &&\n+\techo \"$1\" >d.txt &&\n+\tgit add a.h b.c c/Makefile d.txt &&\n+\tgit commit -m\"$1\"\n+\treturn $?\n+}\n+\n+test_expect_success \"setup\" '\n+\tmkdir c &&\n+\tcreate_files 1 &&\n+\tcreate_files 2\n+'\n+\n+cat >order_file_1 <order_file_2 <expect_diff_headers_none <expect_diff_headers_1 <expect_diff_headers_2 <patch &&\n+\tgrep ^diff patch >actual_diff_headers &&\n+\ttest_cmp expect_diff_headers_none actual_diff_headers\n+'\n+\n+for i in 1 2; do\n+\ttest_expect_success \"orderfile using option ($i)\" \"\n+\tgit diff -Oorder_file_$i HEAD^..HEAD >patch &&\n+\tgrep ^diff patch >actual_diff_headers &&\n+\ttest_cmp expect_diff_headers_$i actual_diff_headers\n+\"\n+done\n+\n+for i in 1 2; do\n+\ttest_expect_success \"orderfile using config ($i)\" \"\n+\tgit -c diff.orderfile=order_file_$i diff HEAD^..HEAD >patch &&\n+\tgrep ^diff patch >actual_diff_headers &&\n+\ttest_cmp expect_diff_headers_$i actual_diff_headers\n+\"\n+done\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"231665","messageId":"xmqqhaal3l3x.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1386312508-7421-1-git-send-email-naesten@gmail.com","subject":"Re: [PATCH v2] diff: Add diff.orderfile configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-06T18:11:46Z","receivedAt":"2013-12-06T18:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> From: Anders Waldenborg \n>\n> diff.orderfile acts as a default for the -O command line option.\n>\n> [sb: fixed testcases & revised docs based on Jonathan Nieder's suggestions]\n>\n> Signed-off-by: Anders Waldenborg \n> Thanks-to: Jonathan Nieder \n> Signed-off-by: Samuel Bronson \n\nThanks for reviving a stalled topic.\n\n> ---\n> *I* even verified that the tests do fail properly when the feature is\n> sabotaged.\n\nSabotaged in what way?\n\n> Documentation/diff-config.txt | 5 +++\n> Documentation/diff-options.txt | 2 ++\n> diff.c | 5 +++\n> t/t4056-diff-order.sh | 79 ++++++++++++++++++++++++++++++++++++++++++\n> 4 files changed, 91 insertions(+)\n> create mode 100755 t/t4056-diff-order.sh\n>\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 223b931..f07b451 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -98,6 +98,11 @@ diff.mnemonicprefix::\n> diff.noprefix::\n> \tIf set, 'git diff' does not show any source or destination prefix.\n> \n> +diff.orderfile::\n> +\tFile indicating how to order files within a diff, using\n> +\tone shell glob pattern per line.\n> +\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n> +\n> diff.renameLimit::\n> \tThe number of files to consider when performing the copy/rename\n> \tdetection; equivalent to the 'git diff' option '-l'.\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index bbed2cd..1af5a5e 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -432,6 +432,8 @@ endif::git-format-patch[]\n> -O::\n> \tOutput the patch in the order specified in the\n> \t, which has one shell glob pattern per line.\n> +\tThis overrides the `diff.orderfile' configuration variable\n> +\t((see linkgit:git-config[1]).\n\nDouble opening parenthesis?\n\nIf somebody has diff.orderfile configuration that points at a custom\nordering, and wants to send out a patch (or show a diff) with the\nstandard order, how would the \"overriding\" command line look like?\nWould it be \"git diff -O/dev/null\"?\n\n> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> new file mode 100755\n> index 0000000..a756b34\n> --- /dev/null\n> +++ b/t/t4056-diff-order.sh\n> @@ -0,0 +1,79 @@\n> +#!/bin/sh\n> +\n> +test_description='diff order'\n> +\n> +. ./test-lib.sh\n> +\n> +create_files () {\n> +\techo \"$1\" >a.h &&\n> +\techo \"$1\" >b.c &&\n> +\techo \"$1\" >c/Makefile &&\n> +\techo \"$1\" >d.txt &&\n> +\tgit add a.h b.c c/Makefile d.txt &&\n> +\tgit commit -m\"$1\"\n> +\treturn $?\n> +}\n\nThat return looks somewhat strange. Does it even need to be there?\n\n> +test_expect_success \"setup\" '\n\nMakes readers wonder why dq is used here, I think.\n\n> +\tmkdir c &&\n> +\tcreate_files 1 &&\n> +\tcreate_files 2\n> +'\n> +\n> +cat >order_file_1 < +*Makefile\n> +*.txt\n> +*.h\n> +*\n> +EOF\n> +cat >order_file_2 < +*Makefile\n> +*.h\n> +*.c\n> +*\n> +EOF\n> +\n> +cat >expect_diff_headers_none < +diff --git a/a.h b/a.h\n> +diff --git a/b.c b/b.c\n> +diff --git a/c/Makefile b/c/Makefile\n> +diff --git a/d.txt b/d.txt\n> +EOF\n> +\n> +cat >expect_diff_headers_1 < +diff --git a/c/Makefile b/c/Makefile\n> +diff --git a/d.txt b/d.txt\n> +diff --git a/a.h b/a.h\n> +diff --git a/b.c b/b.c\n> +EOF\n> +\n> +cat >expect_diff_headers_2 < +diff --git a/c/Makefile b/c/Makefile\n> +diff --git a/a.h b/a.h\n> +diff --git a/b.c b/b.c\n> +diff --git a/d.txt b/d.txt\n> +EOF\n\nAll of these \"cat\" outside the test_expect_* are better be inside\nthe 'setup' section, I think. I.e.\n\n\ttest_expect_success setup '\n \tmkdir c &&\n create_files 1 &&\n create_files 2 &&\n cat >order_file_1 <<-\\EOF &&\n *Makefile\n *.txt\n *.h\n *\n EOF\n cat >order_file_2 <<-\\EOF &&\n\t\t...\n\t\tcat >expect_diff_headers_2 < +test_expect_success \"no order (=tree object order)\" '\n> +\tgit diff HEAD^..HEAD >patch &&\n> +\tgrep ^diff patch >actual_diff_headers &&\n> +\ttest_cmp expect_diff_headers_none actual_diff_headers\n> +'\n\nInstead of grepping, \"git diff --name-only\" would be far easier to\ncheck, no?\n\n> +for i in 1 2; do\n> +\ttest_expect_success \"orderfile using option ($i)\" \"\n> +\tgit diff -Oorder_file_$i HEAD^..HEAD >patch &&\n> +\tgrep ^diff patch >actual_diff_headers &&\n> +\ttest_cmp expect_diff_headers_$i actual_diff_headers\n> +\"\n> +done\n> +for i in 1 2; do\n> +\ttest_expect_success \"orderfile using config ($i)\" \"\n> +\tgit -c diff.orderfile=order_file_$i diff HEAD^..HEAD >patch &&\n> +\tgrep ^diff patch >actual_diff_headers &&\n> +\ttest_cmp expect_diff_headers_$i actual_diff_headers\n> +\"\n> +done\n\nI'd probably write the above like so:\n\n\tfor i in 1 2\n do\n\t\ttest_expect_success \"orderfile using option ($i)\" '\n \tgit diff -Oorder_file_$i --name-only HEAD^ >actual &&\n\t\t\ttest_cmp expect_$i actual\n\t\t'\n\t\ttest_expect_success \"orderfile using config ($i)\" '\n\t\t\ttest_config diff.orderfile order_file_$i &&\n \tgit diff --name-only HEAD^ >actual &&\n\t\t\ttest_cmp expect_$i actual\n\t\t'\n\tdone\n\nPoints to note:\n\n * We eval the scriptlets inside test framework, so using $i as a\n variable inside the single quotes will have the expected result.\n You do not have to worry about extra quoting inside dq pair.\n\n * We do _not_ substitute variables in the test title (perhaps we\n should have designed the test framework to do so, in hindsight),\n so unfortunately the title need to be in dq.\n\n * Use line-breaks instead of semicolons when writing compound\n syntax structures such as \"for/do/done\", \"if/then/elif/else/fi\",\n etc.\n"},{"id":"231692","messageId":"CAJYzjmdg8v6-kZ+xtD9GT=vVTs7AEX_iEoroxdi4F4rjoTogWw@mail.gmail.com","threadId":"35172","inReplyTo":"xmqqhaal3l3x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-07T02:43:22Z","receivedAt":"2013-12-07T02:43:22Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n\n> Thanks for reviving a stalled topic.\n\nI was asking about such a feature in #git and jrnieder was nice enough\nto point me at the stalled patch.\n\n>> *I* even verified that the tests do fail properly when the feature is\n>> sabotaged.\n>\n> Sabotaged in what way?\n\nI commented out the \"options->orderfile = diff_order_file_cfg;\" line.\n\n>> @@ -432,6 +432,8 @@ endif::git-format-patch[]\n>> -O::\n>> Output the patch in the order specified in the\n>> , which has one shell glob pattern per line.\n>> + This overrides the `diff.orderfile' configuration variable\n>> + ((see linkgit:git-config[1]).\n>\n> Double opening parenthesis?\n\nOops, and it looks like I messed up the quoting on diff.orderfile too ...\n\n> If somebody has diff.orderfile configuration that points at a custom\n> ordering, and wants to send out a patch (or show a diff) with the\n> standard order, how would the \"overriding\" command line look like?\n> Would it be \"git diff -O/dev/null\"?\n\nIt looks like that works ... and so do files that don't exist. What\ndo you think should happen with -O file-that-does-not-exist, and how\ndo you suppose it should be tested?\n\nAfter having fixed this, will /dev/null still work everywhere, or will\nwe want a new diff flag to unset the option? (I see that \"git diff\n/dev/null some-file\" works fine with msysgit, which doesn't seem to\nactually be linked with MSYS, but I don't know *why* it works, and I\ndon't know what other non-POSIXoid ports exist.)\n\nFor the moment, I've added this to \"for\" loop (after some changes\nbased on some of your other suggestions):\n\n # I don't think this should just pretend the orderfile was empty?\n test_expect_failure \"override with bogus orderfile ($i)\" '\n test_might_fail git -c diff.orderfile=order_file_$i diff\n-Obogus_file --name-only HEAD^..HEAD >actual_diff_filenames &&\n ! test_cmp expect_diff_filenames_none actual_diff_filenames\n'\n\nDoes this look (modulo gmail's stupid indentation) anything like a\nreasonable approach to testing that? (Of course, you can't actually\ntest it because it depends on other changes I haven't posted yet ...)\n\nAlso, I'm starting to wonder if I shouldn't split this into two patches:\n\n 1. diff: Add tests for -O flag\n 2. diff: Add diff.orderfile configuration variable\n\n(If so, I would obviously want to rewrite the above test to avoid the\nconfiguration option.)\n\n>> + return $?\n>> +}\n>\n> That return looks somewhat strange. Does it even need to be there?\n\nI'm certainly no great expert at shell functions, so I expect it\nisn't. I'm not really sure what possessed me to think it might be\nneeded.\n\n> EOF\n> cat >order_file_2 <<-\\EOF &&\n\nI'd kind of prefer to keep a blank line between one EOF and the next\ncat, if that's okay with you.\n\n>\n> Quoting the EOF like the above will help the readers by signaling\n> them that they do not have to wonder if there is some substitution\n> going on in the here text.\n\nPerhaps, but probably only after they've scrutinized their shell\nmanuals to figure out what the - and the \\ are for. (I had to check\ntwo: dash(1) wasn't clear enough for me about the quoting ...)\n\n>> +test_expect_success \"no order (=tree object order)\" '\n>> + git diff HEAD^..HEAD >patch &&\n>> + grep ^diff patch >actual_diff_headers &&\n>> + test_cmp expect_diff_headers_none actual_diff_headers\n>> +'\n>\n> Instead of grepping, \"git diff --name-only\" would be far easier to\n> check, no?\n\nIt certainly makes for less-cluttered expected output. (I guess\njrnieder didn't know about that trick when he suggested using the\nintermediate file?)\n\n> Points to note:\n>\n> * We eval the scriptlets inside test framework, so using $i as a\n> variable inside the single quotes will have the expected result.\n> You do not have to worry about extra quoting inside dq pair.\n\nHmm. I'm obviously not used to things getting eval'd in the same\nshell instance as my script ...\n\n(Thanks for the review!)\n"},{"id":"231800","messageId":"xmqqiouxzv52.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"CAJYzjmdg8v6-kZ+xtD9GT=vVTs7AEX_iEoroxdi4F4rjoTogWw@mail.gmail.com","subject":"Re: [PATCH v2] diff: Add diff.orderfile configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T19:23:05Z","receivedAt":"2013-12-09T19:23:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n>> If somebody has diff.orderfile configuration that points at a custom\n>> ordering, and wants to send out a patch (or show a diff) with the\n>> standard order, how would the \"overriding\" command line look like?\n>> Would it be \"git diff -O/dev/null\"?\n>\n> It looks like that works ... and so do files that don't exist. What\n> do you think should happen with -O file-that-does-not-exist, and how\n> do you suppose it should be tested?\n\nI think the original code is too loose on diagnosing errors.\nPerhaps something like this is needed.\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex 23e9385..dd103e3 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -20,8 +20,11 @@ static void prepare_order(const char *orderfile)\n \t\treturn;\n \n \tfd = open(orderfile, O_RDONLY);\n-\tif (fd < 0)\n+\tif (fd < 0) {\n+\t\tif (errno != ENOENT || errno != ENOTDIR)\n+\t\t\tdie(_(\"orderfile '%s' does not exist.\"));\n \t\treturn;\n+\t}\n \tif (fstat(fd, &st)) {\n \t\tclose(fd);\n \t\treturn;\n\n> After having fixed this, will /dev/null still work everywhere, or will\n> we want a new diff flag to unset the option? (I see that \"git diff\n> /dev/null some-file\" works fine with msysgit, which doesn't seem to\n> actually be linked with MSYS, but I don't know *why* it works, and I\n> don't know what other non-POSIXoid ports exist.)\n\nI *think* it should be OK to use \"-O/dev/null\" for that purpose, but\nthe primary thing I was hinting at with the rhetoric question was\nthat it probably needs to be documented there.\n\n> Also, I'm starting to wonder if I shouldn't split this into two patches:\n>\n> 1. diff: Add tests for -O flag\n> 2. diff: Add diff.orderfile configuration variable\n>\n> (If so, I would obviously want to rewrite the above test to avoid the\n> configuration option.)\n\nSurely, and thanks.\n\n>> EOF\n>> cat >order_file_2 <<-\\EOF &&\n>\n> I'd kind of prefer to keep a blank line between one EOF and the next\n> cat, if that's okay with you.\n\nAlright. Making it easier to spot the grouping, it would make it\neasier to read.\n\n>> Quoting the EOF like the above will help the readers by signaling\n>> them that they do not have to wonder if there is some substitution\n>> going on in the here text.\n>\n> Perhaps, but probably only after they've scrutinized their shell\n> manuals to figure out what the - and the \\ are for. (I had to check\n> two: dash(1) wasn't clear enough for me about the quoting ...)\n\nYes and no. The no primarily comes from that nobody will stay to be\nnovice forever.\n"},{"id":"232033","messageId":"1387059521-23616-1-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","subject":"[PATCH v3 0/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-14T22:18:38Z","receivedAt":"2013-12-14T22:18:38Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"The original purpose of this patch [series] was to allow specifying\nthe \"-O\" option for \"git diff\" in the config, but I need help with the\nrelative path handling [RFC 3].\n\nIt also added tests for \"git diff -O\", which I have split out because\nthey are independantly useful [PATCH 1].\n\nI also noticed that -O had terrible error handling and could only read\nmmappable files, so I fixed that [PATCH 2].\n\nSamuel Bronson (3):\n diff: Tests for \"git diff -O\"\n diff: Let \"git diff -O\" read orderfile from any file, failing when\n appropriate\n diff: Add diff.orderfile configuration variable\n\n Documentation/diff-config.txt | 5 ++\n Documentation/diff-options.txt | 3 ++\n diff.c | 5 ++\n diffcore-order.c | 23 ++++-----\n t/t4056-diff-order.sh | 105 +++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 126 insertions(+), 15 deletions(-)\n create mode 100755 t/t4056-diff-order.sh\n\n-- \n1.8.4.3\n"},{"id":"232034","messageId":"1387059521-23616-2-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387059521-23616-1-git-send-email-naesten@gmail.com","subject":"[PATCH v3 1/3] diff: Tests for \"git diff -O\"","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-14T22:18:39Z","receivedAt":"2013-12-14T22:18:39Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Heavily adapted from Anders' patch:\n\"diff: Add diff.orderfile configuration variable\"\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..398b3f6\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,72 @@\n+#!/bin/sh\n+\n+test_description='diff order'\n+\n+. ./test-lib.sh\n+\n+create_files () {\n+\techo \"$1\" >a.h &&\n+\techo \"$1\" >b.c &&\n+\techo \"$1\" >c/Makefile &&\n+\techo \"$1\" >d.txt &&\n+\tgit add a.h b.c c/Makefile d.txt &&\n+\tgit commit -m\"$1\"\n+}\n+\n+test_expect_success 'setup' '\n+\tmkdir c &&\n+\tcreate_files 1 &&\n+\tcreate_files 2 &&\n+\n+\tcat >order_file_1 <<-\\EOF &&\n+\t*Makefile\n+\t*.txt\n+\t*.h\n+\t*\n+\tEOF\n+\n+\tcat >order_file_2 <<-\\EOF &&\n+\t*Makefile\n+\t*.h\n+\t*.c\n+\t*\n+\tEOF\n+\n+\tcat >expect_none <<-\\EOF &&\n+\ta.h\n+\tb.c\n+\tc/Makefile\n+\td.txt\n+\tEOF\n+\n+\tcat >expect_1 <<-\\EOF &&\n+\tc/Makefile\n+\td.txt\n+\ta.h\n+\tb.c\n+\tEOF\n+\n+\tcat >expect_2 <<-\\EOF &&\n+\tc/Makefile\n+\ta.h\n+\tb.c\n+\td.txt\n+\tEOF\n+\n+\ttrue\t# end chain of &&\n+'\n+\n+test_expect_success \"no order (=tree object order)\" '\n+\tgit diff --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_none actual\n+'\n+\n+for i in 1 2\n+do\n+\ttest_expect_success \"orderfile using option ($i)\" '\n+\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n+done\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"232035","messageId":"1387059521-23616-3-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387059521-23616-1-git-send-email-naesten@gmail.com","subject":"[PATCH v3 2/3] diff: Let \"git diff -O\" read orderfile from any file, failing when appropriate","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-14T22:18:40Z","receivedAt":"2013-12-14T22:18:40Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"The -O flag really shouldn't silently fail to do anything when given a\npath that it can't read from.\n\nHowever, it should be able to read from un-mappable files, such as\npipes/fifos, /dev/null (as we document in the next patch), or in fact\n*any* empty file (since Linux 2.6.12). (Especially since we will be\ndocumenting \"-O/dev/null\" to override \"diff.orderfile\" when we add that.)\n\n(Note: \"-O/dev/null\" did have the right effect, since the existing error\nhandling essentially worked out to \"silently ignore the orderfile\".)\n\nSo lets toss all of that logic to get the file mmapped and just use\nstrbuf_read_file() instead, which gives us decent error handling\npractically for free.\n\nSigned-off-by: Samuel Bronson \n---\n diffcore-order.c | 23 ++++++++---------------\n t/t4056-diff-order.sh | 23 +++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 15 deletions(-)\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex 23e9385..a63f332 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -10,28 +10,21 @@ static int order_cnt;\n \n static void prepare_order(const char *orderfile)\n {\n-\tint fd, cnt, pass;\n+\tint cnt, pass;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tvoid *map;\n \tchar *cp, *endp;\n-\tstruct stat st;\n-\tsize_t sz;\n+\tssize_t sz;\n \n \tif (order)\n \t\treturn;\n \n-\tfd = open(orderfile, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn;\n-\t}\n-\tsz = xsize_t(st.st_size);\n-\tmap = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tclose(fd);\n-\tif (map == MAP_FAILED)\n-\t\treturn;\n+\tsz = strbuf_read_file(&sb, orderfile, 0);\n+\tif (sz < 0)\n+\t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n+\tmap = strbuf_detach(&sb, NULL);\n \tendp = (char *) map + sz;\n+\n \tfor (pass = 0; pass < 2; pass++) {\n \t\tcnt = 0;\n \t\tcp = map;\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex 398b3f6..eb471e7 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -61,12 +61,35 @@ test_expect_success \"no order (=tree object order)\" '\n \ttest_cmp expect_none actual\n '\n \n+test_expect_success 'missing orderfile' '\n+\trm -f bogus_file &&\n+\ttest_must_fail git diff -Obogus_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success 'unreadable orderfile' '\n+\ttouch unreadable_file &&\n+\tchmod -r unreadable_file &&\n+\ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success 'orderfile is a directory' '\n+\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n+'\n+\n for i in 1 2\n do\n \ttest_expect_success \"orderfile using option ($i)\" '\n \tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n \ttest_cmp expect_$i actual\n '\n+\n+\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n+\trm -f order_fifo &&\n+\tmkfifo order_fifo &&\n+\tcat order_file_$i >order_fifo &\n+\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232036","messageId":"1387059521-23616-4-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387059521-23616-1-git-send-email-naesten@gmail.com","subject":"[RFC v3 3/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-14T22:18:41Z","receivedAt":"2013-12-14T22:18:41Z","isPatch":false,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"diff.orderfile acts as a default for the -O command line option.\n\n[sb: split up aw's original patch; reworked tests and docs]\n\n[FIXME: Relative paths should presumably be interpreted relative to\nrepository root; how should this be accomplished?]\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n Documentation/diff-config.txt | 5 +++++\n Documentation/diff-options.txt | 3 +++\n diff.c | 5 +++++\n t/t4056-diff-order.sh | 10 ++++++++++\n 4 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 223b931..f07b451 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -98,6 +98,11 @@ diff.mnemonicprefix::\n diff.noprefix::\n \tIf set, 'git diff' does not show any source or destination prefix.\n \n+diff.orderfile::\n+\tFile indicating how to order files within a diff, using\n+\tone shell glob pattern per line.\n+\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n+\n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n \tdetection; equivalent to the 'git diff' option '-l'.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex bbed2cd..9b37b2a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -432,6 +432,9 @@ endif::git-format-patch[]\n -O::\n \tOutput the patch in the order specified in the\n \t, which has one shell glob pattern per line.\n+\tThis overrides the `diff.orderfile` configuration variable\n+\t(see linkgit:git-config[1]). To cancel `diff.orderfile`,\n+\tuse `-O/dev/null`.\n \n ifndef::git-format-patch[]\n -R::\ndiff --git a/diff.c b/diff.c\nindex 3950e01..b336aef 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n+\tif (!strcmp(var, \"diff.orderfile\"))\n+\t\treturn git_config_string(&diff_order_file_cfg, var, value);\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \n+\toptions->orderfile = diff_order_file_cfg;\n+\n \tif (diff_no_prefix) {\n \t\toptions->a_prefix = options->b_prefix = \"\";\n \t} else if (!diff_mnemonic_prefix) {\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex eb471e7..50689d1 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -90,6 +90,16 @@ do\n \tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n \ttest_cmp expect_$i actual\n '\n+\n+\ttest_expect_success \"orderfile using config ($i)\" '\n+\tgit -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n+\n+\ttest_expect_success \"cancelling configured orderfile ($i)\" '\n+\tgit -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_none actual\n+'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232058","messageId":"xmqqk3f4prgc.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1387059521-23616-3-git-send-email-naesten@gmail.com","subject":"Re: [PATCH v3 2/3] diff: Let \"git diff -O\" read orderfile from any file, failing when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T18:43:15Z","receivedAt":"2013-12-16T18:43:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> The -O flag really shouldn't silently fail to do anything when given a\n> path that it can't read from.\n>\n> However, it should be able to read from un-mappable files, such as\n> pipes/fifos, /dev/null (as we document in the next patch), or in fact\n> *any* empty file (since Linux 2.6.12).\n\nCould you enlighten the commit log readers a bit better here? Those\nwho know the change in 2.6.12 (i.e. \"'mmapping with length 0 must\nfail', says SUSv3, so we fail\") you have in mind would know what you\nmean by \"in fact any empty file\" even if you did not have \"(since\nLinux 2.6.12)\", but those who do not know it would not be helped\nwith just \"(since Linux 2.6.12)\".\n\n> (Especially since we will be\n> documenting \"-O/dev/null\" to override \"diff.orderfile\" when we add that.)\n>\n> (Note: \"-O/dev/null\" did have the right effect, since the existing error\n> handling essentially worked out to \"silently ignore the orderfile\".)\n>\n> So lets toss all of that logic to get the file mmapped and just use\n> strbuf_read_file() instead, which gives us decent error handling\n> practically for free.\n\nSounds good. In the longer term, we may want to move this\nfile-scope static to per-infocation \"struct diff_options\" and clean\nup the storage used to hold the list of path patterns after we are\ndone with the diff, but that is outside the scope of this series.\n\nThanks.\n\n> Signed-off-by: Samuel Bronson \n> ---\n> diffcore-order.c | 23 ++++++++---------------\n> t/t4056-diff-order.sh | 23 +++++++++++++++++++++++\n> 2 files changed, 31 insertions(+), 15 deletions(-)\n>\n> diff --git a/diffcore-order.c b/diffcore-order.c\n> index 23e9385..a63f332 100644\n> --- a/diffcore-order.c\n> +++ b/diffcore-order.c\n> @@ -10,28 +10,21 @@ static int order_cnt;\n> \n> static void prepare_order(const char *orderfile)\n> {\n> -\tint fd, cnt, pass;\n> +\tint cnt, pass;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> \tvoid *map;\n> \tchar *cp, *endp;\n> -\tstruct stat st;\n> -\tsize_t sz;\n> +\tssize_t sz;\n> \n> \tif (order)\n> \t\treturn;\n> \n> -\tfd = open(orderfile, O_RDONLY);\n> -\tif (fd < 0)\n> -\t\treturn;\n> -\tif (fstat(fd, &st)) {\n> -\t\tclose(fd);\n> -\t\treturn;\n> -\t}\n> -\tsz = xsize_t(st.st_size);\n> -\tmap = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n> -\tclose(fd);\n> -\tif (map == MAP_FAILED)\n> -\t\treturn;\n> +\tsz = strbuf_read_file(&sb, orderfile, 0);\n> +\tif (sz < 0)\n> +\t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n> +\tmap = strbuf_detach(&sb, NULL);\n> \tendp = (char *) map + sz;\n> +\n> \tfor (pass = 0; pass < 2; pass++) {\n> \t\tcnt = 0;\n> \t\tcp = map;\n> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> index 398b3f6..eb471e7 100755\n> --- a/t/t4056-diff-order.sh\n> +++ b/t/t4056-diff-order.sh\n> @@ -61,12 +61,35 @@ test_expect_success \"no order (=tree object order)\" '\n> \ttest_cmp expect_none actual\n> '\n> \n> +test_expect_success 'missing orderfile' '\n> +\trm -f bogus_file &&\n> +\ttest_must_fail git diff -Obogus_file --name-only HEAD^..HEAD\n> +'\n> +\n> +test_expect_success 'unreadable orderfile' '\n> +\ttouch unreadable_file &&\n> +\tchmod -r unreadable_file &&\n> +\ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n> +'\n> +\n> +test_expect_success 'orderfile is a directory' '\n> +\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n> +'\n> +\n> for i in 1 2\n> do\n> \ttest_expect_success \"orderfile using option ($i)\" '\n> \tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n> \ttest_cmp expect_$i actual\n> '\n> +\n> +\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n> +\trm -f order_fifo &&\n> +\tmkfifo order_fifo &&\n> +\tcat order_file_$i >order_fifo &\n> +\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n> +\ttest_cmp expect_$i actual\n> +'\n> done\n> \n> test_done\n"},{"id":"232059","messageId":"xmqqfvpspqyj.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1387059521-23616-4-git-send-email-naesten@gmail.com","subject":"Re: [RFC v3 3/3] diff: Add diff.orderfile configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T18:53:56Z","receivedAt":"2013-12-16T18:53:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> diff.orderfile acts as a default for the -O command line option.\n>\n> [sb: split up aw's original patch; reworked tests and docs]\n>\n> [FIXME: Relative paths should presumably be interpreted relative to\n> repository root; how should this be accomplished?]\n\nDo you mean something like this?\n\n $ cd docs\n $ edit orderfile\n $ git diff -Oordefile\n $ cd subdir\n $ git diff -O../orderfile\n\nPath-like parameters and values given by the end user should be\nrelative to the directory where the end user is (i.e. both -O\nparameters in the above example name docs/orderfile). All Git\nprocesses, even the ones that are capable of being run from a\nsubdirectory, are supposed to first chdir to the top level of the\nworking tree before doing anything else, and adjust the path-like\nthings they get from the end user from the command line accordingly.\nBy the time diffcore_order() to prepare_order() callchain is called,\nwe certainly should have passed that chdir already, so the value of\nthe option needs to be prepended with the \"prefix\" when parsed.\n\nThe value specified for the diff.orderfile configuration can just be\na path relative to the top level of the working tree, I think.\n"},{"id":"232066","messageId":"CAJYzjmcxswLUw3wU6TO_s_vFXYM1mu4HCXZ=8ksWELxXCSt4cg@mail.gmail.com","threadId":"35172","inReplyTo":"xmqqfvpspqyj.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC v3 3/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-16T19:21:13Z","receivedAt":"2013-12-16T19:21:13Z","isPatch":false,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Mon, Dec 16, 2013 at 1:53 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n\n> Path-like parameters and values given by the end user should be\n> relative to the directory where the end user is (i.e. both -O\n> parameters in the above example name docs/orderfile). All Git\n> processes, even the ones that are capable of being run from a\n> subdirectory, are supposed to first chdir to the top level of the\n> working tree before doing anything else, and adjust the path-like\n> things they get from the end user from the command line accordingly.\n> By the time diffcore_order() to prepare_order() callchain is called,\n> we certainly should have passed that chdir already, so the value of\n> the option needs to be prepended with the \"prefix\" when parsed.\n>\n> The value specified for the diff.orderfile configuration can just be\n> a path relative to the top level of the working tree, I think.\n\nOh, cool. So I'll just change the git_config_string() call to use\ngit_config_pathname(), since the user might easily want to use ~\nnotation there, especially in a user-level setting ...\n"},{"id":"232075","messageId":"1387224586-10169-1-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","subject":"[PATCH v4 0/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-16T20:09:43Z","receivedAt":"2013-12-16T20:09:43Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"The original purpose of this patch [series] was to allow specifying\nthe \"-O\" option for \"git diff\" in the config.\n\nIn this version, I've revised the commit message for patch 2, changed\npatch 3 to use git_config_pathname() instead of git_config_string(),\nand removed the FIXME from patch 3's commit message.\n\nSamuel Bronson (3):\n diff: Tests for \"git diff -O\"\n diff: Let \"git diff -O\" read orderfile from any file, fail properly\n diff: Add diff.orderfile configuration variable\n\n Documentation/diff-config.txt | 5 ++\n Documentation/diff-options.txt | 3 ++\n diff.c | 5 ++\n diffcore-order.c | 23 ++++-----\n t/t4056-diff-order.sh | 105 +++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 126 insertions(+), 15 deletions(-)\n create mode 100755 t/t4056-diff-order.sh\n\n-- \n1.8.4.3\n"},{"id":"232076","messageId":"1387224586-10169-2-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387224586-10169-1-git-send-email-naesten@gmail.com","subject":"[PATCH v4 1/3] diff: Tests for \"git diff -O\"","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-16T20:09:44Z","receivedAt":"2013-12-16T20:09:44Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Heavily adapted from Anders' patch:\n\"diff: Add diff.orderfile configuration variable\"\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..398b3f6\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,72 @@\n+#!/bin/sh\n+\n+test_description='diff order'\n+\n+. ./test-lib.sh\n+\n+create_files () {\n+\techo \"$1\" >a.h &&\n+\techo \"$1\" >b.c &&\n+\techo \"$1\" >c/Makefile &&\n+\techo \"$1\" >d.txt &&\n+\tgit add a.h b.c c/Makefile d.txt &&\n+\tgit commit -m\"$1\"\n+}\n+\n+test_expect_success 'setup' '\n+\tmkdir c &&\n+\tcreate_files 1 &&\n+\tcreate_files 2 &&\n+\n+\tcat >order_file_1 <<-\\EOF &&\n+\t*Makefile\n+\t*.txt\n+\t*.h\n+\t*\n+\tEOF\n+\n+\tcat >order_file_2 <<-\\EOF &&\n+\t*Makefile\n+\t*.h\n+\t*.c\n+\t*\n+\tEOF\n+\n+\tcat >expect_none <<-\\EOF &&\n+\ta.h\n+\tb.c\n+\tc/Makefile\n+\td.txt\n+\tEOF\n+\n+\tcat >expect_1 <<-\\EOF &&\n+\tc/Makefile\n+\td.txt\n+\ta.h\n+\tb.c\n+\tEOF\n+\n+\tcat >expect_2 <<-\\EOF &&\n+\tc/Makefile\n+\ta.h\n+\tb.c\n+\td.txt\n+\tEOF\n+\n+\ttrue\t# end chain of &&\n+'\n+\n+test_expect_success \"no order (=tree object order)\" '\n+\tgit diff --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_none actual\n+'\n+\n+for i in 1 2\n+do\n+\ttest_expect_success \"orderfile using option ($i)\" '\n+\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n+done\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"232078","messageId":"1387224586-10169-3-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387224586-10169-1-git-send-email-naesten@gmail.com","subject":"[PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-16T20:09:45Z","receivedAt":"2013-12-16T20:09:45Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"The -O flag really shouldn't silently fail to do anything when given a\npath that it can't read from.\n\nHowever, it should be able to read from un-mmappable files, such as:\n\n * pipes/fifos\n\n * /dev/null: It's a character device (at least on Linux)\n\n * ANY empty file:\n\n Quoting Linux mmap(2), \"SUSv3 specifies that mmap() should fail if\n length is 0. However, in kernels before 2.6.12, mmap() succeeded in\n this case: no mapping was created and the call returned addr. Since\n kernel 2.6.12, mmap() fails with the error EINVAL for this case.\"\n\nWe especially want \"-O/dev/null\" to work, since we will be documenting\nit as the way to cancel \"diff.orderfile\" when we add that.\n\n(Note: \"-O/dev/null\" did have the right effect, since the existing error\nhandling essentially worked out to \"silently ignore the orderfile\". But\nthis was probably more coincidence than anything else.)\n\nSo, lets toss all of that logic to get the file mmapped and just use\nstrbuf_read_file() instead, which gives us decent error handling\npractically for free.\n\nSigned-off-by: Samuel Bronson \n---\n diffcore-order.c | 23 ++++++++---------------\n t/t4056-diff-order.sh | 23 +++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 15 deletions(-)\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex 23e9385..a63f332 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -10,28 +10,21 @@ static int order_cnt;\n \n static void prepare_order(const char *orderfile)\n {\n-\tint fd, cnt, pass;\n+\tint cnt, pass;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tvoid *map;\n \tchar *cp, *endp;\n-\tstruct stat st;\n-\tsize_t sz;\n+\tssize_t sz;\n \n \tif (order)\n \t\treturn;\n \n-\tfd = open(orderfile, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn;\n-\t}\n-\tsz = xsize_t(st.st_size);\n-\tmap = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tclose(fd);\n-\tif (map == MAP_FAILED)\n-\t\treturn;\n+\tsz = strbuf_read_file(&sb, orderfile, 0);\n+\tif (sz < 0)\n+\t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n+\tmap = strbuf_detach(&sb, NULL);\n \tendp = (char *) map + sz;\n+\n \tfor (pass = 0; pass < 2; pass++) {\n \t\tcnt = 0;\n \t\tcp = map;\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex 398b3f6..eb471e7 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -61,12 +61,35 @@ test_expect_success \"no order (=tree object order)\" '\n \ttest_cmp expect_none actual\n '\n \n+test_expect_success 'missing orderfile' '\n+\trm -f bogus_file &&\n+\ttest_must_fail git diff -Obogus_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success 'unreadable orderfile' '\n+\ttouch unreadable_file &&\n+\tchmod -r unreadable_file &&\n+\ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success 'orderfile is a directory' '\n+\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n+'\n+\n for i in 1 2\n do\n \ttest_expect_success \"orderfile using option ($i)\" '\n \tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n \ttest_cmp expect_$i actual\n '\n+\n+\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n+\trm -f order_fifo &&\n+\tmkfifo order_fifo &&\n+\tcat order_file_$i >order_fifo &\n+\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232077","messageId":"1387224586-10169-4-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387224586-10169-1-git-send-email-naesten@gmail.com","subject":"[PATCH v4 3/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-16T20:09:46Z","receivedAt":"2013-12-16T20:09:46Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"diff.orderfile acts as a default for the -O command line option.\n\n[sb: split up aw's original patch; reworked tests and docs,\ntreat option as pathname]\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n Documentation/diff-config.txt | 5 +++++\n Documentation/diff-options.txt | 3 +++\n diff.c | 5 +++++\n t/t4056-diff-order.sh | 10 ++++++++++\n 4 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 223b931..f07b451 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -98,6 +98,11 @@ diff.mnemonicprefix::\n diff.noprefix::\n \tIf set, 'git diff' does not show any source or destination prefix.\n \n+diff.orderfile::\n+\tFile indicating how to order files within a diff, using\n+\tone shell glob pattern per line.\n+\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n+\n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n \tdetection; equivalent to the 'git diff' option '-l'.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex bbed2cd..9b37b2a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -432,6 +432,9 @@ endif::git-format-patch[]\n -O::\n \tOutput the patch in the order specified in the\n \t, which has one shell glob pattern per line.\n+\tThis overrides the `diff.orderfile` configuration variable\n+\t(see linkgit:git-config[1]). To cancel `diff.orderfile`,\n+\tuse `-O/dev/null`.\n \n ifndef::git-format-patch[]\n -R::\ndiff --git a/diff.c b/diff.c\nindex 3950e01..0099b99 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n+\tif (!strcmp(var, \"diff.orderfile\"))\n+\t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \n+\toptions->orderfile = diff_order_file_cfg;\n+\n \tif (diff_no_prefix) {\n \t\toptions->a_prefix = options->b_prefix = \"\";\n \t} else if (!diff_mnemonic_prefix) {\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex eb471e7..50689d1 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -90,6 +90,16 @@ do\n \tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n \ttest_cmp expect_$i actual\n '\n+\n+\ttest_expect_success \"orderfile using config ($i)\" '\n+\tgit -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_$i actual\n+'\n+\n+\ttest_expect_success \"cancelling configured orderfile ($i)\" '\n+\tgit -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_none actual\n+'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232084","messageId":"xmqq4n68o647.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1387224586-10169-3-git-send-email-naesten@gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T21:09:28Z","receivedAt":"2013-12-16T21:09:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> index 398b3f6..eb471e7 100755\n> --- a/t/t4056-diff-order.sh\n> +++ b/t/t4056-diff-order.sh\n> @@ -61,12 +61,35 @@ test_expect_success \"no order (=tree object order)\" '\n> \ttest_cmp expect_none actual\n> '\n> \n> +test_expect_success 'missing orderfile' '\n> +\trm -f bogus_file &&\n> +\ttest_must_fail git diff -Obogus_file --name-only HEAD^..HEAD\n> +'\n> +\n> +test_expect_success 'unreadable orderfile' '\n> +\ttouch unreadable_file &&\n> +\tchmod -r unreadable_file &&\n\nTwo points:\n\n - Unless your primary interest is to change the file timestamp, do\n not use \"touch\"; using \">unreadable_file\" or something instead\n would tell the readers that you only want to make sure it exists\n and do not care about the file timestamp.\n\n - this test probably needs restricted to people with sane\n filesystems; I think POSIXPERM prerequisite and also SANITY\n prerequisite are needed, at least.\n\n> +\ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n> +'\n> +\n> +test_expect_success 'orderfile is a directory' '\n> +\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n> +'\n> +\n> for i in 1 2\n> do\n> \ttest_expect_success \"orderfile using option ($i)\" '\n> \tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n> \ttest_cmp expect_$i actual\n> '\n> +\n> +\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n> +\trm -f order_fifo &&\n> +\tmkfifo order_fifo &&\n> +\tcat order_file_$i >order_fifo &\n> +\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n> +\ttest_cmp expect_$i actual\n> +'\n> done\n> \n> test_done\n"},{"id":"232085","messageId":"xmqqwqj4mqhe.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1387224586-10169-3-git-send-email-naesten@gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T21:32:29Z","receivedAt":"2013-12-16T21:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> for i in 1 2\n> do\n> \ttest_expect_success \"orderfile using option ($i)\" '\n> \tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n> \ttest_cmp expect_$i actual\n> '\n\nThis funny indentation in the previous step needs to be fixed, and\nthe added block below should match.\n\n> +\n> +\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n> +\trm -f order_fifo &&\n\n> +\tmkfifo order_fifo &&\n> +\tcat order_file_$i >order_fifo &\n> +\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n\nI think this part can be racy depending on which between cat and\n\"git diff\" are scheduled first, no? Try running this test under\nload and I think you will see it deadlocked.\n\nBesides, the above breaks && chain; even if mkfifo breaks (hence not\nallowing cat to run), \"git diff\" will go ahead and run, no?\n\n> +\ttest_cmp expect_$i actual\n> +'\n> done\n> \n> test_done\n"},{"id":"232089","messageId":"CAJYzjmcAXiMhKA1XKgnd6V1bto=VDCR6xwKaO6f+UAe-S7hGzA@mail.gmail.com","threadId":"35172","inReplyTo":"xmqq4n68o647.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-17T04:06:50Z","receivedAt":"2013-12-17T04:06:50Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Mon, Dec 16, 2013 at 4:09 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n\n>> +test_expect_success 'unreadable orderfile' '\n>> + touch unreadable_file &&\n>> + chmod -r unreadable_file &&\n\n> - this test probably needs restricted to people with sane\n> filesystems; I think POSIXPERM prerequisite and also SANITY\n> prerequisite are needed, at least.\n\nHmm, yeah, you've got a point; now that I think more carefully, the\nmost FAT can do is something like \"chmod -w\", nothing with the \"r\"\npermissions. Oops.\n"},{"id":"232091","messageId":"CAJYzjmd_EWcQ5OzuZBQwhkfAtdxbPbvhVxUSsh98SzMzyz=-8w@mail.gmail.com","threadId":"35172","inReplyTo":"xmqqwqj4mqhe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-17T05:03:29Z","receivedAt":"2013-12-17T05:03:29Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n>\n>> for i in 1 2\n>> do\n>> test_expect_success \"orderfile using option ($i)\" '\n>> git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n>> test_cmp expect_$i actual\n>> '\n>\n> This funny indentation in the previous step needs to be fixed, and\n> the added block below should match.\n\nEven though this results in oddly-indented --verbose output?\n\n>> + rm -f order_fifo &&\n>> + mkfifo order_fifo &&\n>> + cat order_file_$i >order_fifo &\n>> + git diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n>\n> I think this part can be racy depending on which between cat and\n> \"git diff\" are scheduled first, no? Try running this test under\n> load and I think you will see it deadlocked.\n>\n> Besides, the above breaks && chain; even if mkfifo breaks (hence not\n> allowing cat to run), \"git diff\" will go ahead and run, no?\n\nHmm. Well, what I really wanted to put here was a \"process substitution\":\n\n git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&\n\nbut I did not see this feature listed in the dash(1) manpage, so I\nassumed it wasn't allowed by POSIX. And, having looked, I indeed\ndon't see it mentioned in POSIX either.\n\nI'm not terribly surprised that I screwed up the translation to FIFOs;\nhow would I really want to do it?\n"},{"id":"232107","messageId":"xmqqsitrmkhe.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"CAJYzjmd_EWcQ5OzuZBQwhkfAtdxbPbvhVxUSsh98SzMzyz=-8w@mail.gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-17T17:54:21Z","receivedAt":"2013-12-17T17:54:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano wrote:\n>> Samuel Bronson writes:\n>>\n>>> for i in 1 2\n>>> do\n>>> test_expect_success \"orderfile using option ($i)\" '\n>>> git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n>>> test_cmp expect_$i actual\n>>> '\n>>\n>> This funny indentation in the previous step needs to be fixed, and\n>> the added block below should match.\n>\n> Even though this results in oddly-indented --verbose output?\n>\n>>> + rm -f order_fifo &&\n>>> + mkfifo order_fifo &&\n>>> + cat order_file_$i >order_fifo &\n>>> + git diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n>>\n>> I think this part can be racy depending on which between cat and\n>> \"git diff\" are scheduled first, no? Try running this test under\n>> load and I think you will see it deadlocked.\n>>\n>> Besides, the above breaks && chain; even if mkfifo breaks (hence not\n>> allowing cat to run), \"git diff\" will go ahead and run, no?\n>\n> Hmm. Well, what I really wanted to put here was a \"process substitution\":\n>\n> git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&\n>\n> but I did not see this feature listed in the dash(1) manpage, so I\n> assumed it wasn't allowed by POSIX. And, having looked, I indeed\n> don't see it mentioned in POSIX either.\n>\n> I'm not terribly surprised that I screwed up the translation to FIFOs;\n> how would I really want to do it?\n\nHow about not doing a fifo?\n"},{"id":"232117","messageId":"CALWbr2zXNF-aJHHnBnW1q1yaCmWt-rmMWypBWFanTBAK1pMWiQ@mail.gmail.com","threadId":"35172","inReplyTo":"xmqqsitrmkhe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-12-17T20:37:26Z","receivedAt":"2013-12-17T20:37:26Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Tue, Dec 17, 2013 at 6:54 PM, Junio C Hamano wrote:\n> Samuel Bronson writes:\n>\n>> On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano wrote:\n>>> Samuel Bronson writes:\n>>>\n>>>> for i in 1 2\n>>>> do\n>>>> test_expect_success \"orderfile using option ($i)\" '\n>>>> git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n>>>> test_cmp expect_$i actual\n>>>> '\n>>>\n>>> This funny indentation in the previous step needs to be fixed, and\n>>> the added block below should match.\n>>\n>> Even though this results in oddly-indented --verbose output?\n>>\n>>>> + rm -f order_fifo &&\n>>>> + mkfifo order_fifo &&\n>>>> + cat order_file_$i >order_fifo &\n>>>> + git diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n>>>\n>>> I think this part can be racy depending on which between cat and\n>>> \"git diff\" are scheduled first, no? Try running this test under\n>>> load and I think you will see it deadlocked.\n>>>\n>>> Besides, the above breaks && chain; even if mkfifo breaks (hence not\n>>> allowing cat to run), \"git diff\" will go ahead and run, no?\n>>\n>> Hmm. Well, what I really wanted to put here was a \"process substitution\":\n>>\n>> git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&\n>>\n>> but I did not see this feature listed in the dash(1) manpage, so I\n>> assumed it wasn't allowed by POSIX. And, having looked, I indeed\n>> don't see it mentioned in POSIX either.\n>>\n>> I'm not terribly surprised that I screwed up the translation to FIFOs;\n>> how would I really want to do it?\n>\n> How about not doing a fifo?\n\nThat would certainly defeat the purpose of the test, which is to test\nagainst a fifo :-)\nI'm not sure about the deadlock though. Both read and write will wait\nfor each other to start operating on the fifo.\n\nYou can probably fix the &&-chain by doing something like:\n\n mkfifo order_fifo && {\n cat order_file_$i >order_fifo &\n git diff -O order_fifo --name-only HEAD^..HEAD >actual\n } && ...\n\nAlso, \"rm -f order_fifo\" should probably be done in test_when_finished\nrather than at the beginning of the test.\n\nAntoine,\n"},{"id":"232120","messageId":"xmqq4n67m8og.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"CALWbr2zXNF-aJHHnBnW1q1yaCmWt-rmMWypBWFanTBAK1pMWiQ@mail.gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-17T22:09:19Z","receivedAt":"2013-12-17T22:09:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n>> How about not doing a fifo?\n>\n> That would certainly defeat the purpose of the test, which is to test\n> against a fifo :-)\n\nMy point was that I did not see much value in reading the orderfile\ndata from anything but a file. At that point, you are not testing\nthe \"diff -O\" orderfile option, but if strbuf_readline() reads from\na non-regular file.\n"},{"id":"232123","messageId":"xmqqvbynkr8e.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"CALWbr2zXNF-aJHHnBnW1q1yaCmWt-rmMWypBWFanTBAK1pMWiQ@mail.gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-17T23:11:29Z","receivedAt":"2013-12-17T23:11:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> I'm not sure about the deadlock though. Both read and write will wait\n> for each other to start operating on the fifo.\n\nIt is true only if the fifo already exists. That is, if you did\nthis:\n\n\ta lot of &&\n commands &&\n before &&\n mkfifo fifo &&\n feed >fifo &\n\n\tgit diff -Ofifo\n \nthe consumer may attempt to open and read fifo when the other\nprocess is still running a lot of commands, no?\n\n>\n> You can probably fix the &&-chain by doing something like:\n>\n> mkfifo order_fifo && {\n> cat order_file_$i >order_fifo &\n> git diff -O order_fifo --name-only HEAD^..HEAD >actual\n> } && ...\n>\n> Also, \"rm -f order_fifo\" should probably be done in test_when_finished\n> rather than at the beginning of the test.\n>\n> Antoine,\n"},{"id":"232135","messageId":"CAJYzjmdscmEVuT29wMVoeUoUag=0H-XDq2tkLPRtHEr51kOk5Q@mail.gmail.com","threadId":"35172","inReplyTo":"xmqq4n67m8og.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-18T04:28:25Z","receivedAt":"2013-12-18T04:28:25Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano wrote:\n> My point was that I did not see much value in reading the orderfile\n> data from anything but a file. At that point, you are not testing\n> the \"diff -O\" orderfile option, but if strbuf_readline() reads from\n> a non-regular file.\n\nOh, good point, now that you state it explicitly. I'll remove it.\n"},{"id":"232137","messageId":"xmqqvbymk8vz.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"CAJYzjmdscmEVuT29wMVoeUoUag=0H-XDq2tkLPRtHEr51kOk5Q@mail.gmail.com","subject":"Re: [PATCH v4 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T05:47:44Z","receivedAt":"2013-12-18T05:47:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Bronson writes:\n\n> On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano wrote:\n>> My point was that I did not see much value in reading the orderfile\n>> data from anything but a file. At that point, you are not testing\n>> the \"diff -O\" orderfile option, but if strbuf_readline() reads from\n>> a non-regular file.\n>\n> Oh, good point, now that you state it explicitly. I'll remove it.\n\nOr you can study the fix-up I (tentatively) queued on top of your\nseries in 'pu'. Also see $gmane/239409.\n\nThanks.\n\n24331790 (FIXUP! tests, 2013-12-17)\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex f906dea..db0e427 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -22,14 +22,12 @@ test_expect_success 'setup' '\n \t*Makefile\n \t*.txt\n \t*.h\n-\t*\n \tEOF\n \n \tcat >order_file_2 <<-\\EOF &&\n \t*Makefile\n \t*.h\n \t*.c\n-\t*\n \tEOF\n \n \tcat >expect_none <<-\\EOF &&\n@@ -77,27 +75,30 @@ test_expect_success 'orderfile is a directory' '\n for i in 1 2\n do\n \ttest_expect_success \"orderfile using option ($i)\" '\n-\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n-\ttest_cmp expect_$i actual\n-'\n+\t\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n \n \ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n-\trm -f order_fifo &&\n-\tmkfifo order_fifo &&\n-\tcat order_file_$i >order_fifo &\n-\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n-\ttest_cmp expect_$i actual\n-'\n+\t\trm -f order_fifo &&\n+\t\tmkfifo order_fifo &&\n+\t\t{\n+\t\t\tcat order_file_$i >order_fifo &\n+\t\t} &&\n+\t\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n+\t\twait &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n \n \ttest_expect_success \"orderfile using config ($i)\" '\n-\tgit -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&\n-\ttest_cmp expect_$i actual\n-'\n+\t\tgit -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n \n \ttest_expect_success \"cancelling configured orderfile ($i)\" '\n-\tgit -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&\n-\ttest_cmp expect_none actual\n-'\n+\t\tgit -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_none actual\n+\t'\n done\n \n test_done\n-- \n1.8.5.2-297-g3e57c29\n"},{"id":"232217","messageId":"1387411692-15562-1-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"CADsOX3DBmNituJsiYEBRENQeosASXtV_hd0zUW13cBoDZWHRhg@mail.gmail.com","subject":"[PATCH v5 0/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-19T00:08:09Z","receivedAt":"2013-12-19T00:08:09Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"I expect you've figured out what this patch series is about by now.\nIn this version, I've applied Junio's suggestions from the last\nversion, and also the stuff from the FIXUP commit he made after my\nstuff in the branch he merged into 'pu'.\n\nSamuel Bronson (3):\n diff: Tests for \"git diff -O\"\n diff: Let \"git diff -O\" read orderfile from any file, fail properly\n diff: Add diff.orderfile configuration variable\n\n Documentation/diff-config.txt | 5 ++\n Documentation/diff-options.txt | 3 ++\n diff.c | 5 ++\n diffcore-order.c | 23 ++++-----\n t/t4056-diff-order.sh | 106 +++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 127 insertions(+), 15 deletions(-)\n create mode 100755 t/t4056-diff-order.sh\n\n-- \n1.8.4.3\n"},{"id":"232219","messageId":"1387411692-15562-2-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387411692-15562-1-git-send-email-naesten@gmail.com","subject":"[PATCH v5 1/3] diff: Tests for \"git diff -O\"","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-19T00:08:10Z","receivedAt":"2013-12-19T00:08:10Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"Heavily adapted from Anders' patch:\n\"diff: Add diff.orderfile configuration variable\"\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n t/t4056-diff-order.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 70 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..218f171\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,70 @@\n+#!/bin/sh\n+\n+test_description='diff order'\n+\n+. ./test-lib.sh\n+\n+create_files () {\n+\techo \"$1\" >a.h &&\n+\techo \"$1\" >b.c &&\n+\techo \"$1\" >c/Makefile &&\n+\techo \"$1\" >d.txt &&\n+\tgit add a.h b.c c/Makefile d.txt &&\n+\tgit commit -m\"$1\"\n+}\n+\n+test_expect_success 'setup' '\n+\tmkdir c &&\n+\tcreate_files 1 &&\n+\tcreate_files 2 &&\n+\n+\tcat >order_file_1 <<-\\EOF &&\n+\t*Makefile\n+\t*.txt\n+\t*.h\n+\tEOF\n+\n+\tcat >order_file_2 <<-\\EOF &&\n+\t*Makefile\n+\t*.h\n+\t*.c\n+\tEOF\n+\n+\tcat >expect_none <<-\\EOF &&\n+\ta.h\n+\tb.c\n+\tc/Makefile\n+\td.txt\n+\tEOF\n+\n+\tcat >expect_1 <<-\\EOF &&\n+\tc/Makefile\n+\td.txt\n+\ta.h\n+\tb.c\n+\tEOF\n+\n+\tcat >expect_2 <<-\\EOF &&\n+\tc/Makefile\n+\ta.h\n+\tb.c\n+\td.txt\n+\tEOF\n+\n+\ttrue\t# end chain of &&\n+'\n+\n+test_expect_success \"no order (=tree object order)\" '\n+\tgit diff --name-only HEAD^..HEAD >actual &&\n+\ttest_cmp expect_none actual\n+'\n+\n+for i in 1 2\n+do\n+\ttest_expect_success \"orderfile using option ($i)\" '\n+\t\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n+done\n+\n+test_done\n-- \n1.8.4.3\n"},{"id":"232218","messageId":"1387411692-15562-3-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387411692-15562-1-git-send-email-naesten@gmail.com","subject":"[PATCH v5 2/3] diff: Let \"git diff -O\" read orderfile from any file, fail properly","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-19T00:08:11Z","receivedAt":"2013-12-19T00:08:11Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"The -O flag really shouldn't silently fail to do anything when given a\npath that it can't read from.\n\nHowever, it should be able to read from un-mmappable files, such as:\n\n * pipes/fifos\n\n * /dev/null: It's a character device (at least on Linux)\n\n * ANY empty file:\n\n Quoting Linux mmap(2), \"SUSv3 specifies that mmap() should fail if\n length is 0. However, in kernels before 2.6.12, mmap() succeeded in\n this case: no mapping was created and the call returned addr. Since\n kernel 2.6.12, mmap() fails with the error EINVAL for this case.\"\n\nWe especially want \"-O/dev/null\" to work, since we will be documenting\nit as the way to cancel \"diff.orderfile\" when we add that.\n\n(Note: \"-O/dev/null\" did have the right effect, since the existing error\nhandling essentially worked out to \"silently ignore the orderfile\". But\nthis was probably more coincidence than anything else.)\n\nSo, lets toss all of that logic to get the file mmapped and just use\nstrbuf_read_file() instead, which gives us decent error handling\npractically for free.\n\nSigned-off-by: Samuel Bronson \n---\n diffcore-order.c | 23 ++++++++---------------\n t/t4056-diff-order.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/diffcore-order.c b/diffcore-order.c\nindex 23e9385..a63f332 100644\n--- a/diffcore-order.c\n+++ b/diffcore-order.c\n@@ -10,28 +10,21 @@ static int order_cnt;\n \n static void prepare_order(const char *orderfile)\n {\n-\tint fd, cnt, pass;\n+\tint cnt, pass;\n+\tstruct strbuf sb = STRBUF_INIT;\n \tvoid *map;\n \tchar *cp, *endp;\n-\tstruct stat st;\n-\tsize_t sz;\n+\tssize_t sz;\n \n \tif (order)\n \t\treturn;\n \n-\tfd = open(orderfile, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn;\n-\tif (fstat(fd, &st)) {\n-\t\tclose(fd);\n-\t\treturn;\n-\t}\n-\tsz = xsize_t(st.st_size);\n-\tmap = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tclose(fd);\n-\tif (map == MAP_FAILED)\n-\t\treturn;\n+\tsz = strbuf_read_file(&sb, orderfile, 0);\n+\tif (sz < 0)\n+\t\tdie_errno(_(\"failed to read orderfile '%s'\"), orderfile);\n+\tmap = strbuf_detach(&sb, NULL);\n \tendp = (char *) map + sz;\n+\n \tfor (pass = 0; pass < 2; pass++) {\n \t\tcnt = 0;\n \t\tcp = map;\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex 218f171..0ac1b95 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -59,12 +59,38 @@ test_expect_success \"no order (=tree object order)\" '\n \ttest_cmp expect_none actual\n '\n \n+test_expect_success 'missing orderfile' '\n+\trm -f bogus_file &&\n+\ttest_must_fail git diff -Obogus_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '\n+\t>unreadable_file &&\n+\tchmod -r unreadable_file &&\n+\ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n+'\n+\n+test_expect_success 'orderfile is a directory' '\n+\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n+'\n+\n for i in 1 2\n do\n \ttest_expect_success \"orderfile using option ($i)\" '\n \t\tgit diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&\n \t\ttest_cmp expect_$i actual\n \t'\n+\n+\ttest_expect_success PIPE \"orderfile is fifo ($i)\" '\n+\t\trm -f order_fifo &&\n+\t\tmkfifo order_fifo &&\n+\t\t{\n+\t\t\tcat order_file_$i >order_fifo &\n+\t\t} &&\n+\t\tgit diff -O order_fifo --name-only HEAD^..HEAD >actual &&\n+\t\twait &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232220","messageId":"1387411692-15562-4-git-send-email-naesten@gmail.com","threadId":"35172","inReplyTo":"1387411692-15562-1-git-send-email-naesten@gmail.com","subject":"[PATCH v5 3/3] diff: Add diff.orderfile configuration variable","fromName":"Samuel Bronson","fromEmail":"naesten@gmail.com","sentAt":"2013-12-19T00:08:12Z","receivedAt":"2013-12-19T00:08:12Z","isPatch":true,"sender":{"key":"naesten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13903?v=4"},"body":"diff.orderfile acts as a default for the -O command line option.\n\n[sb: split up aw's original patch; rework tests and docs, treat option\nas pathname]\n\nSigned-off-by: Anders Waldenborg \nSigned-off-by: Samuel Bronson \n---\n Documentation/diff-config.txt | 5 +++++\n Documentation/diff-options.txt | 3 +++\n diff.c | 5 +++++\n t/t4056-diff-order.sh | 10 ++++++++++\n 4 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 223b931..f07b451 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -98,6 +98,11 @@ diff.mnemonicprefix::\n diff.noprefix::\n \tIf set, 'git diff' does not show any source or destination prefix.\n \n+diff.orderfile::\n+\tFile indicating how to order files within a diff, using\n+\tone shell glob pattern per line.\n+\tCan be overridden by the '-O' option to linkgit:git-diff[1].\n+\n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n \tdetection; equivalent to the 'git diff' option '-l'.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex bbed2cd..9b37b2a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -432,6 +432,9 @@ endif::git-format-patch[]\n -O::\n \tOutput the patch in the order specified in the\n \t, which has one shell glob pattern per line.\n+\tThis overrides the `diff.orderfile` configuration variable\n+\t(see linkgit:git-config[1]). To cancel `diff.orderfile`,\n+\tuse `-O/dev/null`.\n \n ifndef::git-format-patch[]\n -R::\ndiff --git a/diff.c b/diff.c\nindex b79432b..f35c83b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+static const char *diff_order_file_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n \t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n+\tif (!strcmp(var, \"diff.orderfile\"))\n+\t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \n+\toptions->orderfile = diff_order_file_cfg;\n+\n \tif (diff_no_prefix) {\n \t\toptions->a_prefix = options->b_prefix = \"\";\n \t} else if (!diff_mnemonic_prefix) {\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex 0ac1b95..acd7683 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -91,6 +91,16 @@ do\n \t\twait &&\n \t\ttest_cmp expect_$i actual\n \t'\n+\n+\ttest_expect_success \"orderfile using config ($i)\" '\n+\t\tgit -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_$i actual\n+\t'\n+\n+\ttest_expect_success \"cancelling configured orderfile ($i)\" '\n+\t\tgit -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&\n+\t\ttest_cmp expect_none actual\n+\t'\n done\n \n test_done\n-- \n1.8.4.3\n"},{"id":"232222","messageId":"xmqqr499fzav.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"1387411692-15562-1-git-send-email-naesten@gmail.com","subject":"Re: [PATCH v5 0/3] diff: Add diff.orderfile configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-19T00:40:40Z","receivedAt":"2013-12-19T00:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Looks good; will replace and merge to 'next', but not today (I am\nalready deep into today's integration cycle).\n\nThanks.\n"},{"id":"233009","messageId":"20140110201031.GI4776@google.com","threadId":"35172","inReplyTo":"1387411692-15562-3-git-send-email-naesten@gmail.com","subject":"[PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-01-10T20:10:31Z","receivedAt":"2014-01-10T20:10:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"There is no guarantee that strbuf_read_file must error out for\ndirectories. On some operating systems (e.g., Debian GNU/kFreeBSD\nwheezy), reading a directory gives its raw content:\n\n\t$ head -c5 < / | cat -A\n\t^AM-|^_^@^L$\n\nAs a result, 'git diff -O/' succeeds instead of erroring out on\nthese systems, causing t4056.5 \"orderfile is a directory\" to fail.\n\nOn some weird OS it might even make sense to pass a directory to the\n-O option and this is not a common user mistake that needs catching.\nRemove the test.\n\nSigned-off-by: Jonathan Nieder \n---\nHi,\n\nt4056 is failing on systems using glibc with the kernel of FreeBSD[1]:\n\n| expecting success: \n| \ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n|\n| a.h\n| b.c\n| c/Makefile\n| d.txt\n| test_must_fail: command succeeded: git diff -O/ --name-only HEAD^..HEAD\n| not ok 5 - orderfile is a directory\n\nHow about this patch?\n\nThanks,\nJonathan\n\n[1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=kfreebsd-amd64&ver=1%3A2.0~next.20140107-1&stamp=1389379274\n\n t/t4056-diff-order.sh | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nindex 1ddd226..9e2b29e 100755\n--- a/t/t4056-diff-order.sh\n+++ b/t/t4056-diff-order.sh\n@@ -68,10 +68,6 @@ test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '\n \ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n '\n \n-test_expect_success 'orderfile is a directory' '\n-\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n-'\n-\n for i in 1 2\n do\n \ttest_expect_success \"orderfile using option ($i)\" '\n-- \n1.8.5.1\n"},{"id":"233014","messageId":"xmqq4n5b8lfg.fsf@gitster.dls.corp.google.com","threadId":"35172","inReplyTo":"20140110201031.GI4776@google.com","subject":"Re: [PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-10T23:30:11Z","receivedAt":"2014-01-10T23:30:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> There is no guarantee that strbuf_read_file must error out for\n> directories. On some operating systems (e.g., Debian GNU/kFreeBSD\n> wheezy), reading a directory gives its raw content:\n>\n> \t$ head -c5 < / | cat -A\n> \t^AM-|^_^@^L$\n>\n> As a result, 'git diff -O/' succeeds instead of erroring out on\n> these systems, causing t4056.5 \"orderfile is a directory\" to fail.\n>\n> On some weird OS it might even make sense to pass a directory to the\n> -O option and this is not a common user mistake that needs catching.\n> Remove the test.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> Hi,\n>\n> t4056 is failing on systems using glibc with the kernel of FreeBSD[1]:\n>\n> | expecting success: \n> | \ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n> |\n> | a.h\n> | b.c\n> | c/Makefile\n> | d.txt\n> | test_must_fail: command succeeded: git diff -O/ --name-only HEAD^..HEAD\n> | not ok 5 - orderfile is a directory\n>\n> How about this patch?\n\nSounds sensible. Thanks.\n\n\n\n> Thanks,\n> Jonathan\n>\n> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=kfreebsd-amd64&ver=1%3A2.0~next.20140107-1&stamp=1389379274\n>\n> t/t4056-diff-order.sh | 4 ----\n> 1 file changed, 4 deletions(-)\n>\n> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> index 1ddd226..9e2b29e 100755\n> --- a/t/t4056-diff-order.sh\n> +++ b/t/t4056-diff-order.sh\n> @@ -68,10 +68,6 @@ test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '\n> \ttest_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD\n> '\n> \n> -test_expect_success 'orderfile is a directory' '\n> -\ttest_must_fail git diff -O/ --name-only HEAD^..HEAD\n> -'\n> -\n> for i in 1 2\n> do\n> \ttest_expect_success \"orderfile using option ($i)\" '\n"}]}