git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] mergetool--lib: fix '--tool-help' to correctly show available tools

From
Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>
Date
Dec 28, 2020, 18:22 UTC
Message-ID
<pull.825.git.1609179751864.gitgitgadget@gmail.com>
From: Philippe Blain <philippe.blain@canada.ca>

Commit 83bbf9b92e (mergetool--lib: improve support for vimdiff-style tool variants, 2020-07-29) introduced a regression in the output of `git mergetool --tool-help` and `git difftool --tool-help` [1].

In function 'show_tool_names' in git-mergetool--lib.sh, we loop over the supported mergetools and their variants and accumulate them in the variable 'variants', separating them with a literal '\n'.

The code then uses 'echo $variants' to turn these '\n' into newlines, but this behaviour is not portable, it just happens to work in some shells, like dash(1)'s 'echo' builtin.

For shells in which 'echo' does not turn '\n' into newlines, the end result is that the only tools that are shown are those that are found and have variants, since the variants are separated by actual newlines in '$variants' because of the several 'echo' calls in mergetools/{bc,vimdiff}::list_tool_variants.

Fix this bug by embedding an actual line feed into `variants` in show_tool_names(). While at it, replace `sort | uniq` by `sort -u`.

To prevent future regressions, add a simple test that counts the number of tools shown by 'git mergetool --tool-help', irrespective of their installed status, by making use of the fact that mergetools are listed by 'git mergetool --tool-help' on lines starting with tabs. Prefix the `git config` commands used at the beginning of the test to remove the fake tools used by the previous test with 'test_might_fail' so that the test can be run independantly of the previous test without failing.

[1] https://lore.kernel.org/git/CADtb9DyozjgAsdFYL8fFBEWmq7iz4=prZYVUdH9W-J5CKVS4OA@mail.gmail.com/
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Based-on-patch-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
    Fix regression in 'git {diff,merge}tool --tool-help'
    
    I went with Johannes' suggestion finally because upon further
    inspection, René's patch for some reason (I did not debug further)
    caused to code to never reach 'any_shown=yes' in show_tool_help,
    therefore changing the output of the command.
    
    I guess it's too late for inclusion in 2.30...
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-825%2Fphil-blain%2Fmergetool-tool-help-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-825/phil-blain/mergetool-tool-help-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/825
 git-mergetool--lib.sh |  6 ++++--
 t/t7610-mergetool.sh  | 10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd8112..78f3647ed97 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,9 +46,11 @@ show_tool_names () {
 		while read scriptname
 		do
 			setup_tool "$scriptname" 2>/dev/null
-			variants="$variants$(list_tool_variants)\n"
+			# We need an actual line feed here
+			variants="$variants
+$(list_tool_variants)"
 		done
-		variants="$(echo "$variants" | sort | uniq)"
+		variants="$(echo "$variants" | sort -u)"
 
 		for toolname in $variants
 		do
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 70afdd06fa7..ebd3af139e5 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -828,4 +828,14 @@ test_expect_success 'mergetool -Oorder-file is honored' '
 	test_cmp expect actual
 '
 
+test_expect_success 'mergetool --tool-help shows all recognized tools' '
+	# Remove fake tools added in previous tests
+	test_might_fail git config --unset merge.tool &&
+	test_might_fail git config --remove-section mergetool.mytool &&
+	test_might_fail git config --remove-section mergetool.mybase &&
+	git mergetool --tool-help >output &&
+	grep "$(printf "\t")" output >mergetools &&
+	test_line_count = 30 mergetools
+'
+
 test_done

base-commit: 4a0de43f4923993377dbbc42cfc0a1054b6c5ccf
-- 
gitgitgadget
Next: Philippe Blain via GitGitGadget
Message 1 of 10 in “mergetool--lib: fix '--tool-help' to correctly show available tools”
  1. mergetool--lib: fix '--tool-help' to correctly show available toolsPhilippe Blain via GitGitGadget, Dec 28, 2020
  2. mergetool--lib: fix '--tool-help' to correctly show available toolsPhilippe Blain via GitGitGadget, Dec 28, 2020
  3. SZEDER GáborJan 6, 2021
  4. Philippe BlainJan 6, 2021
  5. Junio C HamanoJan 6, 2021
  6. Philippe BlainJan 6, 2021
  7. Junio C HamanoJan 6, 2021
  8. Philippe BlainJan 7, 2021
  9. mergetool--lib: fix '--tool-help' to correctly show available toolsPhilippe Blain via GitGitGadget, Jan 7, 2021
  10. Philippe BlainJan 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.