# [PATCH] mergetool--lib: Allow custom commands to override built-ins 4 messages from 2012-09-25 to 2012-09-26. Participants: David Aguilar, Ramkumar Ramachandra, Junio C Hamano. Thread: https://gitlist.dev/t/31644 ## David Aguilar, 2012-09-25 07:48 Subject: [PATCH] mergetool--lib: Allow custom commands to override built-ins Message-ID: <1348559291-71739-1-git-send-email-davvid@gmail.com> URL: https://gitlist.dev/e/1348559291-71739-1-git-send-email-davvid%40gmail.com ``` Allow users to override the default commands provided by the mergetools/* scriptlets. Users occasionally run into problems where they expect to be able to override the built-in tool names. The documentation does not explicitly mention that built-ins cannot be overridden, so it's easy to assume that it should work. Lift this restriction so that built-in tools are handled the same way as user-configured tools. Add tests to guarantee this behavior. A nice benefit of this change is that it protects users from having future versions of git trump their custom configuration with a new built-in tool. C.f.: http://stackoverflow.com/questions/7435002/mergetool-from-gitconfig-being-ignored http://thread.gmane.org/gmane.comp.version-control.msysgit/13188 http://thread.gmane.org/gmane.comp.version-control.git/148267 Signed-off-by: David Aguilar --- The Cc: list includes folks who were involved in the above threads. Hopefully we can do without the workarounds now ;-) git-mergetool--lib.sh | 40 ++++++++++++++++++++++++++++++++++++++-- mergetools/defaults | 28 ++-------------------------- t/t7610-mergetool.sh | 13 +++++++++++++ t/t7800-difftool.sh | 11 +++++++++++ 4 files changed, 64 insertions(+), 28 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 54cb708..f013a03 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -104,13 +104,49 @@ run_merge_tool () { if merge_mode then - merge_cmd "$1" + run_merge_cmd "$1" else - diff_cmd "$1" + run_diff_cmd "$1" fi return $status } +# Run a either a configured or built-in diff tool +run_diff_cmd () { + merge_tool_cmd="$(get_merge_tool_cmd "$1")" + if test -n "$merge_tool_cmd" + then + ( eval $merge_tool_cmd ) + status=$? + return $status + else + diff_cmd "$1" + fi +} + +# Run a either a configured or built-in merge tool +run_merge_cmd () { + merge_tool_cmd="$(get_merge_tool_cmd "$1")" + if test -n "$merge_tool_cmd" + then + trust_exit_code="$(git config --bool \ + mergetool."$1".trustExitCode || echo false)" + if test "$trust_exit_code" = "false" + then + touch "$BACKUP" + ( eval $merge_tool_cmd ) + status=$? + check_unchanged + else + ( eval $merge_tool_cmd ) + status=$? + fi + return $status + else + merge_cmd "$1" + fi +} + list_merge_tool_candidates () { if merge_mode then diff --git a/mergetools/defaults b/mergetools/defaults index 1d8f2a3..21e63ec 100644 --- a/mergetools/defaults +++ b/mergetools/defaults @@ -8,36 +8,12 @@ can_diff () { } diff_cmd () { - merge_tool_cmd="$(get_merge_tool_cmd "$1")" - if test -z "$merge_tool_cmd" - then - status=1 - break - fi - ( eval $merge_tool_cmd ) - status=$? + status=1 return $status } merge_cmd () { - merge_tool_cmd="$(get_merge_tool_cmd "$1")" - if test -z "$merge_tool_cmd" - then - status=1 - break - fi - trust_exit_code="$(git config --bool \ - mergetool."$1".trustExitCode || echo false)" - if test "$trust_exit_code" = "false" - then - touch "$BACKUP" - ( eval $merge_tool_cmd ) - status=$? - check_unchanged - else - ( eval $merge_tool_cmd ) - status=$? - fi + status=1 return $status } diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 6fa0c70..bc38737 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -509,4 +509,17 @@ test_expect_success 'file with no base' ' git reset --hard master >/dev/null 2>&1 ' +test_expect_success 'custom commands override built-ins' ' + git checkout -b test14 branch1 && + git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" && + git config mergetool.defaults.trustExitCode true && + test_must_fail git merge master && + git mergetool --no-prompt --tool defaults -- both && + echo master both added >expected && + test_cmp both expected && + git config --unset mergetool.defaults.cmd && + git config --unset mergetool.defaults.trustExitCode && + git reset --hard master >/dev/null 2>&1 +' + test_done diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 9c3e997..eb1d3f8 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -76,6 +76,17 @@ test_expect_success PERL 'custom commands' ' test "$diff" = "branch" ' +# Ensures that a custom difftool..cmd overrides built-ins +test_expect_success PERL 'custom commands override built-ins' ' + restore_test_defaults && + git config difftool.defaults.cmd "cat \$REMOTE" && + + diff=$(git difftool --tool defaults --no-prompt branch) && + test "$diff" = "master" && + + git config --unset difftool.defaults.cmd +' + # Ensures that git-difftool ignores bogus --tool values test_expect_success PERL 'difftool ignores bad --tool values' ' diff=$(git difftool --no-prompt --tool=bad-tool branch) -- 1.7.12.1.402.g28d36ee ``` ## Ramkumar Ramachandra, 2012-09-26 10:03 Subject: Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins Message-ID: URL: https://gitlist.dev/e/CALkWK0naJt840LfMNBM7EtdyLE5nmJeAsxG-Wttj8c84O5X7jA%40mail.gmail.com In-Reply-To: <1348559291-71739-1-git-send-email-davvid@gmail.com> ``` Hi David, David Aguilar wrote: > diff_cmd () { > - merge_tool_cmd="$(get_merge_tool_cmd "$1")" > - if test -z "$merge_tool_cmd" > - then > - status=1 > - break > - fi > - ( eval $merge_tool_cmd ) > - status=$? > + status=1 > return $status > } Nit: Why not return 1, instead of setting $status and returning it? Ram ``` ## Junio C Hamano, 2012-09-26 14:06 Subject: Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins Message-ID: <7vtxukubds.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtxukubds.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ramkumar Ramachandra writes: > Hi David, > > David Aguilar wrote: >> diff_cmd () { >> - merge_tool_cmd="$(get_merge_tool_cmd "$1")" >> - if test -z "$merge_tool_cmd" >> - then >> - status=1 >> - break >> - fi >> - ( eval $merge_tool_cmd ) >> - status=$? >> + status=1 >> return $status >> } > > Nit: Why not return 1, instead of setting $status and returning it? Perhaps because the caller "run_merge_tool" pays attention to $status that is a global variable? Have you traced the call chain? ``` ## David Aguilar, 2012-09-26 18:31 Subject: Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins Message-ID: URL: https://gitlist.dev/e/CAJDDKr4OkvAvRQFjp0NuzYkE2DSdvXh1vvTdKeaCq%2BbvQA4kuQ%40mail.gmail.com In-Reply-To: <7vtxukubds.fsf@alter.siamese.dyndns.org> ``` On Wed, Sep 26, 2012 at 7:06 AM, Junio C Hamano wrote: > Ramkumar Ramachandra writes: > >> Hi David, >> >> David Aguilar wrote: >>> diff_cmd () { >>> - merge_tool_cmd="$(get_merge_tool_cmd "$1")" >>> - if test -z "$merge_tool_cmd" >>> - then >>> - status=1 >>> - break >>> - fi >>> - ( eval $merge_tool_cmd ) >>> - status=$? >>> + status=1 >>> return $status >>> } >> >> Nit: Why not return 1, instead of setting $status and returning it? > > Perhaps because the caller "run_merge_tool" pays attention to > $status that is a global variable? > > Have you traced the call chain? Exactly. I would like to eliminate globals whenever possible, but this particular topic involved refactoring which aimed to keep existing behavior w.r.t these variables unchanged. -- David ```