Changes in v2: As suggested by Junio: * 3/5: moved the error message to setup_tool itself, and adjusted the commit message * 3/5: made the test more robust * 4/5: adjusted the error message v1: These are a few improvements to improve existing error messages in 'git mergetool', and make sure that errors are not quiet, along with a small completion update in 1/1. Philippe Blain (5): completion: complete '--tool-help' in 'git mergetool' git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool git-mergetool--lib.sh: add error message if 'setup_user_tool' fails git-mergetool--lib.sh: add error message for unknown tool variant git-difftool--helper.sh: exit upon initialize_merge_tool errors contrib/completion/git-completion.bash | 2 +- git-difftool--helper.sh | 8 ++------ git-mergetool--lib.sh | 12 +++++++++--- t/t7610-mergetool.sh | 8 ++++++++ 4 files changed, 20 insertions(+), 10 deletions(-) base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1827%2Fphil-blain%2Fabsent-mergetool-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1827/phil-blain/absent-mergetool-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1827 Range-diff vs v1: 1: 24933ba7130 = 1: 24933ba7130 completion: complete '--tool-help' in 'git mergetool' 2: 6f7f553b283 = 2: 6f7f553b283 git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool 3: 79c3a6ffe8f ! 3: 1d9e95c6cb1 git-mergetool--lib.sh: add error message in 'setup_user_tool' @@ Metadata Author: Philippe Blain <levraiphilippeblain@xxxxxxxxx> ## Commit message ## - git-mergetool--lib.sh: add error message in 'setup_user_tool' + git-mergetool--lib.sh: add error message if 'setup_user_tool' fails In git-mergetool--lib.sh::setup_tool, we check if the given tool is a known builtin tool, a known variant, or a user-defined tool by calling @@ Commit message git mergetool --tool=unknown - which is not very user-friendly. Adjust setup_user_tool to output an - error message before returning if {diff,merge}tool.$tool.cmd is not set. + which is not very user-friendly. Adjust setup_tool to output an error + message before returning if setup_user_tool returned with an error. - Adjust the second call to setup_user_tool in setup_tool to silence this - new error, as this call is only meant to allow users to redefine 'cmd' - for a builtin tool; it is not an error if they have not done so (which - is why we do not check the return status of this call). + Note that we do not check the result of the second call to + setup_user_tool in setup_tool, as this call is only meant to allow users + to redefine 'cmd' for a builtin tool; it is not an error if they have + not done so. Note that this behaviour of quietly failing is a regression dating back to de8dafbada (mergetool: break setup_tool out into separate @@ git-mergetool--lib.sh: check_unchanged () { cmd=$(get_merge_tool_cmd "$1") test -n "$cmd" } - - setup_user_tool () { - merge_tool_cmd=$(get_merge_tool_cmd "$tool") -- test -n "$merge_tool_cmd" || return 1 -+ if test -z "$merge_tool_cmd" -+ then -+ echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'" -+ return 1 -+ fi - - diff_cmd () { - ( eval $merge_tool_cmd ) @@ git-mergetool--lib.sh: setup_tool () { + . "$MERGE_TOOLS_DIR/${tool%[0-9]}" + else + setup_user_tool +- return $? ++ rc=$? ++ if test $rc -ne 0 ++ then ++ echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'" ++ fi ++ return $rc + fi # Now let the user override the default command for the tool. If - # they have not done so then this will return 1 which we ignore. -- setup_user_tool -+ setup_user_tool 2>/dev/null - - if ! list_tool_variants | grep -q "^$tool$" - then ## t/t7610-mergetool.sh ## @@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' ' @@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' ' + git checkout -b test$test_count branch1 && + test_must_fail git merge main && + yes "" | test_must_fail git mergetool --tool=absent >out 2>&1 && -+ test_grep -i "not set for tool" out ++ test_grep "mergetool.absent.cmd not set for tool" out +' + test_done 4: 74b83caa1e5 ! 4: f234e965543 git-mergetool--lib.sh: add error message for unknown tool variant @@ git-mergetool--lib.sh: setup_tool () { if ! list_tool_variants | grep -q "^$tool$" then -+ echo "error: unknown ${tool%[0-9]} variant '$tool'" >&2 ++ echo "error: unknown tool variant '$tool'" >&2 return 1 fi 5: be0b86f0890 = 5: c16e9229ebb git-difftool--helper.sh: exit upon initialize_merge_tool errors -- gitgitgadget
