Fix regression in 'git {diff,merge}tool --tool-help' #825

phil-blain · 2020-12-26T19:20:15Z

Changes since v1:

Changed commit authorship (v1 sent with wrong identity).

v1:
I went with Johannes' suggestion finally because upon further inspection,
René's patch for some reason (I did not debug further) caused to code to never
reach 'any_shown=yes' in show_tool_help, therefore changing the output of the
command.

I guess it's too late for inclusion in 2.30...

CC: Johannes Sixt j6t@kdbg.org, Felipe Contreras felipe.contreras@gmail.com, pudinha rogi@skylittlesystem.org, René Scharfe l.s.r@web.de
cc: SZEDER Gábor szeder.dev@gmail.com
cc: Philippe Blain levraiphilippeblain@gmail.com

phil-blain · 2020-12-28T18:21:26Z

/submit

gitgitgadget · 2020-12-28T18:22:43Z

Submitted as pull.825.git.1609179751864.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v1

To fetch this version to local tag pr-825/phil-blain/mergetool-tool-help-fix-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-825/phil-blain/mergetool-tool-help-fix-v1

phil-blain · 2020-12-28T19:40:51Z

/submit

gitgitgadget · 2020-12-28T19:41:57Z

Submitted as pull.825.v2.git.1609184505071.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v2

To fetch this version to local tag pr-825/phil-blain/mergetool-tool-help-fix-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-825/phil-blain/mergetool-tool-help-fix-v2

gitgitgadget · 2021-01-05T23:49:47Z

This branch is now known as pb/mergetool-tool-help-fix.

gitgitgadget · 2021-01-05T23:49:48Z

This patch series was integrated into seen via git@540925d.

gitgitgadget · 2021-01-06T01:16:34Z

This patch series was integrated into seen via git@5e08912.

gitgitgadget · 2021-01-06T13:19:18Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

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

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

gitgitgadget · 2021-01-06T13:19:20Z

User SZEDER Gábor <szeder.dev@gmail.com> has been added to the cc: list.

gitgitgadget · 2021-01-06T13:37:43Z

On the Git mailing list, Philippe Blain wrote (reply to this):

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.

gitgitgadget · 2021-01-06T13:37:45Z

User Philippe Blain <levraiphilippeblain@gmail.com> has been added to the cc: list.

gitgitgadget · 2021-01-06T21:18:04Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

SZEDER Gábor <szeder.dev@gmail.com> writes:

> 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).
>
> 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, that is a very good point.  Also I can imagine us allowing some
tools to be excluded depending on a build/installation option and/or
which platform you are on, so hardcoded 30 smells a bit too fragile
as an approach for a project like ours.

What was the symptom before the fix?  Is it just missing only some
tools among 30?  Was there some pattern in names of missing one and
the ones that still got output?  Or was it more like "we see nothing
shown"?

Thanks.

gitgitgadget · 2021-01-06T23:14:22Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>> What was the symptom before the fix?  Is it just missing only some
>> tools among 30?  Was there some pattern in names of missing one and
>> the ones that still got output?  Or was it more like "we see nothing
>> shown"?
> ...
> Note that 'vimdiff', 'nvimdiff3' (last variant for vimdiff),
> 'bc' and 'bc4' (last variant for bc) are absent, and
> all other tools that have no variants are absent as well.

Thanks, that is exactly the kind of "some pattern" I wanted to see
us looking for, because I wonder if it is a more robust and cheaper
(maintenance wise) approach to find one single tool that we support,
which does not have, and which is unlikely to gain, any numbered
variants.  If we can find such a tool, we can grep for its name in
the output.

gitgitgadget · 2021-01-07T00:36:46Z

On the Git mailing list, Philippe Blain wrote (reply to this):



Le 2021-01-06 à 18:06, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>>> What was the symptom before the fix?  Is it just missing only some
>>> tools among 30?  Was there some pattern in names of missing one and
>>> the ones that still got output?  Or was it more like "we see nothing
>>> shown"?
>> ...
>> Note that 'vimdiff', 'nvimdiff3' (last variant for vimdiff),
>> 'bc' and 'bc4' (last variant for bc) are absent, and
>> all other tools that have no variants are absent as well.
> 
> Thanks, that is exactly the kind of "some pattern" I wanted to see
> us looking for, because I wonder if it is a more robust and cheaper
> (maintenance wise) approach to find one single tool that we support,
> which does not have, and which is unlikely to gain, any numbered
> variants.  If we can find such a tool, we can grep for its name in
> the output.
> 

Or, just choose a few tools, including some variants, and grep for those?
This way we are not relying on a single tool being shown for the test
to pas...

Commit 83bbf9b (mergetool--lib: improve support for vimdiff-style tool variants, 2020-07-29) introduced a regression in the output of `git mergetool --tool-help` and `git difftool --tool-help` [1]. In function 'show_tool_names' in git-mergetool--lib.sh, we loop over the supported mergetools and their variants and accumulate them in the variable 'variants', separating them with a literal '\n'. The code then uses 'echo $variants' to turn these '\n' into newlines, but this behaviour is not portable, it just happens to work in some shells, like dash(1)'s 'echo' builtin. For shells in which 'echo' does not turn '\n' into newlines, the end result is that the only tools that are shown are the existing variants (except the last variant alphabetically), since the variants are separated by actual newlines in '$variants' because of the several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants. Fix this bug by embedding an actual line feed into `variants` in show_tool_names(). While at it, replace `sort | uniq` by `sort -u`. To prevent future regressions, add a simple test that checks that a few known tools are correctly shown (let's avoid counting the total number of tools to lessen the maintenance burden when new tools are added or if '--tool-help' learns additional logic, like hiding tools depending on the current platform). [1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/ Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> Based-on-patch-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>

gitgitgadget · 2021-01-07T01:06:03Z

This patch series was integrated into seen via git@643f812.

phil-blain · 2021-01-07T01:06:45Z

/submit

gitgitgadget · 2021-01-07T01:09:18Z

Submitted as pull.825.v3.git.1609981745668.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v3

To fetch this version to local tag pr-825/phil-blain/mergetool-tool-help-fix-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-825/phil-blain/mergetool-tool-help-fix-v3

gitgitgadget · 2021-01-07T05:08:14Z

This patch series was integrated into seen via git@76dce6c.

gitgitgadget · 2021-01-07T06:45:10Z

This patch series was integrated into seen via git@a7e94a5.

gitgitgadget · 2021-01-07T07:17:03Z

This patch series was integrated into seen via git@8e2015f.

gitgitgadget · 2021-01-07T13:38:58Z

On the Git mailing list, Philippe Blain wrote (reply to this):



Le 2021-01-06 à 20:09, Philippe Blain via GitGitGadget a écrit :
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> 
> ...
> ---
>      Fix regression in 'git {diff,merge}tool --tool-help'
>      
>      Changes since v1:
>      
>       * Changed commit authorship (v1 sent with wrong identity).
>      
>      v1: I went with Johannes' suggestion finally because upon further
>      inspection, René's patch for some reason (I did not debug further)
>      caused to code to never reach 'any_shown=yes' in show_tool_help,
>      therefore changing the output of the command.
>      
>      I guess it's too late for inclusion in 2.30...

Forgot to summarize changes since v2:

- rewrote the description of the symptom of the bug to reflect what
was really happening
- changed the test to not rely on hard-coding the number of tools,
and just checking a few known tools are shown instead

gitgitgadget · 2021-01-08T00:38:18Z

This patch series was integrated into seen via git@053827b.

gitgitgadget · 2021-01-08T03:32:36Z

This patch series was integrated into seen via git@c6690bd.

gitgitgadget · 2021-01-08T22:51:15Z

This patch series was integrated into seen via git@a34cea0.

gitgitgadget · 2021-01-09T00:47:24Z

This patch series was integrated into seen via git@82989ae.

gitgitgadget · 2021-01-12T02:42:54Z

This patch series was integrated into seen via git@679f5ef.

gitgitgadget · 2021-01-13T01:07:01Z

This patch series was integrated into seen via git@a7e200e.

gitgitgadget · 2021-01-13T01:43:47Z

This patch series was integrated into seen via git@ba0f76b.

gitgitgadget · 2021-01-13T02:13:25Z

This patch series was integrated into seen via git@0d874c1.

gitgitgadget · 2021-01-13T02:13:26Z

This patch series was integrated into next via git@ba0f76b.

gitgitgadget · 2021-01-13T08:51:12Z

This patch series was integrated into seen via git@ba0f76b.

gitgitgadget · 2021-01-13T09:25:19Z

This patch series was integrated into seen via git@fa7b9bb.

gitgitgadget · 2021-01-15T01:24:42Z

This patch series was integrated into seen via git@ba0f76b.

gitgitgadget · 2021-01-15T02:00:27Z

This patch series was integrated into seen via git@bda98ad.

gitgitgadget · 2021-01-15T05:51:32Z

This patch series was integrated into seen via git@ba0f76b.

gitgitgadget · 2021-01-15T07:39:48Z

This patch series was integrated into seen via git@f8d755a.

gitgitgadget · 2021-01-16T00:25:07Z

This patch series was integrated into seen via git@4614585.

gitgitgadget · 2021-01-16T22:04:19Z

This patch series was integrated into seen via git@073552d.

gitgitgadget · 2021-01-16T22:04:20Z

This patch series was integrated into next via git@073552d.

gitgitgadget · 2021-01-16T22:04:21Z

This patch series was integrated into master via git@073552d.

gitgitgadget · 2021-01-16T22:04:24Z

Closed via 073552d.

phil-blain force-pushed the mergetool-tool-help-fix branch from a79d0a0 to 2b9dce3 Compare December 28, 2020 19:38

gitgitgadget bot added the seen label Jan 5, 2021

phil-blain force-pushed the mergetool-tool-help-fix branch from 2b9dce3 to f664219 Compare January 7, 2021 00:53

gitgitgadget bot added the next label Jan 13, 2021

gitgitgadget bot added the master label Jan 16, 2021

gitgitgadget bot closed this Jan 16, 2021

phil-blain deleted the mergetool-tool-help-fix branch August 17, 2021 21:53

Fix regression in 'git {diff,merge}tool --tool-help' #825

Fix regression in 'git {diff,merge}tool --tool-help' #825

Conversation

phil-blain commented Dec 26, 2020 • edited by gitgitgadget bot

phil-blain commented Dec 28, 2020

gitgitgadget bot commented Dec 28, 2020

phil-blain commented Dec 28, 2020

gitgitgadget bot commented Dec 28, 2020

gitgitgadget bot commented Jan 5, 2021

gitgitgadget bot commented Jan 5, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 6, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

phil-blain commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 7, 2021

gitgitgadget bot commented Jan 8, 2021

gitgitgadget bot commented Jan 8, 2021

gitgitgadget bot commented Jan 8, 2021

gitgitgadget bot commented Jan 9, 2021

gitgitgadget bot commented Jan 12, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 13, 2021

gitgitgadget bot commented Jan 15, 2021

gitgitgadget bot commented Jan 15, 2021

gitgitgadget bot commented Jan 15, 2021

gitgitgadget bot commented Jan 15, 2021

gitgitgadget bot commented Jan 16, 2021

gitgitgadget bot commented Jan 16, 2021

gitgitgadget bot commented Jan 16, 2021

gitgitgadget bot commented Jan 16, 2021

gitgitgadget bot commented Jan 16, 2021

phil-blain commented Dec 26, 2020 •

edited by gitgitgadget bot