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

[Junio C Hamano <gitster@pobox.com>]

Seth House <seth@eseth.com> writes:

> 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.