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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2017, 18:12 UTC
Message-ID: <xmqq4m18ump1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170109054238.42599-14-hansenr@google.com>

Richard Hansen <hansenr@google.com> writes:

Show 31 quoted lines

> If rerere is enabled and no pathnames are given, run cd_to_toplevel
> before running 'git diff --name-only' so that 'git diff --name-only'
> sees the files named by 'git rerere remaining', which outputs
> pathnames relative to the top-level directory.
>
> The cd_to_toplevel command could be run after 'git rerere remaining',
> but it is run before just in case 'git rerere remaining' is ever
> changed to print pathnames relative to the current working directory
> rather than relative to the top-level directory.
>
> An alternative approach would be to unconditionally convert all
> relative pathnames (including the orderfile pathname) to be relative
> to the top-level directory and then run cd_to_toplevel before 'git
> diff --name-only', but unfortunately 'git rev-parse --prefix' requires
> valid pathnames, which would break some valid use cases.
>
> This fixes a regression introduced in
> 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
>
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
>  git-mergetool.sh     | 32 ++++++++++++++++++++++++++++++++
>  t/t7610-mergetool.sh |  2 +-
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index b506896dc..22f56c25a 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -456,6 +456,28 @@ main () {
>

While doing the extra cd_to_toplevel added by this patch may not break anything that comes after the existing cd_to_toplevel, I find the result of applying this patch unnecessarily confusing. As "if the user didn't give any pathnames from the command line, ask rerere what paths it thinks are necessary to be handled" is merely a laziness fallback, it feels conceptually wrong to use different invocations of -O$orderfile when rerere is and is not in effect.

I wonder if it makes more sense to always move to toplevel upfront and consistently use path from the toplevel, perhaps like the patch 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").

diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc1..adbbeceb47 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,6 +454,14 @@ main () {
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
+	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
+
 	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
 	then
 		set -- $(git rerere remaining)
@@ -461,14 +469,16 @@ main () {
 		then
 			print_noop_and_exit
 		fi
+	elif test $# -ge 0
+	then
+		eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
+		shift
 	fi
 
 	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