From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 07 Oct 2016 17:16:40 GMT
Subject: Re: [PATCH 4/4] mergetool: honor -O<order-file>
Message-ID: <8f99b183-7f9b-f577-42eb-9ff7398ffbd1@kdbg.org>
In-Reply-To: <1475797679-32712-4-git-send-email-davvid@gmail.com>

Am 07.10.2016 um 01:47 schrieb David Aguilar:
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 65696d8..9dca58b 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -9,7 +9,7 @@
>  # at the discretion of Junio C Hamano.
>  #
>
> -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
> +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O<order-file>] [file to merge] ...'
>  SUBDIRECTORY_OK=Yes
>  NONGIT_OK=Yes
>  OPTIONS_SPEC=
> @@ -390,6 +390,7 @@ print_noop_and_exit () {
>  main () {
>  	prompt=$(git config --bool mergetool.prompt)
>  	guessed_merge_tool=false
> +	order_file=
>
>  	while test $# != 0
>  	do
> @@ -419,6 +420,9 @@ main () {
>  		--prompt)
>  			prompt=true
>  			;;
> +		-O*)
> +			order_file="$1"
> +			;;
>  		--)
>  			shift
>  			break
> @@ -459,8 +463,14 @@ main () {
>  		fi
>  	fi
>
> +	if test -n "$order_file"
> +	then
> +		set -- "$order_file" -- "$@"
> +	else
> +		set -- -- "$@"
> +	fi
>  	files=$(git -c core.quotePath=false \
> -		diff --name-only --diff-filter=U -- "$@")
> +		diff --name-only --diff-filter=U "$@")

You can write this as

	files=$(git -c core.quotePath=false \
		diff --name-only --diff-filter=U \
		${order_file:+"$order_file"} -- "$@")

without the case distinction earlier. This construct is usually used to 
attach the option (-O) in front of the argument, but it is already 
included in the value; so, we use the construct only to avoid an empty 
argument when the option is unset and to get a quoted string when it is set.

>
>  	cd_to_toplevel
>
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index fb2aeef..0f9869a 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -637,6 +637,33 @@ test_expect_success 'diff.orderFile configuration is honored' '
>  	test_cmp expect actual &&
>  	git reset --hard >/dev/null
>  '
> +
> +test_expect_success 'mergetool -Oorder-file is honored' '
> +	test_config diff.orderFile order-file &&
> +	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
> +	test_config mergetool.myecho.trustExitCode true &&
> +	test_must_fail git merge order-file-side1 &&
> +	cat >expect <<-\EOF &&
> +		Merging:
> +		a
> +		b
> +	EOF
> +	git mergetool -O/dev/null --no-prompt --tool myecho |
> +	grep -A 2 Merging: >actual &&
> +	test_cmp expect actual &&
> +	git reset --hard >/dev/null 2>&1 &&
> +
> +	git config --unset diff.orderFile &&
> +	test_must_fail git merge order-file-side1 &&
> +	cat >expect <<-\EOF &&
> +		Merging:
> +		b
> +		a
> +	EOF
> +	git mergetool -Oorder-file --no-prompt --tool myecho |
> +	grep -A 2 Merging: >actual &&

grep -A again. Furthermore, you call git mergetool in the upstream of a 
pipe. This will ignore the exit status.

> +	test_cmp expect actual &&
> +	git reset --hard >/dev/null 2>&1
>  '

Ah, the earlier lone quote should have been added in this patch.

-- Hannes