Re: [PATCH 3/5] git-mergetool--lib.sh: add error message in 'setup_user_tool'
Hi Junio,
Le 2024-11-12 à 20:48, Junio C Hamano a écrit :
Show 22 quoted lines
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> 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.
Show 13 quoted lines
>
>> 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.
Show 20 quoted lines
>> 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.
Philippe Blain <levraiphilippeblain@gmail.com>