{"thread":{"id":"62484","subject":"[PATCH 0/5] git-mergetool: improve error code paths and messages","startedAt":"2024-11-13T00:52:12Z","lastAt":"2024-11-26T04:33:50Z","messageCount":20,"participants":["Philippe Blain via GitGitGadget","Junio C Hamano","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"507168","messageId":"pull.1827.git.1731459128.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":null,"subject":"[PATCH 0/5] git-mergetool: improve error code paths and messages","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:03Z","receivedAt":"2024-11-13T00:52:12Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"These are a few improvements to improve existing error messages in 'git\nmergetool', and make sure that errors are not quiet, along with a small\ncompletion update in 1/1.\n\nPhilippe Blain (5):\n  completion: complete '--tool-help' in 'git mergetool'\n  git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool\n  git-mergetool--lib.sh: add error message in 'setup_user_tool'\n  git-mergetool--lib.sh: add error message for unknown tool variant\n  git-difftool--helper.sh: exit upon initialize_merge_tool errors\n\n contrib/completion/git-completion.bash |  2 +-\n git-difftool--helper.sh                |  8 ++------\n git-mergetool--lib.sh                  | 13 +++++++++----\n t/t7610-mergetool.sh                   |  8 ++++++++\n 4 files changed, 20 insertions(+), 11 deletions(-)\n\n\nbase-commit: b31fb630c0fc6869a33ed717163e8a1210460d94\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1827%2Fphil-blain%2Fabsent-mergetool-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1827/phil-blain/absent-mergetool-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1827\n-- \ngitgitgadget\n"},{"id":"507169","messageId":"24933ba71305caa97fe1f756aef770d40f39fbda.1731459128.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH 1/5] completion: complete '--tool-help' in 'git mergetool'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:04Z","receivedAt":"2024-11-13T00:52:12Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3d4dff3185c..b3b6aa3bae2 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2331,7 +2331,7 @@ _git_mergetool ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--tool= --prompt --no-prompt --gui --no-gui\"\n+\t\t__gitcomp \"--tool= --tool-help --prompt --no-prompt --gui --no-gui\"\n \t\treturn\n \t\t;;\n \tesac\n-- \ngitgitgadget\n\n"},{"id":"507170","messageId":"6f7f553b283078ba3c81190686b150a87d901240.1731459128.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH 2/5] git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:05Z","receivedAt":"2024-11-13T00:52:13Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen\ntool is valid via valid_tool and exit with an error message if not. This\nerror message mentions \"Unknown merge tool\", even if the command the\nuser tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE'\nvariable for a more correct error message.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 1ff26170ffc..269a60ea44c 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -474,7 +474,7 @@ get_merge_tool_path () {\n \tmerge_tool=\"$1\"\n \tif ! valid_tool \"$merge_tool\"\n \tthen\n-\t\techo >&2 \"Unknown merge tool $merge_tool\"\n+\t\techo >&2 \"Unknown $TOOL_MODE tool $merge_tool\"\n \t\texit 1\n \tfi\n \tif diff_mode\n-- \ngitgitgadget\n\n"},{"id":"507171","messageId":"79c3a6ffe8f2872755f76340e2d5ae1a94885456.1731459128.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH 3/5] git-mergetool--lib.sh: add error message in 'setup_user_tool'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:06Z","receivedAt":"2024-11-13T00:52:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn git-mergetool--lib.sh::setup_tool, we check if the given tool is a\nknown builtin tool, a known variant, or a user-defined tool by calling\nsetup_user_tool, and we return with the exit code from setup_user_tool\nif it was called. setup_user_tool checks if {diff,merge}tool.$tool.cmd\nis set and quietly returns with an error if not.\n\nThis leads to the following invocation quietly failing:\n\n\tgit mergetool --tool=unknown\n\nwhich is not very user-friendly. Adjust setup_user_tool to output an\nerror message before returning if {diff,merge}tool.$tool.cmd is not set.\n\nAdjust the second call to setup_user_tool in setup_tool to silence this\nnew error, as this call is only meant to allow users to redefine 'cmd'\nfor a builtin tool; it is not an error if they have not done so (which\nis why we do not check the return status of this call).\n\nNote that this behaviour of quietly failing is a regression dating back\nto de8dafbada (mergetool: break setup_tool out into separate\ninitialization function, 2021-02-09), as before this commit an unknown\nmergetool would be diagnosed in get_merge_tool_path when called from\nrun_merge_tool.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 10 +++++++---\n t/t7610-mergetool.sh  |  8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 269a60ea44c..f4786afc63f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -159,14 +159,18 @@ check_unchanged () {\n }\n \n valid_tool () {\n-\tsetup_tool \"$1\" && return 0\n+\tsetup_tool \"$1\" 2>/dev/null && return 0\n \tcmd=$(get_merge_tool_cmd \"$1\")\n \ttest -n \"$cmd\"\n }\n \n setup_user_tool () {\n \tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n-\ttest -n \"$merge_tool_cmd\" || return 1\n+\tif test -z \"$merge_tool_cmd\"\n+\tthen\n+\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n+\t\treturn 1\n+\tfi\n \n \tdiff_cmd () {\n \t\t( eval $merge_tool_cmd )\n@@ -255,7 +259,7 @@ setup_tool () {\n \n \t# Now let the user override the default command for the tool.  If\n \t# they have not done so then this will return 1 which we ignore.\n-\tsetup_user_tool\n+\tsetup_user_tool 2>/dev/null\n \n \tif ! list_tool_variants | grep -q \"^$tool$\"\n \tthen\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 22b3a85b3e9..82a88107850 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with non-existent tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\ttest_must_fail git merge main &&\n+\tyes \"\" | test_must_fail git mergetool --tool=absent >out 2>&1 &&\n+\ttest_grep -i \"not set for tool\" out\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"507172","messageId":"74b83caa1e5c1a63248dd4dcbaf2cf450f9cf32d.1731459128.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH 4/5] git-mergetool--lib.sh: add error message for unknown tool variant","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:07Z","receivedAt":"2024-11-13T00:52:15Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn setup_tool, we check if the given tool is a known variant of a tool,\nand quietly return with an error if not. This leads to the following\ninvocation quietly failing:\n\n\tgit mergetool --tool=vimdiff4\n\nAdd an error message before returning in this case.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex f4786afc63f..9a00fabba27 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -263,6 +263,7 @@ setup_tool () {\n \n \tif ! list_tool_variants | grep -q \"^$tool$\"\n \tthen\n+\t\techo \"error: unknown ${tool%[0-9]} variant '$tool'\" >&2\n \t\treturn 1\n \tfi\n \n-- \ngitgitgadget\n\n"},{"id":"507173","messageId":"be0b86f08901046878f1b1406f811166d56c7c6d.1731459128.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH 5/5] git-difftool--helper.sh: exit upon initialize_merge_tool errors","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T00:52:08Z","receivedAt":"2024-11-13T00:52:16Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince the introduction of 'initialize_merge_tool' in de8dafbada\n(mergetool: break setup_tool out into separate initialization function,\n2021-02-09), any errors from this function are ignored in\ngit-difftool--helper.sh::launch_merge_tool, which is not the case for\nits call in git-mergetool.sh::merge_file.\n\nDespite the in-code comment, initialize_merge_tool (via its call to\nsetup_tool) does different checks than run_merge_tool, so it makes sense\nto abort early if it encounters errors. Add exit calls if\ninitialize_merge_tool fails.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-difftool--helper.sh | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex dd0c9a5b7f2..d32e47cc09e 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -61,9 +61,7 @@ launch_merge_tool () {\n \t\texport BASE\n \t\teval $GIT_DIFFTOOL_EXTCMD '\"$LOCAL\"' '\"$REMOTE\"'\n \telse\n-\t\tinitialize_merge_tool \"$merge_tool\"\n-\t\t# ignore the error from the above --- run_merge_tool\n-\t\t# will diagnose unusable tool by itself\n+\t\tinitialize_merge_tool \"$merge_tool\" || exit 1\n \t\trun_merge_tool \"$merge_tool\"\n \tfi\n }\n@@ -87,9 +85,7 @@ if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n then\n \tLOCAL=\"$1\"\n \tREMOTE=\"$2\"\n-\tinitialize_merge_tool \"$merge_tool\"\n-\t# ignore the error from the above --- run_merge_tool\n-\t# will diagnose unusable tool by itself\n+\tinitialize_merge_tool \"$merge_tool\" || exit 1\n \trun_merge_tool \"$merge_tool\" false\n \n \tstatus=$?\n-- \ngitgitgadget\n"},{"id":"507178","messageId":"xmqq34jv3ou2.fsf@gitster.g","threadId":"62484","inReplyTo":"6f7f553b283078ba3c81190686b150a87d901240.1731459128.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-13T01:27:33Z","receivedAt":"2024-11-13T01:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> In git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen\n> tool is valid via valid_tool and exit with an error message if not. This\n> error message mentions \"Unknown merge tool\", even if the command the\n> user tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE'\n> variable for a more correct error message.\n\nMakes sense.  Is this something we can easily test to catch future\nregression, or is it too trivial to matter?\n\nI wouldn't mind if the answer were \"the latter\" ;-)\n\nThanks.\n\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 1ff26170ffc..269a60ea44c 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -474,7 +474,7 @@ get_merge_tool_path () {\n>  \tmerge_tool=\"$1\"\n>  \tif ! valid_tool \"$merge_tool\"\n>  \tthen\n> -\t\techo >&2 \"Unknown merge tool $merge_tool\"\n> +\t\techo >&2 \"Unknown $TOOL_MODE tool $merge_tool\"\n>  \t\texit 1\n>  \tfi\n>  \tif diff_mode\n"},{"id":"507179","messageId":"xmqqwmh729ah.fsf@gitster.g","threadId":"62484","inReplyTo":"79c3a6ffe8f2872755f76340e2d5ae1a94885456.1731459128.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] git-mergetool--lib.sh: add error message in 'setup_user_tool'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-13T01:48:38Z","receivedAt":"2024-11-13T01:48:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  valid_tool () {\n> -\tsetup_tool \"$1\" && return 0\n> +\tsetup_tool \"$1\" 2>/dev/null && return 0\n>  \tcmd=$(get_merge_tool_cmd \"$1\")\n>  \ttest -n \"$cmd\"\n>  }\n\nAs we are checking if a tool is valid, it is normal for setup_tool\nto fail when we are checking is not valid (aka \"fails to get set\nup\").  There is no need to show an error message for such a failure,\nas the callers of valid_tool would do so if they wish.  OK.\n\n>  setup_user_tool () {\n>  \tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n> -\ttest -n \"$merge_tool_cmd\" || return 1\n> +\tif test -z \"$merge_tool_cmd\"\n> +\tthen\n> +\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n> +\t\treturn 1\n> +\tfi\n\nThere are only two callers of setup_user_tool, and one of them\nsquelches this message by sending it to /dev/null.  The error\nmessage composed here does not use anything that is unique to the\nfunction (in other words, $tool and ${TOOL_MODE} are available to\nits callers).\n\nI wonder if it is a better design to leave this one as-is, and\ninstead show the error message from the other caller of\nsetup_user_tool that does not squelch the message?  Are we planning\nto add more callers of this function that want to show the same\nmessage?\n\n>  \tdiff_cmd () {\n>  \t\t( eval $merge_tool_cmd )\n> @@ -255,7 +259,7 @@ setup_tool () {\n>  \n>  \t# Now let the user override the default command for the tool.  If\n>  \t# they have not done so then this will return 1 which we ignore.\n> -\tsetup_user_tool\n> +\tsetup_user_tool 2>/dev/null\n\nIf we did that, then this change can be dropped.  Instead, a few\nlines above this hunk, we can give the error message ourselves from\nthis setup_tool function.\n\n>  \tif ! list_tool_variants | grep -q \"^$tool$\"\n>  \tthen\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> index 22b3a85b3e9..82a88107850 100755\n> --- a/t/t7610-mergetool.sh\n> +++ b/t/t7610-mergetool.sh\n> @@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '\n>  \tgit commit -m \"branch1 resolved with mergetool\"\n>  '\n>  \n> +test_expect_success 'mergetool with non-existent tool' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\tgit checkout -b test$test_count branch1 &&\n> +\ttest_must_fail git merge main &&\n> +\tyes \"\" | test_must_fail git mergetool --tool=absent >out 2>&1 &&\n> +\ttest_grep -i \"not set for tool\" out\n> +'\n\nWhy \"-i\"?  I do not offhand see the reason why we want to be loose\nhere.\n\nThe \"${TOOL_MODE}tool\" part may also want to be verified, perhaps,\nwhich was related to the topic of the fix in [2/5]?\n"},{"id":"507180","messageId":"xmqqr07f28ow.fsf@gitster.g","threadId":"62484","inReplyTo":"74b83caa1e5c1a63248dd4dcbaf2cf450f9cf32d.1731459128.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] git-mergetool--lib.sh: add error message for unknown tool variant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-13T02:01:35Z","receivedAt":"2024-11-13T02:01:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> In setup_tool, we check if the given tool is a known variant of a tool,\n> and quietly return with an error if not. This leads to the following\n> invocation quietly failing:\n>\n> \tgit mergetool --tool=vimdiff4\n>\n> Add an error message before returning in this case.\n\nMakes sense, but ...\n\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index f4786afc63f..9a00fabba27 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -263,6 +263,7 @@ setup_tool () {\n>  \n>  \tif ! list_tool_variants | grep -q \"^$tool$\"\n>  \tthen\n> +\t\techo \"error: unknown ${tool%[0-9]} variant '$tool'\" >&2\n\n... I do not understand why you strip a single digit from the end.\n\n    git mergetool --tool=nvimdiff4\n\nsays 'nvimdiff4' is not known as a variant of 'nvimdiff', but\nwouldn't it still be a variant of 'vimdiff'?  Of course,\n\n    git mergetool --tool=nvimdiff48\n\ngets a vastly different error message ;-)\n\nSaying\n\n\techo >&2 \"error: unknown variant '$tool'\"\n\nmay be sufficient, perhaps?  I dunno.\n\n\n>  \t\treturn 1\n>  \tfi\n"},{"id":"507954","messageId":"c6d95106-e2d9-37d7-211c-b23199ceb258@gmail.com","threadId":"62484","inReplyTo":"xmqq34jv3ou2.fsf@gitster.g","subject":"Re: [PATCH 2/5] git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-11-22T18:57:48Z","receivedAt":"2024-11-22T18:57:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio (sorry for a late response),\n\nLe 2024-11-12 à 20:27, Junio C Hamano a écrit :\n> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> In git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen\n>> tool is valid via valid_tool and exit with an error message if not. This\n>> error message mentions \"Unknown merge tool\", even if the command the\n>> user tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE'\n>> variable for a more correct error message.\n> \n> Makes sense.  Is this something we can easily test to catch future\n> regression, or is it too trivial to matter?\n> \n> I wouldn't mind if the answer were \"the latter\" ;-)\n\nWith the changes in the next commit of the series, this particular error\nbecomes hard to trigger, as setup_user_tool will return with an error\nbefore the error message change in this patch is reached. So I would way\nit is not worth to add a test for this particular code path since it seems like\nit becomes unreachable in the next commit (but I could be wrong). So mostly\n\"the latter\" is my answer.\n\nThanks,\nPhilippe.\n"},{"id":"507955","messageId":"47763d09-8e3c-ea58-c8f7-0580c7d2291a@gmail.com","threadId":"62484","inReplyTo":"xmqqwmh729ah.fsf@gitster.g","subject":"Re: [PATCH 3/5] git-mergetool--lib.sh: add error message in 'setup_user_tool'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-11-22T19:02:01Z","receivedAt":"2024-11-22T19:02:03Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2024-11-12 à 20:48, Junio C Hamano a écrit :\n> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>>  setup_user_tool () {\n>>  \tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n>> -\ttest -n \"$merge_tool_cmd\" || return 1\n>> +\tif test -z \"$merge_tool_cmd\"\n>> +\tthen\n>> +\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n>> +\t\treturn 1\n>> +\tfi\n> \n> There are only two callers of setup_user_tool, and one of them\n> squelches this message by sending it to /dev/null.  The error\n> message composed here does not use anything that is unique to the\n> function (in other words, $tool and ${TOOL_MODE} are available to\n> its callers).\n> \n> I wonder if it is a better design to leave this one as-is, and\n> instead show the error message from the other caller of\n> setup_user_tool that does not squelch the message?  Are we planning\n> to add more callers of this function that want to show the same\n> message?\n\nI don't think we are planning to add more callers, no.\n\n> \n>>  \tdiff_cmd () {\n>>  \t\t( eval $merge_tool_cmd )\n>> @@ -255,7 +259,7 @@ setup_tool () {\n>>  \n>>  \t# Now let the user override the default command for the tool.  If\n>>  \t# they have not done so then this will return 1 which we ignore.\n>> -\tsetup_user_tool\n>> +\tsetup_user_tool 2>/dev/null\n> \n> If we did that, then this change can be dropped.  Instead, a few\n> lines above this hunk, we can give the error message ourselves from\n> this setup_tool function.\n\nI agree it could be done this way, I can change if it we wish.\n\n\n>>  \tif ! list_tool_variants | grep -q \"^$tool$\"\n>>  \tthen\n>> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n>> index 22b3a85b3e9..82a88107850 100755\n>> --- a/t/t7610-mergetool.sh\n>> +++ b/t/t7610-mergetool.sh\n>> @@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '\n>>  \tgit commit -m \"branch1 resolved with mergetool\"\n>>  '\n>>  \n>> +test_expect_success 'mergetool with non-existent tool' '\n>> +\ttest_when_finished \"git reset --hard\" &&\n>> +\tgit checkout -b test$test_count branch1 &&\n>> +\ttest_must_fail git merge main &&\n>> +\tyes \"\" | test_must_fail git mergetool --tool=absent >out 2>&1 &&\n>> +\ttest_grep -i \"not set for tool\" out\n>> +'\n> \n> Why \"-i\"?  I do not offhand see the reason why we want to be loose\n> here.\n\nIndeed this is a leftover from my bisection test in which I had to \nbe a bit more loose. I'll remove that flag.\n \n> The \"${TOOL_MODE}tool\" part may also want to be verified, perhaps,\n> which was related to the topic of the fix in [2/5]?\n\nYes, I guess I could make the pattern stricter. I'll update that.\n"},{"id":"507956","messageId":"506f944b-033a-07ad-2c46-581eeb9799ef@gmail.com","threadId":"62484","inReplyTo":"xmqqr07f28ow.fsf@gitster.g","subject":"Re: [PATCH 4/5] git-mergetool--lib.sh: add error message for unknown tool variant","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-11-22T19:08:08Z","receivedAt":"2024-11-22T19:08:10Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Le 2024-11-12 à 21:01, Junio C Hamano a écrit :\n> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> In setup_tool, we check if the given tool is a known variant of a tool,\n>> and quietly return with an error if not. This leads to the following\n>> invocation quietly failing:\n>>\n>> \tgit mergetool --tool=vimdiff4\n>>\n>> Add an error message before returning in this case.\n> \n> Makes sense, but ...\n> \n>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n>> ---\n>>  git-mergetool--lib.sh | 1 +\n>>  1 file changed, 1 insertion(+)\n>>\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index f4786afc63f..9a00fabba27 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -263,6 +263,7 @@ setup_tool () {\n>>  \n>>  \tif ! list_tool_variants | grep -q \"^$tool$\"\n>>  \tthen\n>> +\t\techo \"error: unknown ${tool%[0-9]} variant '$tool'\" >&2\n> \n> ... I do not understand why you strip a single digit from the end.\n> \n>     git mergetool --tool=nvimdiff4\n> \n> says 'nvimdiff4' is not known as a variant of 'nvimdiff', but\n> wouldn't it still be a variant of 'vimdiff'?  Of course,\n> \n>     git mergetool --tool=nvimdiff48\n> \n> gets a vastly different error message ;-)\n> \n> Saying\n> \n> \techo >&2 \"error: unknown variant '$tool'\"\n> \n> may be sufficient, perhaps?  I dunno.\n\nthe stripping of the last digit is just because I copied from \nthe 'if' a few lines above, where we source \"$MERGE_TOOLS_DIR/${tool%[0-9]}\".\nIn MERGE_TOOLS_DIR we have 'nvimdiff' and 'gvimdiff' that simply source vimdiff,\nso this works. But I agree that we can simplify the error message, I'll do that.\n"},{"id":"507960","messageId":"ee32f2fd-7b82-2c63-5607-a2ce501856e7@gmail.com","threadId":"62484","inReplyTo":"c6d95106-e2d9-37d7-211c-b23199ceb258@gmail.com","subject":"Re: [PATCH 2/5] git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2024-11-22T19:32:29Z","receivedAt":"2024-11-22T19:32:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Le 2024-11-22 à 13:57, Philippe Blain a écrit :\n> Hi Junio (sorry for a late response),\n> \n> Le 2024-11-12 à 20:27, Junio C Hamano a écrit :\n>> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>>\n>>> In git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen\n>>> tool is valid via valid_tool and exit with an error message if not. This\n>>> error message mentions \"Unknown merge tool\", even if the command the\n>>> user tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE'\n>>> variable for a more correct error message.\n>>\n>> Makes sense.  Is this something we can easily test to catch future\n>> regression, or is it too trivial to matter?\n>>\n>> I wouldn't mind if the answer were \"the latter\" ;-)\n> \n> With the changes in the next commit of the series,\n\ncorrection: with the changes in 3/5 and 5/5,\n\n> this particular error\n> becomes hard to trigger, as setup_user_tool will return with an error\n> before the error message change in this patch is reached. So I would way\n> it is not worth to add a test for this particular code path since it seems like\n> it becomes unreachable in the next commit (but I could be wrong). So mostly\n> \"the latter\" is my answer.\n\n"},{"id":"507961","messageId":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.git.1731459128.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] git-mergetool: improve error code paths and messages","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:17Z","receivedAt":"2024-11-22T19:50:27Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes in v2: As suggested by Junio:\n\n * 3/5: moved the error message to setup_tool itself, and adjusted the\n   commit message\n * 3/5: made the test more robust\n * 4/5: adjusted the error message\n\nv1: These are a few improvements to improve existing error messages in 'git\nmergetool', and make sure that errors are not quiet, along with a small\ncompletion update in 1/1.\n\nPhilippe Blain (5):\n  completion: complete '--tool-help' in 'git mergetool'\n  git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool\n  git-mergetool--lib.sh: add error message if 'setup_user_tool' fails\n  git-mergetool--lib.sh: add error message for unknown tool variant\n  git-difftool--helper.sh: exit upon initialize_merge_tool errors\n\n contrib/completion/git-completion.bash |  2 +-\n git-difftool--helper.sh                |  8 ++------\n git-mergetool--lib.sh                  | 12 +++++++++---\n t/t7610-mergetool.sh                   |  8 ++++++++\n 4 files changed, 20 insertions(+), 10 deletions(-)\n\n\nbase-commit: b31fb630c0fc6869a33ed717163e8a1210460d94\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1827%2Fphil-blain%2Fabsent-mergetool-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1827/phil-blain/absent-mergetool-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1827\n\nRange-diff vs v1:\n\n 1:  24933ba7130 = 1:  24933ba7130 completion: complete '--tool-help' in 'git mergetool'\n 2:  6f7f553b283 = 2:  6f7f553b283 git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool\n 3:  79c3a6ffe8f ! 3:  1d9e95c6cb1 git-mergetool--lib.sh: add error message in 'setup_user_tool'\n     @@ Metadata\n      Author: Philippe Blain <levraiphilippeblain@gmail.com>\n      \n       ## Commit message ##\n     -    git-mergetool--lib.sh: add error message in 'setup_user_tool'\n     +    git-mergetool--lib.sh: add error message if 'setup_user_tool' fails\n      \n          In git-mergetool--lib.sh::setup_tool, we check if the given tool is a\n          known builtin tool, a known variant, or a user-defined tool by calling\n     @@ Commit message\n      \n                  git mergetool --tool=unknown\n      \n     -    which is not very user-friendly. Adjust setup_user_tool to output an\n     -    error message before returning if {diff,merge}tool.$tool.cmd is not set.\n     +    which is not very user-friendly. Adjust setup_tool to output an error\n     +    message before returning if setup_user_tool returned with an error.\n      \n     -    Adjust the second call to setup_user_tool in setup_tool to silence this\n     -    new error, as this call is only meant to allow users to redefine 'cmd'\n     -    for a builtin tool; it is not an error if they have not done so (which\n     -    is why we do not check the return status of this call).\n     +    Note that we do not check the result of the second call to\n     +    setup_user_tool in setup_tool, as this call is only meant to allow users\n     +    to redefine 'cmd' for a builtin tool; it is not an error if they have\n     +    not done so.\n      \n          Note that this behaviour of quietly failing is a regression dating back\n          to de8dafbada (mergetool: break setup_tool out into separate\n     @@ git-mergetool--lib.sh: check_unchanged () {\n       \tcmd=$(get_merge_tool_cmd \"$1\")\n       \ttest -n \"$cmd\"\n       }\n     - \n     - setup_user_tool () {\n     - \tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n     --\ttest -n \"$merge_tool_cmd\" || return 1\n     -+\tif test -z \"$merge_tool_cmd\"\n     -+\tthen\n     -+\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n     -+\t\treturn 1\n     -+\tfi\n     - \n     - \tdiff_cmd () {\n     - \t\t( eval $merge_tool_cmd )\n      @@ git-mergetool--lib.sh: setup_tool () {\n     + \t\t. \"$MERGE_TOOLS_DIR/${tool%[0-9]}\"\n     + \telse\n     + \t\tsetup_user_tool\n     +-\t\treturn $?\n     ++\t\trc=$?\n     ++\t\tif test $rc -ne 0\n     ++\t\tthen\n     ++\t\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n     ++\t\tfi\n     ++\t\treturn $rc\n     + \tfi\n       \n       \t# Now let the user override the default command for the tool.  If\n     - \t# they have not done so then this will return 1 which we ignore.\n     --\tsetup_user_tool\n     -+\tsetup_user_tool 2>/dev/null\n     - \n     - \tif ! list_tool_variants | grep -q \"^$tool$\"\n     - \tthen\n      \n       ## t/t7610-mergetool.sh ##\n      @@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' '\n     @@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' '\n      +\tgit checkout -b test$test_count branch1 &&\n      +\ttest_must_fail git merge main &&\n      +\tyes \"\" | test_must_fail git mergetool --tool=absent >out 2>&1 &&\n     -+\ttest_grep -i \"not set for tool\" out\n     ++\ttest_grep \"mergetool.absent.cmd not set for tool\" out\n      +'\n      +\n       test_done\n 4:  74b83caa1e5 ! 4:  f234e965543 git-mergetool--lib.sh: add error message for unknown tool variant\n     @@ git-mergetool--lib.sh: setup_tool () {\n       \n       \tif ! list_tool_variants | grep -q \"^$tool$\"\n       \tthen\n     -+\t\techo \"error: unknown ${tool%[0-9]} variant '$tool'\" >&2\n     ++\t\techo \"error: unknown tool variant '$tool'\" >&2\n       \t\treturn 1\n       \tfi\n       \n 5:  be0b86f0890 = 5:  c16e9229ebb git-difftool--helper.sh: exit upon initialize_merge_tool errors\n\n-- \ngitgitgadget\n"},{"id":"507962","messageId":"24933ba71305caa97fe1f756aef770d40f39fbda.1732305022.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] completion: complete '--tool-help' in 'git mergetool'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:18Z","receivedAt":"2024-11-22T19:50:27Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3d4dff3185c..b3b6aa3bae2 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2331,7 +2331,7 @@ _git_mergetool ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--tool= --prompt --no-prompt --gui --no-gui\"\n+\t\t__gitcomp \"--tool= --tool-help --prompt --no-prompt --gui --no-gui\"\n \t\treturn\n \t\t;;\n \tesac\n-- \ngitgitgadget\n\n"},{"id":"507963","messageId":"6f7f553b283078ba3c81190686b150a87d901240.1732305022.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:19Z","receivedAt":"2024-11-22T19:50:28Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen\ntool is valid via valid_tool and exit with an error message if not. This\nerror message mentions \"Unknown merge tool\", even if the command the\nuser tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE'\nvariable for a more correct error message.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 1ff26170ffc..269a60ea44c 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -474,7 +474,7 @@ get_merge_tool_path () {\n \tmerge_tool=\"$1\"\n \tif ! valid_tool \"$merge_tool\"\n \tthen\n-\t\techo >&2 \"Unknown merge tool $merge_tool\"\n+\t\techo >&2 \"Unknown $TOOL_MODE tool $merge_tool\"\n \t\texit 1\n \tfi\n \tif diff_mode\n-- \ngitgitgadget\n\n"},{"id":"507964","messageId":"1d9e95c6cb14df3d3225210bde3c4877cb4cc540.1732305022.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] git-mergetool--lib.sh: add error message if 'setup_user_tool' fails","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:20Z","receivedAt":"2024-11-22T19:50:30Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn git-mergetool--lib.sh::setup_tool, we check if the given tool is a\nknown builtin tool, a known variant, or a user-defined tool by calling\nsetup_user_tool, and we return with the exit code from setup_user_tool\nif it was called. setup_user_tool checks if {diff,merge}tool.$tool.cmd\nis set and quietly returns with an error if not.\n\nThis leads to the following invocation quietly failing:\n\n\tgit mergetool --tool=unknown\n\nwhich is not very user-friendly. Adjust setup_tool to output an error\nmessage before returning if setup_user_tool returned with an error.\n\nNote that we do not check the result of the second call to\nsetup_user_tool in setup_tool, as this call is only meant to allow users\nto redefine 'cmd' for a builtin tool; it is not an error if they have\nnot done so.\n\nNote that this behaviour of quietly failing is a regression dating back\nto de8dafbada (mergetool: break setup_tool out into separate\ninitialization function, 2021-02-09), as before this commit an unknown\nmergetool would be diagnosed in get_merge_tool_path when called from\nrun_merge_tool.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 9 +++++++--\n t/t7610-mergetool.sh  | 8 ++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 269a60ea44c..d7e410d9481 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -159,7 +159,7 @@ check_unchanged () {\n }\n \n valid_tool () {\n-\tsetup_tool \"$1\" && return 0\n+\tsetup_tool \"$1\" 2>/dev/null && return 0\n \tcmd=$(get_merge_tool_cmd \"$1\")\n \ttest -n \"$cmd\"\n }\n@@ -250,7 +250,12 @@ setup_tool () {\n \t\t. \"$MERGE_TOOLS_DIR/${tool%[0-9]}\"\n \telse\n \t\tsetup_user_tool\n-\t\treturn $?\n+\t\trc=$?\n+\t\tif test $rc -ne 0\n+\t\tthen\n+\t\t\techo >&2 \"error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'\"\n+\t\tfi\n+\t\treturn $rc\n \tfi\n \n \t# Now let the user override the default command for the tool.  If\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 22b3a85b3e9..c077aba7ced 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with non-existent tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\ttest_must_fail git merge main &&\n+\tyes \"\" | test_must_fail git mergetool --tool=absent >out 2>&1 &&\n+\ttest_grep \"mergetool.absent.cmd not set for tool\" out\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"507965","messageId":"f234e965543322d96fd6aa41d12f0f52c3599206.1732305022.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] git-mergetool--lib.sh: add error message for unknown tool variant","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:21Z","receivedAt":"2024-11-22T19:50:31Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nIn setup_tool, we check if the given tool is a known variant of a tool,\nand quietly return with an error if not. This leads to the following\ninvocation quietly failing:\n\n\tgit mergetool --tool=vimdiff4\n\nAdd an error message before returning in this case.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-mergetool--lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex d7e410d9481..11ea181259f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -264,6 +264,7 @@ setup_tool () {\n \n \tif ! list_tool_variants | grep -q \"^$tool$\"\n \tthen\n+\t\techo \"error: unknown tool variant '$tool'\" >&2\n \t\treturn 1\n \tfi\n \n-- \ngitgitgadget\n\n"},{"id":"507966","messageId":"c16e9229ebb46916a59dc9c9fdc7b973480153d4.1732305022.git.gitgitgadget@gmail.com","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] git-difftool--helper.sh: exit upon initialize_merge_tool errors","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-22T19:50:22Z","receivedAt":"2024-11-22T19:50:33Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince the introduction of 'initialize_merge_tool' in de8dafbada\n(mergetool: break setup_tool out into separate initialization function,\n2021-02-09), any errors from this function are ignored in\ngit-difftool--helper.sh::launch_merge_tool, which is not the case for\nits call in git-mergetool.sh::merge_file.\n\nDespite the in-code comment, initialize_merge_tool (via its call to\nsetup_tool) does different checks than run_merge_tool, so it makes sense\nto abort early if it encounters errors. Add exit calls if\ninitialize_merge_tool fails.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n git-difftool--helper.sh | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex dd0c9a5b7f2..d32e47cc09e 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -61,9 +61,7 @@ launch_merge_tool () {\n \t\texport BASE\n \t\teval $GIT_DIFFTOOL_EXTCMD '\"$LOCAL\"' '\"$REMOTE\"'\n \telse\n-\t\tinitialize_merge_tool \"$merge_tool\"\n-\t\t# ignore the error from the above --- run_merge_tool\n-\t\t# will diagnose unusable tool by itself\n+\t\tinitialize_merge_tool \"$merge_tool\" || exit 1\n \t\trun_merge_tool \"$merge_tool\"\n \tfi\n }\n@@ -87,9 +85,7 @@ if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n then\n \tLOCAL=\"$1\"\n \tREMOTE=\"$2\"\n-\tinitialize_merge_tool \"$merge_tool\"\n-\t# ignore the error from the above --- run_merge_tool\n-\t# will diagnose unusable tool by itself\n+\tinitialize_merge_tool \"$merge_tool\" || exit 1\n \trun_merge_tool \"$merge_tool\" false\n \n \tstatus=$?\n-- \ngitgitgadget\n"},{"id":"508134","messageId":"xmqqr06y1umr.fsf@gitster.g","threadId":"62484","inReplyTo":"pull.1827.v2.git.1732305022.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] git-mergetool: improve error code paths and messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T04:33:48Z","receivedAt":"2024-11-26T04:33:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Changes in v2: As suggested by Junio:\n>\n>  * 3/5: moved the error message to setup_tool itself, and adjusted the\n>    commit message\n>  * 3/5: made the test more robust\n>  * 4/5: adjusted the error message\n\nI think the above changes all looked good.\n\nLet me mark the topic for 'next'.\n\nThanks.\n"}]}