Re: [PATCH 3/5] git-mergetool--lib.sh: add error message in 'setup_user_tool'

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Junio,

Le 2024-11-12 à 20:48, Junio C Hamano a écrit :
> "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.

> 
>>  	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.