threads / bug / 54851

[BUG] Regression in 'git mergetool --tool-help'

Subject: [BUG] Regression in 'git mergetool --tool-help'

## tl;dr

13 messages between Dec 19, 2020 and Dec 21, 2020.

replies: 12people: 5as markdown or json

Philippe Blain· Dec 19, 2020, 04:23 UTC · lore
Hi all,

It seems that 83bbf9b92e (mergetool--lib: improve support for vimdiff-style tool variants, 2020-07-29) introduced a regression in the output shown by 'git mergetool --tool-help'. Even if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH, they are not shown at all when the above command is invoked. Only vimdiff, gvimdiff and their *2 and *3 variants are listed.

The commit above was found by bisecting starting from 2.29.2 knowing 2.26.0 is OK. I can reproduce the bug on Ubuntu 18 and Ubuntu 14 but not on Ubuntu 20 (all LTS versions and all with Git 2.29.2) so maybe something else is also at play here...

Cheers,
Philippe.
Felipe Contreras· Dec 19, 2020, 04:50 UTC · re: Philippe Blain · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

On Fri, Dec 18, 2020 at 10:27 PM Philippe Blain <levraiphilippeblain@gmail.com> wrote:

Show 8 quoted lines
> It seems that 83bbf9b92e (mergetool--lib: improve support for
> vimdiff-style tool variants,
> 2020-07-29) introduced a regression in the output shown by 'git
> mergetool --tool-help'.
> Even if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH,
> they are not shown at all when
> the above command is invoked. Only vimdiff, gvimdiff and their *2 and
> *3 variants are listed.
How about this?
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,7 +46,7 @@ show_tool_names () {
                while read scriptname
                do
                        setup_tool "$scriptname" 2>/dev/null
-                       variants="$variants$(list_tool_variants)\n"
+                       variants="$variants$(list_tool_variants)"$'\n'
                done
                variants="$(echo "$variants" | sort | uniq)"

-- 
Felipe Contreras
Junio C Hamano· Dec 19, 2020, 05:33 UTC · re: Felipe Contreras · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 24 quoted lines
> On Fri, Dec 18, 2020 at 10:27 PM Philippe Blain
> <levraiphilippeblain@gmail.com> wrote:
>
>> It seems that 83bbf9b92e (mergetool--lib: improve support for
>> vimdiff-style tool variants,
>> 2020-07-29) introduced a regression in the output shown by 'git
>> mergetool --tool-help'.
>> Even if I have 'meld', 'kdiff3' and 'xxdiff' installed and in PATH,
>> they are not shown at all when
>> the above command is invoked. Only vimdiff, gvimdiff and their *2 and
>> *3 variants are listed.
>
> How about this?
>
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -46,7 +46,7 @@ show_tool_names () {
>                 while read scriptname
>                 do
>                         setup_tool "$scriptname" 2>/dev/null
> -                       variants="$variants$(list_tool_variants)\n"
> +                       variants="$variants$(list_tool_variants)"$'\n'
>                 done
>                 variants="$(echo "$variants" | sort | uniq)"

Ah, I didn't see your variant before sending mine. $'\n' would work with bash but we prefer not to rely on bashisms here.

Felipe Contreras· Dec 19, 2020, 12:10 UTC · re: Junio C Hamano · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Junio C Hamano wrote:
Show 16 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > How about this?
> >
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -46,7 +46,7 @@ show_tool_names () {
> >                 while read scriptname
> >                 do
> >                         setup_tool "$scriptname" 2>/dev/null
> > -                       variants="$variants$(list_tool_variants)\n"
> > +                       variants="$variants$(list_tool_variants)"$'\n'
> >                 done
> >                 variants="$(echo "$variants" | sort | uniq)"
> 
> Ah, I didn't see your variant before sending mine.  $'\n' would work
> with bash but we prefer not to rely on bashisms here.
Yeah, but it's not even an RFC/PATCH. It's not proposed as a solution.
It's just to focus the eyes of the relevant parties on the likely issue.
-- 
Felipe Contreras
Junio C Hamano· Dec 19, 2020, 17:13 UTC · re: Felipe Contreras · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 21 quoted lines
> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> > How about this?
>> >
>> > --- a/git-mergetool--lib.sh
>> > +++ b/git-mergetool--lib.sh
>> > @@ -46,7 +46,7 @@ show_tool_names () {
>> >                 while read scriptname
>> >                 do
>> >                         setup_tool "$scriptname" 2>/dev/null
>> > -                       variants="$variants$(list_tool_variants)\n"
>> > +                       variants="$variants$(list_tool_variants)"$'\n'
>> >                 done
>> >                 variants="$(echo "$variants" | sort | uniq)"
>> 
>> Ah, I didn't see your variant before sending mine.  $'\n' would work
>> with bash but we prefer not to rely on bashisms here.
>
> Yeah, but it's not even an RFC/PATCH. It's not proposed as a solution.
>
> It's just to focus the eyes of the relevant parties on the likely issue.

I think that served its purpose well, especially as the message went to the original contributor CC'ed. I improved on it by mentioning that we do not welcome $'\n', in case the original contributor did not know it and blindly took the suggestion.

Thanks.
Philippe Blain· Dec 20, 2020, 02:13 UTC · re: Junio C Hamano · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Hi Junio, Hi Felipe,

Le 2020-12-19 à 12:13, Junio C Hamano a écrit :
Show 20 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
>> Junio C Hamano wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>> How about this?
>>>>
>>>> --- a/git-mergetool--lib.sh
>>>> +++ b/git-mergetool--lib.sh
>>>> @@ -46,7 +46,7 @@ show_tool_names () {
>>>>                  while read scriptname
>>>>                  do
>>>>                          setup_tool "$scriptname" 2>/dev/null
>>>> -                       variants="$variants$(list_tool_variants)\n"
>>>> +                       variants="$variants$(list_tool_variants)"$'\n'
>>>>                  done
>>>>                  variants="$(echo "$variants" | sort | uniq)"
>>>
>>> Ah, I didn't see your variant before sending mine.  $'\n' would work
>>> with bash but we prefer not to rely on bashisms here.
>>

Thanks for both answers. Felipe's solution does the trick, but Junio's does not; it seems we do have to have a newline there. The following also works, and I think is portable:

diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh
index 2defef28cd..6f03975493 100644
--- i/git-mergetool--lib.sh
+++ w/git-mergetool--lib.sh
@@ -46,7 +46,7 @@ show_tool_names () {
  		while read scriptname
  		do
  			setup_tool "$scriptname" 2>/dev/null
-			variants="$variants$(list_tool_variants)\n"
+			variants="$(echo "$variants" && list_tool_variants)"
  		done
  		variants="$(echo "$variants" | sort | uniq)"

I figured out what was different between the different Ubuntu versions I was testing:
the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system
has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).

I'll try to send a formal patch with the diff above, time permitting...

Also of note: I was wondering why 'araxis' was showing up in the list of available tools,
as Araxis Merge is macOS and Windows only (!). Turns out that the imagemagick package installs
a /usr/bin/compare symlink to /usr/bin/compare-im6.q16, so since Araxis merge is registered in
mergetools/araxis::translate_merge_tool_path as 'compare', it ends up in the list of available
merge tools... I think we could do better here but I'm not sure how.

Cheers,

Philippe.
Johannes Sixt· Dec 20, 2020, 10:28 UTC · re: Philippe Blain · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Am 20.12.20 um 03:13 schrieb Philippe Blain:
Show 25 quoted lines
> Thanks for both answers. Felipe's solution does the trick, but Junio's
> does not;
> it seems we do have to have a newline there. The following also works,
> and I think
> is portable:
> 
> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh
> index 2defef28cd..6f03975493 100644
> --- i/git-mergetool--lib.sh
> +++ w/git-mergetool--lib.sh
> @@ -46,7 +46,7 @@ show_tool_names () {
>          while read scriptname
>          do
>              setup_tool "$scriptname" 2>/dev/null
> -            variants="$variants$(list_tool_variants)\n"
> +            variants="$(echo "$variants" && list_tool_variants)"
>          done
>          variants="$(echo "$variants" | sort | uniq)"
> 
> I figured out what was different between the different Ubuntu versions I
> was testing:
> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system
> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).
> 
> I'll try to send a formal patch with the diff above, time permitting...

If possible, please do not use sub-processes like in your suggested patch. How about

		variants="$variants
$(list_tool_variants)"

It leaves a blank line at the beginning of $variants instead of the end and should not make a difference in the outcome of

	variants="$(echo "$variants" | sort | uniq)"
BTW, is `sort -u` not available everywhere?
-- Hannes
René Scharfe· Dec 20, 2020, 16:23 UTC · re: Johannes Sixt · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Am 20.12.20 um 11:28 schrieb Johannes Sixt:
Show 32 quoted lines
> Am 20.12.20 um 03:13 schrieb Philippe Blain:
>> Thanks for both answers. Felipe's solution does the trick, but Junio's
>> does not;
>> it seems we do have to have a newline there. The following also works,
>> and I think
>> is portable:
>>
>> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh
>> index 2defef28cd..6f03975493 100644
>> --- i/git-mergetool--lib.sh
>> +++ w/git-mergetool--lib.sh
>> @@ -46,7 +46,7 @@ show_tool_names () {
>>          while read scriptname
>>          do
>>              setup_tool "$scriptname" 2>/dev/null
>> -            variants="$variants$(list_tool_variants)\n"
>> +            variants="$(echo "$variants" && list_tool_variants)"
>>          done
>>          variants="$(echo "$variants" | sort | uniq)"
>>
>> I figured out what was different between the different Ubuntu versions I
>> was testing:
>> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system
>> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).
>>
>> I'll try to send a formal patch with the diff above, time permitting...
>
> If possible, please do not use sub-processes like in your suggested
> patch. How about
>
> 		variants="$variants
> $(list_tool_variants)"

This still starts a subshell in the last line. How about something like this?

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd811..79d5ed1fa9 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,11 +46,9 @@ show_tool_names () {
 		while read scriptname
 		do
 			setup_tool "$scriptname" 2>/dev/null
-			variants="$variants$(list_tool_variants)\n"
-		done
-		variants="$(echo "$variants" | sort | uniq)"
-
-		for toolname in $variants
+			list_tool_variants
+		done | sort | uniq |
+		while read toolname
 		do
 			if setup_tool "$toolname" 2>/dev/null &&
 				(eval "$condition" "$toolname")

It requires setup_tool to be silent, though.

> It leaves a blank line at the beginning of $variants instead of the end
> and should not make a difference in the outcome of
>
> 	variants="$(echo "$variants" | sort | uniq)"
>
> BTW, is `sort -u` not available everywhere?

It's used by the function mergetool_find_win32_cmd in the same file
and by several test scripts, so that shouldn't be a problem.

René
Junio C Hamano· Dec 20, 2020, 18:11 UTC · re: René Scharfe · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

René Scharfe <l.s.r@web.de> writes:
Show 24 quoted lines
> This still starts a subshell in the last line.  How about something
> like this?
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 7225abd811..79d5ed1fa9 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -46,11 +46,9 @@ show_tool_names () {
>  		while read scriptname
>  		do
>  			setup_tool "$scriptname" 2>/dev/null
> -			variants="$variants$(list_tool_variants)\n"
> -		done
> -		variants="$(echo "$variants" | sort | uniq)"
> -
> -		for toolname in $variants
> +			list_tool_variants
> +		done | sort | uniq |
> +		while read toolname
>  		do
>  			if setup_tool "$toolname" 2>/dev/null &&
>  				(eval "$condition" "$toolname")
>
> It requires setup_tool to be silent, though.

Another thing it depends on is that the side-effect of setup_tool in the first loop does not matter in the end, as it now is done in the upstream of a pipe. It is a safe assumption to make (setup_tool is called again in the later loop, so in the original it was called twice), I think.

>> BTW, is `sort -u` not available everywhere?
>
> It's used by the function mergetool_find_win32_cmd in the same file
> and by several test scripts, so that shouldn't be a problem.
"sort -u" is safe; it is even in POSIX.

Having said that, when finding out how portable a construct we already use is across the platforms we support, my recommendation is to pretty much ignore what we do in a function or a file that we know is only used by a single platform. Stuff written for Windows, for example, can assume that we use a particular implementation of system-supplied tools and libraries. The use of "sort -u" in mergetool_find_win32_cmd can be legitimately justified with "We know we use only GNU sed" and "we taught -u to our busybox sed already", for example, if these statements are true.

Thanks.
Philippe Blain· Dec 20, 2020, 23:47 UTC · re: René Scharfe · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Hi everyone,
Le 2020-12-20 à 11:23, René Scharfe a écrit :
Show 59 quoted lines
> Am 20.12.20 um 11:28 schrieb Johannes Sixt:
>> Am 20.12.20 um 03:13 schrieb Philippe Blain:
>>> Thanks for both answers. Felipe's solution does the trick, but Junio's
>>> does not;
>>> it seems we do have to have a newline there. The following also works,
>>> and I think
>>> is portable:
>>>
>>> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh
>>> index 2defef28cd..6f03975493 100644
>>> --- i/git-mergetool--lib.sh
>>> +++ w/git-mergetool--lib.sh
>>> @@ -46,7 +46,7 @@ show_tool_names () {
>>>           while read scriptname
>>>           do
>>>               setup_tool "$scriptname" 2>/dev/null
>>> -            variants="$variants$(list_tool_variants)\n"
>>> +            variants="$(echo "$variants" && list_tool_variants)"
>>>           done
>>>           variants="$(echo "$variants" | sort | uniq)"
>>>
>>> I figured out what was different between the different Ubuntu versions I
>>> was testing:
>>> the Ubuntu 14 and 18 systems have Bash as /bin/sh, but my Ubuntu 20 system
>>> has /usr/bin/dash as /bin/sh (the default for Ubuntu these days).
>>>
>>> I'll try to send a formal patch with the diff above, time permitting...
>>
>> If possible, please do not use sub-processes like in your suggested
>> patch. How about
>>
>> 		variants="$variants
>> $(list_tool_variants)"
> 
> This still starts a subshell in the last line.  How about something
> like this?
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 7225abd811..79d5ed1fa9 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -46,11 +46,9 @@ show_tool_names () {
>   		while read scriptname
>   		do
>   			setup_tool "$scriptname" 2>/dev/null
> -			variants="$variants$(list_tool_variants)\n"
> -		done
> -		variants="$(echo "$variants" | sort | uniq)"
> -
> -		for toolname in $variants
> +			list_tool_variants
> +		done | sort | uniq |
> +		while read toolname
>   		do
>   			if setup_tool "$toolname" 2>/dev/null &&
>   				(eval "$condition" "$toolname")
> 
> It requires setup_tool to be silent, though.
>

Thanks René and Johannes for two additional suggestions; both work correctly. Junio, I retried yours and verified my quoting and I confirm it does not work.

I think I prefer René's suggestion.
Thanks all,
Philippe.
Junio C Hamano· Dec 21, 2020, 00:41 UTC · re: Philippe Blain · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Philippe Blain <levraiphilippeblain@gmail.com> writes:
Show 6 quoted lines
> Thanks René and Johannes for two additional suggestions; both work correctly.
> Junio, I retried yours and verified my quoting and I confirm it does not work.
>
> I think I prefer René's suggestion.
>
> Thanks all,
I see what was wrong with my variant.

We force IFS to be LF and nothing else upfront, which defeats the attempt to split $variants at SP and turn them into "one token per line" with "printf '%s\n' $variants | ..."

In any case, I find René's variant easier to follow, too.  
Thanks, all.
Junio C Hamano· Dec 20, 2020, 15:34 UTC · re: Philippe Blain · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

Philippe Blain <levraiphilippeblain@gmail.com> writes:
> Thanks for both answers. Felipe's solution does the trick, but Junio's does not;
> it seems we do have to have a newline there.

Curious. Are you sure you changed the second echo to printf and left the argument after '%s\n' unquoted, i.e. $variants and not "$variants"?

Show 10 quoted lines
> diff --git i/git-mergetool--lib.sh w/git-mergetool--lib.sh
> index 2defef28cd..6f03975493 100644
> --- i/git-mergetool--lib.sh
> +++ w/git-mergetool--lib.sh
> @@ -46,7 +46,7 @@ show_tool_names () {
>  		while read scriptname
>  		do
>  			setup_tool "$scriptname" 2>/dev/null
> -			variants="$variants$(list_tool_variants)\n"
> +			variants="$(echo "$variants" && list_tool_variants)"

Gee, echoing the variable double-quoted inside $()? That's as ugly as a command substitution construct can go X-<.

>  		done
>  		variants="$(echo "$variants" | sort | uniq)"
Junio C Hamano· Dec 19, 2020, 05:16 UTC · re: Philippe Blain · lore

Re: [BUG] Regression in 'git mergetool --tool-help'

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 204a5acd66..29fecc340f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -43,7 +43,14 @@ show_tool_names () {
 
 	shown_any=
 	( cd "$MERGE_TOOLS_DIR" && ls ) | {
-		while read toolname
+		while read scriptname
+		do
+			setup_tool "$scriptname" 2>/dev/null
+			variants="$variants$(list_tool_variants)\n"

Ahh, that does look quite bogus as \n won't do anything useful
there.  And I do not think we _need_ to use LF there.

+		done
+		variants="$(echo "$variants" | sort | uniq)"

+
+		for toolname in $variants

Just replace \n with " " (whitespace) and do something like this,
perhaps?

			variants="$variantes$(list_tool_variants) "
		done
		variants=$(printf "%s\n" $variants | sort | uniq)

← back to recent threads