Hi Junio, Le 2024-11-12 à 20:48, Junio C Hamano a écrit : > "Philippe Blain via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> 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 > > There are only two callers of setup_user_tool, and one of them > squelches this message by sending it to /dev/null. The error > message composed here does not use anything that is unique to the > function (in other words, $tool and ${TOOL_MODE} are available to > its callers). > > I wonder if it is a better design to leave this one as-is, and > instead show the error message from the other caller of > setup_user_tool that does not squelch the message? Are we planning > to add more callers of this function that want to show the same > message? I don't think we are planning to add more callers, no. > >> diff_cmd () { >> ( eval $merge_tool_cmd ) >> @@ -255,7 +259,7 @@ setup_tool () { >> >> # 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 we did that, then this change can be dropped. Instead, a few > lines above this hunk, we can give the error message ourselves from > this setup_tool function. I agree it could be done this way, I can change if it we wish. >> if ! list_tool_variants | grep -q "^$tool$" >> then >> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh >> index 22b3a85b3e9..82a88107850 100755 >> --- a/t/t7610-mergetool.sh >> +++ b/t/t7610-mergetool.sh >> @@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' ' >> git commit -m "branch1 resolved with mergetool" >> ' >> >> +test_expect_success 'mergetool with non-existent tool' ' >> + test_when_finished "git reset --hard" && >> + 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 >> +' > > Why "-i"? I do not offhand see the reason why we want to be loose > here. Indeed this is a leftover from my bisection test in which I had to be a bit more loose. I'll remove that flag. > The "${TOOL_MODE}tool" part may also want to be verified, perhaps, > which was related to the topic of the fix in [2/5]? Yes, I guess I could make the pattern stricter. I'll update that.