Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled

[Richard Hansen <hansenr@google.com>]

[-- Attachment #1: Type: text/plain, Size: 5501 bytes --]

On 2017-01-09 14:05, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does.  The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.

Thanks for this; I'll cook up a reroll.

I tried this approach before I emailed the v3 reroll, except I left out 
the "--" argument to rev-parse.  This caused the tests to fail due to 
"ambiguous argument: unknown revision or path not in the working tree". 
I didn't think to add the "--" because I got it in my head that "--" 
shouldn't be used with --prefix because it shows up in the output, plus 
the example in the rev-parse documentation doesn't use it.  I'll patch 
rev-parse's man page to use "--" in the example.

Thanks,
Richard

>
> I removed both of the comment blocks as the code always works with
> the worktree-relative pathname after this patch while adjusting
> end-user supplied paths from relative to original cwd.  As that is
> how the core parts of the system (including the parts written in C)
> work, even though an explanation you did in the log message is
> needed to explain why the change was needed and what the change
> intended to do to readers of "git log", it is not necessary to
> explain it to the readers of the latest code, which is what the
> in-code comment is about.
>
> The single-liner addition to the test creates a branch whose name is
> the same as the specified orderfile to deliberately create a
> confusing situation.  I haven't tried, but I am fairly sure that the
> test will demonstrate how broken the orderfile=$(...) in the
> original is, if you apply the test part of the attached patch,
> without the changes to git-mergetool.sh, to your version.
>
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 22f56c25a2..21f82d5b58 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,53 +454,34 @@ main () {
>  	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>  	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>
> -	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> +	prefix=$(git rev-parse --show-prefix) || exit 1
> +	cd_to_toplevel
> +
> +	if test -n "$orderfile"
>  	then
> -		# The pathnames output by the 'git rerere remaining'
> -		# command below are relative to the top-level
> -		# directory but the 'git diff --name-only' command
> -		# further below expects the pathnames to be relative
> -		# to the current working directory.  Thus, we cd to
> -		# the top-level directory before running 'git diff
> -		# --name-only'.  We change directories even earlier
> -		# (before running 'git rerere remaining') in case 'git
> -		# rerere remaining' is ever changed to output
> -		# pathnames relative to the current working directory.
> -		#
> -		# Changing directories breaks a relative $orderfile
> -		# pathname argument, so fix it up to be relative to
> -		# the top-level directory.
> -
> -		prefix=$(git rev-parse --show-prefix) || exit 1
> -		cd_to_toplevel
> -		if test -n "$orderfile"
> -		then
> -			orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
> -		fi
> +		orderfile=$(
> +			git rev-parse --prefix "$prefix" -- "$orderfile" |
> +			sed -e 1d
> +		)
> +	fi
>
> +	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> +	then
>  		set -- $(git rerere remaining)
>  		if test $# -eq 0
>  		then
>  			print_noop_and_exit
>  		fi
> +	elif test $# -ge 0
> +	then
> +		eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> +		shift
>  	fi
>
> -	# Note:  The pathnames output by 'git diff --name-only' are
> -	# relative to the top-level directory, but it expects input
> -	# pathnames to be relative to the current working directory.
> -	# Thus:
> -	#   * Either cd_to_toplevel must not be run before this or all
> -	#     relative input pathnames must be converted to be
> -	#     relative to the top-level directory (or absolute).
> -	#   * Either cd_to_toplevel must be run after this or all
> -	#     relative output pathnames must be converted to be
> -	#     relative to the current working directory (or absolute).
>  	files=$(git -c core.quotePath=false \
>  		diff --name-only --diff-filter=U \
>  		${orderfile:+"-O$orderfile"} -- "$@")
>
> -	cd_to_toplevel
> -
>  	if test -z "$files"
>  	then
>  		print_noop_and_exit
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index dfd641d34b..180dd7057a 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -678,6 +678,11 @@ test_expect_success 'diff.orderFile configuration is honored' '
>  		b
>  		a
>  	EOF
> +
> +	# make sure "order-file" that is ambiguous between
> +	# rev and path is understood correctly.
> +	git branch order-file HEAD &&
> +
>  	git mergetool --no-prompt --tool myecho >output &&
>  	git grep --no-index -h -A2 Merging: output >actual &&
>  	test_cmp expect actual
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]