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

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

[Tao Klerks]

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

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

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

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

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

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.

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

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

[Tao Klerks]

On Fri, Sep 9, 2022 at 7:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > 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"
>
> 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?

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.

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

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.

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

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

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

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

[Tao Klerks]

On Fri, Sep 9, 2022 at 8:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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".
>

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.