git-mergetool: improve error code paths and messages #1827

gitgitgadget · 2024-11-13T01:52:45Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > valid_tool () { > - setup_tool "$1" && return 0 > + setup_tool "$1" 2>/dev/null && return 0 > cmd=$(get_merge_tool_cmd "$1") > test -n "$cmd" > } As we are checking if a tool is valid, it is normal for setup_tool to fail when we are checking is not valid (aka "fails to get set up"). There is no need to show an error message for such a failure, as the callers of valid_tool would do so if they wish. OK. > 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 There are only two callers of setup_user_tool, and one of them squelches this message by sending it to /dev/null. The error message composed here does not use anything that is unique to the function (in other words, $tool and ${TOOL_MODE} are available to its callers). I wonder if it is a better design to leave this one as-is, and instead show the error message from the other caller of setup_user_tool that does not squelch the message? Are we planning to add more callers of this function that want to show the same message? > 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 we did that, then this change can be dropped. Instead, a few lines above this hunk, we can give the error message ourselves from this setup_tool function. > if ! list_tool_variants | grep -q "^$tool$" > then > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh > index 22b3a85b3e9..82a88107850 100755 > --- a/t/t7610-mergetool.sh > +++ b/t/t7610-mergetool.sh > @@ -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 > +' Why "-i"? I do not offhand see the reason why we want to be loose here. The "${TOOL_MODE}tool" part may also want to be verified, perhaps, which was related to the topic of the fix in [2/5]?

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Junio, Le 2024-11-12 à 20:48, Junio C Hamano a écrit : > "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> 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 > > There are only two callers of setup_user_tool, and one of them > squelches this message by sending it to /dev/null. The error > message composed here does not use anything that is unique to the > function (in other words, $tool and ${TOOL_MODE} are available to > its callers). > > I wonder if it is a better design to leave this one as-is, and > instead show the error message from the other caller of > setup_user_tool that does not squelch the message? Are we planning > to add more callers of this function that want to show the same > message? I don't think we are planning to add more callers, no. > >> 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 we did that, then this change can be dropped. Instead, a few > lines above this hunk, we can give the error message ourselves from > this setup_tool function. I agree it could be done this way, I can change if it we wish. >> if ! list_tool_variants | grep -q "^$tool$" >> then >> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh >> index 22b3a85b3e9..82a88107850 100755 >> --- a/t/t7610-mergetool.sh >> +++ b/t/t7610-mergetool.sh >> @@ -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 >> +' > > Why "-i"? I do not offhand see the reason why we want to be loose > here. Indeed this is a leftover from my bisection test in which I had to be a bit more loose. I'll remove that flag. > The "${TOOL_MODE}tool" part may also want to be verified, perhaps, > which was related to the topic of the fix in [2/5]? Yes, I guess I could make the pattern stricter. I'll update that.

gitgitgadget · 2024-11-13T02:04:52Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Philippe Blain <levraiphilippeblain@gmail.com> > > 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. Makes sense, but ... > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > --- > git-mergetool--lib.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index f4786afc63f..9a00fabba27 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -263,6 +263,7 @@ setup_tool () { > > if ! list_tool_variants | grep -q "^$tool$" > then > + echo "error: unknown ${tool%[0-9]} variant '$tool'" >&2 ... I do not understand why you strip a single digit from the end. git mergetool --tool=nvimdiff4 says 'nvimdiff4' is not known as a variant of 'nvimdiff', but wouldn't it still be a variant of 'vimdiff'? Of course, git mergetool --tool=nvimdiff48 gets a vastly different error message ;-) Saying echo >&2 "error: unknown variant '$tool'" may be sufficient, perhaps? I dunno. > return 1 > fi

On the Git mailing list, Philippe Blain wrote (reply to this):

Le 2024-11-12 à 21:01, Junio C Hamano a écrit : > "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Philippe Blain <levraiphilippeblain@gmail.com> >> >> 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. > > Makes sense, but ... > >> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> >> --- >> git-mergetool--lib.sh | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh >> index f4786afc63f..9a00fabba27 100644 >> --- a/git-mergetool--lib.sh >> +++ b/git-mergetool--lib.sh >> @@ -263,6 +263,7 @@ setup_tool () { >> >> if ! list_tool_variants | grep -q "^$tool$" >> then >> + echo "error: unknown ${tool%[0-9]} variant '$tool'" >&2 > > ... I do not understand why you strip a single digit from the end. > > git mergetool --tool=nvimdiff4 > > says 'nvimdiff4' is not known as a variant of 'nvimdiff', but > wouldn't it still be a variant of 'vimdiff'? Of course, > > git mergetool --tool=nvimdiff48 > > gets a vastly different error message ;-) > > Saying > > echo >&2 "error: unknown variant '$tool'" > > may be sufficient, perhaps? I dunno. 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.

gitgitgadget · 2024-11-13T01:31:46Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Philippe Blain <levraiphilippeblain@gmail.com> > > 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. 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. > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > --- > git-mergetool--lib.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 1ff26170ffc..269a60ea44c 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -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

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Junio (sorry for a late response), Le 2024-11-12 à 20:27, Junio C Hamano a écrit : > "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Philippe Blain <levraiphilippeblain@gmail.com> >> >> 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. > > 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" ;-) 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.

On the Git mailing list, Philippe Blain wrote (reply to this):

Le 2024-11-22 à 13:57, Philippe Blain a écrit : > Hi Junio (sorry for a late response), > > Le 2024-11-12 à 20:27, Junio C Hamano a écrit : >> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Philippe Blain <levraiphilippeblain@gmail.com> >>> >>> 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. >> >> 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" ;-) > > With the changes in the next commit of the series, correction: with the changes in 3/5 and 5/5, > 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.

-Original file line number
+Diff line change
@@ Expand Up / @@ -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
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -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
     }
@@ Expand All / @@ -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=$?
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git-mergetool: improve error code paths and messages #1827

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Nov 13, 2024

gitgitgadget bot Nov 22, 2024

gitgitgadget bot Nov 13, 2024

gitgitgadget bot Nov 22, 2024

gitgitgadget bot Nov 13, 2024

gitgitgadget bot Nov 22, 2024

gitgitgadget bot Nov 22, 2024

-Original file line number
+Diff line change
@@ Expand Up / @@ -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"
     }
@@ Expand Down Expand Up / @@ -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
@@ Expand All / @@ -259,6 +264,7 @@ setup_tool () { @@
     	if ! list_tool_variants | grep -q "^$tool$"
     	then
+    		echo "error: unknown tool variant '$tool'" >&2
     		return 1
     	fi
@@ Expand Down Expand Up / @@ -474,7 +480,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
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -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

git-mergetool: improve error code paths and messages #1827

git-mergetool: improve error code paths and messages #1827

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Nov 13, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 22, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 13, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 22, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 13, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 22, 2024

Choose a reason for hiding this comment

gitgitgadget bot Nov 22, 2024

Choose a reason for hiding this comment