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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2021, 11:24 UTC
Message-ID: <xmqqh7np9gqn.fsf@gitster.c.googlers.com>
In-Reply-To: <20210110072902.GA247325@ellen>

Seth House <seth@eseth.com> writes:

Show 6 quoted lines

> On Sat, Jan 09, 2021 at 10:40:20PM -0800, Junio C Hamano wrote:
>> 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.
>
> That fixup does the trick on my machine too. Thank you.

Note that with t7800 fixed with the patch, non Windows jobs all seem to pass, but t7610 seems to have problem(s) on Windows.

https://github.com/git/git/runs/1675932107?check_suite_focus=true#step:7:10373

> How wary are you of continuing with `initialize_merge_tool`? Do you see
> a better approach to get `automerge_enabled `into scope? While it is
> a nice feature to have, is it worth the risk vs. reward?

Hmph. We need to have a way to define helper routines customized for the tool, and in the codebase without this series, it is the job for setup_tool. It defines fallback implementations, allows tool specific customizations.

Your initialize_merge_tool is just a thin wrapper around setup_tool. It calls setup_tool, and if the function exits with non-zero status, returns with status==1 (and otherwise returns with status==0). As I expect all the callers of setup_tool or initialize_merge_tool would either ignore the status or check if it succeeded (i.e. compare $? against 0 and any non-zero values are treated equally), it does not seem to do anything useful.

I think we may be able to get rid of initialize_merge_tool, but you would need to call setup_tool in places initialize_merge_tool was called in your patch, as you must have needed to make sure that the tool specific customizations have been carried out before going forward in these places.

So, no, I do not see a reason to be wary of initialize/setup.

With or without the seemingly needless initialize wrapper, I think calling setup before starting to do certain operations that need tool specific customization is just necessary. The same machanism has been in use to give can_merge/can_diff to each tool and the way it works ought to be fairly well understood.

It is a different story if it makes sense not to exit when you see failure from initialize/setup, and instead _skip_ running helpers like run_merge_tool. It was just a mistake we all make every once in a while (i.e. a bug), and I am reasonably sure that we will introduce more of them but we will be capable of fixing all.

Thanks.