From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Wed, 06 Jan 2021 13:34:25 GMT
Subject: Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools
Message-ID: <11b3c261-14e6-0293-02a6-92bd1ea680d2@gmail.com>
In-Reply-To: <20210106131651.GQ8396@szeder.dev>

Hi Gábor,

Le 2021-01-06 à 08:16, SZEDER Gábor a écrit :
> On Mon, Dec 28, 2020 at 07:41:44PM +0000, Philippe Blain via GitGitGadget wrote:
>> To prevent future regressions, add a simple test that counts the number
>> of tools shown by 'git mergetool --tool-help', irrespective of their
>> installed status, by making use of the fact that mergetools are listed
>> by 'git mergetool --tool-help' on lines starting with tabs. Prefix the
>> `git config` commands used at the beginning of the test to remove the
>> fake tools used by the previous test with 'test_might_fail' so that the
>> test can be run independantly of the previous test without failing.
> 
>> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
>> index 70afdd06fa7..ebd3af139e5 100755
>> --- a/t/t7610-mergetool.sh
>> +++ b/t/t7610-mergetool.sh
>> @@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '
>>   	test_cmp expect actual
>>   '
>>   
>> +test_expect_success 'mergetool --tool-help shows all recognized tools' '
>> +	# Remove fake tools added in previous tests
>> +	test_might_fail git config --unset merge.tool &&
>> +	test_might_fail git config --remove-section mergetool.mytool &&
>> +	test_might_fail git config --remove-section mergetool.mybase &&
>> +	git mergetool --tool-help >output &&
>> +	grep "$(printf "\t")" output >mergetools &&
>> +	test_line_count = 30 mergetools
>> +'
> 
> This new test fails when the topic 'pb/mergetool-tool-help-fix' is
> built and tested in isolation, because 'git mergetool --tool-help'
> lists only 29 tools instead of the expected 30.  The reason is that
> 'pb/mergetool-tool-help-fix' doesn't include commit 6bc9082c0f
> (mergetools/bc: add `bc4` to the alias list for Beyond Compare,
> 2020-11-11), which added that 30th tool (and is already part of
> v2.30.0).

Indeed. The branch I submitted is based on v2.30.0-rc2, but Junio based
pb/mergetool-tool-help-fix on v2.29.0-rc0~165^2, so the number of supported
tools is "wrong".

> 
> It also makes me wonder whether this is the right way to test this
> fix, because we'll need to adjust this test case every time we add
> support for a new merge tool (which arguably doesn't happen that
> often, but since we are already at 30 it doesn't seem to be that rare
> either).

Yes, it does. I thought about that, and I came to the conclusion that it
was the easiest way to prevent a regression like the one this patch is fixing.
I tought about doing it a different way, like having the test count the available
mergetools itself, and comparing the number of tools shown by '--tool-help' with
that number, but I chose not to do that because it seemed to be more complicated
(and would end up kind of re-implementing what '--tool-help' does, in a way...)

If you think of another of how we could test it, I can try it.

Cheers,

Philippe.