git mergetool, merge.tool, merge.guitool and DISPLAY

Hi folks,

I've just become aware today of what *seems* like a very strange discrepancy or limitation in "git mergetool":

When you use it without having configured a "merge.tool", it auto-selects the first available tool from a predefined list, which appears to be hardcoded in "git-mergetool--lib.sh", with some conditions around "$DISPLAY", "$GNOME_DESKTOP_SESSION_ID" and "${VISUAL:-$EDITOR}".

In this "auto-selection" situation, no GUI-based merge tool will be offered/selected if you're not in a GUI session.

When you configure your tools, you can configure "merge.tool" for the default, and "merge.guitool" for GUI contexts - so far so good, sounds consistent.

However, once you've configured these two settings, "git mergetool" will never select the GUI tool you've configured unless you very *explicitly* tell it to, by specifying the --gui argument. The sensible auto-selection based on "DISPLAY" disappears.

The upshot, as I understand it, is that the only way to get a GUI when I'm connected with an X session, and get a terminal-based mergetool when I'm not, without having to be aware and issue different commands, is to accept whatever tooling default order is hardcoded in "git-mergetool--lib.sh"

Is this intentional / is there any logic here, or is this just unfortunate, a result of the auto-selection evolving more intelligently than the built-in explicit "--gui" selection, over time??

If I wanted to improve this, what would be a more sensible approach?
* Assume "--gui" if there is a DISPLAY and "--no-gui" has not been
specified? (behavior change for some existing users)
* Add a new configuration to enable such an "auto-gui respecting
config" mode (no behavior change)

(I have not looked into "git difftool", but I assume the same arguments and issues apply - if so an equivalent improvement would need to be made there of course)

Thanks for any thoughts, Tao

Tao Klerks <tao@klerks.biz> writes:

We've had merge.tool almost forever but merge.guitool is a more recent invention in late 2018.

60aced3d (mergetool: fallback to tool when guitool unavailable, 2019-04-29) says something interesting:

    The behavior for when difftool or mergetool are called without `--gui`
    should be identical with or without this patch.

So, either we broke that promise since then, or the above commit was already broken, or the tool was already broken before that? In any case, I do not think of a good reason why configured .guitool is not automatically honored and .tool ignored when we know we are in an GUI environment. In other words, the choice of the tool should probably go like:

    are we in GUI? (determined by an explicit --gui, --no-gui, or env)
    if so
	pick one from configured .guitool (or from the fallback default
	list of tools)
    else
        pick one from configured .tool (or from the fallback default
        list of non-GUI tools)

I would think.

On Fri, Sep 9, 2022 at 7:07 PM Junio C Hamano <gitster@pobox.com> wrote:

Thanks for the quick feedback!

I don't think that promise was then, or is now, broken. The behavior without `--gui` is to always use the merge.tool (and never the merge.guitool). That is in fact my complaint - that I believe there should be an auto-selection of merge.tool or merge.guitool, depending on the DISPLAY variable, when neither `--gui` nor `--no-gui` have been specified.

My proposed behavior would be different to the current and original behavior, for users that have added a merge.guitool to their config in the meantime. If DISPLAY-sensitivity had been added at the time (before any users had a merge.guitool) it would have been a safe change without change in behavior for existing users, but now that some users do already have a merge.guitool, that is no longer the case.

Excellent, I agree. My concern then is whether this behavior should be placed behind a new config switch, to avoid surprising users who might have come to expect the current (in my opinion suboptimal) behavior, or whether invoking the merge.guitool instead of the merge.tool, when there is a merge.guitool of course and the DISPLAY is set, would be a net improvement even for those users and should just be implemented.

Tao Klerks <tao@klerks.biz> writes:

I wonder if we can think of a new config knob as giving the default value for the --gui option that is a tristate <false, auto, true>. When the knob is not set, it default to 'false', which is the current behaviour. The default most likely to be the most sensible would be 'auto' which is "see the environment to guess if use of GUI is appropriate".

On Fri, Sep 9, 2022 at 8:48 PM Junio C Hamano <gitster@pobox.com> wrote:

I just submitted an RFC patch along those lines. I have a few "niggles" I'd like feedback on, but in general I'm happy with it.