Re* [PATCH v2] fixup! mergetool: add automerge configuration

Junio C Hamano <gitster@pobox.com> writes:

It seems that 51b27370 (mergetool: break setup_tool out into separate initialization function, 2020-12-28) is the culprit.

git-difftool--helper is taught to do this:

    diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
    index 46af3e60b7..234dd6944e 100755
    --- a/git-difftool--helper.sh
    +++ b/git-difftool--helper.sh
     ...
    @@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
     then
            LOCAL="$1"
            REMOTE="$2"
    +	initialize_merge_tool "$merge_tool" &&
            run_merge_tool "$merge_tool" false
     else
            # Launch the merge tool on each path provided by 'git diff'

But the thing is, t7800.5 gives a name of merge-tool that does not exist, and expects difftool to notice it and error out. The callchain starting from the above "run_merge_tool" should look like

    run_merge_tool () {
        merge_tool_path=$(get_merge_tool_path ...) || exit
	...

which in turn calls

    get_merge_tool_path () {
        # A merge tool has been set, so verify that it's valid.
        merge_tool="$1"
        if ! valid_tool "$merge_tool"
        then
                echo >&2 "Unknown merge tool $merge_tool"
                exit 1
        fi

and "valid_tool bad-tool" would return false, which lets us safely exit with the error message.

But the call to initialize_merge_tool inserted above, that makes us to SKIP the call to run_merge_tool, would defeat the error detection.

An ugly workaround patch that caters only to difftool breakage is attached at the end; I did not look if a similar treatment is necessary for the mergetool side.

By the way, debugging this was somewhat painful as difftool is partly rewritten in C. If it were still pure script, it would have been a lot easier to diagnose with a single "set -x" somewhere X-<.

----- >8 ---------- >8 ---------- >8 ---------- >8 -----
From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 9 Jan 2021 22:35:18 -0800
Subject: [PATCH] fixup! mergetool: break setup_tool out into
    separate initialization function

At least difftool wants to see even a broken tool name in its call to run_merge_tool and have it diagnose an error. &&-cascading the call to initialize_merge_tool and run_merge_tool means that a bogus tool name that does not please initialize_merge_tool is not even seen by run_merge_tool and the error goes unnoticed.

I didn't audit the original commit for its use of initialize_merge_tool on the merge-tool side. It may share the same issue, or it may not.

This should fix the errors we've been seeing in t7800.5 (and
possibly others) with this topic.
---
 git-difftool--helper.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)