{"thread":{"id":"54890","subject":"[PATCH] mergetool--lib: fix '--tool-help' to correctly show available tools","startedAt":"2020-12-28T18:23:39Z","lastAt":"2021-01-07T13:37:50Z","messageCount":10,"participants":["Philippe Blain via GitGitGadget","SZEDER Gábor","Philippe Blain","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"413081","messageId":"pull.825.git.1609179751864.gitgitgadget@gmail.com","threadId":"54890","inReplyTo":null,"subject":"[PATCH] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-28T18:22:31Z","receivedAt":"2020-12-28T18:23:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <philippe.blain@canada.ca>\n\nCommit 83bbf9b92e (mergetool--lib: improve support for vimdiff-style tool\nvariants, 2020-07-29) introduced a regression in the output of `git mergetool\n--tool-help` and `git difftool --tool-help` [1].\n\nIn function 'show_tool_names' in git-mergetool--lib.sh, we loop over the\nsupported mergetools and their variants and accumulate them in the variable\n'variants', separating them with a literal '\\n'.\n\nThe code then uses 'echo $variants' to turn these '\\n' into newlines, but this\nbehaviour is not portable, it just happens to work in some shells, like\ndash(1)'s 'echo' builtin.\n\nFor shells in which 'echo' does not turn '\\n' into newlines, the end result is\nthat the only tools that are shown are those that are found and have variants,\nsince the variants are separated by actual newlines in '$variants' because of\nthe several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n\nFix this bug by embedding an actual line feed into `variants` in\nshow_tool_names(). While at it, replace `sort | uniq` by `sort -u`.\n\nTo prevent future regressions, add a simple test that counts the number\nof tools shown by 'git mergetool --tool-help', irrespective of their\ninstalled status, by making use of the fact that mergetools are listed\nby 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n`git config` commands used at the beginning of the test to remove the\nfake tools used by the previous test with 'test_might_fail' so that the\ntest can be run independantly of the previous test without failing.\n\n[1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/\n\nReported-by: Philippe Blain <levraiphilippeblain@gmail.com>\nBased-on-patch-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    Fix regression in 'git {diff,merge}tool --tool-help'\n    \n    I went with Johannes' suggestion finally because upon further\n    inspection, René's patch for some reason (I did not debug further)\n    caused to code to never reach 'any_shown=yes' in show_tool_help,\n    therefore changing the output of the command.\n    \n    I guess it's too late for inclusion in 2.30...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-825%2Fphil-blain%2Fmergetool-tool-help-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/825\n\n git-mergetool--lib.sh |  6 ++++--\n t/t7610-mergetool.sh  | 10 ++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7225abd8112..78f3647ed97 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,9 +46,11 @@ show_tool_names () {\n \t\twhile read scriptname\n \t\tdo\n \t\t\tsetup_tool \"$scriptname\" 2>/dev/null\n-\t\t\tvariants=\"$variants$(list_tool_variants)\\n\"\n+\t\t\t# We need an actual line feed here\n+\t\t\tvariants=\"$variants\n+$(list_tool_variants)\"\n \t\tdone\n-\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n+\t\tvariants=\"$(echo \"$variants\" | sort -u)\"\n \n \t\tfor toolname in $variants\n \t\tdo\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 70afdd06fa7..ebd3af139e5 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'mergetool --tool-help shows all recognized tools' '\n+\t# Remove fake tools added in previous tests\n+\ttest_might_fail git config --unset merge.tool &&\n+\ttest_might_fail git config --remove-section mergetool.mytool &&\n+\ttest_might_fail git config --remove-section mergetool.mybase &&\n+\tgit mergetool --tool-help >output &&\n+\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n+\ttest_line_count = 30 mergetools\n+'\n+\n test_done\n\nbase-commit: 4a0de43f4923993377dbbc42cfc0a1054b6c5ccf\n-- \ngitgitgadget\n"},{"id":"413103","messageId":"pull.825.v2.git.1609184505071.gitgitgadget@gmail.com","threadId":"54890","inReplyTo":"pull.825.git.1609179751864.gitgitgadget@gmail.com","subject":"[PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-28T19:41:44Z","receivedAt":"2020-12-28T23:23:04Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nCommit 83bbf9b92e (mergetool--lib: improve support for vimdiff-style tool\nvariants, 2020-07-29) introduced a regression in the output of `git mergetool\n--tool-help` and `git difftool --tool-help` [1].\n\nIn function 'show_tool_names' in git-mergetool--lib.sh, we loop over the\nsupported mergetools and their variants and accumulate them in the variable\n'variants', separating them with a literal '\\n'.\n\nThe code then uses 'echo $variants' to turn these '\\n' into newlines, but this\nbehaviour is not portable, it just happens to work in some shells, like\ndash(1)'s 'echo' builtin.\n\nFor shells in which 'echo' does not turn '\\n' into newlines, the end result is\nthat the only tools that are shown are those that are found and have variants,\nsince the variants are separated by actual newlines in '$variants' because of\nthe several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n\nFix this bug by embedding an actual line feed into `variants` in\nshow_tool_names(). While at it, replace `sort | uniq` by `sort -u`.\n\nTo prevent future regressions, add a simple test that counts the number\nof tools shown by 'git mergetool --tool-help', irrespective of their\ninstalled status, by making use of the fact that mergetools are listed\nby 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n`git config` commands used at the beginning of the test to remove the\nfake tools used by the previous test with 'test_might_fail' so that the\ntest can be run independantly of the previous test without failing.\n\n[1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/\n\nReported-by: Philippe Blain <levraiphilippeblain@gmail.com>\nBased-on-patch-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    Fix regression in 'git {diff,merge}tool --tool-help'\n    \n    Changes since v1:\n    \n     * Changed commit authorship (v1 sent with wrong identity).\n    \n    v1: I went with Johannes' suggestion finally because upon further\n    inspection, René's patch for some reason (I did not debug further)\n    caused to code to never reach 'any_shown=yes' in show_tool_help,\n    therefore changing the output of the command.\n    \n    I guess it's too late for inclusion in 2.30...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-825%2Fphil-blain%2Fmergetool-tool-help-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/825\n\nRange-diff vs v1:\n\n 1:  a79d0a09daa ! 1:  2b9dce31fd0 mergetool--lib: fix '--tool-help' to correctly show available tools\n     @@\n       ## Metadata ##\n     -Author: Philippe Blain <philippe.blain@canada.ca>\n     +Author: Philippe Blain <levraiphilippeblain@gmail.com>\n      \n       ## Commit message ##\n          mergetool--lib: fix '--tool-help' to correctly show available tools\n\n\n git-mergetool--lib.sh |  6 ++++--\n t/t7610-mergetool.sh  | 10 ++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7225abd8112..78f3647ed97 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,9 +46,11 @@ show_tool_names () {\n \t\twhile read scriptname\n \t\tdo\n \t\t\tsetup_tool \"$scriptname\" 2>/dev/null\n-\t\t\tvariants=\"$variants$(list_tool_variants)\\n\"\n+\t\t\t# We need an actual line feed here\n+\t\t\tvariants=\"$variants\n+$(list_tool_variants)\"\n \t\tdone\n-\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n+\t\tvariants=\"$(echo \"$variants\" | sort -u)\"\n \n \t\tfor toolname in $variants\n \t\tdo\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 70afdd06fa7..ebd3af139e5 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'mergetool --tool-help shows all recognized tools' '\n+\t# Remove fake tools added in previous tests\n+\ttest_might_fail git config --unset merge.tool &&\n+\ttest_might_fail git config --remove-section mergetool.mytool &&\n+\ttest_might_fail git config --remove-section mergetool.mybase &&\n+\tgit mergetool --tool-help >output &&\n+\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n+\ttest_line_count = 30 mergetools\n+'\n+\n test_done\n\nbase-commit: 4a0de43f4923993377dbbc42cfc0a1054b6c5ccf\n-- \ngitgitgadget\n"},{"id":"413577","messageId":"20210106131651.GQ8396@szeder.dev","threadId":"54890","inReplyTo":"pull.825.v2.git.1609184505071.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-01-06T13:16:51Z","receivedAt":"2021-01-06T13:17:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 28, 2020 at 07:41:44PM +0000, Philippe Blain via GitGitGadget wrote:\n> To prevent future regressions, add a simple test that counts the number\n> of tools shown by 'git mergetool --tool-help', irrespective of their\n> installed status, by making use of the fact that mergetools are listed\n> by 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n> `git config` commands used at the beginning of the test to remove the\n> fake tools used by the previous test with 'test_might_fail' so that the\n> test can be run independantly of the previous test without failing.\n\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> index 70afdd06fa7..ebd3af139e5 100755\n> --- a/t/t7610-mergetool.sh\n> +++ b/t/t7610-mergetool.sh\n> @@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'mergetool --tool-help shows all recognized tools' '\n> +\t# Remove fake tools added in previous tests\n> +\ttest_might_fail git config --unset merge.tool &&\n> +\ttest_might_fail git config --remove-section mergetool.mytool &&\n> +\ttest_might_fail git config --remove-section mergetool.mybase &&\n> +\tgit mergetool --tool-help >output &&\n> +\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n> +\ttest_line_count = 30 mergetools\n> +'\n\nThis new test fails when the topic 'pb/mergetool-tool-help-fix' is\nbuilt and tested in isolation, because 'git mergetool --tool-help'\nlists only 29 tools instead of the expected 30.  The reason is that\n'pb/mergetool-tool-help-fix' doesn't include commit 6bc9082c0f\n(mergetools/bc: add `bc4` to the alias list for Beyond Compare,\n2020-11-11), which added that 30th tool (and is already part of\nv2.30.0).\n\nIt also makes me wonder whether this is the right way to test this\nfix, because we'll need to adjust this test case every time we add\nsupport for a new merge tool (which arguably doesn't happen that\noften, but since we are already at 30 it doesn't seem to be that rare\neither).\n\n"},{"id":"413578","messageId":"11b3c261-14e6-0293-02a6-92bd1ea680d2@gmail.com","threadId":"54890","inReplyTo":"20210106131651.GQ8396@szeder.dev","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-01-06T13:34:25Z","receivedAt":"2021-01-06T13:35:10Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Gábor,\n\nLe 2021-01-06 à 08:16, SZEDER Gábor a écrit :\n> On Mon, Dec 28, 2020 at 07:41:44PM +0000, Philippe Blain via GitGitGadget wrote:\n>> To prevent future regressions, add a simple test that counts the number\n>> of tools shown by 'git mergetool --tool-help', irrespective of their\n>> installed status, by making use of the fact that mergetools are listed\n>> by 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n>> `git config` commands used at the beginning of the test to remove the\n>> fake tools used by the previous test with 'test_might_fail' so that the\n>> test can be run independantly of the previous test without failing.\n> \n>> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n>> index 70afdd06fa7..ebd3af139e5 100755\n>> --- a/t/t7610-mergetool.sh\n>> +++ b/t/t7610-mergetool.sh\n>> @@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n>>   \ttest_cmp expect actual\n>>   '\n>>   \n>> +test_expect_success 'mergetool --tool-help shows all recognized tools' '\n>> +\t# Remove fake tools added in previous tests\n>> +\ttest_might_fail git config --unset merge.tool &&\n>> +\ttest_might_fail git config --remove-section mergetool.mytool &&\n>> +\ttest_might_fail git config --remove-section mergetool.mybase &&\n>> +\tgit mergetool --tool-help >output &&\n>> +\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n>> +\ttest_line_count = 30 mergetools\n>> +'\n> \n> This new test fails when the topic 'pb/mergetool-tool-help-fix' is\n> built and tested in isolation, because 'git mergetool --tool-help'\n> lists only 29 tools instead of the expected 30.  The reason is that\n> 'pb/mergetool-tool-help-fix' doesn't include commit 6bc9082c0f\n> (mergetools/bc: add `bc4` to the alias list for Beyond Compare,\n> 2020-11-11), which added that 30th tool (and is already part of\n> v2.30.0).\n\nIndeed. The branch I submitted is based on v2.30.0-rc2, but Junio based\npb/mergetool-tool-help-fix on v2.29.0-rc0~165^2, so the number of supported\ntools is \"wrong\".\n\n> \n> It also makes me wonder whether this is the right way to test this\n> fix, because we'll need to adjust this test case every time we add\n> support for a new merge tool (which arguably doesn't happen that\n> often, but since we are already at 30 it doesn't seem to be that rare\n> either).\n\nYes, it does. I thought about that, and I came to the conclusion that it\nwas the easiest way to prevent a regression like the one this patch is fixing.\nI tought about doing it a different way, like having the test count the available\nmergetools itself, and comparing the number of tools shown by '--tool-help' with\nthat number, but I chose not to do that because it seemed to be more complicated\n(and would end up kind of re-implementing what '--tool-help' does, in a way...)\n\nIf you think of another of how we could test it, I can try it.\n\nCheers,\n\nPhilippe.\n"},{"id":"413607","messageId":"xmqq5z49rclc.fsf@gitster.c.googlers.com","threadId":"54890","inReplyTo":"20210106131651.GQ8396@szeder.dev","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T21:15:43Z","receivedAt":"2021-01-06T21:16:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Mon, Dec 28, 2020 at 07:41:44PM +0000, Philippe Blain via GitGitGadget wrote:\n>> To prevent future regressions, add a simple test that counts the number\n>> of tools shown by 'git mergetool --tool-help', irrespective of their\n>> installed status, by making use of the fact that mergetools are listed\n>> by 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n>> `git config` commands used at the beginning of the test to remove the\n>> fake tools used by the previous test with 'test_might_fail' so that the\n>> test can be run independantly of the previous test without failing.\n>\n>> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n>> index 70afdd06fa7..ebd3af139e5 100755\n>> --- a/t/t7610-mergetool.sh\n>> +++ b/t/t7610-mergetool.sh\n>> @@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n>>  \ttest_cmp expect actual\n>>  '\n>>  \n>> +test_expect_success 'mergetool --tool-help shows all recognized tools' '\n>> +\t# Remove fake tools added in previous tests\n>> +\ttest_might_fail git config --unset merge.tool &&\n>> +\ttest_might_fail git config --remove-section mergetool.mytool &&\n>> +\ttest_might_fail git config --remove-section mergetool.mybase &&\n>> +\tgit mergetool --tool-help >output &&\n>> +\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n>> +\ttest_line_count = 30 mergetools\n>> +'\n>\n> This new test fails when the topic 'pb/mergetool-tool-help-fix' is\n> built and tested in isolation, because 'git mergetool --tool-help'\n> lists only 29 tools instead of the expected 30.  The reason is that\n> 'pb/mergetool-tool-help-fix' doesn't include commit 6bc9082c0f\n> (mergetools/bc: add `bc4` to the alias list for Beyond Compare,\n> 2020-11-11), which added that 30th tool (and is already part of\n> v2.30.0).\n>\n> It also makes me wonder whether this is the right way to test this\n> fix, because we'll need to adjust this test case every time we add\n> support for a new merge tool (which arguably doesn't happen that\n> often, but since we are already at 30 it doesn't seem to be that rare\n> either).\n\nYes, that is a very good point.  Also I can imagine us allowing some\ntools to be excluded depending on a build/installation option and/or\nwhich platform you are on, so hardcoded 30 smells a bit too fragile\nas an approach for a project like ours.\n\nWhat was the symptom before the fix?  Is it just missing only some\ntools among 30?  Was there some pattern in names of missing one and\nthe ones that still got output?  Or was it more like \"we see nothing\nshown\"?\n\nThanks.\n"},{"id":"413620","messageId":"3664fffc-c2a0-62d6-298b-8e95f3c58a68@gmail.com","threadId":"54890","inReplyTo":"xmqq5z49rclc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-01-06T22:59:10Z","receivedAt":"2021-01-06T23:00:13Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2021-01-06 à 16:15, Junio C Hamano a écrit :\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n>> On Mon, Dec 28, 2020 at 07:41:44PM +0000, Philippe Blain via GitGitGadget wrote:\n>>> To prevent future regressions, add a simple test that counts the number\n>>> of tools shown by 'git mergetool --tool-help', irrespective of their\n>>> installed status, by making use of the fact that mergetools are listed\n>>> by 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n>>> `git config` commands used at the beginning of the test to remove the\n>>> fake tools used by the previous test with 'test_might_fail' so that the\n>>> test can be run independantly of the previous test without failing.\n>>\n>>> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n>>> index 70afdd06fa7..ebd3af139e5 100755\n>>> --- a/t/t7610-mergetool.sh\n>>> +++ b/t/t7610-mergetool.sh\n>>> @@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n>>>   \ttest_cmp expect actual\n>>>   '\n>>>   \n>>> +test_expect_success 'mergetool --tool-help shows all recognized tools' '\n>>> +\t# Remove fake tools added in previous tests\n>>> +\ttest_might_fail git config --unset merge.tool &&\n>>> +\ttest_might_fail git config --remove-section mergetool.mytool &&\n>>> +\ttest_might_fail git config --remove-section mergetool.mybase &&\n>>> +\tgit mergetool --tool-help >output &&\n>>> +\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n>>> +\ttest_line_count = 30 mergetools\n>>> +'\n>>\n>> This new test fails when the topic 'pb/mergetool-tool-help-fix' is\n>> built and tested in isolation, because 'git mergetool --tool-help'\n>> lists only 29 tools instead of the expected 30.  The reason is that\n>> 'pb/mergetool-tool-help-fix' doesn't include commit 6bc9082c0f\n>> (mergetools/bc: add `bc4` to the alias list for Beyond Compare,\n>> 2020-11-11), which added that 30th tool (and is already part of\n>> v2.30.0).\n>>\n>> It also makes me wonder whether this is the right way to test this\n>> fix, because we'll need to adjust this test case every time we add\n>> support for a new merge tool (which arguably doesn't happen that\n>> often, but since we are already at 30 it doesn't seem to be that rare\n>> either).\n> \n> Yes, that is a very good point.  Also I can imagine us allowing some\n> tools to be excluded depending on a build/installation option and/or\n> which platform you are on, so hardcoded 30 smells a bit too fragile\n> as an approach for a project like ours.\n\nAs I said in my response to Gabor [1], this was a simple attempt at\npreventing a future breakage. I agree that it could be improved, but\nI'm not sure how without actually re-implementing the code of\n'git mergetool --tool-help' in the test itself, especially if more\nlogic is added as you say for eg. hiding some tools depending\non platform/installation options.\n\n> \n> What was the symptom before the fix?  Is it just missing only some\n> tools among 30?  Was there some pattern in names of missing one and\n> the ones that still got output?  Or was it more like \"we see nothing\n> shown\"?\n> \n\nThe only tools that are shown are the variants, except the alphabetically last variant,\nbecause as I wrote in the commit message,\n\nFor shells in which 'echo' does not turn '\\n' into newlines, the end result is\nthat the only tools that are shown are those that are found and have variants,\nsince the variants are separated by actual newlines in '$variants' because of\nthe several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n\n[Note: I should re-roll and remove \"that are found\" since that bit is not true).\n\nFor example, I get this with Bash 4.4.19 as /bin/sh on Ubuntu 18.04:\n\n---\n$ git mergetool --tool-help\n'git mergetool --tool=<tool>' may be set to one of the following:\n                 gvimdiff\n                 gvimdiff2\n                 gvimdiff3\n                 vimdiff2\n                 vimdiff3\n\nThe following tools are valid, but not currently available:\n                 bc3\n                 nvimdiff\n                 nvimdiff2\n\nSome of the tools listed above only work in a windowed\nenvironment. If run in a terminal-only session, they will fail.\n---\n\nNote that 'vimdiff', 'nvimdiff3' (last variant for vimdiff),\n'bc' and 'bc4' (last variant for bc) are absent, and\nall other tools that have no variants are absent as well.\n\nI was able to understand what was happening by running 'git mergetool'\nwith Bash tracing:\n\nPATH=$(git --exec-path):$PATH \\\nPS4='+(${BASH_SOURCE}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }' \\\nbash -x $(git --exec-path)/git-mergetool --tool-help\n\nP.S.\nI noticed that all tools are shown correctly on my Mac,\nwhich has Bash 3.2.57 as /bin/sh. So behaviour of 'echo' regarding\n'\\n' depends on both the shell and its version (!)\n\n\n[1] https://lore.kernel.org/git/11b3c261-14e6-0293-02a6-92bd1ea680d2@gmail.com/\n"},{"id":"413621","messageId":"xmqqeeixpsvo.fsf@gitster.c.googlers.com","threadId":"54890","inReplyTo":"3664fffc-c2a0-62d6-298b-8e95f3c58a68@gmail.com","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T23:06:51Z","receivedAt":"2021-01-06T23:07:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n>> What was the symptom before the fix?  Is it just missing only some\n>> tools among 30?  Was there some pattern in names of missing one and\n>> the ones that still got output?  Or was it more like \"we see nothing\n>> shown\"?\n> ...\n> Note that 'vimdiff', 'nvimdiff3' (last variant for vimdiff),\n> 'bc' and 'bc4' (last variant for bc) are absent, and\n> all other tools that have no variants are absent as well.\n\nThanks, that is exactly the kind of \"some pattern\" I wanted to see\nus looking for, because I wonder if it is a more robust and cheaper\n(maintenance wise) approach to find one single tool that we support,\nwhich does not have, and which is unlikely to gain, any numbered\nvariants.  If we can find such a tool, we can grep for its name in\nthe output.\n\n\n\n"},{"id":"413629","messageId":"e9a1e45b-70b4-2478-1669-4308c5902c5b@gmail.com","threadId":"54890","inReplyTo":"xmqqeeixpsvo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-01-07T00:32:28Z","receivedAt":"2021-01-07T00:33:28Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2021-01-06 à 18:06, Junio C Hamano a écrit :\n> Philippe Blain <levraiphilippeblain@gmail.com> writes:\n> \n>>> What was the symptom before the fix?  Is it just missing only some\n>>> tools among 30?  Was there some pattern in names of missing one and\n>>> the ones that still got output?  Or was it more like \"we see nothing\n>>> shown\"?\n>> ...\n>> Note that 'vimdiff', 'nvimdiff3' (last variant for vimdiff),\n>> 'bc' and 'bc4' (last variant for bc) are absent, and\n>> all other tools that have no variants are absent as well.\n> \n> Thanks, that is exactly the kind of \"some pattern\" I wanted to see\n> us looking for, because I wonder if it is a more robust and cheaper\n> (maintenance wise) approach to find one single tool that we support,\n> which does not have, and which is unlikely to gain, any numbered\n> variants.  If we can find such a tool, we can grep for its name in\n> the output.\n> \n\nOr, just choose a few tools, including some variants, and grep for those?\nThis way we are not relying on a single tool being shown for the test\nto pas...\n"},{"id":"413630","messageId":"pull.825.v3.git.1609981745668.gitgitgadget@gmail.com","threadId":"54890","inReplyTo":"pull.825.v2.git.1609184505071.gitgitgadget@gmail.com","subject":"[PATCH v3] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-07T01:09:05Z","receivedAt":"2021-01-07T01:09:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nCommit 83bbf9b92e (mergetool--lib: improve support for vimdiff-style tool\nvariants, 2020-07-29) introduced a regression in the output of `git mergetool\n--tool-help` and `git difftool --tool-help` [1].\n\nIn function 'show_tool_names' in git-mergetool--lib.sh, we loop over the\nsupported mergetools and their variants and accumulate them in the variable\n'variants', separating them with a literal '\\n'.\n\nThe code then uses 'echo $variants' to turn these '\\n' into newlines, but this\nbehaviour is not portable, it just happens to work in some shells, like\ndash(1)'s 'echo' builtin.\n\nFor shells in which 'echo' does not turn '\\n' into newlines, the end\nresult is that the only tools that are shown are the existing variants\n(except the last variant alphabetically), since the variants are\nseparated by actual newlines in '$variants' because of the several\n'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n\nFix this bug by embedding an actual line feed into `variants` in\nshow_tool_names(). While at it, replace `sort | uniq` by `sort -u`.\n\nTo prevent future regressions, add a simple test that checks that a few\nknown tools are correctly shown (let's avoid counting the total number\nof tools to lessen the maintenance burden when new tools are added or if\n'--tool-help' learns additional logic, like hiding tools depending on\nthe current platform).\n\n[1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/\n\nReported-by: Philippe Blain <levraiphilippeblain@gmail.com>\nBased-on-patch-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    Fix regression in 'git {diff,merge}tool --tool-help'\n    \n    Changes since v1:\n    \n     * Changed commit authorship (v1 sent with wrong identity).\n    \n    v1: I went with Johannes' suggestion finally because upon further\n    inspection, René's patch for some reason (I did not debug further)\n    caused to code to never reach 'any_shown=yes' in show_tool_help,\n    therefore changing the output of the command.\n    \n    I guess it's too late for inclusion in 2.30...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-825%2Fphil-blain%2Fmergetool-tool-help-fix-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/825\n\nRange-diff vs v2:\n\n 1:  2b9dce31fd0 ! 1:  f66421939ec mergetool--lib: fix '--tool-help' to correctly show available tools\n     @@ Commit message\n          behaviour is not portable, it just happens to work in some shells, like\n          dash(1)'s 'echo' builtin.\n      \n     -    For shells in which 'echo' does not turn '\\n' into newlines, the end result is\n     -    that the only tools that are shown are those that are found and have variants,\n     -    since the variants are separated by actual newlines in '$variants' because of\n     -    the several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n     +    For shells in which 'echo' does not turn '\\n' into newlines, the end\n     +    result is that the only tools that are shown are the existing variants\n     +    (except the last variant alphabetically), since the variants are\n     +    separated by actual newlines in '$variants' because of the several\n     +    'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.\n      \n          Fix this bug by embedding an actual line feed into `variants` in\n          show_tool_names(). While at it, replace `sort | uniq` by `sort -u`.\n      \n     -    To prevent future regressions, add a simple test that counts the number\n     -    of tools shown by 'git mergetool --tool-help', irrespective of their\n     -    installed status, by making use of the fact that mergetools are listed\n     -    by 'git mergetool --tool-help' on lines starting with tabs. Prefix the\n     -    `git config` commands used at the beginning of the test to remove the\n     -    fake tools used by the previous test with 'test_might_fail' so that the\n     -    test can be run independantly of the previous test without failing.\n     +    To prevent future regressions, add a simple test that checks that a few\n     +    known tools are correctly shown (let's avoid counting the total number\n     +    of tools to lessen the maintenance burden when new tools are added or if\n     +    '--tool-help' learns additional logic, like hiding tools depending on\n     +    the current platform).\n      \n          [1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/\n      \n     @@ t/t7610-mergetool.sh: test_expect_success 'mergetool -Oorder-file is honored' '\n       \ttest_cmp expect actual\n       '\n       \n     -+test_expect_success 'mergetool --tool-help shows all recognized tools' '\n     -+\t# Remove fake tools added in previous tests\n     -+\ttest_might_fail git config --unset merge.tool &&\n     -+\ttest_might_fail git config --remove-section mergetool.mytool &&\n     -+\ttest_might_fail git config --remove-section mergetool.mybase &&\n     -+\tgit mergetool --tool-help >output &&\n     -+\tgrep \"$(printf \"\\t\")\" output >mergetools &&\n     -+\ttest_line_count = 30 mergetools\n     ++test_expect_success 'mergetool --tool-help shows recognized tools' '\n     ++\t# Check a few known tools are correctly shown\n     ++\tgit mergetool --tool-help >mergetools &&\n     ++\tgrep vimdiff mergetools &&\n     ++\tgrep vimdiff3 mergetools &&\n     ++\tgrep gvimdiff2 mergetools &&\n     ++\tgrep araxis mergetools &&\n     ++\tgrep xxdiff mergetools &&\n     ++\tgrep meld mergetools\n      +'\n      +\n       test_done\n\n\n git-mergetool--lib.sh |  6 ++++--\n t/t7610-mergetool.sh  | 11 +++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7225abd8112..78f3647ed97 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,9 +46,11 @@ show_tool_names () {\n \t\twhile read scriptname\n \t\tdo\n \t\t\tsetup_tool \"$scriptname\" 2>/dev/null\n-\t\t\tvariants=\"$variants$(list_tool_variants)\\n\"\n+\t\t\t# We need an actual line feed here\n+\t\t\tvariants=\"$variants\n+$(list_tool_variants)\"\n \t\tdone\n-\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n+\t\tvariants=\"$(echo \"$variants\" | sort -u)\"\n \n \t\tfor toolname in $variants\n \t\tdo\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 70afdd06fa7..6ac75b5d4c0 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -828,4 +828,15 @@ test_expect_success 'mergetool -Oorder-file is honored' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'mergetool --tool-help shows recognized tools' '\n+\t# Check a few known tools are correctly shown\n+\tgit mergetool --tool-help >mergetools &&\n+\tgrep vimdiff mergetools &&\n+\tgrep vimdiff3 mergetools &&\n+\tgrep gvimdiff2 mergetools &&\n+\tgrep araxis mergetools &&\n+\tgrep xxdiff mergetools &&\n+\tgrep meld mergetools\n+'\n+\n test_done\n\nbase-commit: 4a0de43f4923993377dbbc42cfc0a1054b6c5ccf\n-- \ngitgitgadget\n"},{"id":"413686","messageId":"e5d24e6e-9a69-aa1f-483b-b892497a0e20@gmail.com","threadId":"54890","inReplyTo":"pull.825.v3.git.1609981745668.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] mergetool--lib: fix '--tool-help' to correctly show available tools","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-01-07T13:37:05Z","receivedAt":"2021-01-07T13:37:50Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2021-01-06 à 20:09, Philippe Blain via GitGitGadget a écrit :\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n> \n> ...\n> ---\n>      Fix regression in 'git {diff,merge}tool --tool-help'\n>      \n>      Changes since v1:\n>      \n>       * Changed commit authorship (v1 sent with wrong identity).\n>      \n>      v1: I went with Johannes' suggestion finally because upon further\n>      inspection, René's patch for some reason (I did not debug further)\n>      caused to code to never reach 'any_shown=yes' in show_tool_help,\n>      therefore changing the output of the command.\n>      \n>      I guess it's too late for inclusion in 2.30...\n\nForgot to summarize changes since v2:\n\n- rewrote the description of the symptom of the bug to reflect what\nwas really happening\n- changed the test to not rely on hard-coding the number of tools,\nand just checking a few known tools are shown instead\n"}]}