git-mergetool: improve error code paths and messages #1827
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,7 +159,7 @@ check_unchanged () { | |
| } | ||
|
|
||
| valid_tool () { | ||
| 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 | ||
| 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 | ||
|
|
@@ -259,6 +264,7 @@ setup_tool () { | |
|
|
||
|
There was a problem hiding this comment. 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
> fiThere was a problem hiding this comment. 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. |
||
| if ! list_tool_variants | grep -q "^$tool$" | ||
| then | ||
| echo "error: unknown tool variant '$tool'" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
|
|
@@ -474,7 +480,7 @@ get_merge_tool_path () { | |
| merge_tool="$1" | ||
|
There was a problem hiding this comment. 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_modeThere was a problem hiding this comment. 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.There was a problem hiding this comment. 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.
|
||
| if ! valid_tool "$merge_tool" | ||
| then | ||
| echo >&2 "Unknown $TOOL_MODE tool $merge_tool" | ||
| exit 1 | ||
| fi | ||
| if diff_mode | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Philippe Blain wrote (reply to this):