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

From: Richard Hansen <hansenr@google.com>
To: Johannes Sixt <j6t@kdbg.org>
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 12:09:34 -0500	[thread overview]
Message-ID: <10bd75ad-15d6-5694-bdf1-6b820d0b3201@google.com> (raw)
In-Reply-To: <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org>

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

On 2017-01-10 01:17, Johannes Sixt wrote:
> 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?

Yes.  I've been burned so many times by the shell not stopping when 
there's an error that I always like to do things one step at a time with 
error checking, even if it is less efficient.

> 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',

Whoa, really?  That's surprising and feels wrong.

(BTW, I just noticed that guess_merge_tool sets IFS to a space without 
resetting it afterward.  That function is always run in a subshell so 
there's no problem at the moment, but it's still a bit risky.)

> so
>
>         set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>
> will do. (Except that it would not detect errors.)

I think I would prefer to leave it as-is in case IFS is ever changed 
back to the default.

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

I'll reroll if I hear other votes in favor of changing.  Thanks for 
reviewing!

-Richard

>
> -- Hannes
>

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