From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 06 Dec 2013 18:11:46 GMT
Subject: Re: [PATCH v2] diff: Add diff.orderfile configuration variable
Message-ID: <xmqqhaal3l3x.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1386312508-7421-1-git-send-email-naesten@gmail.com>

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.