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

phil-blain · 2024-11-11T21:18:32Z

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.

CC: Seth House [email protected]
CC: David Aguilar [email protected]
CC: Johannes Sixt [email protected]

Signed-off-by: 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. Signed-off-by: Philippe Blain <[email protected]>

phil-blain · 2024-11-12T23:17:27Z

/submit

phil-blain · 2024-11-12T23:19:28Z

/submit

phil-blain · 2024-11-13T00:49:57Z

/submit

gitgitgadget · 2024-11-13T00:52:25Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1827/phil-blain/absent-mergetool-v1

To fetch this version to local tag pr-1827/phil-blain/absent-mergetool-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1827/phil-blain/absent-mergetool-v1

phil-blain · 2024-11-13T01:17:45Z

Hi @dscho !

My first two tries (one, two) at /submit failed because (if I understand correctly) the node.js v18.20.5 released today was not yet up at https://nodejs.org/dist/v18.20.5/, although the folder existed already. I guess this is just bad luck and there is nothing much Gitgitgadget can do ? And changes would need to go into the NodeTool Azure task to make it more robust ?

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

git-mergetool--lib.sh

@@ -474,7 +474,7 @@ get_merge_tool_path () {
 	merge_tool="$1"


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

"Philippe Blain via GitGitGadget" <[email protected]> writes: > 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. 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 <[email protected]> > --- > 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" <[email protected]> writes: > >> 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. > > 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" <[email protected]> writes: >> >>> 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. >> >> 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.

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

git-mergetool--lib.sh

@@ -159,14 +159,18 @@ check_unchanged () {
 }


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

"Philippe Blain via GitGitGadget" <[email protected]> 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" <[email protected]> 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

git-mergetool--lib.sh

@@ -255,10 +259,11 @@ 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



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

"Philippe Blain via GitGitGadget" <[email protected]> writes: > 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. Makes sense, but ... > Signed-off-by: Philippe Blain <[email protected]> > --- > 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" <[email protected]> writes: > >> 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. > > Makes sense, but ... > >> Signed-off-by: Philippe Blain <[email protected]> >> --- >> 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-13T04:32:00Z

This patch series was integrated into seen via git@a35aba8.

dscho · 2024-11-13T08:15:10Z

My first two tries (one, two) at /submit failed because (if I understand correctly) the node.js v18.20.5 released today was not yet up at https://nodejs.org/dist/v18.20.5/, although the folder existed already. I guess this is just bad luck and there is nothing much Gitgitgadget can do ? And changes would need to go into the NodeTool Azure task to make it more robust ?

@phil-blain sorry for the woes! Your analysis is indeed correct.

I now changed the Pipeline definition to use v1 of that task (instead of v0), and I checked off the "Check for Latest Version" checkbox. That should be good enough.

gitgitgadget · 2024-11-15T04:49:37Z

This patch series was integrated into seen via git@37181c6.

gitgitgadget · 2024-11-16T08:10:56Z

This patch series was integrated into seen via git@5006b1c.

gitgitgadget · 2024-11-18T23:31:02Z

This patch series was integrated into seen via git@6d9c2d4.

gitgitgadget · 2024-11-19T07:47:18Z

This patch series was integrated into seen via git@011d5cd.

gitgitgadget · 2024-11-20T06:47:59Z

This patch series was integrated into seen via git@ad8e306.

gitgitgadget · 2024-11-21T06:31:26Z

This patch series was integrated into seen via git@208a0e2.

gitgitgadget · 2024-11-22T02:42:17Z

This patch series was integrated into seen via git@67867a8.

gitgitgadget · 2024-11-22T08:13:44Z

This patch series was integrated into seen via git@4ff66cb.

gitgitgadget · 2024-11-22T19:02:09Z

User Philippe Blain <[email protected]> has been added to the cc: list.

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 de8dafb (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. Signed-off-by: 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. Signed-off-by: Philippe Blain <[email protected]>

Since the introduction of 'initialize_merge_tool' in de8dafb (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. Signed-off-by: Philippe Blain <[email protected]>

phil-blain · 2024-11-22T19:49:37Z

/submit

gitgitgadget · 2024-11-22T19:50:38Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1827/phil-blain/absent-mergetool-v2

To fetch this version to local tag pr-1827/phil-blain/absent-mergetool-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1827/phil-blain/absent-mergetool-v2

phil-blain added 2 commits November 11, 2024 16:10

completion: complete '--tool-help' in 'git mergetool'

24933ba

Signed-off-by: Philippe Blain <[email protected]>

gitgitgadget bot reviewed Nov 13, 2024

View reviewed changes

gitgitgadget bot added the seen label Nov 13, 2024

phil-blain added 3 commits November 22, 2024 14:25

phil-blain force-pushed the absent-mergetool branch from be0b86f to c16e922 Compare November 22, 2024 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phil-blain commented Nov 11, 2024 •

edited

Loading

phil-blain commented Nov 12, 2024

phil-blain commented Nov 12, 2024

phil-blain commented Nov 13, 2024

gitgitgadget bot commented Nov 13, 2024

phil-blain commented Nov 13, 2024

gitgitgadget bot Nov 13, 2024

gitgitgadget bot Nov 22, 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 commented Nov 13, 2024

dscho commented Nov 13, 2024

gitgitgadget bot commented Nov 15, 2024

gitgitgadget bot commented Nov 16, 2024

gitgitgadget bot commented Nov 18, 2024

gitgitgadget bot commented Nov 19, 2024

gitgitgadget bot commented Nov 20, 2024

gitgitgadget bot commented Nov 21, 2024

gitgitgadget bot commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

phil-blain commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

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

Are you sure you want to change the base?

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

Conversation

phil-blain commented Nov 11, 2024 • edited Loading

phil-blain commented Nov 12, 2024

phil-blain commented Nov 12, 2024

phil-blain commented Nov 13, 2024

gitgitgadget bot commented Nov 13, 2024

phil-blain commented Nov 13, 2024

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

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 commented Nov 13, 2024

dscho commented Nov 13, 2024

gitgitgadget bot commented Nov 15, 2024

gitgitgadget bot commented Nov 16, 2024

gitgitgadget bot commented Nov 18, 2024

gitgitgadget bot commented Nov 19, 2024

gitgitgadget bot commented Nov 20, 2024

gitgitgadget bot commented Nov 21, 2024

gitgitgadget bot commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

phil-blain commented Nov 22, 2024

gitgitgadget bot commented Nov 22, 2024

phil-blain commented Nov 11, 2024 •

edited

Loading