[PATCH] diff: Add diff.orderfile configuration variable

[PATCH] diff: Add diff.orderfile configuration variable

[Anders Waldenborg]

diff.orderfile acts as a default for the -O command line option.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---

 Documentation/diff-config.txt |  4 +++
 diff.c                        |  5 +++
 t/t4056-diff-order.sh         | 74 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

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

+diff.orderfile::
+ Path to file to use for ordering the files in the diff, each line
+ is a shell glob pattern; equivalent to the 'git diff' option '-O'.
+
 diff.renameLimit::
  The number of files to consider when performing the copy/rename
  detection; equivalent to the 'git diff' option '-l'.
diff --git a/diff.c b/diff.c
index a04a34d..e66f031 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char
*value, void *cb)
  return git_config_string(&external_diff_cmd_cfg, var, value);
  if (!strcmp(var, "diff.wordregex"))
  return git_config_string(&diff_word_regex_cfg, var, value);
+ if (!strcmp(var, "diff.orderfile"))
+ return git_config_string(&diff_order_file_cfg, var, value);

  if (!strcmp(var, "diff.ignoresubmodules"))
  handle_ignore_submodules_arg(&default_diff_options, value);
@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
  options->detect_rename = diff_detect_rename_default;
  options->xdl_opts |= diff_algorithm;

+ options->orderfile = diff_order_file_cfg;
+
  if (diff_no_prefix) {
  options->a_prefix = options->b_prefix = "";
  } else if (!diff_mnemonic_prefix) {
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..fd005d6
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='diff order'
+
+. ./test-lib.sh
+
+_test_create_files () {
+ mkdir c
+ echo "$1" >a.h
+ echo "$1" >b.c
+ echo "$1" >c/Makefile
+ echo "$1" >d.txt
+ git add a.h b.c c/Makefile d.txt && \
+ git commit -m"$1"
+}
+
+cat >order_file_1 <<EOF
+*Makefile
+*.txt
+*.h
+*
+EOF
+cat >order_file_2 <<EOF
+*.h
+*.c
+*Makefile
+*
+EOF
+
+cat >expect_diff_headers_none <<EOF
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/d.txt b/d.txt
+EOF
+
+cat >expect_diff_headers_1 <<EOF
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/d.txt b/d.txt
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+EOF
+
+cat >expect_diff_headers_2 <<EOF
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/d.txt b/d.txt
+EOF
+
+test_expect_success "setup" '_test_create_files 1 && _test_create_files 2'
+
+test_expect_success "no order (=tree object order)" '
+ git diff HEAD^..HEAD | grep ^diff >actual_diff_headers &&
+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_none actual_diff_headers'
+
+test_expect_success "orderfile using option" '
+ git diff -Oorder_file_1 HEAD^..HEAD | grep ^diff >actual_diff_headers &&
+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_1 actual_diff_headers &&
+ git diff -Oorder_file_2 HEAD^..HEAD | grep ^diff >actual_diff_headers &&
+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_2 actual_diff_headers'
+
+test_expect_success "orderfile using config" '
+ git -c diff.orderfile=order_file_1 diff HEAD^..HEAD | grep ^diff
>actual_diff_headers &&
+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_1 actual_diff_headers &&
+ git -c diff.orderfile=order_file_2 diff HEAD^..HEAD | grep ^diff
>actual_diff_headers &&
+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_2 actual_diff_headers'
+
+test_done
-- 
1.8.4.1.559.gdb9bdfb.dirty

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

[Jonathan Nieder]

Hi,

Anders Waldenborg wrote:

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

Thanks.

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

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

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

Nits:

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

How about something like the following?

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

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

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

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

If a caller wants to parse diff config and also wants to make a diff
without using the config (the example I'm imagining is an alternative
implemention fo "git log -p --cherry-pick"), can they do that?  It's
tempting to move handling of configuration into a separate function.
(Perhaps it's not worth worrying about that until someone needs the
flexibility, though.)
> --- /dev/null
> +++ b/t/t4056-diff-order.sh
> @@ -0,0 +1,74 @@
> +#!/bin/sh
> +
> +test_description='diff order'
> +
> +. ./test-lib.sh
> +
> +_test_create_files () {

Why the leading underscore?

[...]
> +test_expect_success "setup" '_test_create_files 1 && _test_create_files 2'

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

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

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

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

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

> + test_debug actual_diff_headers

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

> + test_cmp expect_diff_headers_none actual_diff_headers'

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

Hope that helps,
Jonathan

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

[Anders Waldenborg]

(Jonathan, sorry if you got this multiple times, it seems I forgot to Cc list)

On Mon, Oct 21, 2013 at 8:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Should the git-diff(1) manpage get a note about this setting as
> well (perhaps in a new CONFIGURATION section)?

I'll add a reference to the documentation for the -O option at least.
That is how --check, --color, --dirstat and others do it, I guess that
could be moved to a CONFIGURATION section later?

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

It already did, didn't it? At least diff.context, diff.renames and
diff.color seems to affect diff_setup(), no?

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

Right, patch-ids are not stable wrt ordering. That might be a problem
if some tool stores patch-ids. But maybe that even is a separate bug?
Should patch-id always reorder the files internally? Is it expected
that "git diff -Oorder1  | git patch-id" and "git diff -Oorder2 | git
patch-id" gives same patch id?

It gets very interesting in an imaginative "git log -p --cherry-pick"
which caches patch-ids on disk, one would want one stable ordering for
calculating the patchid, while the displayed patch should respect the
user requested order.

I guess that in most cases one would want to respect user configured
ordering. Should diff_setup grow an argument "ignore_config"? Or
should we maybe add an --no-order-file option that easily be set as a
flag in diff_options in those cases?

> Hope that helps,

It does. Thanks! I have updated patch as per your other comments.

 anders

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

[Samuel Bronson]

From: Anders Waldenborg <anders@0x63.nu>

diff.orderfile acts as a default for the -O command line option.

[sb: fixed testcases & revised docs based on Jonathan Nieder's suggestions]

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
*I* even verified that the tests do fail properly when the feature is
sabotaged.

 Documentation/diff-config.txt  |  5 +++
 Documentation/diff-options.txt |  2 ++
 diff.c                         |  5 +++
 t/t4056-diff-order.sh          | 79 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 223b931..f07b451 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -98,6 +98,11 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.orderfile::
+	File indicating how to order files within a diff, using
+	one shell glob pattern per line.
+	Can be overridden by the '-O' option to linkgit:git-diff[1].
+
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the 'git diff' option '-l'.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bbed2cd..1af5a5e 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -432,6 +432,8 @@ endif::git-format-patch[]
 -O<orderfile>::
 	Output the patch in the order specified in the
 	<orderfile>, which has one shell glob pattern per line.
+	This overrides the `diff.orderfile' configuration variable
+	((see linkgit:git-config[1]).
 
 ifndef::git-format-patch[]
 -R::
diff --git a/diff.c b/diff.c
index e34bf97..a92b570 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
+	if (!strcmp(var, "diff.orderfile"))
+		return git_config_string(&diff_order_file_cfg, var, value);
 
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
 
+	options->orderfile = diff_order_file_cfg;
+
 	if (diff_no_prefix) {
 		options->a_prefix = options->b_prefix = "";
 	} else if (!diff_mnemonic_prefix) {
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..a756b34
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='diff order'
+
+. ./test-lib.sh
+
+create_files () {
+	echo "$1" >a.h &&
+	echo "$1" >b.c &&
+	echo "$1" >c/Makefile &&
+	echo "$1" >d.txt &&
+	git add a.h b.c c/Makefile d.txt &&
+	git commit -m"$1"
+	return $?
+}
+
+test_expect_success "setup" '
+	mkdir c &&
+	create_files 1 &&
+	create_files 2
+'
+
+cat >order_file_1 <<EOF
+*Makefile
+*.txt
+*.h
+*
+EOF
+cat >order_file_2 <<EOF
+*Makefile
+*.h
+*.c
+*
+EOF
+
+cat >expect_diff_headers_none <<EOF
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/d.txt b/d.txt
+EOF
+
+cat >expect_diff_headers_1 <<EOF
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/d.txt b/d.txt
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+EOF
+
+cat >expect_diff_headers_2 <<EOF
+diff --git a/c/Makefile b/c/Makefile
+diff --git a/a.h b/a.h
+diff --git a/b.c b/b.c
+diff --git a/d.txt b/d.txt
+EOF
+
+test_expect_success "no order (=tree object order)" '
+	git diff HEAD^..HEAD >patch &&
+	grep ^diff patch >actual_diff_headers &&
+	test_cmp expect_diff_headers_none actual_diff_headers
+'
+
+for i in 1 2; do
+	test_expect_success "orderfile using option ($i)" "
+	git diff -Oorder_file_$i HEAD^..HEAD >patch &&
+	grep ^diff patch >actual_diff_headers &&
+	test_cmp expect_diff_headers_$i actual_diff_headers
+"
+done
+
+for i in 1 2; do
+	test_expect_success "orderfile using config ($i)" "
+	git -c diff.orderfile=order_file_$i diff HEAD^..HEAD >patch &&
+	grep ^diff patch >actual_diff_headers &&
+	test_cmp expect_diff_headers_$i actual_diff_headers
+"
+done
+
+test_done
-- 
1.8.4.3

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

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> From: Anders Waldenborg <anders@0x63.nu>
>
> diff.orderfile acts as a default for the -O command line option.
>
> [sb: fixed testcases & revised docs based on Jonathan Nieder's suggestions]
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Samuel Bronson <naesten@gmail.com>

Thanks for reviving a stalled topic.

> ---
> *I* even verified that the tests do fail properly when the feature is
> sabotaged.

Sabotaged in what way?

>  Documentation/diff-config.txt  |  5 +++
>  Documentation/diff-options.txt |  2 ++
>  diff.c                         |  5 +++
>  t/t4056-diff-order.sh          | 79 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 91 insertions(+)
>  create mode 100755 t/t4056-diff-order.sh
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 223b931..f07b451 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -98,6 +98,11 @@ diff.mnemonicprefix::
>  diff.noprefix::
>  	If set, 'git diff' does not show any source or destination prefix.
>  
> +diff.orderfile::
> +	File indicating how to order files within a diff, using
> +	one shell glob pattern per line.
> +	Can be overridden by the '-O' option to linkgit:git-diff[1].
> +
>  diff.renameLimit::
>  	The number of files to consider when performing the copy/rename
>  	detection; equivalent to the 'git diff' option '-l'.
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index bbed2cd..1af5a5e 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -432,6 +432,8 @@ endif::git-format-patch[]
>  -O<orderfile>::
>  	Output the patch in the order specified in the
>  	<orderfile>, which has one shell glob pattern per line.
> +	This overrides the `diff.orderfile' configuration variable
> +	((see linkgit:git-config[1]).

Double opening parenthesis?

If somebody has diff.orderfile configuration that points at a custom
ordering, and wants to send out a patch (or show a diff) with the
standard order, how would the "overriding" command line look like?
Would it be "git diff -O/dev/null"?

> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> new file mode 100755
> index 0000000..a756b34
> --- /dev/null
> +++ b/t/t4056-diff-order.sh
> @@ -0,0 +1,79 @@
> +#!/bin/sh
> +
> +test_description='diff order'
> +
> +. ./test-lib.sh
> +
> +create_files () {
> +	echo "$1" >a.h &&
> +	echo "$1" >b.c &&
> +	echo "$1" >c/Makefile &&
> +	echo "$1" >d.txt &&
> +	git add a.h b.c c/Makefile d.txt &&
> +	git commit -m"$1"
> +	return $?
> +}

That return looks somewhat strange.  Does it even need to be there?

> +test_expect_success "setup" '

Makes readers wonder why dq is used here, I think.

> +	mkdir c &&
> +	create_files 1 &&
> +	create_files 2
> +'
> +
> +cat >order_file_1 <<EOF
> +*Makefile
> +*.txt
> +*.h
> +*
> +EOF
> +cat >order_file_2 <<EOF
> +*Makefile
> +*.h
> +*.c
> +*
> +EOF
> +
> +cat >expect_diff_headers_none <<EOF
> +diff --git a/a.h b/a.h
> +diff --git a/b.c b/b.c
> +diff --git a/c/Makefile b/c/Makefile
> +diff --git a/d.txt b/d.txt
> +EOF
> +
> +cat >expect_diff_headers_1 <<EOF
> +diff --git a/c/Makefile b/c/Makefile
> +diff --git a/d.txt b/d.txt
> +diff --git a/a.h b/a.h
> +diff --git a/b.c b/b.c
> +EOF
> +
> +cat >expect_diff_headers_2 <<EOF
> +diff --git a/c/Makefile b/c/Makefile
> +diff --git a/a.h b/a.h
> +diff --git a/b.c b/b.c
> +diff --git a/d.txt b/d.txt
> +EOF

All of these "cat" outside the test_expect_* are better be inside
the 'setup' section, I think.  I.e.

	test_expect_success setup '
        	mkdir c &&
                create_files 1 &&
                create_files 2 &&
                cat >order_file_1 <<-\EOF &&
                *Makefile
                *.txt
                *.h
                *
                EOF
                cat >order_file_2 <<-\EOF &&
		...
		cat >expect_diff_headers_2 <<EOF
                ...
                EOF
	'

Quoting the EOF like the above will help the readers by signaling
them that they do not have to wonder if there is some substitution
going on in the here text.

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

Instead of grepping, "git diff --name-only" would be far easier to
check, no?

> +for i in 1 2; do
> +	test_expect_success "orderfile using option ($i)" "
> +	git diff -Oorder_file_$i HEAD^..HEAD >patch &&
> +	grep ^diff patch >actual_diff_headers &&
> +	test_cmp expect_diff_headers_$i actual_diff_headers
> +"
> +done
> +for i in 1 2; do
> +	test_expect_success "orderfile using config ($i)" "
> +	git -c diff.orderfile=order_file_$i diff HEAD^..HEAD >patch &&
> +	grep ^diff patch >actual_diff_headers &&
> +	test_cmp expect_diff_headers_$i actual_diff_headers
> +"
> +done

I'd probably write the above like so:

	for i in 1 2
        do
		test_expect_success "orderfile using option ($i)" '
                	git diff -Oorder_file_$i --name-only HEAD^ >actual &&
			test_cmp expect_$i actual
		'
		test_expect_success "orderfile using config ($i)" '
			test_config diff.orderfile order_file_$i &&
                	git diff --name-only HEAD^ >actual &&
			test_cmp expect_$i actual
		'
	done

Points to note:

 * We eval the scriptlets inside test framework, so using $i as a
   variable inside the single quotes will have the expected result.
   You do not have to worry about extra quoting inside dq pair.

 * We do _not_ substitute variables in the test title (perhaps we
   should have designed the test framework to do so, in hindsight),
   so unfortunately the title need to be in dq.

 * Use line-breaks instead of semicolons when writing compound
   syntax structures such as "for/do/done", "if/then/elif/else/fi",
   etc.

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

[Samuel Bronson]

On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:

> Thanks for reviving a stalled topic.

I was asking about such a feature in #git and jrnieder was nice enough
to point me at the stalled patch.

>> *I* even verified that the tests do fail properly when the feature is
>> sabotaged.
>
> Sabotaged in what way?

I commented out the "options->orderfile = diff_order_file_cfg;" line.

>> @@ -432,6 +432,8 @@ endif::git-format-patch[]
>>  -O<orderfile>::
>>       Output the patch in the order specified in the
>>       <orderfile>, which has one shell glob pattern per line.
>> +     This overrides the `diff.orderfile' configuration variable
>> +     ((see linkgit:git-config[1]).
>
> Double opening parenthesis?

Oops, and it looks like I messed up the quoting on diff.orderfile too ...

> If somebody has diff.orderfile configuration that points at a custom
> ordering, and wants to send out a patch (or show a diff) with the
> standard order, how would the "overriding" command line look like?
> Would it be "git diff -O/dev/null"?

It looks like that works ... and so do files that don't exist.  What
do you think should happen with -O file-that-does-not-exist, and how
do you suppose it should be tested?

After having fixed this, will /dev/null still work everywhere, or will
we want a new diff flag to unset the option?  (I see that "git diff
/dev/null some-file" works fine with msysgit, which doesn't seem to
actually be linked with MSYS, but I don't know *why* it works, and I
don't know what other non-POSIXoid ports exist.)

For the moment, I've added this to "for" loop (after some changes
based on some of your other suggestions):

    # I don't think this should just pretend the orderfile was empty?
    test_expect_failure "override with bogus orderfile ($i)" '
    test_might_fail git -c diff.orderfile=order_file_$i diff
-Obogus_file --name-only HEAD^..HEAD >actual_diff_filenames &&
    ! test_cmp expect_diff_filenames_none actual_diff_filenames
'

Does this look (modulo gmail's stupid indentation) anything like a
reasonable approach to testing that?  (Of course, you can't actually
test it because it depends on other changes I haven't posted yet ...)

Also, I'm starting to wonder if I shouldn't split this into two patches:

    1.  diff: Add tests for -O flag
    2.  diff: Add diff.orderfile configuration variable

(If so, I would obviously want to rewrite the above test to avoid the
configuration option.)

>> +     return $?
>> +}
>
> That return looks somewhat strange.  Does it even need to be there?

I'm certainly no great expert at shell functions, so I expect it
isn't.  I'm not really sure what possessed me to think it might be
needed.

>                 EOF
>                 cat >order_file_2 <<-\EOF &&

I'd kind of prefer to keep a blank line between one EOF and the next
cat, if that's okay with you.

>
> Quoting the EOF like the above will help the readers by signaling
> them that they do not have to wonder if there is some substitution
> going on in the here text.

Perhaps, but probably only after they've scrutinized their shell
manuals to figure out what the - and the \ are for.  (I had to check
two: dash(1) wasn't clear enough for me about the quoting ...)

>> +test_expect_success "no order (=tree object order)" '
>> +     git diff HEAD^..HEAD >patch &&
>> +     grep ^diff patch >actual_diff_headers &&
>> +     test_cmp expect_diff_headers_none actual_diff_headers
>> +'
>
> Instead of grepping, "git diff --name-only" would be far easier to
> check, no?

It certainly makes for less-cluttered expected output.  (I guess
jrnieder didn't know about that trick when he suggested using the
intermediate file?)

> Points to note:
>
>  * We eval the scriptlets inside test framework, so using $i as a
>    variable inside the single quotes will have the expected result.
>    You do not have to worry about extra quoting inside dq pair.

Hmm.  I'm obviously not used to things getting eval'd in the same
shell instance as my script ...

(Thanks for the review!)

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

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

>> If somebody has diff.orderfile configuration that points at a custom
>> ordering, and wants to send out a patch (or show a diff) with the
>> standard order, how would the "overriding" command line look like?
>> Would it be "git diff -O/dev/null"?
>
> It looks like that works ... and so do files that don't exist.  What
> do you think should happen with -O file-that-does-not-exist, and how
> do you suppose it should be tested?

I think the original code is too loose on diagnosing errors.
Perhaps something like this is needed.

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..dd103e3 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -20,8 +20,11 @@ static void prepare_order(const char *orderfile)
 		return;
 
 	fd = open(orderfile, O_RDONLY);
-	if (fd < 0)
+	if (fd < 0) {
+		if (errno != ENOENT || errno != ENOTDIR)
+			die(_("orderfile '%s' does not exist."));
 		return;
+	}
 	if (fstat(fd, &st)) {
 		close(fd);
 		return;

> After having fixed this, will /dev/null still work everywhere, or will
> we want a new diff flag to unset the option?  (I see that "git diff
> /dev/null some-file" works fine with msysgit, which doesn't seem to
> actually be linked with MSYS, but I don't know *why* it works, and I
> don't know what other non-POSIXoid ports exist.)

I *think* it should be OK to use "-O/dev/null" for that purpose, but
the primary thing I was hinting at with the rhetoric question was
that it probably needs to be documented there.

> Also, I'm starting to wonder if I shouldn't split this into two patches:
>
>     1.  diff: Add tests for -O flag
>     2.  diff: Add diff.orderfile configuration variable
>
> (If so, I would obviously want to rewrite the above test to avoid the
> configuration option.)

Surely, and thanks.

>>                 EOF
>>                 cat >order_file_2 <<-\EOF &&
>
> I'd kind of prefer to keep a blank line between one EOF and the next
> cat, if that's okay with you.

Alright.  Making it easier to spot the grouping, it would make it
easier to read.

>> Quoting the EOF like the above will help the readers by signaling
>> them that they do not have to wonder if there is some substitution
>> going on in the here text.
>
> Perhaps, but probably only after they've scrutinized their shell
> manuals to figure out what the - and the \ are for.  (I had to check
> two: dash(1) wasn't clear enough for me about the quoting ...)

Yes and no.  The no primarily comes from that nobody will stay to be
novice forever.

[PATCH v3 0/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

The original purpose of this patch [series] was to allow specifying
the "-O" option for "git diff" in the config, but I need help with the
relative path handling [RFC 3].

It also added tests for "git diff -O", which I have split out because
they are independantly useful [PATCH 1].

I also noticed that -O had terrible error handling and could only read
mmappable files, so I fixed that [PATCH 2].

Samuel Bronson (3):
  diff: Tests for "git diff -O"
  diff: Let "git diff -O" read orderfile from any file, failing when
    appropriate
  diff: Add diff.orderfile configuration variable

 Documentation/diff-config.txt  |   5 ++
 Documentation/diff-options.txt |   3 ++
 diff.c                         |   5 ++
 diffcore-order.c               |  23 ++++-----
 t/t4056-diff-order.sh          | 105 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+), 15 deletions(-)
 create mode 100755 t/t4056-diff-order.sh

-- 
1.8.4.3

[PATCH v3 1/3] diff: Tests for "git diff -O"

[Samuel Bronson]

Heavily adapted from Anders' patch:
"diff: Add diff.orderfile configuration variable"

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..398b3f6
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='diff order'
+
+. ./test-lib.sh
+
+create_files () {
+	echo "$1" >a.h &&
+	echo "$1" >b.c &&
+	echo "$1" >c/Makefile &&
+	echo "$1" >d.txt &&
+	git add a.h b.c c/Makefile d.txt &&
+	git commit -m"$1"
+}
+
+test_expect_success 'setup' '
+	mkdir c &&
+	create_files 1 &&
+	create_files 2 &&
+
+	cat >order_file_1 <<-\EOF &&
+	*Makefile
+	*.txt
+	*.h
+	*
+	EOF
+
+	cat >order_file_2 <<-\EOF &&
+	*Makefile
+	*.h
+	*.c
+	*
+	EOF
+
+	cat >expect_none <<-\EOF &&
+	a.h
+	b.c
+	c/Makefile
+	d.txt
+	EOF
+
+	cat >expect_1 <<-\EOF &&
+	c/Makefile
+	d.txt
+	a.h
+	b.c
+	EOF
+
+	cat >expect_2 <<-\EOF &&
+	c/Makefile
+	a.h
+	b.c
+	d.txt
+	EOF
+
+	true	# end chain of &&
+'
+
+test_expect_success "no order (=tree object order)" '
+	git diff --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_none actual
+'
+
+for i in 1 2
+do
+	test_expect_success "orderfile using option ($i)" '
+	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
+done
+
+test_done
-- 
1.8.4.3

[PATCH v3 2/3] diff: Let "git diff -O" read orderfile from any file, failing when appropriate

[Samuel Bronson]

The -O flag really shouldn't silently fail to do anything when given a
path that it can't read from.

However, it should be able to read from un-mappable files, such as
pipes/fifos, /dev/null (as we document in the next patch), or in fact
*any* empty file (since Linux 2.6.12).  (Especially since we will be
documenting "-O/dev/null" to override "diff.orderfile" when we add that.)

(Note: "-O/dev/null" did have the right effect, since the existing error
handling essentially worked out to "silently ignore the orderfile".)

So lets toss all of that logic to get the file mmapped and just use
strbuf_read_file() instead, which gives us decent error handling
practically for free.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..a63f332 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -10,28 +10,21 @@ static int order_cnt;
 
 static void prepare_order(const char *orderfile)
 {
-	int fd, cnt, pass;
+	int cnt, pass;
+	struct strbuf sb = STRBUF_INIT;
 	void *map;
 	char *cp, *endp;
-	struct stat st;
-	size_t sz;
+	ssize_t sz;
 
 	if (order)
 		return;
 
-	fd = open(orderfile, O_RDONLY);
-	if (fd < 0)
-		return;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return;
-	}
-	sz = xsize_t(st.st_size);
-	map = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
-	close(fd);
-	if (map == MAP_FAILED)
-		return;
+	sz = strbuf_read_file(&sb, orderfile, 0);
+	if (sz < 0)
+		die_errno(_("failed to read orderfile '%s'"), orderfile);
+	map = strbuf_detach(&sb, NULL);
 	endp = (char *) map + sz;
+
 	for (pass = 0; pass < 2; pass++) {
 		cnt = 0;
 		cp = map;
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 398b3f6..eb471e7 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -61,12 +61,35 @@ test_expect_success "no order (=tree object order)" '
 	test_cmp expect_none actual
 '
 
+test_expect_success 'missing orderfile' '
+	rm -f bogus_file &&
+	test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'unreadable orderfile' '
+	touch unreadable_file &&
+	chmod -r unreadable_file &&
+	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'orderfile is a directory' '
+	test_must_fail git diff -O/ --name-only HEAD^..HEAD
+'
+
 for i in 1 2
 do
 	test_expect_success "orderfile using option ($i)" '
 	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
 	test_cmp expect_$i actual
 '
+
+	test_expect_success PIPE "orderfile is fifo ($i)" '
+	rm -f order_fifo &&
+	mkfifo order_fifo &&
+	cat order_file_$i >order_fifo &
+	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
 done
 
 test_done
-- 
1.8.4.3

Re: [PATCH v3 2/3] diff: Let "git diff -O" read orderfile from any file, failing when appropriate

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> The -O flag really shouldn't silently fail to do anything when given a
> path that it can't read from.
>
> However, it should be able to read from un-mappable files, such as
> pipes/fifos, /dev/null (as we document in the next patch), or in fact
> *any* empty file (since Linux 2.6.12).

Could you enlighten the commit log readers a bit better here?  Those
who know the change in 2.6.12 (i.e. "'mmapping with length 0 must
fail', says SUSv3, so we fail") you have in mind would know what you
mean by "in fact any empty file" even if you did not have "(since
Linux 2.6.12)", but those who do not know it would not be helped
with just "(since Linux 2.6.12)".

> (Especially since we will be
> documenting "-O/dev/null" to override "diff.orderfile" when we add that.)
>
> (Note: "-O/dev/null" did have the right effect, since the existing error
> handling essentially worked out to "silently ignore the orderfile".)
>
> So lets toss all of that logic to get the file mmapped and just use
> strbuf_read_file() instead, which gives us decent error handling
> practically for free.

Sounds good.  In the longer term, we may want to move this
file-scope static to per-infocation "struct diff_options" and clean
up the storage used to hold the list of path patterns after we are
done with the diff, but that is outside the scope of this series.

Thanks.

> Signed-off-by: Samuel Bronson <naesten@gmail.com>
> ---
>  diffcore-order.c      | 23 ++++++++---------------
>  t/t4056-diff-order.sh | 23 +++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/diffcore-order.c b/diffcore-order.c
> index 23e9385..a63f332 100644
> --- a/diffcore-order.c
> +++ b/diffcore-order.c
> @@ -10,28 +10,21 @@ static int order_cnt;
>  
>  static void prepare_order(const char *orderfile)
>  {
> -	int fd, cnt, pass;
> +	int cnt, pass;
> +	struct strbuf sb = STRBUF_INIT;
>  	void *map;
>  	char *cp, *endp;
> -	struct stat st;
> -	size_t sz;
> +	ssize_t sz;
>  
>  	if (order)
>  		return;
>  
> -	fd = open(orderfile, O_RDONLY);
> -	if (fd < 0)
> -		return;
> -	if (fstat(fd, &st)) {
> -		close(fd);
> -		return;
> -	}
> -	sz = xsize_t(st.st_size);
> -	map = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
> -	close(fd);
> -	if (map == MAP_FAILED)
> -		return;
> +	sz = strbuf_read_file(&sb, orderfile, 0);
> +	if (sz < 0)
> +		die_errno(_("failed to read orderfile '%s'"), orderfile);
> +	map = strbuf_detach(&sb, NULL);
>  	endp = (char *) map + sz;
> +
>  	for (pass = 0; pass < 2; pass++) {
>  		cnt = 0;
>  		cp = map;
> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> index 398b3f6..eb471e7 100755
> --- a/t/t4056-diff-order.sh
> +++ b/t/t4056-diff-order.sh
> @@ -61,12 +61,35 @@ test_expect_success "no order (=tree object order)" '
>  	test_cmp expect_none actual
>  '
>  
> +test_expect_success 'missing orderfile' '
> +	rm -f bogus_file &&
> +	test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
> +'
> +
> +test_expect_success 'unreadable orderfile' '
> +	touch unreadable_file &&
> +	chmod -r unreadable_file &&
> +	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
> +'
> +
> +test_expect_success 'orderfile is a directory' '
> +	test_must_fail git diff -O/ --name-only HEAD^..HEAD
> +'
> +
>  for i in 1 2
>  do
>  	test_expect_success "orderfile using option ($i)" '
>  	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>  	test_cmp expect_$i actual
>  '
> +
> +	test_expect_success PIPE "orderfile is fifo ($i)" '
> +	rm -f order_fifo &&
> +	mkfifo order_fifo &&
> +	cat order_file_$i >order_fifo &
> +	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
> +	test_cmp expect_$i actual
> +'
>  done
>  
>  test_done

[RFC v3 3/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

diff.orderfile acts as a default for the -O command line option.

[sb: split up aw's original patch; reworked tests and docs]

[FIXME: Relative paths should presumably be interpreted relative to
repository root; how should this be accomplished?]

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 223b931..f07b451 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -98,6 +98,11 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.orderfile::
+	File indicating how to order files within a diff, using
+	one shell glob pattern per line.
+	Can be overridden by the '-O' option to linkgit:git-diff[1].
+
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the 'git diff' option '-l'.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bbed2cd..9b37b2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -432,6 +432,9 @@ endif::git-format-patch[]
 -O<orderfile>::
 	Output the patch in the order specified in the
 	<orderfile>, which has one shell glob pattern per line.
+	This overrides the `diff.orderfile` configuration variable
+	(see linkgit:git-config[1]).  To cancel `diff.orderfile`,
+	use `-O/dev/null`.
 
 ifndef::git-format-patch[]
 -R::
diff --git a/diff.c b/diff.c
index 3950e01..b336aef 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
+	if (!strcmp(var, "diff.orderfile"))
+		return git_config_string(&diff_order_file_cfg, var, value);
 
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
 
+	options->orderfile = diff_order_file_cfg;
+
 	if (diff_no_prefix) {
 		options->a_prefix = options->b_prefix = "";
 	} else if (!diff_mnemonic_prefix) {
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index eb471e7..50689d1 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -90,6 +90,16 @@ do
 	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
 	test_cmp expect_$i actual
 '
+
+	test_expect_success "orderfile using config ($i)" '
+	git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
+
+	test_expect_success "cancelling configured orderfile ($i)" '
+	git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_none actual
+'
 done
 
 test_done
-- 
1.8.4.3

Re: [RFC v3 3/3] diff: Add diff.orderfile configuration variable

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> diff.orderfile acts as a default for the -O command line option.
>
> [sb: split up aw's original patch; reworked tests and docs]
>
> [FIXME: Relative paths should presumably be interpreted relative to
> repository root; how should this be accomplished?]

Do you mean something like this?

    $ cd docs
    $ edit orderfile
    $ git diff -Oordefile
    $ cd subdir
    $ git diff -O../orderfile

Path-like parameters and values given by the end user should be
relative to the directory where the end user is (i.e. both -O
parameters in the above example name docs/orderfile).  All Git
processes, even the ones that are capable of being run from a
subdirectory, are supposed to first chdir to the top level of the
working tree before doing anything else, and adjust the path-like
things they get from the end user from the command line accordingly.
By the time diffcore_order() to prepare_order() callchain is called,
we certainly should have passed that chdir already, so the value of
the option needs to be prepended with the "prefix" when parsed.

The value specified for the diff.orderfile configuration can just be
a path relative to the top level of the working tree, I think.

Re: [RFC v3 3/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

On Mon, Dec 16, 2013 at 1:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:

> Path-like parameters and values given by the end user should be
> relative to the directory where the end user is (i.e. both -O
> parameters in the above example name docs/orderfile).  All Git
> processes, even the ones that are capable of being run from a
> subdirectory, are supposed to first chdir to the top level of the
> working tree before doing anything else, and adjust the path-like
> things they get from the end user from the command line accordingly.
> By the time diffcore_order() to prepare_order() callchain is called,
> we certainly should have passed that chdir already, so the value of
> the option needs to be prepended with the "prefix" when parsed.
>
> The value specified for the diff.orderfile configuration can just be
> a path relative to the top level of the working tree, I think.

Oh, cool.  So I'll just change the git_config_string() call to use
git_config_pathname(), since the user might easily want to use ~
notation there, especially in a user-level setting ...

[PATCH v4 0/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

The original purpose of this patch [series] was to allow specifying
the "-O" option for "git diff" in the config.

In this version, I've revised the commit message for patch 2, changed
patch 3 to use git_config_pathname() instead of git_config_string(),
and removed the FIXME from patch 3's commit message.

Samuel Bronson (3):
  diff: Tests for "git diff -O"
  diff: Let "git diff -O" read orderfile from any file, fail properly
  diff: Add diff.orderfile configuration variable

 Documentation/diff-config.txt  |   5 ++
 Documentation/diff-options.txt |   3 ++
 diff.c                         |   5 ++
 diffcore-order.c               |  23 ++++-----
 t/t4056-diff-order.sh          | 105 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+), 15 deletions(-)
 create mode 100755 t/t4056-diff-order.sh

-- 
1.8.4.3

[PATCH v4 1/3] diff: Tests for "git diff -O"

[Samuel Bronson]

Heavily adapted from Anders' patch:
"diff: Add diff.orderfile configuration variable"

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..398b3f6
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='diff order'
+
+. ./test-lib.sh
+
+create_files () {
+	echo "$1" >a.h &&
+	echo "$1" >b.c &&
+	echo "$1" >c/Makefile &&
+	echo "$1" >d.txt &&
+	git add a.h b.c c/Makefile d.txt &&
+	git commit -m"$1"
+}
+
+test_expect_success 'setup' '
+	mkdir c &&
+	create_files 1 &&
+	create_files 2 &&
+
+	cat >order_file_1 <<-\EOF &&
+	*Makefile
+	*.txt
+	*.h
+	*
+	EOF
+
+	cat >order_file_2 <<-\EOF &&
+	*Makefile
+	*.h
+	*.c
+	*
+	EOF
+
+	cat >expect_none <<-\EOF &&
+	a.h
+	b.c
+	c/Makefile
+	d.txt
+	EOF
+
+	cat >expect_1 <<-\EOF &&
+	c/Makefile
+	d.txt
+	a.h
+	b.c
+	EOF
+
+	cat >expect_2 <<-\EOF &&
+	c/Makefile
+	a.h
+	b.c
+	d.txt
+	EOF
+
+	true	# end chain of &&
+'
+
+test_expect_success "no order (=tree object order)" '
+	git diff --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_none actual
+'
+
+for i in 1 2
+do
+	test_expect_success "orderfile using option ($i)" '
+	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
+done
+
+test_done
-- 
1.8.4.3

[PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Samuel Bronson]

The -O flag really shouldn't silently fail to do anything when given a
path that it can't read from.

However, it should be able to read from un-mmappable files, such as:

 * pipes/fifos

 * /dev/null:  It's a character device (at least on Linux)

 * ANY empty file:

   Quoting Linux mmap(2), "SUSv3 specifies that mmap() should fail if
   length is 0.  However, in kernels before 2.6.12, mmap() succeeded in
   this case: no mapping was created and the call returned addr.  Since
   kernel 2.6.12, mmap() fails with the error EINVAL for this case."

We especially want "-O/dev/null" to work, since we will be documenting
it as the way to cancel "diff.orderfile" when we add that.

(Note: "-O/dev/null" did have the right effect, since the existing error
handling essentially worked out to "silently ignore the orderfile".  But
this was probably more coincidence than anything else.)

So, lets toss all of that logic to get the file mmapped and just use
strbuf_read_file() instead, which gives us decent error handling
practically for free.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..a63f332 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -10,28 +10,21 @@ static int order_cnt;
 
 static void prepare_order(const char *orderfile)
 {
-	int fd, cnt, pass;
+	int cnt, pass;
+	struct strbuf sb = STRBUF_INIT;
 	void *map;
 	char *cp, *endp;
-	struct stat st;
-	size_t sz;
+	ssize_t sz;
 
 	if (order)
 		return;
 
-	fd = open(orderfile, O_RDONLY);
-	if (fd < 0)
-		return;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return;
-	}
-	sz = xsize_t(st.st_size);
-	map = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
-	close(fd);
-	if (map == MAP_FAILED)
-		return;
+	sz = strbuf_read_file(&sb, orderfile, 0);
+	if (sz < 0)
+		die_errno(_("failed to read orderfile '%s'"), orderfile);
+	map = strbuf_detach(&sb, NULL);
 	endp = (char *) map + sz;
+
 	for (pass = 0; pass < 2; pass++) {
 		cnt = 0;
 		cp = map;
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 398b3f6..eb471e7 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -61,12 +61,35 @@ test_expect_success "no order (=tree object order)" '
 	test_cmp expect_none actual
 '
 
+test_expect_success 'missing orderfile' '
+	rm -f bogus_file &&
+	test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'unreadable orderfile' '
+	touch unreadable_file &&
+	chmod -r unreadable_file &&
+	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'orderfile is a directory' '
+	test_must_fail git diff -O/ --name-only HEAD^..HEAD
+'
+
 for i in 1 2
 do
 	test_expect_success "orderfile using option ($i)" '
 	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
 	test_cmp expect_$i actual
 '
+
+	test_expect_success PIPE "orderfile is fifo ($i)" '
+	rm -f order_fifo &&
+	mkfifo order_fifo &&
+	cat order_file_$i >order_fifo &
+	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
 done
 
 test_done
-- 
1.8.4.3

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> index 398b3f6..eb471e7 100755
> --- a/t/t4056-diff-order.sh
> +++ b/t/t4056-diff-order.sh
> @@ -61,12 +61,35 @@ test_expect_success "no order (=tree object order)" '
>  	test_cmp expect_none actual
>  '
>  
> +test_expect_success 'missing orderfile' '
> +	rm -f bogus_file &&
> +	test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
> +'
> +
> +test_expect_success 'unreadable orderfile' '
> +	touch unreadable_file &&
> +	chmod -r unreadable_file &&

Two points:

  - Unless your primary interest is to change the file timestamp, do
    not use "touch"; using ">unreadable_file" or something instead
    would tell the readers that you only want to make sure it exists
    and do not care about the file timestamp.

  - this test probably needs restricted to people with sane
    filesystems; I think POSIXPERM prerequisite and also SANITY
    prerequisite are needed, at least.

> +	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
> +'
> +
> +test_expect_success 'orderfile is a directory' '
> +	test_must_fail git diff -O/ --name-only HEAD^..HEAD
> +'
> +
>  for i in 1 2
>  do
>  	test_expect_success "orderfile using option ($i)" '
>  	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>  	test_cmp expect_$i actual
>  '
> +
> +	test_expect_success PIPE "orderfile is fifo ($i)" '
> +	rm -f order_fifo &&
> +	mkfifo order_fifo &&
> +	cat order_file_$i >order_fifo &
> +	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
> +	test_cmp expect_$i actual
> +'
>  done
>  
>  test_done

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Samuel Bronson]

On Mon, Dec 16, 2013 at 4:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:

>> +test_expect_success 'unreadable orderfile' '
>> +     touch unreadable_file &&
>> +     chmod -r unreadable_file &&

>   - this test probably needs restricted to people with sane
>     filesystems; I think POSIXPERM prerequisite and also SANITY
>     prerequisite are needed, at least.

Hmm, yeah, you've got a point; now that I think more carefully, the
most FAT can do is something like "chmod -w", nothing with the "r"
permissions.  Oops.

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

>  for i in 1 2
>  do
>  	test_expect_success "orderfile using option ($i)" '
>  	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>  	test_cmp expect_$i actual
>  '

This funny indentation in the previous step needs to be fixed, and
the added block below should match.

> +
> +	test_expect_success PIPE "orderfile is fifo ($i)" '
> +	rm -f order_fifo &&

> +	mkfifo order_fifo &&
> +	cat order_file_$i >order_fifo &
> +	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&

I think this part can be racy depending on which between cat and
"git diff" are scheduled first, no?  Try running this test under
load and I think you will see it deadlocked.

Besides, the above breaks && chain; even if mkfifo breaks (hence not
allowing cat to run), "git diff" will go ahead and run, no?

> +	test_cmp expect_$i actual
> +'
>  done
>  
>  test_done

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Samuel Bronson]

On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:
>
>>  for i in 1 2
>>  do
>>       test_expect_success "orderfile using option ($i)" '
>>       git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>>       test_cmp expect_$i actual
>>  '
>
> This funny indentation in the previous step needs to be fixed, and
> the added block below should match.

Even though this results in oddly-indented --verbose output?

>> +     rm -f order_fifo &&
>> +     mkfifo order_fifo &&
>> +     cat order_file_$i >order_fifo &
>> +     git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
>
> I think this part can be racy depending on which between cat and
> "git diff" are scheduled first, no?  Try running this test under
> load and I think you will see it deadlocked.
>
> Besides, the above breaks && chain; even if mkfifo breaks (hence not
> allowing cat to run), "git diff" will go ahead and run, no?

Hmm.  Well, what I really wanted to put here was a "process substitution":

    git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&

but I did not see this feature listed in the dash(1) manpage, so I
assumed it wasn't allowed by POSIX.  And, having looked, I indeed
don't see it mentioned in POSIX either.

I'm not terribly surprised that I screwed up the translation to FIFOs;
how would I really want to do it?

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Samuel Bronson <naesten@gmail.com> writes:
>>
>>>  for i in 1 2
>>>  do
>>>       test_expect_success "orderfile using option ($i)" '
>>>       git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>>>       test_cmp expect_$i actual
>>>  '
>>
>> This funny indentation in the previous step needs to be fixed, and
>> the added block below should match.
>
> Even though this results in oddly-indented --verbose output?
>
>>> +     rm -f order_fifo &&
>>> +     mkfifo order_fifo &&
>>> +     cat order_file_$i >order_fifo &
>>> +     git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
>>
>> I think this part can be racy depending on which between cat and
>> "git diff" are scheduled first, no?  Try running this test under
>> load and I think you will see it deadlocked.
>>
>> Besides, the above breaks && chain; even if mkfifo breaks (hence not
>> allowing cat to run), "git diff" will go ahead and run, no?
>
> Hmm.  Well, what I really wanted to put here was a "process substitution":
>
>     git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&
>
> but I did not see this feature listed in the dash(1) manpage, so I
> assumed it wasn't allowed by POSIX.  And, having looked, I indeed
> don't see it mentioned in POSIX either.
>
> I'm not terribly surprised that I screwed up the translation to FIFOs;
> how would I really want to do it?

How about not doing a fifo?

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Antoine Pelisse]

On Tue, Dec 17, 2013 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:
>
>> On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Samuel Bronson <naesten@gmail.com> writes:
>>>
>>>>  for i in 1 2
>>>>  do
>>>>       test_expect_success "orderfile using option ($i)" '
>>>>       git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>>>>       test_cmp expect_$i actual
>>>>  '
>>>
>>> This funny indentation in the previous step needs to be fixed, and
>>> the added block below should match.
>>
>> Even though this results in oddly-indented --verbose output?
>>
>>>> +     rm -f order_fifo &&
>>>> +     mkfifo order_fifo &&
>>>> +     cat order_file_$i >order_fifo &
>>>> +     git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
>>>
>>> I think this part can be racy depending on which between cat and
>>> "git diff" are scheduled first, no?  Try running this test under
>>> load and I think you will see it deadlocked.
>>>
>>> Besides, the above breaks && chain; even if mkfifo breaks (hence not
>>> allowing cat to run), "git diff" will go ahead and run, no?
>>
>> Hmm.  Well, what I really wanted to put here was a "process substitution":
>>
>>     git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&
>>
>> but I did not see this feature listed in the dash(1) manpage, so I
>> assumed it wasn't allowed by POSIX.  And, having looked, I indeed
>> don't see it mentioned in POSIX either.
>>
>> I'm not terribly surprised that I screwed up the translation to FIFOs;
>> how would I really want to do it?
>
> How about not doing a fifo?

That would certainly defeat the purpose of the test, which is to test
against a fifo :-)
I'm not sure about the deadlock though. Both read and write will wait
for each other to start operating on the fifo.

You can probably fix the &&-chain by doing something like:

    mkfifo order_fifo && {
        cat order_file_$i >order_fifo &
        git diff -O order_fifo --name-only HEAD^..HEAD >actual
    } && ...

Also, "rm -f order_fifo" should probably be done in test_when_finished
rather than at the beginning of the test.

Antoine,

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

>> How about not doing a fifo?
>
> That would certainly defeat the purpose of the test, which is to test
> against a fifo :-)

My point was that I did not see much value in reading the orderfile
data from anything but a file.  At that point, you are not testing
the "diff -O" orderfile option, but if strbuf_readline() reads from
a non-regular file.

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Samuel Bronson]

On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> My point was that I did not see much value in reading the orderfile
> data from anything but a file.  At that point, you are not testing
> the "diff -O" orderfile option, but if strbuf_readline() reads from
> a non-regular file.

Oh, good point, now that you state it explicitly.  I'll remove it.

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> My point was that I did not see much value in reading the orderfile
>> data from anything but a file.  At that point, you are not testing
>> the "diff -O" orderfile option, but if strbuf_readline() reads from
>> a non-regular file.
>
> Oh, good point, now that you state it explicitly.  I'll remove it.

Or you can study the fix-up I (tentatively) queued on top of your
series in 'pu'.  Also see $gmane/239409.

Thanks.

24331790 (FIXUP! tests, 2013-12-17)

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index f906dea..db0e427 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -22,14 +22,12 @@ test_expect_success 'setup' '
 	*Makefile
 	*.txt
 	*.h
-	*
 	EOF
 
 	cat >order_file_2 <<-\EOF &&
 	*Makefile
 	*.h
 	*.c
-	*
 	EOF
 
 	cat >expect_none <<-\EOF &&
@@ -77,27 +75,30 @@ test_expect_success 'orderfile is a directory' '
 for i in 1 2
 do
 	test_expect_success "orderfile using option ($i)" '
-	git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
-	test_cmp expect_$i actual
-'
+		git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_$i actual
+	'
 
 	test_expect_success PIPE "orderfile is fifo ($i)" '
-	rm -f order_fifo &&
-	mkfifo order_fifo &&
-	cat order_file_$i >order_fifo &
-	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
-	test_cmp expect_$i actual
-'
+		rm -f order_fifo &&
+		mkfifo order_fifo &&
+		{
+			cat order_file_$i >order_fifo &
+		} &&
+		git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
+		wait &&
+		test_cmp expect_$i actual
+	'
 
 	test_expect_success "orderfile using config ($i)" '
-	git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&
-	test_cmp expect_$i actual
-'
+		git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_$i actual
+	'
 
 	test_expect_success "cancelling configured orderfile ($i)" '
-	git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&
-	test_cmp expect_none actual
-'
+		git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_none actual
+	'
 done
 
 test_done
-- 
1.8.5.2-297-g3e57c29

Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> I'm not sure about the deadlock though. Both read and write will wait
> for each other to start operating on the fifo.

It is true only if the fifo already exists.  That is, if you did
this:

	a lot of &&
        commands &&
        before &&
        mkfifo fifo &&
        feed >fifo &

	git diff -Ofifo
        
the consumer may attempt to open and read fifo when the other
process is still running a lot of commands, no?

>
> You can probably fix the &&-chain by doing something like:
>
>     mkfifo order_fifo && {
>         cat order_file_$i >order_fifo &
>         git diff -O order_fifo --name-only HEAD^..HEAD >actual
>     } && ...
>
> Also, "rm -f order_fifo" should probably be done in test_when_finished
> rather than at the beginning of the test.
>
> Antoine,

[PATCH v4 3/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

diff.orderfile acts as a default for the -O command line option.

[sb: split up aw's original patch; reworked tests and docs,
treat option as pathname]

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 223b931..f07b451 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -98,6 +98,11 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.orderfile::
+	File indicating how to order files within a diff, using
+	one shell glob pattern per line.
+	Can be overridden by the '-O' option to linkgit:git-diff[1].
+
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the 'git diff' option '-l'.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bbed2cd..9b37b2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -432,6 +432,9 @@ endif::git-format-patch[]
 -O<orderfile>::
 	Output the patch in the order specified in the
 	<orderfile>, which has one shell glob pattern per line.
+	This overrides the `diff.orderfile` configuration variable
+	(see linkgit:git-config[1]).  To cancel `diff.orderfile`,
+	use `-O/dev/null`.
 
 ifndef::git-format-patch[]
 -R::
diff --git a/diff.c b/diff.c
index 3950e01..0099b99 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
+	if (!strcmp(var, "diff.orderfile"))
+		return git_config_pathname(&diff_order_file_cfg, var, value);
 
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
 
+	options->orderfile = diff_order_file_cfg;
+
 	if (diff_no_prefix) {
 		options->a_prefix = options->b_prefix = "";
 	} else if (!diff_mnemonic_prefix) {
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index eb471e7..50689d1 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -90,6 +90,16 @@ do
 	git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
 	test_cmp expect_$i actual
 '
+
+	test_expect_success "orderfile using config ($i)" '
+	git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_$i actual
+'
+
+	test_expect_success "cancelling configured orderfile ($i)" '
+	git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_none actual
+'
 done
 
 test_done
-- 
1.8.4.3

[PATCH v5 0/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

I expect you've figured out what this patch series is about by now.
In this version, I've applied Junio's suggestions from the last
version, and also the stuff from the FIXUP commit he made after my
stuff in the branch he merged into 'pu'.

Samuel Bronson (3):
  diff: Tests for "git diff -O"
  diff: Let "git diff -O" read orderfile from any file, fail properly
  diff: Add diff.orderfile configuration variable

 Documentation/diff-config.txt  |   5 ++
 Documentation/diff-options.txt |   3 ++
 diff.c                         |   5 ++
 diffcore-order.c               |  23 ++++-----
 t/t4056-diff-order.sh          | 106 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 127 insertions(+), 15 deletions(-)
 create mode 100755 t/t4056-diff-order.sh

-- 
1.8.4.3

Re: [PATCH v5 0/3] diff: Add diff.orderfile configuration variable

[Junio C Hamano]

Looks good; will replace and merge to 'next', but not today (I am
already deep into today's integration cycle).

Thanks.

[PATCH v5 1/3] diff: Tests for "git diff -O"

[Samuel Bronson]

Heavily adapted from Anders' patch:
"diff: Add diff.orderfile configuration variable"

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 t/t4056-diff-order.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..218f171
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='diff order'
+
+. ./test-lib.sh
+
+create_files () {
+	echo "$1" >a.h &&
+	echo "$1" >b.c &&
+	echo "$1" >c/Makefile &&
+	echo "$1" >d.txt &&
+	git add a.h b.c c/Makefile d.txt &&
+	git commit -m"$1"
+}
+
+test_expect_success 'setup' '
+	mkdir c &&
+	create_files 1 &&
+	create_files 2 &&
+
+	cat >order_file_1 <<-\EOF &&
+	*Makefile
+	*.txt
+	*.h
+	EOF
+
+	cat >order_file_2 <<-\EOF &&
+	*Makefile
+	*.h
+	*.c
+	EOF
+
+	cat >expect_none <<-\EOF &&
+	a.h
+	b.c
+	c/Makefile
+	d.txt
+	EOF
+
+	cat >expect_1 <<-\EOF &&
+	c/Makefile
+	d.txt
+	a.h
+	b.c
+	EOF
+
+	cat >expect_2 <<-\EOF &&
+	c/Makefile
+	a.h
+	b.c
+	d.txt
+	EOF
+
+	true	# end chain of &&
+'
+
+test_expect_success "no order (=tree object order)" '
+	git diff --name-only HEAD^..HEAD >actual &&
+	test_cmp expect_none actual
+'
+
+for i in 1 2
+do
+	test_expect_success "orderfile using option ($i)" '
+		git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_$i actual
+	'
+done
+
+test_done
-- 
1.8.4.3

[PATCH v5 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

[Samuel Bronson]

The -O flag really shouldn't silently fail to do anything when given a
path that it can't read from.

However, it should be able to read from un-mmappable files, such as:

 * pipes/fifos

 * /dev/null:  It's a character device (at least on Linux)

 * ANY empty file:

   Quoting Linux mmap(2), "SUSv3 specifies that mmap() should fail if
   length is 0.  However, in kernels before 2.6.12, mmap() succeeded in
   this case: no mapping was created and the call returned addr.  Since
   kernel 2.6.12, mmap() fails with the error EINVAL for this case."

We especially want "-O/dev/null" to work, since we will be documenting
it as the way to cancel "diff.orderfile" when we add that.

(Note: "-O/dev/null" did have the right effect, since the existing error
handling essentially worked out to "silently ignore the orderfile".  But
this was probably more coincidence than anything else.)

So, lets toss all of that logic to get the file mmapped and just use
strbuf_read_file() instead, which gives us decent error handling
practically for free.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..a63f332 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -10,28 +10,21 @@ static int order_cnt;
 
 static void prepare_order(const char *orderfile)
 {
-	int fd, cnt, pass;
+	int cnt, pass;
+	struct strbuf sb = STRBUF_INIT;
 	void *map;
 	char *cp, *endp;
-	struct stat st;
-	size_t sz;
+	ssize_t sz;
 
 	if (order)
 		return;
 
-	fd = open(orderfile, O_RDONLY);
-	if (fd < 0)
-		return;
-	if (fstat(fd, &st)) {
-		close(fd);
-		return;
-	}
-	sz = xsize_t(st.st_size);
-	map = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
-	close(fd);
-	if (map == MAP_FAILED)
-		return;
+	sz = strbuf_read_file(&sb, orderfile, 0);
+	if (sz < 0)
+		die_errno(_("failed to read orderfile '%s'"), orderfile);
+	map = strbuf_detach(&sb, NULL);
 	endp = (char *) map + sz;
+
 	for (pass = 0; pass < 2; pass++) {
 		cnt = 0;
 		cp = map;
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 218f171..0ac1b95 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -59,12 +59,38 @@ test_expect_success "no order (=tree object order)" '
 	test_cmp expect_none actual
 '
 
+test_expect_success 'missing orderfile' '
+	rm -f bogus_file &&
+	test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
+'
+
+test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '
+	>unreadable_file &&
+	chmod -r unreadable_file &&
+	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'orderfile is a directory' '
+	test_must_fail git diff -O/ --name-only HEAD^..HEAD
+'
+
 for i in 1 2
 do
 	test_expect_success "orderfile using option ($i)" '
 		git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
 		test_cmp expect_$i actual
 	'
+
+	test_expect_success PIPE "orderfile is fifo ($i)" '
+		rm -f order_fifo &&
+		mkfifo order_fifo &&
+		{
+			cat order_file_$i >order_fifo &
+		} &&
+		git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
+		wait &&
+		test_cmp expect_$i actual
+	'
 done
 
 test_done
-- 
1.8.4.3

[PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out

[Jonathan Nieder]

There is no guarantee that strbuf_read_file must error out for
directories.  On some operating systems (e.g., Debian GNU/kFreeBSD
wheezy), reading a directory gives its raw content:

	$ head -c5 < / | cat -A
	^AM-|^_^@^L$

As a result, 'git diff -O/' succeeds instead of erroring out on
these systems, causing t4056.5 "orderfile is a directory" to fail.

On some weird OS it might even make sense to pass a directory to the
-O option and this is not a common user mistake that needs catching.
Remove the test.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

t4056 is failing on systems using glibc with the kernel of FreeBSD[1]:

| expecting success: 
| 	test_must_fail git diff -O/ --name-only HEAD^..HEAD
|
| a.h
| b.c
| c/Makefile
| d.txt
| test_must_fail: command succeeded: git diff -O/ --name-only HEAD^..HEAD
| not ok 5 - orderfile is a directory

How about this patch?

Thanks,
Jonathan

[1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=kfreebsd-amd64&ver=1%3A2.0~next.20140107-1&stamp=1389379274

 t/t4056-diff-order.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 1ddd226..9e2b29e 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -68,10 +68,6 @@ test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '
 	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
 '
 
-test_expect_success 'orderfile is a directory' '
-	test_must_fail git diff -O/ --name-only HEAD^..HEAD
-'
-
 for i in 1 2
 do
 	test_expect_success "orderfile using option ($i)" '
-- 
1.8.5.1

Re: [PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> There is no guarantee that strbuf_read_file must error out for
> directories.  On some operating systems (e.g., Debian GNU/kFreeBSD
> wheezy), reading a directory gives its raw content:
>
> 	$ head -c5 < / | cat -A
> 	^AM-|^_^@^L$
>
> As a result, 'git diff -O/' succeeds instead of erroring out on
> these systems, causing t4056.5 "orderfile is a directory" to fail.
>
> On some weird OS it might even make sense to pass a directory to the
> -O option and this is not a common user mistake that needs catching.
> Remove the test.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Hi,
>
> t4056 is failing on systems using glibc with the kernel of FreeBSD[1]:
>
> | expecting success: 
> | 	test_must_fail git diff -O/ --name-only HEAD^..HEAD
> |
> | a.h
> | b.c
> | c/Makefile
> | d.txt
> | test_must_fail: command succeeded: git diff -O/ --name-only HEAD^..HEAD
> | not ok 5 - orderfile is a directory
>
> How about this patch?

Sounds sensible. Thanks.



> Thanks,
> Jonathan
>
> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=kfreebsd-amd64&ver=1%3A2.0~next.20140107-1&stamp=1389379274
>
>  t/t4056-diff-order.sh | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> index 1ddd226..9e2b29e 100755
> --- a/t/t4056-diff-order.sh
> +++ b/t/t4056-diff-order.sh
> @@ -68,10 +68,6 @@ test_expect_success POSIXPERM,SANITY 'unreadable orderfile' '
>  	test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
>  '
>  
> -test_expect_success 'orderfile is a directory' '
> -	test_must_fail git diff -O/ --name-only HEAD^..HEAD
> -'
> -
>  for i in 1 2
>  do
>  	test_expect_success "orderfile using option ($i)" '

[PATCH v5 3/3] diff: Add diff.orderfile configuration variable

[Samuel Bronson]

diff.orderfile acts as a default for the -O command line option.

[sb: split up aw's original patch; rework tests and docs, treat option
as pathname]

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 223b931..f07b451 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -98,6 +98,11 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.orderfile::
+	File indicating how to order files within a diff, using
+	one shell glob pattern per line.
+	Can be overridden by the '-O' option to linkgit:git-diff[1].
+
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
 	detection; equivalent to the 'git diff' option '-l'.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bbed2cd..9b37b2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -432,6 +432,9 @@ endif::git-format-patch[]
 -O<orderfile>::
 	Output the patch in the order specified in the
 	<orderfile>, which has one shell glob pattern per line.
+	This overrides the `diff.orderfile` configuration variable
+	(see linkgit:git-config[1]).  To cancel `diff.orderfile`,
+	use `-O/dev/null`.
 
 ifndef::git-format-patch[]
 -R::
diff --git a/diff.c b/diff.c
index b79432b..f35c83b 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -201,6 +202,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return git_config_string(&external_diff_cmd_cfg, var, value);
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
+	if (!strcmp(var, "diff.orderfile"))
+		return git_config_pathname(&diff_order_file_cfg, var, value);
 
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
@@ -3207,6 +3210,8 @@ void diff_setup(struct diff_options *options)
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
 
+	options->orderfile = diff_order_file_cfg;
+
 	if (diff_no_prefix) {
 		options->a_prefix = options->b_prefix = "";
 	} else if (!diff_mnemonic_prefix) {
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 0ac1b95..acd7683 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -91,6 +91,16 @@ do
 		wait &&
 		test_cmp expect_$i actual
 	'
+
+	test_expect_success "orderfile using config ($i)" '
+		git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_$i actual
+	'
+
+	test_expect_success "cancelling configured orderfile ($i)" '
+		git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only HEAD^..HEAD >actual &&
+		test_cmp expect_none actual
+	'
 done
 
 test_done
-- 
1.8.4.3