Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled

From: Johannes Sixt <j6t@kdbg.org>
To: Richard Hansen <hansenr@google.com>
Cc: git@vger.kernel.org, davvid@gmail.com, sbeller@google.com,
	simon@ruderich.org, gitster@pobox.com
Subject: Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
Date: Tue, 10 Jan 2017 07:17:51 +0100	[thread overview]
Message-ID: <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org> (raw)
In-Reply-To: <20170109232941.43637-15-hansenr@google.com>

Am 10.01.2017 um 00:29 schrieb Richard Hansen:
> The pathnames output by the 'git rerere remaining' command are
> relative to the top-level directory but the 'git diff --name-only'
> command expects its pathname arguments to be relative to the current
> working directory.  Run cd_to_toplevel before running 'git diff
> --name-only' and adjust any relative pathnames so that 'git mergetool'
> does not fail when run from a subdirectory with rerere enabled.
>
> This fixes a regression introduced in
> 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
>
> Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
>  git-mergetool.sh     | 16 ++++++++++++++--
>  t/t7610-mergetool.sh |  7 ++++++-
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index b506896dc..cba6bbd05 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,6 +454,15 @@ 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
> +		orderfile=$(printf %s\\n "$orderfile" | sed -e 1d)

Is the purpose of this complication only to detect errors of the git 
invocation? IMHO, we could dispense with that, but others might 
disagree. I am arguing because this adds yet another process; but it is 
only paid when -O is used, so...

> +	fi
> +
>  	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
>  	then
>  		set -- $(git rerere remaining)
> @@ -461,14 +470,17 @@ main () {
>  		then
>  			print_noop_and_exit
>  		fi
> +	elif test $# -ge 0
> +	then
> +		files_quoted=$(git rev-parse --sq --prefix "$prefix" -- "$@") || exit 1
> +		eval "set -- $files_quoted"

BTW, the --sq and eval business is not required here. At this point, 
$IFS = $'\n', so

		set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")

will do. (Except that it would not detect errors.)

> +		shift
>  	fi

As I don't see anything wrong with what you have written, these comments 
alone do not warrant another re-roll.

-- Hannes