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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2020, 18:11 UTC
Message-ID: <xmqqwnxcmjoy.fsf@gitster.c.googlers.com>
In-Reply-To: <a9e6f3aa-e88e-6cc2-fb16-c26bdd3bf4d3@web.de>

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.