Re: [PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY

From: David Aguilar <davvid@gmail.com>
Date: Apr 4, 2023, 09:46 UTC
Message-ID: <ZCvx/REMF69xkXJg@gmail.com>
In-Reply-To: <pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com>

On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:

Show 17 quoted lines

> From: Tao Klerks <tao@klerks.biz>
> 
> When no merge.tool or diff.tool is configured or manually selected, the
> selection of a default tool is sensitive to the DISPLAY variable; in a
> GUI session a gui-specific tool will be proposed if found, and
> otherwise a terminal-based one. This "GUI-optimizing" behavior is
> important because a GUI can make a huge difference to a user's ability
> to understand and correctly complete a non-trivial conflicting merge.
> 
> Some time ago the merge.guitool and diff.guitool config options were
> introduced to enable users to configure both a GUI tool, and a non-GUI
> tool (with fallback if no GUI tool configured), in the same environment.
> 
> Unfortunately, the --gui argument introduced to support the selection of
> the guitool is still explicit. When using configured tools, there is no
> equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
> environment" behavior.

I'm not sure what problem this is solving. Please help me understand. This commit seems to be talking about the behavior w.r.t. the DISPLAY variable and also about automatically enabling `--gui` automatically.

Regarding `--gui`: Someone that configures `mergetool.guiDefault` means that all of their invocations should be as-if they had written `--gui` on the command-line.

So.. if you find that you always use `--gui` then why are you not configuring `merge.tool` directly and instead configuring `merge.guitool`?

The purpose of `--gui` is to provide an override for use by tools (such as Git Cola) to execute an optional, 2nd merge tool separate from that which the user might configure for command-line use.

This patch basically makes the `merge.tool` configuration useless, so I don't understand why someone would want to configure this option rather than just configuring `merge.tool` directly, leading to an overall simpler and easier to understand system.

If we add this boolean then it becomes harder to understand. I'm not sure it really provides any additional value to the user.

From my vantage point, the user is better off configuring `merge.tool` directly rather than the tool becoming more complex.

That said, I probably don't understand and I'd be happy to be enlightened.

Show 7 quoted lines

> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration
> options, difftool.guiDefault and mergetool.guiDefault, supporting a special
> value "auto" which causes the corresponding tool or guitool to be selected
> depending on the presence of a non-empty DISPLAY value. Also support "true"
> to say "default to the guitool (unless --no-gui is passed on the
> commandline)", and "false" as the previous default behavior when these new
> configuration options are not specified.

The rationale here is slightly surprising to me as well.

Apparently the user cannot be bothered to configure `merge.tool` (because we care about this DISPLAY behavior) yet the user is expected to configure `merge.guiDefault` instead?

I think it'd be better to encourage users to go ahead and configure `merge.tool`, but again, I'm probably missing context around why.

Small patch note below...

Show 10 quoted lines

> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 992124cc67c..e4e820e6809 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -75,6 +75,11 @@ then
>  		merge_tool="$GIT_DIFF_TOOL"
>  	else
>  		merge_tool="$(get_merge_tool)"
> +		subshell_exit_status=$?
> +		if test $subshell_exit_status -gt 1

Is exit status 1 special here? Should this be "if test $subshell_exit_status != 0" instead?

-- 
David