{"thread":{"id":"54851","subject":"[BUG] Regression in 'git mergetool --tool-help'","startedAt":"2020-12-19T04:24:57Z","lastAt":"2020-12-21T05:48:02Z","messageCount":13,"participants":["Philippe Blain","Felipe Contreras","Junio C Hamano","Johannes Sixt","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"412616","messageId":"CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com","threadId":"54851","inReplyTo":null,"subject":"[BUG] Regression in 'git mergetool --tool-help'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-19T04:23:41Z","receivedAt":"2020-12-19T04:24:57Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi all,\n\nIt seems that 83bbf9b92e (mergetool--lib: improve support for\nvimdiff-style tool variants,\n2020-07-29) introduced a regression in the output shown by 'git\nmergetool --tool-help'.\nEven if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH,\nthey are not shown at all when\nthe above command is invoked. Only vimdiff, gvimdiff and their *2 and\n*3 variants are listed.\n\nThe commit above was found by bisecting starting from 2.29.2 knowing\n2.26.0 is OK. I can\nreproduce the bug on Ubuntu 18 and Ubuntu 14 but not on Ubuntu 20 (all\nLTS versions and all\nwith Git 2.29.2) so maybe something else is also at play here...\n\nCheers,\n\nPhilippe.\n"},{"id":"412618","messageId":"CAMP44s15CJVGRg5Qxcgqq8xVkDhpVyxND67-iQZF9uZ_PRstEQ@mail.gmail.com","threadId":"54851","inReplyTo":"CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T04:50:48Z","receivedAt":"2020-12-19T04:51:42Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 18, 2020 at 10:27 PM Philippe Blain\n<levraiphilippeblain@gmail.com> wrote:\n\n> It seems that 83bbf9b92e (mergetool--lib: improve support for\n> vimdiff-style tool variants,\n> 2020-07-29) introduced a regression in the output shown by 'git\n> mergetool --tool-help'.\n> Even if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH,\n> they are not shown at all when\n> the above command is invoked. Only vimdiff, gvimdiff and their *2 and\n> *3 variants are listed.\n\nHow about this?\n\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,7 +46,7 @@ show_tool_names () {\n                while read scriptname\n                do\n                        setup_tool \"$scriptname\" 2>/dev/null\n-                       variants=\"$variants$(list_tool_variants)\\n\"\n+                       variants=\"$variants$(list_tool_variants)\"$'\\n'\n                done\n                variants=\"$(echo \"$variants\" | sort | uniq)\"\n\n\n-- \nFelipe Contreras\n"},{"id":"412619","messageId":"xmqqmtyaqsto.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T05:16:03Z","receivedAt":"2020-12-19T05:17:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 204a5acd66..29fecc340f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -43,7 +43,14 @@ show_tool_names () {\n \n \tshown_any=\n \t( cd \"$MERGE_TOOLS_DIR\" && ls ) | {\n-\t\twhile read toolname\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\nAhh, that does look quite bogus as \\n won't do anything useful\nthere.  And I do not think we _need_ to use LF there.\n\n+\t\tdone\n+\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n\n+\n+\t\tfor toolname in $variants\n\nJust replace \\n with \" \" (whitespace) and do something like this,\nperhaps?\n\n\t\t\tvariants=\"$variantes$(list_tool_variants) \"\n\t\tdone\n\t\tvariants=$(printf \"%s\\n\" $variants | sort | uniq)\n\n"},{"id":"412621","messageId":"xmqqczz6qs0p.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"CAMP44s15CJVGRg5Qxcgqq8xVkDhpVyxND67-iQZF9uZ_PRstEQ@mail.gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T05:33:26Z","receivedAt":"2020-12-19T05:35:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Fri, Dec 18, 2020 at 10:27 PM Philippe Blain\n> <levraiphilippeblain@gmail.com> wrote:\n>\n>> It seems that 83bbf9b92e (mergetool--lib: improve support for\n>> vimdiff-style tool variants,\n>> 2020-07-29) introduced a regression in the output shown by 'git\n>> mergetool --tool-help'.\n>> Even if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH,\n>> they are not shown at all when\n>> the above command is invoked. Only vimdiff, gvimdiff and their *2 and\n>> *3 variants are listed.\n>\n> How about this?\n>\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -46,7 +46,7 @@ show_tool_names () {\n>                 while read scriptname\n>                 do\n>                         setup_tool \"$scriptname\" 2>/dev/null\n> -                       variants=\"$variants$(list_tool_variants)\\n\"\n> +                       variants=\"$variants$(list_tool_variants)\"$'\\n'\n>                 done\n>                 variants=\"$(echo \"$variants\" | sort | uniq)\"\n\nAh, I didn't see your variant before sending mine.  $'\\n' would work\nwith bash but we prefer not to rely on bashisms here.\n\n"},{"id":"412631","messageId":"5fddeda8a5a92_1de0de2088a@natae.notmuch","threadId":"54851","inReplyTo":"xmqqczz6qs0p.fsf@gitster.c.googlers.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T12:10:16Z","receivedAt":"2020-12-19T12:11:15Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> > How about this?\n> >\n> > --- a/git-mergetool--lib.sh\n> > +++ b/git-mergetool--lib.sh\n> > @@ -46,7 +46,7 @@ show_tool_names () {\n> >                 while read scriptname\n> >                 do\n> >                         setup_tool \"$scriptname\" 2>/dev/null\n> > -                       variants=\"$variants$(list_tool_variants)\\n\"\n> > +                       variants=\"$variants$(list_tool_variants)\"$'\\n'\n> >                 done\n> >                 variants=\"$(echo \"$variants\" | sort | uniq)\"\n> \n> Ah, I didn't see your variant before sending mine.  $'\\n' would work\n> with bash but we prefer not to rely on bashisms here.\n\nYeah, but it's not even an RFC/PATCH. It's not proposed as a solution.\n\nIt's just to focus the eyes of the relevant parties on the likely issue.\n\n-- \nFelipe Contreras\n"},{"id":"412652","messageId":"xmqqsg81pvm8.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"5fddeda8a5a92_1de0de2088a@natae.notmuch","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T17:13:19Z","receivedAt":"2020-12-19T17:14:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>> > How about this?\n>> >\n>> > --- a/git-mergetool--lib.sh\n>> > +++ b/git-mergetool--lib.sh\n>> > @@ -46,7 +46,7 @@ show_tool_names () {\n>> >                 while read scriptname\n>> >                 do\n>> >                         setup_tool \"$scriptname\" 2>/dev/null\n>> > -                       variants=\"$variants$(list_tool_variants)\\n\"\n>> > +                       variants=\"$variants$(list_tool_variants)\"$'\\n'\n>> >                 done\n>> >                 variants=\"$(echo \"$variants\" | sort | uniq)\"\n>> \n>> Ah, I didn't see your variant before sending mine.  $'\\n' would work\n>> with bash but we prefer not to rely on bashisms here.\n>\n> Yeah, but it's not even an RFC/PATCH. It's not proposed as a solution.\n>\n> It's just to focus the eyes of the relevant parties on the likely issue.\n\nI think that served its purpose well, especially as the message went\nto the original contributor CC'ed.  I improved on it by mentioning\nthat we do not welcome $'\\n', in case the original contributor did\nnot know it and blindly took the suggestion.\n\nThanks.\n\n"},{"id":"412672","messageId":"c4f2330f-715b-1745-5ad7-5477f4e833db@gmail.com","threadId":"54851","inReplyTo":"xmqqsg81pvm8.fsf@gitster.c.googlers.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-20T02:13:21Z","receivedAt":"2020-12-20T02:14:39Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\nHi Felipe,\n\nLe 2020-12-19 à 12:13, Junio C Hamano a écrit :\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n>> Junio C Hamano wrote:\n>>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>>> How about this?\n>>>>\n>>>> --- a/git-mergetool--lib.sh\n>>>> +++ b/git-mergetool--lib.sh\n>>>> @@ -46,7 +46,7 @@ show_tool_names () {\n>>>>                  while read scriptname\n>>>>                  do\n>>>>                          setup_tool \"$scriptname\" 2>/dev/null\n>>>> -                       variants=\"$variants$(list_tool_variants)\\n\"\n>>>> +                       variants=\"$variants$(list_tool_variants)\"$'\\n'\n>>>>                  done\n>>>>                  variants=\"$(echo \"$variants\" | sort | uniq)\"\n>>>\n>>> Ah, I didn't see your variant before sending mine.  $'\\n' would work\n>>> with bash but we prefer not to rely on bashisms here.\n>>\n\nThanks for both answers. Felipe's solution does the trick, but Junio's does not;\nit seems we do have to have a newline there. The following also works, and I think\nis portable:\n\ndiff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh\nindex 2defef28cd..6f03975493 100644\n--- i/git-mergetool--lib.sh\n+++ w/git-mergetool--lib.sh\n@@ -46,7 +46,7 @@ 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\tvariants=\"$(echo \"$variants\" && list_tool_variants)\"\n  \t\tdone\n  \t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n\nI figured out what was different between the different Ubuntu versions I was testing:\nthe Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system\nhas /usr/bin/dash as /bin/sh (the default for Ubuntu these days).\n\nI'll try to send a formal patch with the diff above, time permitting...\n\nAlso of note: I was wondering why 'araxis' was showing up in the list of available tools,\nas Araxis Merge is macOS and Windows only (!). Turns out that the imagemagick package installs\na /usr/bin/compare symlink to /usr/bin/compare-im6.q16, so since Araxis merge is registered in\nmergetools/araxis::translate_merge_tool_path as 'compare', it ends up in the list of available\nmerge tools... I think we could do better here but I'm not sure how.\n\nCheers,\n\nPhilippe.\n"},{"id":"412679","messageId":"568f42a0-f630-64ca-9f77-183dcaa56d1e@kdbg.org","threadId":"54851","inReplyTo":"c4f2330f-715b-1745-5ad7-5477f4e833db@gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-12-20T10:28:32Z","receivedAt":"2020-12-20T10:29:34Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.12.20 um 03:13 schrieb Philippe Blain:\n> Thanks for both answers. Felipe's solution does the trick, but Junio's\n> does not;\n> it seems we do have to have a newline there. The following also works,\n> and I think\n> is portable:\n> \n> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh\n> index 2defef28cd..6f03975493 100644\n> --- i/git-mergetool--lib.sh\n> +++ w/git-mergetool--lib.sh\n> @@ -46,7 +46,7 @@ show_tool_names () {\n>          while read scriptname\n>          do\n>              setup_tool \"$scriptname\" 2>/dev/null\n> -            variants=\"$variants$(list_tool_variants)\\n\"\n> +            variants=\"$(echo \"$variants\" && list_tool_variants)\"\n>          done\n>          variants=\"$(echo \"$variants\" | sort | uniq)\"\n> \n> I figured out what was different between the different Ubuntu versions I\n> was testing:\n> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system\n> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).\n> \n> I'll try to send a formal patch with the diff above, time permitting...\n\nIf possible, please do not use sub-processes like in your suggested\npatch. How about\n\n\t\tvariants=\"$variants\n$(list_tool_variants)\"\n\nIt leaves a blank line at the beginning of $variants instead of the end\nand should not make a difference in the outcome of\n\n\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n\nBTW, is `sort -u` not available everywhere?\n\n-- Hannes\n"},{"id":"412680","messageId":"xmqq1rfko5j4.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"c4f2330f-715b-1745-5ad7-5477f4e833db@gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-20T15:34:23Z","receivedAt":"2020-12-20T15:35:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Thanks for both answers. Felipe's solution does the trick, but Junio's does not;\n> it seems we do have to have a newline there.\n\nCurious.  Are you sure you changed the second echo to printf and\nleft the argument after '%s\\n' unquoted, i.e. $variants and not\n\"$variants\"?\n\n> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh\n> index 2defef28cd..6f03975493 100644\n> --- i/git-mergetool--lib.sh\n> +++ w/git-mergetool--lib.sh\n> @@ -46,7 +46,7 @@ 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\tvariants=\"$(echo \"$variants\" && list_tool_variants)\"\n\nGee, echoing the variable double-quoted inside $()?  That's as ugly\nas a command substitution construct can go X-<.\n\n>  \t\tdone\n>  \t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n\n"},{"id":"412681","messageId":"a9e6f3aa-e88e-6cc2-fb16-c26bdd3bf4d3@web.de","threadId":"54851","inReplyTo":"568f42a0-f630-64ca-9f77-183dcaa56d1e@kdbg.org","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-12-20T16:23:26Z","receivedAt":"2020-12-20T16:25:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.12.20 um 11:28 schrieb Johannes Sixt:\n> Am 20.12.20 um 03:13 schrieb Philippe Blain:\n>> Thanks for both answers. Felipe's solution does the trick, but Junio's\n>> does not;\n>> it seems we do have to have a newline there. The following also works,\n>> and I think\n>> is portable:\n>>\n>> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh\n>> index 2defef28cd..6f03975493 100644\n>> --- i/git-mergetool--lib.sh\n>> +++ w/git-mergetool--lib.sh\n>> @@ -46,7 +46,7 @@ show_tool_names () {\n>>          while read scriptname\n>>          do\n>>              setup_tool \"$scriptname\" 2>/dev/null\n>> -            variants=\"$variants$(list_tool_variants)\\n\"\n>> +            variants=\"$(echo \"$variants\" && list_tool_variants)\"\n>>          done\n>>          variants=\"$(echo \"$variants\" | sort | uniq)\"\n>>\n>> I figured out what was different between the different Ubuntu versions I\n>> was testing:\n>> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system\n>> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).\n>>\n>> I'll try to send a formal patch with the diff above, time permitting...\n>\n> If possible, please do not use sub-processes like in your suggested\n> patch. How about\n>\n> \t\tvariants=\"$variants\n> $(list_tool_variants)\"\n\nThis still starts a subshell in the last line.  How about something\nlike this?\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7225abd811..79d5ed1fa9 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,11 +46,9 @@ 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\tdone\n-\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n-\n-\t\tfor toolname in $variants\n+\t\t\tlist_tool_variants\n+\t\tdone | sort | uniq |\n+\t\twhile read toolname\n \t\tdo\n \t\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n \t\t\t\t(eval \"$condition\" \"$toolname\")\n\nIt requires setup_tool to be silent, though.\n\n> It leaves a blank line at the beginning of $variants instead of the end\n> and should not make a difference in the outcome of\n>\n> \tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n>\n> BTW, is `sort -u` not available everywhere?\n\nIt's used by the function mergetool_find_win32_cmd in the same file\nand by several test scripts, so that shouldn't be a problem.\n\nRené\n"},{"id":"412682","messageId":"xmqqwnxcmjoy.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"a9e6f3aa-e88e-6cc2-fb16-c26bdd3bf4d3@web.de","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-20T18:11:25Z","receivedAt":"2020-12-20T18:12:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> This still starts a subshell in the last line.  How about something\n> like this?\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 7225abd811..79d5ed1fa9 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -46,11 +46,9 @@ 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\tdone\n> -\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n> -\n> -\t\tfor toolname in $variants\n> +\t\t\tlist_tool_variants\n> +\t\tdone | sort | uniq |\n> +\t\twhile read toolname\n>  \t\tdo\n>  \t\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n>  \t\t\t\t(eval \"$condition\" \"$toolname\")\n>\n> It requires setup_tool to be silent, though.\n\nAnother thing it depends on is that the side-effect of setup_tool in\nthe first loop does not matter in the end, as it now is done in the\nupstream of a pipe.  It is a safe assumption to make (setup_tool is\ncalled again in the later loop, so in the original it was called\ntwice), I think.\n\n>> BTW, is `sort -u` not available everywhere?\n>\n> It's used by the function mergetool_find_win32_cmd in the same file\n> and by several test scripts, so that shouldn't be a problem.\n\n\"sort -u\" is safe; it is even in POSIX.\n\nHaving said that, when finding out how portable a construct we\nalready use is across the platforms we support, my recommendation is\nto pretty much ignore what we do in a function or a file that we\nknow is only used by a single platform.  Stuff written for Windows,\nfor example, can assume that we use a particular implementation of\nsystem-supplied tools and libraries.  The use of \"sort -u\" in\nmergetool_find_win32_cmd can be legitimately justified with \"We know\nwe use only GNU sed\" and \"we taught -u to our busybox sed already\",\nfor example, if these statements are true.\n\nThanks.\n"},{"id":"412683","messageId":"xmqqpn34m1n0.fsf@gitster.c.googlers.com","threadId":"54851","inReplyTo":"2e7485b6-86d9-4de2-a823-dd84fc7386b0@gmail.com","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-21T00:41:23Z","receivedAt":"2020-12-21T00:42:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Thanks René and Johannes for two additional suggestions; both work correctly.\n> Junio, I retried yours and verified my quoting and I confirm it does not work.\n>\n> I think I prefer René's suggestion.\n>\n> Thanks all,\n\nI see what was wrong with my variant.\n\nWe force IFS to be LF and nothing else upfront, which defeats the\nattempt to split $variants at SP and turn them into \"one token per\nline\" with \"printf '%s\\n' $variants | ...\"\n\nIn any case, I find René's variant easier to follow, too.  \n\nThanks, all.\n"},{"id":"412700","messageId":"2e7485b6-86d9-4de2-a823-dd84fc7386b0@gmail.com","threadId":"54851","inReplyTo":"a9e6f3aa-e88e-6cc2-fb16-c26bdd3bf4d3@web.de","subject":"Re: [BUG] Regression in 'git mergetool --tool-help'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-20T23:47:29Z","receivedAt":"2020-12-21T05:48:02Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi everyone,\n\nLe 2020-12-20 à 11:23, René Scharfe a écrit :\n> Am 20.12.20 um 11:28 schrieb Johannes Sixt:\n>> Am 20.12.20 um 03:13 schrieb Philippe Blain:\n>>> Thanks for both answers. Felipe's solution does the trick, but Junio's\n>>> does not;\n>>> it seems we do have to have a newline there. The following also works,\n>>> and I think\n>>> is portable:\n>>>\n>>> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh\n>>> index 2defef28cd..6f03975493 100644\n>>> --- i/git-mergetool--lib.sh\n>>> +++ w/git-mergetool--lib.sh\n>>> @@ -46,7 +46,7 @@ show_tool_names () {\n>>>           while read scriptname\n>>>           do\n>>>               setup_tool \"$scriptname\" 2>/dev/null\n>>> -            variants=\"$variants$(list_tool_variants)\\n\"\n>>> +            variants=\"$(echo \"$variants\" && list_tool_variants)\"\n>>>           done\n>>>           variants=\"$(echo \"$variants\" | sort | uniq)\"\n>>>\n>>> I figured out what was different between the different Ubuntu versions I\n>>> was testing:\n>>> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system\n>>> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).\n>>>\n>>> I'll try to send a formal patch with the diff above, time permitting...\n>>\n>> If possible, please do not use sub-processes like in your suggested\n>> patch. How about\n>>\n>> \t\tvariants=\"$variants\n>> $(list_tool_variants)\"\n> \n> This still starts a subshell in the last line.  How about something\n> like this?\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 7225abd811..79d5ed1fa9 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -46,11 +46,9 @@ 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\tdone\n> -\t\tvariants=\"$(echo \"$variants\" | sort | uniq)\"\n> -\n> -\t\tfor toolname in $variants\n> +\t\t\tlist_tool_variants\n> +\t\tdone | sort | uniq |\n> +\t\twhile read toolname\n>   \t\tdo\n>   \t\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n>   \t\t\t\t(eval \"$condition\" \"$toolname\")\n> \n> It requires setup_tool to be silent, though.\n>\n\nThanks René and Johannes for two additional suggestions; both work correctly.\nJunio, I retried yours and verified my quoting and I confirm it does not work.\n\nI think I prefer René's suggestion.\n\nThanks all,\n\nPhilippe.\n"}]}