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

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

[Philippe Blain]

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.

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

[Felipe Contreras]

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


-- 
Felipe Contreras

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

[Junio C Hamano]

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)

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

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

[Felipe Contreras]

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.

-- 
Felipe Contreras

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

[Junio C Hamano]

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

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

[Philippe Blain]

Hi Junio,
Hi Felipe,

Le 2020-12-19 à 12:13, Junio C Hamano a écrit :
> 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.

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

[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)"

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

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

[Junio C Hamano]

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"?

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

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

[René Scharfe]

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.

> 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é

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

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

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

[Philippe Blain]

Hi everyone,

Le 2020-12-20 à 11:23, René Scharfe a écrit :
> 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.

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

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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