Re: [BUG] Regression in 'git mergetool --tool-help'
From: Junio C Hamano <hidden>
Date: 2020-12-20 18:12:28
René Scharfe [off-list ref] writes:
quoted hunk
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.
quoted
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.