[v2,0/5] git-mergetool: improve error code paths and messages

Message

Philippe Blain via GitGitGadget Nov. 22, 2024, 7:50 p.m. UTC

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 <[e-mail]>
      
       ## 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

Comments

Junio C Hamano Nov. 26, 2024, 4:33 a.m. UTC | #1

"Philippe Blain via GitGitGadget" <[e-mail]> writes:

> 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

I think the above changes all looked good.

Let me mark the topic for 'next'.

Thanks.