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

Samuel Bronson [off-list ref] writes:

From: Anders Waldenborg <redacted>

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 <redacted>
Thanks-to: Jonathan Nieder [off-list ref]
Signed-off-by: Samuel Bronson <redacted>

Thanks for reviving a stalled topic.

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

Sabotaged in what way?

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

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.

On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano [off-list ref] wrote:

Samuel Bronson [off-list ref] 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.

quoted

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

quoted

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

quoted

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

quoted

+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!)