From: Richard Hansen Date: Mon, 09 Jan 2017 22:57:33 GMT Subject: Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled Message-ID: <472a8db7-a2d1-5f3a-a43c-0942e744f90b@google.com> In-Reply-To: On 2017-01-09 14:05, Junio C Hamano wrote: > 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. Thanks for this; I'll cook up a reroll. I tried this approach before I emailed the v3 reroll, except I left out the "--" argument to rev-parse. This caused the tests to fail due to "ambiguous argument: unknown revision or path not in the working tree". I didn't think to add the "--" because I got it in my head that "--" shouldn't be used with --prefix because it shows up in the output, plus the example in the rev-parse documentation doesn't use it. I'll patch rev-parse's man page to use "--" in the example. Thanks, Richard > > 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 > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh > index dfd641d34b..180dd7057a 100755 > --- a/t/t7610-mergetool.sh > +++ b/t/t7610-mergetool.sh > @@ -678,6 +678,11 @@ test_expect_success 'diff.orderFile configuration is honored' ' > b > a > EOF > + > + # make sure "order-file" that is ambiguous between > + # rev and path is understood correctly. > + git branch order-file HEAD && > + > git mergetool --no-prompt --tool myecho >output && > git grep --no-index -h -A2 Merging: output >actual && > test_cmp expect actual >