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@xxxxxxxxx> Signed-off-by: Richard Hansen <hansenr@xxxxxxxxxx> --- 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 fiAs 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: smime.p7s>>