git-mergetool: improve error code paths and messages

These are a few improvements to improve existing error messages in 'git mergetool', and make sure that errors are not quiet, along with a small completion update in 1/1.

Philippe Blain (5):
completion: complete '--tool-help' in 'git mergetool' git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool git-mergetool--lib.sh: add error message in 'setup_user_tool' git-mergetool--lib.sh: add error message for unknown tool variant git-difftool--helper.sh: exit upon initialize_merge_tool errors

 contrib/completion/git-completion.bash |  2 +-
 git-difftool--helper.sh                |  8 ++------
 git-mergetool--lib.sh                  | 13 +++++++++----
 t/t7610-mergetool.sh                   |  8 ++++++++
 4 files changed, 20 insertions(+), 11 deletions(-)

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1827%2Fphil-blain%2Fabsent-mergetool-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1827/phil-blain/absent-mergetool-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1827

gitgitgadget

From: Philippe Blain <[email protected]>

---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3d4dff3185c..b3b6aa3bae2 100644
@@ -2331,7 +2331,7 @@ _git_mergetool ()
 		return
 		;;
 	--*)
-		__gitcomp "--tool= --prompt --no-prompt --gui --no-gui"
+		__gitcomp "--tool= --tool-help --prompt --no-prompt --gui --no-gui"
 		return
 		;;
 	esac

gitgitgadget

From: Philippe Blain <[email protected]>

In git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen tool is valid via valid_tool and exit with an error message if not. This error message mentions "Unknown merge tool", even if the command the user tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE' variable for a more correct error message.

---
 git-mergetool--lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1ff26170ffc..269a60ea44c 100644
@@ -474,7 +474,7 @@ get_merge_tool_path () {
 	merge_tool="$1"
 	if ! valid_tool "$merge_tool"
 	then
-		echo >&2 "Unknown merge tool $merge_tool"
+		echo >&2 "Unknown $TOOL_MODE tool $merge_tool"
 		exit 1
 	fi
 	if diff_mode

gitgitgadget

From: Philippe Blain <[email protected]>

In git-mergetool--lib.sh::setup_tool, we check if the given tool is a known builtin tool, a known variant, or a user-defined tool by calling setup_user_tool, and we return with the exit code from setup_user_tool if it was called. setup_user_tool checks if {diff,merge}tool.$tool.cmd is set and quietly returns with an error if not.

This leads to the following invocation quietly failing:

	git mergetool --tool=unknown

which is not very user-friendly. Adjust setup_user_tool to output an error message before returning if {diff,merge}tool.$tool.cmd is not set.

Adjust the second call to setup_user_tool in setup_tool to silence this new error, as this call is only meant to allow users to redefine 'cmd' for a builtin tool; it is not an error if they have not done so (which is why we do not check the return status of this call).

Note that this behaviour of quietly failing is a regression dating back to de8dafbada (mergetool: break setup_tool out into separate initialization function, 2021-02-09), as before this commit an unknown mergetool would be diagnosed in get_merge_tool_path when called from run_merge_tool.

---
 git-mergetool--lib.sh | 10 +++++++---
 t/t7610-mergetool.sh  |  8 ++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

index 269a60ea44c..f4786afc63f 100644
@@ -159,14 +159,18 @@ check_unchanged () {
 }
 
 valid_tool () {
-	setup_tool "$1" && return 0
+	setup_tool "$1" 2>/dev/null && return 0
 	cmd=$(get_merge_tool_cmd "$1")
 	test -n "$cmd"
 }
 
 setup_user_tool () {
 	merge_tool_cmd=$(get_merge_tool_cmd "$tool")
-	test -n "$merge_tool_cmd" || return 1
+	if test -z "$merge_tool_cmd"
+	then
+		echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'"
+		return 1
+	fi
 
 	diff_cmd () {
 		( eval $merge_tool_cmd )

@@ -255,7 +259,7 @@ setup_tool () {
 
 	# Now let the user override the default command for the tool.  If
 	# they have not done so then this will return 1 which we ignore.
-	setup_user_tool
+	setup_user_tool 2>/dev/null
 
 	if ! list_tool_variants | grep -q "^$tool$"
 	then

index 22b3a85b3e9..82a88107850 100755
@@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'mergetool with non-existent tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	test_must_fail git merge main &&
+	yes "" | test_must_fail git mergetool --tool=absent >out 2>&1 &&
+	test_grep -i "not set for tool" out
+'
+
 test_done

gitgitgadget

From: Philippe Blain <[email protected]>

In setup_tool, we check if the given tool is a known variant of a tool, and quietly return with an error if not. This leads to the following invocation quietly failing:

	git mergetool --tool=vimdiff4

Add an error message before returning in this case.

---
 git-mergetool--lib.sh | 1 +
 1 file changed, 1 insertion(+)

index f4786afc63f..9a00fabba27 100644
@@ -263,6 +263,7 @@ setup_tool () {
 
 	if ! list_tool_variants | grep -q "^$tool$"
 	then
+		echo "error: unknown ${tool%[0-9]} variant '$tool'" >&2
 		return 1
 	fi
 

gitgitgadget

From: Philippe Blain <[email protected]>

Since the introduction of 'initialize_merge_tool' in de8dafbada (mergetool: break setup_tool out into separate initialization function, 2021-02-09), any errors from this function are ignored in git-difftool--helper.sh::launch_merge_tool, which is not the case for its call in git-mergetool.sh::merge_file.

Despite the in-code comment, initialize_merge_tool (via its call to setup_tool) does different checks than run_merge_tool, so it makes sense to abort early if it encounters errors. Add exit calls if initialize_merge_tool fails.

---
 git-difftool--helper.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

index dd0c9a5b7f2..d32e47cc09e 100755
@@ -61,9 +61,7 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
-		initialize_merge_tool "$merge_tool"
-		# ignore the error from the above --- run_merge_tool
-		# will diagnose unusable tool by itself
+		initialize_merge_tool "$merge_tool" || exit 1
 		run_merge_tool "$merge_tool"
 	fi
 }

@@ -87,9 +85,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
-	initialize_merge_tool "$merge_tool"
-	# ignore the error from the above --- run_merge_tool
-	# will diagnose unusable tool by itself
+	initialize_merge_tool "$merge_tool" || exit 1
 	run_merge_tool "$merge_tool" false
 
 	status=$?

gitgitgadget

Makes sense. Is this something we can easily test to catch future regression, or is it too trivial to matter?

I wouldn't mind if the answer were "the latter" ;-)

Thanks.

Makes sense, but ...

    git mergetool --tool=nvimdiff4

    git mergetool --tool=nvimdiff48

Hi Junio (sorry for a late response),

Le 2024-11-12 à 20:27, Junio C Hamano a écrit :

With the changes in the next commit of the series, this particular error becomes hard to trigger, as setup_user_tool will return with an error before the error message change in this patch is reached. So I would way it is not worth to add a test for this particular code path since it seems like it becomes unreachable in the next commit (but I could be wrong). So mostly "the latter" is my answer.

Thanks,
Philippe.

Hi Junio,

Le 2024-11-12 à 20:48, Junio C Hamano a écrit :

I don't think we are planning to add more callers, no.

I agree it could be done this way, I can change if it we wish.

Indeed this is a leftover from my bisection test in which I had to be a bit more loose. I'll remove that flag.

Yes, I guess I could make the pattern stricter. I'll update that.

Le 2024-11-12 à 21:01, Junio C Hamano a écrit :

the stripping of the last digit is just because I copied from
the 'if' a few lines above, where we source "$MERGE_TOOLS_DIR/${tool%[0-9]}". In MERGE_TOOLS_DIR we have 'nvimdiff' and 'gvimdiff' that simply source vimdiff, so this works. But I agree that we can simplify the error message, I'll do that.

Le 2024-11-22 à 13:57, Philippe Blain a écrit :

correction: with the changes in 3/5 and 5/5,

Changes in v2: As suggested by Junio:

* 3/5: moved the error message to setup_tool itself, and adjusted the commit message
* 3/5: made the test more robust
* 4/5: adjusted the error message

v1: These are a few improvements to improve existing error messages in 'git mergetool', and make sure that errors are not quiet, along with a small completion update in 1/1.

Philippe Blain (5):
completion: complete '--tool-help' in 'git mergetool' git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool git-mergetool--lib.sh: add error message if 'setup_user_tool' fails git-mergetool--lib.sh: add error message for unknown tool variant git-difftool--helper.sh: exit upon initialize_merge_tool errors

 contrib/completion/git-completion.bash |  2 +-
 git-difftool--helper.sh                |  8 ++------
 git-mergetool--lib.sh                  | 12 +++++++++---
 t/t7610-mergetool.sh                   |  8 ++++++++
 4 files changed, 20 insertions(+), 10 deletions(-)

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1827%2Fphil-blain%2Fabsent-mergetool-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1827/phil-blain/absent-mergetool-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1827

Range-diff vs v1:

 1:  24933ba7130 = 1:  24933ba7130 completion: complete '--tool-help' in 'git mergetool'
 2:  6f7f553b283 = 2:  6f7f553b283 git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool
 3:  79c3a6ffe8f ! 3:  1d9e95c6cb1 git-mergetool--lib.sh: add error message in 'setup_user_tool'

@@ Metadata
Author: Philippe Blain <[email protected]>

## Commit message ##
- git-mergetool--lib.sh: add error message in 'setup_user_tool'
+ git-mergetool--lib.sh: add error message if 'setup_user_tool' fails

In git-mergetool--lib.sh::setup_tool, we check if the given tool is a known builtin tool, a known variant, or a user-defined tool by calling
@@ Commit message

git mergetool --tool=unknown

- which is not very user-friendly. Adjust setup_user_tool to output an
- error message before returning if {diff,merge}tool.$tool.cmd is not set.
+ which is not very user-friendly. Adjust setup_tool to output an error
+ message before returning if setup_user_tool returned with an error.

- Adjust the second call to setup_user_tool in setup_tool to silence this
- new error, as this call is only meant to allow users to redefine 'cmd'
- for a builtin tool; it is not an error if they have not done so (which
- is why we do not check the return status of this call).
+ Note that we do not check the result of the second call to
+ setup_user_tool in setup_tool, as this call is only meant to allow users
+ to redefine 'cmd' for a builtin tool; it is not an error if they have
+ not done so.

Note that this behaviour of quietly failing is a regression dating back to de8dafbada (mergetool: break setup_tool out into separate
@@ git-mergetool--lib.sh: check_unchanged () {
cmd=$(get_merge_tool_cmd "$1")
test -n "$cmd"
}
-
- setup_user_tool () {
- merge_tool_cmd=$(get_merge_tool_cmd "$tool")
-- test -n "$merge_tool_cmd" || return 1
-+ if test -z "$merge_tool_cmd"
-+ then
-+ echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'"
-+ return 1
-+ fi
-
- diff_cmd () {
- ( eval $merge_tool_cmd )
@@ git-mergetool--lib.sh: setup_tool () {
+ . "$MERGE_TOOLS_DIR/${tool%[0-9]}"
+ else
+ setup_user_tool
+- return $?
++ rc=$?
++ if test $rc -ne 0
++ then
++ echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'"
++ fi
++ return $rc
+ fi

# Now let the user override the default command for the tool. If
- # they have not done so then this will return 1 which we ignore.
-- setup_user_tool
-+ setup_user_tool 2>/dev/null
-
- if ! list_tool_variants | grep -q "^$tool$"
- then

## t/t7610-mergetool.sh ##
@@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' '
@@ t/t7610-mergetool.sh: test_expect_success 'mergetool with guiDefault' '
+ git checkout -b test$test_count branch1 &&
+ test_must_fail git merge main &&
+ yes "" | test_must_fail git mergetool --tool=absent >out 2>&1 &&
-+ test_grep -i "not set for tool" out
++ test_grep "mergetool.absent.cmd not set for tool" out
+'
+
test_done
4: 74b83caa1e5 ! 4: f234e965543 git-mergetool--lib.sh: add error message for unknown tool variant

     @@ git-mergetool--lib.sh: setup_tool () {
       
       	if ! list_tool_variants | grep -q "^$tool$"
       	then
     -+		echo "error: unknown ${tool%[0-9]} variant '$tool'" >&2
     ++		echo "error: unknown tool variant '$tool'" >&2
       		return 1
       	fi

5: be0b86f0890 = 5: c16e9229ebb git-difftool--helper.sh: exit upon initialize_merge_tool errors

gitgitgadget

From: Philippe Blain <[email protected]>

---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3d4dff3185c..b3b6aa3bae2 100644
@@ -2331,7 +2331,7 @@ _git_mergetool ()
 		return
 		;;
 	--*)
-		__gitcomp "--tool= --prompt --no-prompt --gui --no-gui"
+		__gitcomp "--tool= --tool-help --prompt --no-prompt --gui --no-gui"
 		return
 		;;
 	esac

gitgitgadget

From: Philippe Blain <[email protected]>

In git-mergetool--lib.sh::get_merge_tool_path, we check if the chosen tool is valid via valid_tool and exit with an error message if not. This error message mentions "Unknown merge tool", even if the command the user tried was 'git difftool --tool=unknown'. Use the global 'TOOL_MODE' variable for a more correct error message.

---
 git-mergetool--lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1ff26170ffc..269a60ea44c 100644
@@ -474,7 +474,7 @@ get_merge_tool_path () {
 	merge_tool="$1"
 	if ! valid_tool "$merge_tool"
 	then
-		echo >&2 "Unknown merge tool $merge_tool"
+		echo >&2 "Unknown $TOOL_MODE tool $merge_tool"
 		exit 1
 	fi
 	if diff_mode

gitgitgadget

From: Philippe Blain <[email protected]>

In git-mergetool--lib.sh::setup_tool, we check if the given tool is a known builtin tool, a known variant, or a user-defined tool by calling setup_user_tool, and we return with the exit code from setup_user_tool if it was called. setup_user_tool checks if {diff,merge}tool.$tool.cmd is set and quietly returns with an error if not.

This leads to the following invocation quietly failing:

	git mergetool --tool=unknown

which is not very user-friendly. Adjust setup_tool to output an error message before returning if setup_user_tool returned with an error.

Note that we do not check the result of the second call to setup_user_tool in setup_tool, as this call is only meant to allow users to redefine 'cmd' for a builtin tool; it is not an error if they have not done so.

Note that this behaviour of quietly failing is a regression dating back to de8dafbada (mergetool: break setup_tool out into separate initialization function, 2021-02-09), as before this commit an unknown mergetool would be diagnosed in get_merge_tool_path when called from run_merge_tool.

---
 git-mergetool--lib.sh | 9 +++++++--
 t/t7610-mergetool.sh  | 8 ++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

index 269a60ea44c..d7e410d9481 100644
@@ -159,7 +159,7 @@ check_unchanged () {
 }
 
 valid_tool () {
-	setup_tool "$1" && return 0
+	setup_tool "$1" 2>/dev/null && return 0
 	cmd=$(get_merge_tool_cmd "$1")
 	test -n "$cmd"
 }

@@ -250,7 +250,12 @@ setup_tool () {
 		. "$MERGE_TOOLS_DIR/${tool%[0-9]}"
 	else
 		setup_user_tool
-		return $?
+		rc=$?
+		if test $rc -ne 0
+		then
+			echo >&2 "error: ${TOOL_MODE}tool.$tool.cmd not set for tool '$tool'"
+		fi
+		return $rc
 	fi
 
 	# Now let the user override the default command for the tool.  If

index 22b3a85b3e9..c077aba7ced 100755
@@ -898,4 +898,12 @@ test_expect_success 'mergetool with guiDefault' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'mergetool with non-existent tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	test_must_fail git merge main &&
+	yes "" | test_must_fail git mergetool --tool=absent >out 2>&1 &&
+	test_grep "mergetool.absent.cmd not set for tool" out
+'
+
 test_done

gitgitgadget

From: Philippe Blain <[email protected]>

In setup_tool, we check if the given tool is a known variant of a tool, and quietly return with an error if not. This leads to the following invocation quietly failing:

	git mergetool --tool=vimdiff4

Add an error message before returning in this case.

---
 git-mergetool--lib.sh | 1 +
 1 file changed, 1 insertion(+)

index d7e410d9481..11ea181259f 100644
@@ -264,6 +264,7 @@ setup_tool () {
 
 	if ! list_tool_variants | grep -q "^$tool$"
 	then
+		echo "error: unknown tool variant '$tool'" >&2
 		return 1
 	fi
 

gitgitgadget

From: Philippe Blain <[email protected]>

Since the introduction of 'initialize_merge_tool' in de8dafbada (mergetool: break setup_tool out into separate initialization function, 2021-02-09), any errors from this function are ignored in git-difftool--helper.sh::launch_merge_tool, which is not the case for its call in git-mergetool.sh::merge_file.

Despite the in-code comment, initialize_merge_tool (via its call to setup_tool) does different checks than run_merge_tool, so it makes sense to abort early if it encounters errors. Add exit calls if initialize_merge_tool fails.

---
 git-difftool--helper.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

index dd0c9a5b7f2..d32e47cc09e 100755
@@ -61,9 +61,7 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
-		initialize_merge_tool "$merge_tool"
-		# ignore the error from the above --- run_merge_tool
-		# will diagnose unusable tool by itself
+		initialize_merge_tool "$merge_tool" || exit 1
 		run_merge_tool "$merge_tool"
 	fi
 }

@@ -87,9 +85,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
-	initialize_merge_tool "$merge_tool"
-	# ignore the error from the above --- run_merge_tool
-	# will diagnose unusable tool by itself
+	initialize_merge_tool "$merge_tool" || exit 1
 	run_merge_tool "$merge_tool" false
 
 	status=$?

gitgitgadget

I think the above changes all looked good.

Let me mark the topic for 'next'.

Thanks.