diff: Add diff.orderfile configuration variable

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

---

 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

index 223b931..51f9190 100644
@@ -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'.

index a04a34d..e66f031 100644
@@ -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) {

+ 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

+ test_debug actual_diff_headers
+ test_cmp expect_diff_headers_2 actual_diff_headers' +
+test_done

1.8.4.1.559.gdb9bdfb.dirty

Hi,

Thanks.

[...]

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

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

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?

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

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?

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

anders

From: Anders Waldenborg <[email protected]>

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

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

Thanks-to: Jonathan Nieder <[email protected]>

---
*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

index 223b931..f07b451 100644
@@ -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'.

index bbed2cd..1af5a5e 100644
@@ -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::

index e34bf97..a92b570 100644
@@ -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) {

1.8.4.3

Thanks for reviving a stalled topic.

Sabotaged in what way?

	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
	'

	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

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

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

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

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.)

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.

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

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 ...)

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

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

(Thanks for the review!)

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

index 23e9385..dd103e3 100644
@@ -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;

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.

Surely, and thanks.

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

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

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

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

---
 t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

new file mode 100755
index 0000000..398b3f6
@@ -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

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.

---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

index 23e9385..a63f332 100644
@@ -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;

index 398b3f6..eb471e7 100755
@@ -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

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)".

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.

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?]

---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

index 223b931..f07b451 100644
@@ -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'.

index bbed2cd..9b37b2a 100644
@@ -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::

index 3950e01..b336aef 100644
@@ -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) {

index eb471e7..50689d1 100755
@@ -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

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.

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 ...

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

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

---
 t/t4056-diff-order.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

new file mode 100755
index 0000000..398b3f6
@@ -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

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.

---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

index 23e9385..a63f332 100644
@@ -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;

index 398b3f6..eb471e7 100755
@@ -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

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.

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

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,

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)

index f906dea..db0e427 100755
@@ -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

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?

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]

---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

index 223b931..f07b451 100644
@@ -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'.

index bbed2cd..9b37b2a 100644
@@ -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::

index 3950e01..0099b99 100644
@@ -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) {

index eb471e7..50689d1 100755
@@ -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

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

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

---
 t/t4056-diff-order.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

new file mode 100755
index 0000000..218f171
@@ -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

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.

---
 diffcore-order.c      | 23 ++++++++---------------
 t/t4056-diff-order.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 15 deletions(-)

index 23e9385..a63f332 100644
@@ -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;

index 218f171..0ac1b95 100755
@@ -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

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.

---
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(-)

index 1ddd226..9e2b29e 100755
@@ -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

Sounds sensible. Thanks.

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]

---
 Documentation/diff-config.txt  |  5 +++++
 Documentation/diff-options.txt |  3 +++
 diff.c                         |  5 +++++
 t/t4056-diff-order.sh          | 10 ++++++++++
 4 files changed, 23 insertions(+)

index 223b931..f07b451 100644
@@ -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'.

index bbed2cd..9b37b2a 100644
@@ -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::

index b79432b..f35c83b 100644
@@ -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) {

index 0ac1b95..acd7683 100755
@@ -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

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

Thanks.