From: Johannes Sixt Date: Mon, 09 Jan 2017 19:53:15 GMT Subject: Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled Message-ID: In-Reply-To: Am 09.01.2017 um 20:05 schrieb Junio C Hamano: > Junio C Hamano writes: > >> I wonder if it makes more sense to always move to toplevel upfront >> and consistently use path from the toplevel, perhaps like the patch > > s/the patch/the attached patch/ I meant. > >> 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"). > > And here is a squash-able patch to illustrate what I mean. > > I removed both of the comment blocks as the code always works with > the worktree-relative pathname after this patch while adjusting > end-user supplied paths from relative to original cwd. As that is > how the core parts of the system (including the parts written in C) > work, even though an explanation you did in the log message is > needed to explain why the change was needed and what the change > intended to do to readers of "git log", it is not necessary to > explain it to the readers of the latest code, which is what the > in-code comment is about. > > The single-liner addition to the test creates a branch whose name is > the same as the specified orderfile to deliberately create a > confusing situation. I haven't tried, but I am fairly sure that the > test will demonstrate how broken the orderfile=$(...) in the > original is, if you apply the test part of the attached patch, > without the changes to git-mergetool.sh, to your version. > > > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 22f56c25a2..21f82d5b58 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -454,53 +454,34 @@ main () { > merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" > merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)" > > - if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR" > + prefix=$(git rev-parse --show-prefix) || exit 1 > + cd_to_toplevel > + > + if test -n "$orderfile" > then > - # The pathnames output by the 'git rerere remaining' > - # command below are relative to the top-level > - # directory but the 'git diff --name-only' command > - # further below expects the pathnames to be relative > - # to the current working directory. Thus, we cd to > - # the top-level directory before running 'git diff > - # --name-only'. We change directories even earlier > - # (before running 'git rerere remaining') in case 'git > - # rerere remaining' is ever changed to output > - # pathnames relative to the current working directory. > - # > - # Changing directories breaks a relative $orderfile > - # pathname argument, so fix it up to be relative to > - # the top-level directory. > - > - 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 > + orderfile=$( > + git rev-parse --prefix "$prefix" -- "$orderfile" | > + sed -e 1d > + ) > + fi > > + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR" > + then > set -- $(git rerere remaining) > if test $# -eq 0 > then > print_noop_and_exit > fi > + elif test $# -ge 0 > + then > + eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")" > + shift > fi > > - # Note: The pathnames output by 'git diff --name-only' are > - # relative to the top-level directory, but it expects input > - # pathnames to be relative to the current working directory. > - # Thus: > - # * Either cd_to_toplevel must not be run before this or all > - # relative input pathnames must be converted to be > - # relative to the top-level directory (or absolute). > - # * Either cd_to_toplevel must be run after this or all > - # relative output pathnames must be converted to be > - # relative to the current working directory (or absolute). > 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 The control flow after this patch looks much more like what I had in mind. Thanks. -- Hannes