Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
- From
- Richard Hansen <hansenr@google.com>
- Date
- Jan 10, 2017, 17:09 UTC
- Message-ID
- <10bd75ad-15d6-5694-bdf1-6b820d0b3201@google.com>
- In-Reply-To
- <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org>
On 2017-01-10 01:17, Johannes Sixt wrote:
Show 36 quoted lines
> 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.
Show 20 quoted lines
> 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.)
Show 5 quoted lines
> 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.
Show 6 quoted lines
> >> + 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 >