Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+20 −10
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24933ba
completion: complete '--tool-help' in 'git mergetool'
phil-blain Nov 11, 2024
6f7f553
git-mergetool--lib.sh: use TOOL_MODE when erroring about unknown tool
phil-blain Nov 10, 2024
1d9e95c
git-mergetool--lib.sh: add error message if 'setup_user_tool' fails
phil-blain Nov 8, 2024
f234e96
git-mergetool--lib.sh: add error message for unknown tool variant
phil-blain Nov 11, 2024
c16e922
git-difftool--helper.sh: exit upon initialize_merge_tool errors
phil-blain Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/completion/git-completion.bash
View file
Open in desktop
Original file line number Diff 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
2 changes: 1 addition & 1 deletion git-mergetool--lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ get_merge_tool_path () {
merge_tool="$1"
Copy link

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):

"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

Copy link

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):

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.

Copy link

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):

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 merge tool $merge_tool"
echo >&2 "Unknown $TOOL_MODE tool $merge_tool"
exit 1
fi
if diff_mode
Expand Down