Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
On 2017-01-09 18:29, Junio C Hamano wrote: Junio C Hamanowrites: 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. By the way, I didn't think this through, but how is the orderfile that comes from the configuration file handled when it is not an absolute path? Good question; it does whatever 'git diff' does, and that case isn't documented. It's also unclear if the globs are like the .gitignore patterns. I would expect it to act like $GIT_DIR/info/exclude, but I haven't checked. I think it is _wrong_ to take it as relative to where the user started the program. Agreed. The -O parameter from the command line, when is not absolute, should be taken as relative to where the user _thinks_ s/he is, Agreed. but when it comes from the diff.orderfile configuration and it is not absolute, it should be taken as relative to the top of the working tree. Agreed. As we always cd_to_top with the suggested SQUASH, it means that the orderfile that came from the configuration does not have to be touched, while the orderfile given via -O on the command line needs prefixing. Yes. By unconditionally running cd_to_top we should get the behavior we want even if 'git diff' uses the current working directory rather than the top-level directory. I'll poke at 'git diff' to see what it does. -Richard smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
Junio C Hamanowrites: > By the way, I didn't think this through, but how is the orderfile > that comes from the configuration file handled when it is not an > absolute path? I think it is _wrong_ to take it as relative to > where the user started the program. Answering my own question: the program does not handle configured orderfile at all, so there is no gotcha ;-)
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
Junio C Hamanowrites: > 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. By the way, I didn't think this through, but how is the orderfile that comes from the configuration file handled when it is not an absolute path? I think it is _wrong_ to take it as relative to where the user started the program. The -O parameter from the command line, when is not absolute, should be taken as relative to where the user _thinks_ s/he is, but when it comes from the diff.orderfile configuration and it is not absolute, it should be taken as relative to the top of the working tree. As we always cd_to_top with the suggested SQUASH, it means that the orderfile that came from the configuration does not have to be touched, while the orderfile given via -O on the command line needs prefixing.
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
On 2017-01-09 14:05, Junio C Hamano wrote: Junio C Hamanowrites: 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 -
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
Am 09.01.2017 um 20:05 schrieb Junio C Hamano: Junio C Hamanowrites: 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
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
Junio C Hamanowrites: > 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 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 @@
Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
Richard Hansenwrites: > If rerere is enabled and no pathnames are given, run cd_to_toplevel > before running 'git diff --name-only' so that 'git diff --name-only' > sees the files named by 'git rerere remaining', which outputs > pathnames relative to the top-level directory. > > The cd_to_toplevel command could be run after 'git rerere remaining', > but it is run before just in case 'git rerere remaining' is ever > changed to print pathnames relative to the current working directory > rather than relative to the top-level directory. > > An alternative approach would be to unconditionally convert all > relative pathnames (including the orderfile pathname) to be relative > to the top-level directory and then run cd_to_toplevel before 'git > diff --name-only', but unfortunately 'git rev-parse --prefix' requires > valid pathnames, which would break some valid use cases. > > This fixes a regression introduced in > 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0). > > Signed-off-by: Richard Hansen > --- > git-mergetool.sh | 32 > t/t7610-mergetool.sh | 2 +- > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/git-mergetool.sh b/git-mergetool.sh > index b506896dc..22f56c25a 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -456,6 +456,28 @@ main () { > While doing the extra cd_to_toplevel added by this patch may not break anything that comes after the existing cd_to_toplevel, I find the result of applying this patch unnecessarily confusing. As "if the user didn't give any pathnames from the command line, ask rerere what paths it thinks are necessary to be handled" is merely a laziness fallback, it feels conceptually wrong to use different invocations of -O$orderfile when rerere is and is not in effect. I wonder if it makes more sense to always move to toplevel upfront and consistently use path from the toplevel, perhaps like the patch 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"). diff --git a/git-mergetool.sh b/git-mergetool.sh index b506896dc1..adbbeceb47 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -454,6 +454,14 @@ 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 + fi + if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR" then set -- $(git rerere remaining) @@ -461,14 +469,16 @@ main () { then print_noop_and_exit fi + elif test $# -ge 0 + then + eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")" + shift fi 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