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

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

[Tao Klerks via GitGitGadget]

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.

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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    RFC: mergetool: new config guiDefault supports auto-toggling gui by
    DISPLAY
    
    I'm confortable that with this patch we do the right thing, but I'm not
    sure about a couple of specific code patterns and implementation
    choices:
    
     * In difftool.c, I added a new enum and arg validation function, but
       didn't know where in the file to place them. Is there any guidance on
       this that I missed?
    
     * In git-mergetool--lib.sh, I used an awkward "tr" call to make the
       config value case-insensitive (to support "Auto" for example); Would
       relying on bash 4's case conversion support be better?
    
     * In git-mergetool--lib.sh the way I implemented the "auto" special
       value means that if you put an arbitrary value in the config, eg the
       typo "uato", you get an error about it being an invalid boolean
       config value; is that OK? Is there a better way to handle "boolean or
       special value" config validation? Are there any examples?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1381

 Documentation/config/difftool.txt  |  7 +++++
 Documentation/config/mergetool.txt |  7 +++++
 Documentation/git-difftool.txt     | 10 +++++---
 Documentation/git-mergetool.txt    |  9 ++++---
 builtin/difftool.c                 | 40 ++++++++++++++++++++++++-----
 git-mergetool--lib.sh              | 26 +++++++++++++++++++
 git-mergetool.sh                   |  2 +-
 t/t7610-mergetool.sh               | 39 ++++++++++++++++++++++++++++
 t/t7800-difftool.sh                | 41 ++++++++++++++++++++++++++++++
 9 files changed, 165 insertions(+), 16 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index a3f82112102..8c2f7460be4 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.guiDefault::
+	Set 'true' to use the diff.guitool by default (equivalent to specifying
+	the "--gui" argument), or "auto" to select diff.guitool or diff.tool
+	depending on the presence of a DISPLAY environment variable value. The
+	default is 'false', where the "--gui" argument must be provided
+	explicitly for the diff.guitool to be used.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90b38097002..bc1a85abc95 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -85,3 +85,10 @@ mergetool.writeToTemp::
 
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
+
+mergetool.guiDefault::
+	Set 'true' to use the merge.guitool by default (equivalent to
+	specifying the "--gui" argument), or "auto" to select merge.guitool
+	or merge.tool depending on the presence of a DISPLAY environment
+	variable value. The default is 'false', where the "--gui" argument
+	must be provided explicitly for the merge.guitool to be used.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 9d14c3c9f09..a09dfb072d5 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.
 --[no-]gui::
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
-	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting. If `diff.guitool`
-	is not set, we will fallback in the order of `merge.guitool`,
-	`diff.tool`, `merge.tool` until a tool is found.
+	`diff.guitool` variable instead of `diff.tool`. This may be
+	autoselected using the configuration variable
+	`difftool.guiDefault`. The `--no-gui` option can be used to
+	override these settings. If `diff.guitool` is not set, we will
+	fallback in the order of `merge.guitool`, `diff.tool`,
+	`merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c44e205629b..07535f6576e 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.
 	the default merge tool will be read from the configured
 	`merge.guitool` variable instead of `merge.tool`. If
 	`merge.guitool` is not set, we will fallback to the tool
-	configured under `merge.tool`.
+	configured under `merge.tool`. This may be autoselected using
+	the configuration variable `mergetool.guiDefault`.
 
 --no-gui::
-	This overrides a previous `-g` or `--gui` setting and reads the
-	default merge tool will be read from the configured `merge.tool`
-	variable.
+	This overrides a previous `-g` or `--gui` setting or
+	`mergetool.guiDefault` configuration and reads the default merge
+	tool from the configured `merge.tool` variable.
 
 -O<orderfile>::
 	Process files in the order specified in the
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..2a7d5ec5f42 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -680,14 +680,37 @@ static int run_file_diff(int prompt, const char *prefix,
 	return run_command(child);
 }
 
+enum difftool_gui_mode {
+	GUI_DISABLED = -1,
+	GUI_BY_CONFIG = 0,
+	GUI_ENABLED = 1
+};
+
+static int difftool_opt_gui(const struct option *opt,
+			      const char *optarg, int unset)
+{
+	enum difftool_gui_mode *mode;
+	mode = opt->value;
+
+	BUG_ON_OPT_ARG(optarg);
+
+	if (unset)
+		*mode = GUI_DISABLED;
+	else
+		*mode = GUI_ENABLED;
+	return 0;
+}
+
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
-	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0, no_index = 0;
+	int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
+	    no_index = 0;
+	enum difftool_gui_mode gui_mode;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
-		OPT_BOOL('g', "gui", &use_gui_tool,
-			 N_("use `diff.guitool` instead of `diff.tool`")),
+		OPT_CALLBACK_F('g', "gui", &gui_mode, NULL,
+			       N_("use `diff.guitool` instead of `diff.tool`"),
+			       PARSE_OPT_NOARG, difftool_opt_gui),
 		OPT_BOOL('d', "dir-diff", &dir_diff,
 			 N_("perform a full-directory diff")),
 		OPT_SET_INT_F('y', "no-prompt", &prompt,
@@ -732,13 +755,16 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
-	die_for_incompatible_opt3(use_gui_tool, "--gui",
+	die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
 				  !!difftool_cmd, "--tool",
 				  !!extcmd, "--extcmd");
 
-	if (use_gui_tool)
+	if (gui_mode == GUI_ENABLED)
 		setenv("GIT_MERGETOOL_GUI", "true", 1);
-	else if (difftool_cmd) {
+	else if (gui_mode == GUI_DISABLED)
+		setenv("GIT_MERGETOOL_GUI", "false", 1);
+
+	if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
 		else
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9f99201bcca..8dbd04e5c5c 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -97,7 +97,33 @@ merge_mode () {
 	test "$TOOL_MODE" = merge
 }
 
+get_gui_default () {
+	if diff_mode
+	then
+		GUI_DEFAULT_KEY="difftool.guiDefault"
+	else
+		GUI_DEFAULT_KEY="mergetool.guiDefault"
+	fi
+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')
+	if [ "$GUI_DEFAULT_CONFIG_LCASE" = "auto" ]
+	then
+		if [ -n "$DISPLAY" ]
+		then
+			GUI_DEFAULT=true
+		else
+			GUI_DEFAULT=false
+		fi
+	else
+		GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)
+	fi
+	echo $GUI_DEFAULT
+}
+
 gui_mode () {
+	if [ -z "$GIT_MERGETOOL_GUI" ]
+	then
+		GIT_MERGETOOL_GUI=$(get_gui_default)
+	fi
 	test "$GIT_MERGETOOL_GUI" = true
 }
 
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f751d9cfe20..f510ff9ea70 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -451,7 +451,7 @@ print_noop_and_exit () {
 
 main () {
 	prompt=$(git config --bool mergetool.prompt)
-	GIT_MERGETOOL_GUI=false
+	GIT_MERGETOOL_GUI=
 	guessed_merge_tool=false
 	orderfile=
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729adb..3a807fd681c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '
 	git commit -m "test resolved with mergetool"
 '
 
+test_expect_success 'mergetool with guiDefault' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge main &&
+
+	test_config mergetool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+	yes "" | git mergetool both &&
+	yes "" | git mergetool file1 file1 &&
+
+	DISPLAY= && export DISPLAY &&
+	yes "" | git mergetool file2 "spaced name" &&
+
+	test_config mergetool.guiDefault true &&
+	yes "" | git mergetool subdir/file3 &&
+
+	yes "d" | git mergetool file11 &&
+	yes "d" | git mergetool file12 &&
+	yes "l" | git mergetool submod &&
+
+
+	echo "gui main updated" >expect &&
+	test_cmp expect file1 &&
+
+	echo "main new" >expect &&
+	test_cmp expect file2 &&
+
+	echo "gui main new sub" >expect &&
+	test_cmp expect subdir/file3 &&
+
+	echo "branch1 submodule" >expect &&
+	test_cmp expect submod/bar &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_done
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 096456292c0..ef785c55747 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -155,6 +155,47 @@ test_expect_success 'difftool honors --gui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault auto &&
+	DISPLAY= && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool with guiDefault true selects gui tool' '
+	difftool_test_setup &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	DISPLAY= && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	DISPLAY=Something && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'difftool --gui last setting wins' '
 	difftool_test_setup &&
 	: >expect &&

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df
-- 
gitgitgadget

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

[Tao Klerks]

On Wed, Oct 12, 2022 at 5:59 PM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>

Quick clarification - this was intended to be an RFC.

I forgot that with GitGitGadget at the moment (until we do something
to explicitly support RFCs) a single-commit patch needs the *commit*
to have the RFC prefix, instead of the Merge Request, as the commit's
subject is used instead of the merge request's subject in a
single-commit patch.

I will make sure any later RFC versions have the prefix as appropriate.

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.
>
> 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.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     RFC: mergetool: new config guiDefault supports auto-toggling gui by
>     DISPLAY

This somehow felt somewhat familiar, so I had to go the list archive
to find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,
which seems to be the previous discussion.

It would have been much easier if you gave readers the original
context that inspired this design.

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

[Tao Klerks]

On Wed, Oct 12, 2022 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> This somehow felt somewhat familiar, so I had to go the list archive
> to find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,
> which seems to be the previous discussion.
>
> It would have been much easier if you gave readers the original
> context that inspired this design.
>

My apologies, I should have included this link in the commit message -
I tried to "fix it" by reviving the thread in question, but clearly
too little too late.

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +enum difftool_gui_mode {
> +	GUI_DISABLED = -1,
> +	GUI_BY_CONFIG = 0,
> +	GUI_ENABLED = 1
> +};
> +
> +static int difftool_opt_gui(const struct option *opt,
> +			      const char *optarg, int unset)
> +{
> +	enum difftool_gui_mode *mode;
> +	mode = opt->value;
> +
> +	BUG_ON_OPT_ARG(optarg);
> +
> +	if (unset)
> +		*mode = GUI_DISABLED;
> +	else
> +		*mode = GUI_ENABLED;
> +	return 0;
> +}
> +
>  int cmd_difftool(int argc, const char **argv, const char *prefix)
>  {
> -	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> -	    tool_help = 0, no_index = 0;
> +	int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
> +	    no_index = 0;
> +	enum difftool_gui_mode gui_mode;

This is left uninitialized ...


>  	static char *difftool_cmd = NULL, *extcmd = NULL;
>  	struct option builtin_difftool_options[] = {
> -		OPT_BOOL('g', "gui", &use_gui_tool,
> -			 N_("use `diff.guitool` instead of `diff.tool`")),
> +		OPT_CALLBACK_F('g', "gui", &gui_mode, NULL,
> +			       N_("use `diff.guitool` instead of `diff.tool`"),
> +			       PARSE_OPT_NOARG, difftool_opt_gui),

... and its address is used here.  When "--no-gui" or "--gui" option
is given, the callback function will fill either _DISABLED or _ENABLED
to it.

But without any command line argument?  Isn't gui_mode variable ...

>  		OPT_BOOL('d', "dir-diff", &dir_diff,
>  			 N_("perform a full-directory diff")),
>  		OPT_SET_INT_F('y', "no-prompt", &prompt,
> @@ -732,13 +755,16 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
>  	} else if (dir_diff)
>  		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
>  

... still uninitialized here?  The old use_gui_tool was initialized
to 0 so it wouldn't have had this problem.

> -	die_for_incompatible_opt3(use_gui_tool, "--gui",
> +	die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
>  				  !!difftool_cmd, "--tool",
>  				  !!extcmd, "--extcmd");
>  
> -	if (use_gui_tool)
> +	if (gui_mode == GUI_ENABLED)
>  		setenv("GIT_MERGETOOL_GUI", "true", 1);

I suspect that there is no need to introduce a enum. The flow would
probably be

 * git_config(difftool_config) would learn to parse the .guiDefault
   option and initialize use_gui_tool to -1 when set to "auto" (and
   to 0 with "false", to 1 with "true").

 * Call to parse_options() then overwrites use_gui_tool with either
   0 or 1 when --no-gui or --gui is given.

 * After parse_options() returns, use_gui_tool can be examined and
   when it is 0 or 1, then nothing need to change.  The current code
   before this patch is doing what the user wants when an explicit
   command line option is given.

 * When use_gui_tool is -1, we need a new code that sets it to
   either 0 or 1 depending on the running environment.

But what is curious is that nothing in C code even looks at .guiDefault
configuration, so I am not sure why you would even need to change
anything in builtin/difftool.c file at all.

Puzzled.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 9f99201bcca..8dbd04e5c5c 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -97,7 +97,33 @@ merge_mode () {
>  	test "$TOOL_MODE" = merge
>  }
>  
> +get_gui_default () {
> +	if diff_mode
> +	then
> +		GUI_DEFAULT_KEY="difftool.guiDefault"
> +	else
> +		GUI_DEFAULT_KEY="mergetool.guiDefault"
> +	fi
> +	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')

Avoid [:class:] when 'A-Z' 'a-z' is sufficient.  Easier to read and
you do not even need to worry about portability that way.

> +	if [ "$GUI_DEFAULT_CONFIG_LCASE" = "auto" ]
> +	then
> +		if [ -n "$DISPLAY" ]
> +		then
> +			GUI_DEFAULT=true
> +		else
> +			GUI_DEFAULT=false
> +		fi
> +	else
> +		GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)
> +	fi
> +	echo $GUI_DEFAULT
> +}

Check Documentation/CodingGuidelines with special attention to the
"for shell scripts" section?

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

[Tao Klerks]

On Thu, Oct 13, 2022 at 11:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +     enum difftool_gui_mode gui_mode;
>
> This is left uninitialized ...
>

This is clearly a bug, and you've just solved a mystery for me. At one
point one test behaved "crazily", reporting an error as though --gui
had been specified in conflict with another option, despite it not
having been specified at all! I was unable to reproduce this behavior
in any way, and uncomfortably declared it must have been a "bit-flip"
error. Now I understand why, and how this class of error can occur in
C. Thank you for spotting this.

I had incorrectly assumed that it would be implicitly initialized to
0, meaning GUI_BY_CONFIG. Even if that had been true, it was a mistake
on my part to not be explicit about this default value - I failed to
use the enum for its intended purpose of making the code easy to
understand.

>
> [...] Isn't gui_mode variable ...
> ... still uninitialized here?  The old use_gui_tool was initialized
> to 0 so it wouldn't have had this problem.

Absolutely. This meant that *occasionally*, it would look like --gui
or --no-gui was specified even though it was not. Most of the time the
uninitialized value did not exactly match 1 or -1, so it looked like
things were working correctly, even though they were doing so
"accidentally".

>
> > -     die_for_incompatible_opt3(use_gui_tool, "--gui",
> > +     die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
> >                                 !!difftool_cmd, "--tool",
> >                                 !!extcmd, "--extcmd");
> >
> > -     if (use_gui_tool)
> > +     if (gui_mode == GUI_ENABLED)
> >               setenv("GIT_MERGETOOL_GUI", "true", 1);
>
> I suspect that there is no need to introduce a enum.

An enum is unnecessary, I only added it in order for a tristate
"gui_mode" value to be legible.

Case in point, you proposed that -1 could mean "_BY_CONFIG", whereas I
(possibly against all prior logic in this codebase) assumed it would
make more sense to have -1 be "_FALSE" in this case; the enum helps
avoid any confusion. If you find the enum has a nontrivial cost or
fails to improve understandability I am happy to remove it and use
simple int values instead.

> The flow would
> probably be
>
>  * git_config(difftool_config) would learn to parse the .guiDefault
>    option and initialize use_gui_tool to -1 when set to "auto" (and
>    to 0 with "false", to 1 with "true").
>
>  * Call to parse_options() then overwrites use_gui_tool with either
>    0 or 1 when --no-gui or --gui is given.
>
>  * After parse_options() returns, use_gui_tool can be examined and
>    when it is 0 or 1, then nothing need to change.  The current code
>    before this patch is doing what the user wants when an explicit
>    command line option is given.
>
>  * When use_gui_tool is -1, we need a new code that sets it to
>    either 0 or 1 depending on the running environment.
>
> But what is curious is that nothing in C code even looks at .guiDefault
> configuration, so I am not sure why you would even need to change
> anything in builtin/difftool.c file at all.
>
> Puzzled.
>

There are two interesting things here I guess:

1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
OPT_BOOL arg type?
Because OPT_BOOL always overwrites with either true or false, so in
order to have use_gui_tool retain its "_BY_CONFIG" value in the
absence of an arg, the explicit arg handler function is required.

2. Why is the "auto" case not explicitly addressed in this code at all?
Because both difftool and mergetool use the same tool-finding logic in
git-mergetool--lib.sh. The C code is just a (complex) "shell", setting
the environment such that git-mergetool--lib.sh knows what to do. Wrt
the gui decision, the implementation here has it set
"GIT_MERGETOOL_GUI" to "true" or "false" if the user has made an
explicit decision, and if it is blank or unset that means "use the
config". The tristate signal in difftool.c is "GUI, NO_GUI or
USE_CONFIG". It would be possible to parse the config in C and change
the tristate signal being passed down to instead be "GUI, NO_GUI or
AUTO", but I believe it would make things more complicated.

I understand "interesting things that need to be explained" are
something one tries to avoid in the source code - I will see if adding
a couple of well-placed comments makes things cleaner.

>
> Avoid [:class:] when 'A-Z' 'a-z' is sufficient.  Easier to read and
> you do not even need to worry about portability that way.
>

Sounds good!

>
> Check Documentation/CodingGuidelines with special attention to the
> "for shell scripts" section?

Will do.

As always, thank you for taking the time to look at this! V2 coming soon.

[PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY

[Tao Klerks via GitGitGadget]

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.

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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    RFC: mergetool: new config guiDefault supports auto-toggling gui by
    DISPLAY
    
    I'm reasonably comfortable that with this patch we do the right thing,
    but I'm not sure about one remaining implementation choice:
    
     * In git-mergetool--lib.sh the way I implemented the "auto" special
       value means that if you put an arbitrary value in the config, eg the
       typo "uato", you get an error about it being an invalid boolean
       config value; is that OK? Is there a better way to handle "boolean or
       special value" config validation? Are there any examples?
    
    V2:
    
     * Fix uninitialized variable that caused buggy behavior and lack of
       clarity
     * Fix enum values to better match Junio's expectations
     * Add comment about relationship between C and shell scripts
     * Fix "tr" call to use simple ANSI ranges rather than character classes
     * Fix shell test to use "test" rather than brackets
     * Add better test coverage by using case-inconsistent "Auto" in test
     * Add test to validate command-line override of configured default in
       difftool

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1381

Range-diff vs v1:

 1:  4e3495343db ! 1:  70d0e0d09fa mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
     @@ Metadata
      Author: Tao Klerks <tao@klerks.biz>
      
       ## Commit message ##
     -    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
     +    RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
      
          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
     @@ Commit message
          equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
          environment" behavior.
      
     -    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.
     +    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.
      
          Signed-off-by: Tao Klerks <tao@klerks.biz>
      
     @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,
       }
       
      +enum difftool_gui_mode {
     -+	GUI_DISABLED = -1,
     -+	GUI_BY_CONFIG = 0,
     ++	GUI_BY_CONFIG = -1,
     ++	GUI_DISABLED = 0,
      +	GUI_ENABLED = 1
      +};
      +
     @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,
      -	    tool_help = 0, no_index = 0;
      +	int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
      +	    no_index = 0;
     -+	enum difftool_gui_mode gui_mode;
     ++	enum difftool_gui_mode gui_mode = GUI_BY_CONFIG;
       	static char *difftool_cmd = NULL, *extcmd = NULL;
       	struct option builtin_difftool_options[] = {
      -		OPT_BOOL('g', "gui", &use_gui_tool,
     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr
       				  !!extcmd, "--extcmd");
       
      -	if (use_gui_tool)
     ++	/*
     ++	 * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;
     ++	 * empty or unset means "use the difftool.guiDefault config or default to
     ++	 * false".
     ++	 */
      +	if (gui_mode == GUI_ENABLED)
       		setenv("GIT_MERGETOOL_GUI", "true", 1);
      -	else if (difftool_cmd) {
     @@ git-mergetool--lib.sh: merge_mode () {
      +	else
      +		GUI_DEFAULT_KEY="mergetool.guiDefault"
      +	fi
     -+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')
     -+	if [ "$GUI_DEFAULT_CONFIG_LCASE" = "auto" ]
     ++	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')
     ++	if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
      +	then
     -+		if [ -n "$DISPLAY" ]
     ++		if test -n "$DISPLAY"
      +		then
      +			GUI_DEFAULT=true
      +		else
     @@ t/t7800-difftool.sh: test_expect_success 'difftool honors --gui' '
      +	difftool_test_setup &&
      +	test_config diff.guitool bogus-tool &&
      +	test_config diff.tool test-tool &&
     -+	test_config difftool.guiDefault auto &&
     ++	test_config difftool.guiDefault Auto &&
      +	DISPLAY= && export DISPLAY &&
      +
      +	echo branch >expect &&
     @@ t/t7800-difftool.sh: test_expect_success 'difftool honors --gui' '
      +	git difftool --no-prompt branch >actual &&
      +	test_cmp expect actual
      +'
     ++
     ++test_expect_success 'difftool --no-gui trumps config guiDefault' '
     ++	difftool_test_setup &&
     ++	test_config diff.guitool bogus-tool &&
     ++	test_config diff.tool test-tool &&
     ++	test_config difftool.guiDefault true &&
     ++
     ++	echo branch >expect &&
     ++	git difftool --no-prompt --no-gui branch >actual &&
     ++	test_cmp expect actual
     ++'
      +
       test_expect_success 'difftool --gui last setting wins' '
       	difftool_test_setup &&


 Documentation/config/difftool.txt  |  7 ++++
 Documentation/config/mergetool.txt |  7 ++++
 Documentation/git-difftool.txt     | 10 +++---
 Documentation/git-mergetool.txt    |  9 +++---
 builtin/difftool.c                 | 45 ++++++++++++++++++++++----
 git-mergetool--lib.sh              | 26 +++++++++++++++
 git-mergetool.sh                   |  2 +-
 t/t7610-mergetool.sh               | 39 ++++++++++++++++++++++
 t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++
 9 files changed, 181 insertions(+), 16 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index a3f82112102..8c2f7460be4 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.guiDefault::
+	Set 'true' to use the diff.guitool by default (equivalent to specifying
+	the "--gui" argument), or "auto" to select diff.guitool or diff.tool
+	depending on the presence of a DISPLAY environment variable value. The
+	default is 'false', where the "--gui" argument must be provided
+	explicitly for the diff.guitool to be used.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90b38097002..bc1a85abc95 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -85,3 +85,10 @@ mergetool.writeToTemp::
 
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
+
+mergetool.guiDefault::
+	Set 'true' to use the merge.guitool by default (equivalent to
+	specifying the "--gui" argument), or "auto" to select merge.guitool
+	or merge.tool depending on the presence of a DISPLAY environment
+	variable value. The default is 'false', where the "--gui" argument
+	must be provided explicitly for the merge.guitool to be used.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 9d14c3c9f09..a09dfb072d5 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.
 --[no-]gui::
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
-	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting. If `diff.guitool`
-	is not set, we will fallback in the order of `merge.guitool`,
-	`diff.tool`, `merge.tool` until a tool is found.
+	`diff.guitool` variable instead of `diff.tool`. This may be
+	autoselected using the configuration variable
+	`difftool.guiDefault`. The `--no-gui` option can be used to
+	override these settings. If `diff.guitool` is not set, we will
+	fallback in the order of `merge.guitool`, `diff.tool`,
+	`merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c44e205629b..07535f6576e 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.
 	the default merge tool will be read from the configured
 	`merge.guitool` variable instead of `merge.tool`. If
 	`merge.guitool` is not set, we will fallback to the tool
-	configured under `merge.tool`.
+	configured under `merge.tool`. This may be autoselected using
+	the configuration variable `mergetool.guiDefault`.
 
 --no-gui::
-	This overrides a previous `-g` or `--gui` setting and reads the
-	default merge tool will be read from the configured `merge.tool`
-	variable.
+	This overrides a previous `-g` or `--gui` setting or
+	`mergetool.guiDefault` configuration and reads the default merge
+	tool from the configured `merge.tool` variable.
 
 -O<orderfile>::
 	Process files in the order specified in the
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..374bb7df34f 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -680,14 +680,37 @@ static int run_file_diff(int prompt, const char *prefix,
 	return run_command(child);
 }
 
+enum difftool_gui_mode {
+	GUI_BY_CONFIG = -1,
+	GUI_DISABLED = 0,
+	GUI_ENABLED = 1
+};
+
+static int difftool_opt_gui(const struct option *opt,
+			      const char *optarg, int unset)
+{
+	enum difftool_gui_mode *mode;
+	mode = opt->value;
+
+	BUG_ON_OPT_ARG(optarg);
+
+	if (unset)
+		*mode = GUI_DISABLED;
+	else
+		*mode = GUI_ENABLED;
+	return 0;
+}
+
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
-	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0, no_index = 0;
+	int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
+	    no_index = 0;
+	enum difftool_gui_mode gui_mode = GUI_BY_CONFIG;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
-		OPT_BOOL('g', "gui", &use_gui_tool,
-			 N_("use `diff.guitool` instead of `diff.tool`")),
+		OPT_CALLBACK_F('g', "gui", &gui_mode, NULL,
+			       N_("use `diff.guitool` instead of `diff.tool`"),
+			       PARSE_OPT_NOARG, difftool_opt_gui),
 		OPT_BOOL('d', "dir-diff", &dir_diff,
 			 N_("perform a full-directory diff")),
 		OPT_SET_INT_F('y', "no-prompt", &prompt,
@@ -732,13 +755,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
-	die_for_incompatible_opt3(use_gui_tool, "--gui",
+	die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
 				  !!difftool_cmd, "--tool",
 				  !!extcmd, "--extcmd");
 
-	if (use_gui_tool)
+	/*
+	 * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;
+	 * empty or unset means "use the difftool.guiDefault config or default to
+	 * false".
+	 */
+	if (gui_mode == GUI_ENABLED)
 		setenv("GIT_MERGETOOL_GUI", "true", 1);
-	else if (difftool_cmd) {
+	else if (gui_mode == GUI_DISABLED)
+		setenv("GIT_MERGETOOL_GUI", "false", 1);
+
+	if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
 		else
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9f99201bcca..ff08eb7ae93 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -97,7 +97,33 @@ merge_mode () {
 	test "$TOOL_MODE" = merge
 }
 
+get_gui_default () {
+	if diff_mode
+	then
+		GUI_DEFAULT_KEY="difftool.guiDefault"
+	else
+		GUI_DEFAULT_KEY="mergetool.guiDefault"
+	fi
+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')
+	if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
+	then
+		if test -n "$DISPLAY"
+		then
+			GUI_DEFAULT=true
+		else
+			GUI_DEFAULT=false
+		fi
+	else
+		GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)
+	fi
+	echo $GUI_DEFAULT
+}
+
 gui_mode () {
+	if [ -z "$GIT_MERGETOOL_GUI" ]
+	then
+		GIT_MERGETOOL_GUI=$(get_gui_default)
+	fi
 	test "$GIT_MERGETOOL_GUI" = true
 }
 
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f751d9cfe20..f510ff9ea70 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -451,7 +451,7 @@ print_noop_and_exit () {
 
 main () {
 	prompt=$(git config --bool mergetool.prompt)
-	GIT_MERGETOOL_GUI=false
+	GIT_MERGETOOL_GUI=
 	guessed_merge_tool=false
 	orderfile=
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729adb..3a807fd681c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '
 	git commit -m "test resolved with mergetool"
 '
 
+test_expect_success 'mergetool with guiDefault' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge main &&
+
+	test_config mergetool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+	yes "" | git mergetool both &&
+	yes "" | git mergetool file1 file1 &&
+
+	DISPLAY= && export DISPLAY &&
+	yes "" | git mergetool file2 "spaced name" &&
+
+	test_config mergetool.guiDefault true &&
+	yes "" | git mergetool subdir/file3 &&
+
+	yes "d" | git mergetool file11 &&
+	yes "d" | git mergetool file12 &&
+	yes "l" | git mergetool submod &&
+
+
+	echo "gui main updated" >expect &&
+	test_cmp expect file1 &&
+
+	echo "main new" >expect &&
+	test_cmp expect file2 &&
+
+	echo "gui main new sub" >expect &&
+	test_cmp expect subdir/file3 &&
+
+	echo "branch1 submodule" >expect &&
+	test_cmp expect submod/bar &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_done
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 096456292c0..1e47d3c05bd 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault Auto &&
+	DISPLAY= && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool with guiDefault true selects gui tool' '
+	difftool_test_setup &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	DISPLAY= && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	DISPLAY=Something && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool --no-gui trumps config guiDefault' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	echo branch >expect &&
+	git difftool --no-prompt --no-gui branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'difftool --gui last setting wins' '
 	difftool_test_setup &&
 	: >expect &&

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df
-- 
gitgitgadget

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

[Eric Sunshine]

"On Fri, Oct 14, 2022 at 4:07 AM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> 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.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
> diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
> @@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.
> +difftool.guiDefault::
> +       Set 'true' to use the diff.guitool by default (equivalent to specifying
> +       the "--gui" argument), or "auto" to select diff.guitool or diff.tool
> +       depending on the presence of a DISPLAY environment variable value. The
> +       default is 'false', where the "--gui" argument must be provided
> +       explicitly for the diff.guitool to be used.

Let's use backticks rather than double-quotes to ensure that these get
typeset similar to other documentation; i.e. `--gui`, `auto`,
`diff.guitool`, `diff.tool`, `DISPLAY`.

> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> @@ -85,3 +85,10 @@ mergetool.writeToTemp::
> +mergetool.guiDefault::
> +       Set 'true' to use the merge.guitool by default (equivalent to
> +       specifying the "--gui" argument), or "auto" to select merge.guitool
> +       or merge.tool depending on the presence of a DISPLAY environment
> +       variable value. The default is 'false', where the "--gui" argument
> +       must be provided explicitly for the merge.guitool to be used.

Ditto.

> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> @@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.
>  --[no-]gui::
>         When 'git-difftool' is invoked with the `-g` or `--gui` option
>         the default diff tool will be read from the configured
> +       `diff.guitool` variable instead of `diff.tool`. This may be
> +       autoselected using the configuration variable
> +       `difftool.guiDefault`. The `--no-gui` option can be used to
> +       override these settings. If `diff.guitool` is not set, we will
> +       fallback in the order of `merge.guitool`, `diff.tool`,
> +       `merge.tool` until a tool is found.

Correct use of backticks here. Good.

Probably want: s/autoselected/auto-selected/ or /.../selected automatically/

> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> @@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.
> +       configured under `merge.tool`. This may be autoselected using
> +       the configuration variable `mergetool.guiDefault`.

Ditto: "autoselected"

>  --no-gui::
> +       This overrides a previous `-g` or `--gui` setting or
> +       `mergetool.guiDefault` configuration and reads the default merge
> +       tool from the configured `merge.tool` variable.

Backticks; good.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> @@ -97,7 +97,33 @@ merge_mode () {
> +get_gui_default () {
> +       if diff_mode
> +       then
> +               GUI_DEFAULT_KEY="difftool.guiDefault"
> +       else
> +               GUI_DEFAULT_KEY="mergetool.guiDefault"
> +       fi
> +       GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')

Too many spaces before pipe symbol.

Nit: It doesn't matter in this case, but you could safeguard against
(possible?) future problems by using double-quotes in `--get
"$GUI_DEFAULT_KEY"`.

> +       if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
> +       then
> +               if test -n "$DISPLAY"
> +               then
> +                       GUI_DEFAULT=true
> +               else
> +                       GUI_DEFAULT=false
> +               fi
> +       else
> +               GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)

Ditto: `--get "$GUI_DEFAULT_KEY"`

> +       fi
> +       echo $GUI_DEFAULT
> +}
> +
>  gui_mode () {
> +       if [ -z "$GIT_MERGETOOL_GUI" ]

Style: if test -z "$GIT_MERGETOOL_GUI"

> +       then
> +               GIT_MERGETOOL_GUI=$(get_gui_default)
> +       fi
>         test "$GIT_MERGETOOL_GUI" = true
>  }
>
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> @@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '
> +test_expect_success 'mergetool with guiDefault' '
> +       test_config mergetool.guiDefault true &&
> +       yes "" | git mergetool subdir/file3 &&
> +
> +       yes "d" | git mergetool file11 &&
> +       yes "d" | git mergetool file12 &&
> +       yes "l" | git mergetool submod &&
> +
> +
> +       echo "gui main updated" >expect &&
> +       test_cmp expect file1 &&

Accidental double-spacing?

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

> 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
> OPT_BOOL arg type?
> Because OPT_BOOL always overwrites with either true or false, so in
> order to have use_gui_tool retain its "_BY_CONFIG" value in the
> absence of an arg, the explicit arg handler function is required.

That is exactly why you want to initialize the variable to something
other than 0 or 1 (say "-1") and use that as "not explicitly
specified from the command line" aka _BY_CONFIG value, isn't it?

And once you do so, you do not need callback and you do not need
enum.  The original "int" can take tri-state, (unspecified = -1,
false = 0, true = 1).

Am I missing something?

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

[Tao Klerks]

On Fri, Oct 14, 2022 at 10:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> backticks
> selected automatically
> spaces
> test

Ack on all these, my apologies, and thank you!

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

[Tao Klerks]

On Fri, Oct 14, 2022 at 11:11 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
> > OPT_BOOL arg type?
> > Because OPT_BOOL always overwrites with either true or false, so in
> > order to have use_gui_tool retain its "_BY_CONFIG" value in the
> > absence of an arg, the explicit arg handler function is required.
>
> That is exactly why you want to initialize the variable to something
> other than 0 or 1 (say "-1") and use that as "not explicitly
> specified from the command line" aka _BY_CONFIG value, isn't it?
>
> And once you do so, you do not need callback and you do not need
> enum.  The original "int" can take tri-state, (unspecified = -1,
> false = 0, true = 1).
>
> Am I missing something?

Heh, I guess one of us must be, and it's probably me :)

My understanding, from ad-hoc testing, is that using OPT_BOOL causes
the target variable ("use_gui_tool" in this case) to *always* be set
to either "1" or "0", regardless of whether the user sets the main
flag, the --no form of the flag, or sets nothing at all. As far as I
can tell, OPT_BOOL never leaves the original value, so it cannot be
used to track tri-state "true, false or not specified" logic. Maybe I
made a mistake somewhere, and that's not really or always how it
behaves.

I can re-test, and/or try to dig into the option parsing logic to
understand how it works - the time I tried, I got hopelessly lost and
was relieved to find that OPT_CALLBACK_F existed and seemed to be used
for exactly these kinds of use cases.

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

> My understanding, from ad-hoc testing, is that using OPT_BOOL causes
> the target variable ("use_gui_tool" in this case) to *always* be set
> to either "1" or "0", regardless of whether the user sets the main
> flag, the --no form of the flag, or sets nothing at all.

You are saying "git gc --keep-largest-pack" is broken ;-)

OPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there
is no "set to this default value if no command line option is given"
involved.  At least I do not recall allowing such a code in.

Thanks.

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

[Junio C Hamano]

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

>      * In git-mergetool--lib.sh the way I implemented the "auto" special
>        value means that if you put an arbitrary value in the config, eg the
>        typo "uato", you get an error about it being an invalid boolean
>        config value; is that OK? Is there a better way to handle "boolean or
>        special value" config validation? Are there any examples?

I think the ideal behaviour would be:

 * Unless running difftool and difftool.guiChoice has a wrong value,
   or running mergetool and mergetool.guiChoice has a wrong value,
   we should not even complain.

 * If the command line says --gui or --no-gui that makes the setting
   irrelevant, it is OK for us to give a warning to remind the user
   that they may want to fix the spelling of the variable, but
   otherwise go ahead and perform the action as they asked us to.

 * If the command line lacks --gui or --no-gui, we do need to have a
   usable value in the configuration, and we should error out
   without spawning either gui or no-gui tool backend.

It may be usable without the second one and always fail difftool and
mergetool until the setting gets fixed, but that is less than ideal.
We do allow less than ideal code in, as long as it is an improvement
over the status quo, and its presence does not make it harder to
later get closer to the ideal.

Thanks.

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

[Tao Klerks]

On Fri, Oct 14, 2022 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > My understanding, from ad-hoc testing, is that using OPT_BOOL causes
> > the target variable ("use_gui_tool" in this case) to *always* be set
> > to either "1" or "0", regardless of whether the user sets the main
> > flag, the --no form of the flag, or sets nothing at all.
>
> You are saying "git gc --keep-largest-pack" is broken ;-)

Now that I look at that, that does appear to have been implied by what
I was saying, yes!

>
> OPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there
> is no "set to this default value if no command line option is given"
> involved.  At least I do not recall allowing such a code in.
>

I can confirm that it works the way you expected. I was sure I had
experienced such behavior during my early attempts at tristate
handling here, but in retrospect I was very much mistaken, not only
because --keep-largest-pack is not broken, but also because I just
ripped out all the arg-parsing function and enum stuff, and it all
works fine with -1, 0 and 1 as you pointed out it should. I don't know
exactly how I messed up my early testing, but I don't suppose it
matters.

Substantial simplification in the next update.

Thanks!

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

[Tao Klerks]

On Fri, Oct 14, 2022 at 5:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >      * In git-mergetool--lib.sh the way I implemented the "auto" special
> >        value means that if you put an arbitrary value in the config, eg the
> >        typo "uato", you get an error about it being an invalid boolean
> >        config value; is that OK? Is there a better way to handle "boolean or
> >        special value" config validation? Are there any examples?
>
> I think the ideal behaviour would be:
>
>  * Unless running difftool and difftool.guiChoice has a wrong value,
>    or running mergetool and mergetool.guiChoice has a wrong value,
>    we should not even complain.

This is how it behaves right now, yes.

>
>  * If the command line says --gui or --no-gui that makes the setting
>    irrelevant, it is OK for us to give a warning to remind the user
>    that they may want to fix the spelling of the variable, but
>    otherwise go ahead and perform the action as they asked us to.

In the current implementation, there is no warning if the choice has
been explicitly made - there is no reason to check the configured
default under such circumstances.

>
>  * If the command line lacks --gui or --no-gui, we do need to have a
>    usable value in the configuration, and we should error out
>    without spawning either gui or no-gui tool backend.

This is *not* the current behavior - currently an error is printed,
but execution continues with a no-gui default. I will correct this.

>
> It may be usable without the second one and always fail difftool and
> mergetool until the setting gets fixed, but that is less than ideal.
> We do allow less than ideal code in, as long as it is an improvement
> over the status quo, and its presence does not make it harder to
> later get closer to the ideal.

Thanks for the logic table, sounds good.

However, my concern was with the precise form of the error if a
configured value is neither trueish, not falseish, nor a case-tolerant
form of "auto": under such circumstances, instead of a dedicated error
message along the lines of "fatal: bad config value 'autod' for
'difftool.guidefault' - must be boolean or 'auto'", we get the
*default* error message for *boolean* config settings: "fatal: bad
boolean config value 'autod' for 'difftool.guidefault'". This can of
course be corrected, I just need to work out how, and exactly what
form the error message should then take.

Should we be elaborating an error message for this specific tristate
config value, or is it "normal" fail with a slightly-misleading
message suggesting it is a simple boolean config in such cases?

Thanks,
Tao

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

>>  * If the command line says --gui or --no-gui that makes the setting
>>    irrelevant, it is OK for us to give a warning to remind the user
>>    that they may want to fix the spelling of the variable, but
>>    otherwise go ahead and perform the action as they asked us to.
>
> In the current implementation, there is no warning if the choice has
> been explicitly made - there is no reason to check the configured
> default under such circumstances.

Yeah, not making any noise is better.  I only meant that if the
implementation warns in this irrelevant case, it is OK (aka
"acceptable, not necessarily desired") as long as it does not stop.

>>  * If the command line lacks --gui or --no-gui, we do need to have a
>>    usable value in the configuration, and we should error out
>>    without spawning either gui or no-gui tool backend.
>
> This is *not* the current behavior - currently an error is printed,
> but execution continues with a no-gui default. I will correct this.

Sounds good.

> 'difftool.guidefault' - must be boolean or 'auto'", we get the
> *default* error message for *boolean* config settings: "fatal: bad
> boolean config value 'autod' for 'difftool.guidefault'".

Yeah, I do not think it is a problem, and it is not misleading, as
long as the user knows how to ask further information with "git
difftool --help" and the help page says what the acceptable values
are other than Boolean yes/no.

Thanks.

[PATCH v3] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY

[Tao Klerks via GitGitGadget]

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.

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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    RFC: mergetool: new config guiDefault supports auto-toggling gui by
    DISPLAY
    
    I'm reasonably comfortable that with this patch we do the right thing,
    but I'm not sure about yet another remaining implementation detail:
    
     * After implementing Junio's recommended "fail if defaulting config is
       consulted and is invalid" flow, there now needs to be a distinction
       between subshell exit code 1, which was used before and indicates
       "tool not found or broken; falling back to default" and other
       (higher) exit codes, which newly mean "something went wrong, stop!".
       The resulting code looks awkward, I can't tell whether I'm missing a
       code or even commenting pattern that would make it clearer.
    
    V3:
    
     * Simplify C code to use OPT_BOOL with an int rather than a custom
       option-parsing function with an enum
     * Fix doc to more extensively use backticks for config keys / values /
       args
     * Fix more shell script formatting issues
     * Change error-handling in mergetool and difftool helpers to exit if
       defaulting config is invalid

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1381

Range-diff vs v2:

 1:  70d0e0d09fa ! 1:  dbb80499218 RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
     @@ Documentation/config/difftool.txt: See the `--trust-exit-code` option in linkgit
       	Prompt before each invocation of the diff tool.
      +
      +difftool.guiDefault::
     -+	Set 'true' to use the diff.guitool by default (equivalent to specifying
     -+	the "--gui" argument), or "auto" to select diff.guitool or diff.tool
     -+	depending on the presence of a DISPLAY environment variable value. The
     -+	default is 'false', where the "--gui" argument must be provided
     -+	explicitly for the diff.guitool to be used.
     ++	Set `true` to use the `diff.guitool` by default (equivalent to specifying
     ++	the `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`
     ++	depending on the presence of a `DISPLAY` environment variable value. The
     ++	default is `false`, where the `--gui` argument must be provided
     ++	explicitly for the `diff.guitool` to be used.
      
       ## Documentation/config/mergetool.txt ##
      @@ Documentation/config/mergetool.txt: mergetool.writeToTemp::
     @@ Documentation/config/mergetool.txt: mergetool.writeToTemp::
       	Prompt before each invocation of the merge resolution program.
      +
      +mergetool.guiDefault::
     -+	Set 'true' to use the merge.guitool by default (equivalent to
     -+	specifying the "--gui" argument), or "auto" to select merge.guitool
     -+	or merge.tool depending on the presence of a DISPLAY environment
     -+	variable value. The default is 'false', where the "--gui" argument
     -+	must be provided explicitly for the merge.guitool to be used.
     ++	Set `true` to use the `merge.guitool` by default (equivalent to
     ++	specifying the `--gui` argument), or `auto` to select `merge.guitool`
     ++	or `merge.tool` depending on the presence of a `DISPLAY` environment
     ++	variable value. The default is `false`, where the `--gui` argument
     ++	must be provided explicitly for the `merge.guitool` to be used.
      
       ## Documentation/git-difftool.txt ##
      @@ Documentation/git-difftool.txt: instead.  `--no-symlinks` is the default on Windows.
     @@ Documentation/git-difftool.txt: instead.  `--no-symlinks` is the default on Wind
      -	is not set, we will fallback in the order of `merge.guitool`,
      -	`diff.tool`, `merge.tool` until a tool is found.
      +	`diff.guitool` variable instead of `diff.tool`. This may be
     -+	autoselected using the configuration variable
     ++	selected automatically using the configuration variable
      +	`difftool.guiDefault`. The `--no-gui` option can be used to
      +	override these settings. If `diff.guitool` is not set, we will
      +	fallback in the order of `merge.guitool`, `diff.tool`,
     @@ Documentation/git-mergetool.txt: success of the resolution after the custom tool
      
       ## builtin/difftool.c ##
      @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,
     - 	return run_command(child);
     - }
       
     -+enum difftool_gui_mode {
     -+	GUI_BY_CONFIG = -1,
     -+	GUI_DISABLED = 0,
     -+	GUI_ENABLED = 1
     -+};
     -+
     -+static int difftool_opt_gui(const struct option *opt,
     -+			      const char *optarg, int unset)
     -+{
     -+	enum difftool_gui_mode *mode;
     -+	mode = opt->value;
     -+
     -+	BUG_ON_OPT_ARG(optarg);
     -+
     -+	if (unset)
     -+		*mode = GUI_DISABLED;
     -+	else
     -+		*mode = GUI_ENABLED;
     -+	return 0;
     -+}
     -+
       int cmd_difftool(int argc, const char **argv, const char *prefix)
       {
      -	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
     --	    tool_help = 0, no_index = 0;
     -+	int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
     -+	    no_index = 0;
     -+	enum difftool_gui_mode gui_mode = GUI_BY_CONFIG;
     ++	int use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,
     + 	    tool_help = 0, no_index = 0;
       	static char *difftool_cmd = NULL, *extcmd = NULL;
       	struct option builtin_difftool_options[] = {
     --		OPT_BOOL('g', "gui", &use_gui_tool,
     --			 N_("use `diff.guitool` instead of `diff.tool`")),
     -+		OPT_CALLBACK_F('g', "gui", &gui_mode, NULL,
     -+			       N_("use `diff.guitool` instead of `diff.tool`"),
     -+			       PARSE_OPT_NOARG, difftool_opt_gui),
     - 		OPT_BOOL('d', "dir-diff", &dir_diff,
     - 			 N_("perform a full-directory diff")),
     - 		OPT_SET_INT_F('y', "no-prompt", &prompt,
      @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *prefix)
       	} else if (dir_diff)
       		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
       
      -	die_for_incompatible_opt3(use_gui_tool, "--gui",
     -+	die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
     ++	die_for_incompatible_opt3(use_gui_tool == 1, "--gui",
       				  !!difftool_cmd, "--tool",
       				  !!extcmd, "--extcmd");
       
     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr
      +	 * empty or unset means "use the difftool.guiDefault config or default to
      +	 * false".
      +	 */
     -+	if (gui_mode == GUI_ENABLED)
     ++	if (use_gui_tool == 1)
       		setenv("GIT_MERGETOOL_GUI", "true", 1);
      -	else if (difftool_cmd) {
     -+	else if (gui_mode == GUI_DISABLED)
     ++	else if (use_gui_tool == 0)
      +		setenv("GIT_MERGETOOL_GUI", "false", 1);
      +
      +	if (difftool_cmd) {
     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr
       			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
       		else
      
     + ## git-difftool--helper.sh ##
     +@@ git-difftool--helper.sh: then
     + 		merge_tool="$GIT_DIFF_TOOL"
     + 	else
     + 		merge_tool="$(get_merge_tool)"
     ++		subshell_exit_status=$?
     ++		if test $subshell_exit_status -gt 1
     ++		then
     ++			exit $subshell_exit_status
     ++		fi
     + 	fi
     + fi
     + 
     +
       ## git-mergetool--lib.sh ##
      @@ git-mergetool--lib.sh: merge_mode () {
       	test "$TOOL_MODE" = merge
     @@ git-mergetool--lib.sh: merge_mode () {
      +	else
      +		GUI_DEFAULT_KEY="mergetool.guiDefault"
      +	fi
     -+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')
     ++	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get "$GUI_DEFAULT_KEY" | tr 'A-Z' 'a-z')
      +	if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
      +	then
      +		if test -n "$DISPLAY"
     @@ git-mergetool--lib.sh: merge_mode () {
      +			GUI_DEFAULT=false
      +		fi
      +	else
     -+		GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)
     ++		GUI_DEFAULT=$(git config --default false --bool --get "$GUI_DEFAULT_KEY")
     ++		subshell_exit_status=$?
     ++		if test $subshell_exit_status -ne 0
     ++		then
     ++			exit $subshell_exit_status
     ++		fi
      +	fi
      +	echo $GUI_DEFAULT
      +}
      +
       gui_mode () {
     -+	if [ -z "$GIT_MERGETOOL_GUI" ]
     ++	if test -z "$GIT_MERGETOOL_GUI"
      +	then
      +		GIT_MERGETOOL_GUI=$(get_gui_default)
     ++		if test $? -ne 0
     ++		then
     ++			exit 2
     ++		fi
      +	fi
       	test "$GIT_MERGETOOL_GUI" = true
       }
       
     +@@ git-mergetool--lib.sh: get_merge_tool () {
     + 	is_guessed=false
     + 	# Check if a merge tool has been configured
     + 	merge_tool=$(get_configured_merge_tool)
     ++	subshell_exit_status=$?
     ++	if test $subshell_exit_status -gt "1"
     ++	then
     ++		exit $subshell_exit_status
     ++	fi
     + 	# Try to guess an appropriate merge tool if no tool has been set.
     + 	if test -z "$merge_tool"
     + 	then
      
       ## git-mergetool.sh ##
      @@ git-mergetool.sh: print_noop_and_exit () {
     @@ git-mergetool.sh: print_noop_and_exit () {
       	guessed_merge_tool=false
       	orderfile=
       
     +@@ git-mergetool.sh: main () {
     + 
     + 	if test -z "$merge_tool"
     + 	then
     +-		if ! merge_tool=$(get_merge_tool)
     ++		merge_tool=$(get_merge_tool)
     ++		subshell_exit_status=$?
     ++		if test $subshell_exit_status = 1
     + 		then
     + 			guessed_merge_tool=true
     ++		elif test $subshell_exit_status -gt 1
     ++		then
     ++			exit $subshell_exit_status
     + 		fi
     + 	fi
     + 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
      
       ## t/t7610-mergetool.sh ##
      @@ t/t7610-mergetool.sh: test_expect_success 'mergetool hideResolved' '
     @@ t/t7610-mergetool.sh: test_expect_success 'mergetool hideResolved' '
      +	yes "d" | git mergetool file12 &&
      +	yes "l" | git mergetool submod &&
      +
     -+
      +	echo "gui main updated" >expect &&
      +	test_cmp expect file1 &&
      +


 Documentation/config/difftool.txt  |  7 ++++
 Documentation/config/mergetool.txt |  7 ++++
 Documentation/git-difftool.txt     | 10 +++---
 Documentation/git-mergetool.txt    |  9 +++---
 builtin/difftool.c                 | 16 ++++++---
 git-difftool--helper.sh            |  5 +++
 git-mergetool--lib.sh              | 40 +++++++++++++++++++++++
 git-mergetool.sh                   |  9 ++++--
 t/t7610-mergetool.sh               | 38 ++++++++++++++++++++++
 t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++
 10 files changed, 179 insertions(+), 14 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index a3f82112102..447c40d85a2 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.guiDefault::
+	Set `true` to use the `diff.guitool` by default (equivalent to specifying
+	the `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`
+	depending on the presence of a `DISPLAY` environment variable value. The
+	default is `false`, where the `--gui` argument must be provided
+	explicitly for the `diff.guitool` to be used.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90b38097002..a615bf24187 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -85,3 +85,10 @@ mergetool.writeToTemp::
 
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
+
+mergetool.guiDefault::
+	Set `true` to use the `merge.guitool` by default (equivalent to
+	specifying the `--gui` argument), or `auto` to select `merge.guitool`
+	or `merge.tool` depending on the presence of a `DISPLAY` environment
+	variable value. The default is `false`, where the `--gui` argument
+	must be provided explicitly for the `merge.guitool` to be used.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 9d14c3c9f09..ac0ac6fa022 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.
 --[no-]gui::
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
-	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting. If `diff.guitool`
-	is not set, we will fallback in the order of `merge.guitool`,
-	`diff.tool`, `merge.tool` until a tool is found.
+	`diff.guitool` variable instead of `diff.tool`. This may be
+	selected automatically using the configuration variable
+	`difftool.guiDefault`. The `--no-gui` option can be used to
+	override these settings. If `diff.guitool` is not set, we will
+	fallback in the order of `merge.guitool`, `diff.tool`,
+	`merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c44e205629b..07535f6576e 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.
 	the default merge tool will be read from the configured
 	`merge.guitool` variable instead of `merge.tool`. If
 	`merge.guitool` is not set, we will fallback to the tool
-	configured under `merge.tool`.
+	configured under `merge.tool`. This may be autoselected using
+	the configuration variable `mergetool.guiDefault`.
 
 --no-gui::
-	This overrides a previous `-g` or `--gui` setting and reads the
-	default merge tool will be read from the configured `merge.tool`
-	variable.
+	This overrides a previous `-g` or `--gui` setting or
+	`mergetool.guiDefault` configuration and reads the default merge
+	tool from the configured `merge.tool` variable.
 
 -O<orderfile>::
 	Process files in the order specified in the
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..2b89c5dc865 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -682,7 +682,7 @@ static int run_file_diff(int prompt, const char *prefix,
 
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
-	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
+	int use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,
 	    tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
@@ -732,13 +732,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
-	die_for_incompatible_opt3(use_gui_tool, "--gui",
+	die_for_incompatible_opt3(use_gui_tool == 1, "--gui",
 				  !!difftool_cmd, "--tool",
 				  !!extcmd, "--extcmd");
 
-	if (use_gui_tool)
+	/*
+	 * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;
+	 * empty or unset means "use the difftool.guiDefault config or default to
+	 * false".
+	 */
+	if (use_gui_tool == 1)
 		setenv("GIT_MERGETOOL_GUI", "true", 1);
-	else if (difftool_cmd) {
+	else if (use_gui_tool == 0)
+		setenv("GIT_MERGETOOL_GUI", "false", 1);
+
+	if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
 		else
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
+		then
+			exit $subshell_exit_status
+		fi
 	fi
 fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9f99201bcca..1ff26170ffc 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -97,7 +97,42 @@ merge_mode () {
 	test "$TOOL_MODE" = merge
 }
 
+get_gui_default () {
+	if diff_mode
+	then
+		GUI_DEFAULT_KEY="difftool.guiDefault"
+	else
+		GUI_DEFAULT_KEY="mergetool.guiDefault"
+	fi
+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get "$GUI_DEFAULT_KEY" | tr 'A-Z' 'a-z')
+	if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
+	then
+		if test -n "$DISPLAY"
+		then
+			GUI_DEFAULT=true
+		else
+			GUI_DEFAULT=false
+		fi
+	else
+		GUI_DEFAULT=$(git config --default false --bool --get "$GUI_DEFAULT_KEY")
+		subshell_exit_status=$?
+		if test $subshell_exit_status -ne 0
+		then
+			exit $subshell_exit_status
+		fi
+	fi
+	echo $GUI_DEFAULT
+}
+
 gui_mode () {
+	if test -z "$GIT_MERGETOOL_GUI"
+	then
+		GIT_MERGETOOL_GUI=$(get_gui_default)
+		if test $? -ne 0
+		then
+			exit 2
+		fi
+	fi
 	test "$GIT_MERGETOOL_GUI" = true
 }
 
@@ -467,6 +502,11 @@ get_merge_tool () {
 	is_guessed=false
 	# Check if a merge tool has been configured
 	merge_tool=$(get_configured_merge_tool)
+	subshell_exit_status=$?
+	if test $subshell_exit_status -gt "1"
+	then
+		exit $subshell_exit_status
+	fi
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f751d9cfe20..8a922893f75 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -451,7 +451,7 @@ print_noop_and_exit () {
 
 main () {
 	prompt=$(git config --bool mergetool.prompt)
-	GIT_MERGETOOL_GUI=false
+	GIT_MERGETOOL_GUI=
 	guessed_merge_tool=false
 	orderfile=
 
@@ -511,9 +511,14 @@ main () {
 
 	if test -z "$merge_tool"
 	then
-		if ! merge_tool=$(get_merge_tool)
+		merge_tool=$(get_merge_tool)
+		subshell_exit_status=$?
+		if test $subshell_exit_status = 1
 		then
 			guessed_merge_tool=true
+		elif test $subshell_exit_status -gt 1
+		then
+			exit $subshell_exit_status
 		fi
 	fi
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 8cc64729adb..ba0d2ae76de 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -860,4 +860,42 @@ test_expect_success 'mergetool hideResolved' '
 	git commit -m "test resolved with mergetool"
 '
 
+test_expect_success 'mergetool with guiDefault' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge main &&
+
+	test_config mergetool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+	yes "" | git mergetool both &&
+	yes "" | git mergetool file1 file1 &&
+
+	DISPLAY= && export DISPLAY &&
+	yes "" | git mergetool file2 "spaced name" &&
+
+	test_config mergetool.guiDefault true &&
+	yes "" | git mergetool subdir/file3 &&
+
+	yes "d" | git mergetool file11 &&
+	yes "d" | git mergetool file12 &&
+	yes "l" | git mergetool submod &&
+
+	echo "gui main updated" >expect &&
+	test_cmp expect file1 &&
+
+	echo "main new" >expect &&
+	test_cmp expect file2 &&
+
+	echo "gui main new sub" >expect &&
+	test_cmp expect subdir/file3 &&
+
+	echo "branch1 submodule" >expect &&
+	test_cmp expect submod/bar &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_done
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 096456292c0..1e47d3c05bd 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault Auto &&
+	DISPLAY= && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool with guiDefault true selects gui tool' '
+	difftool_test_setup &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	DISPLAY= && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	DISPLAY=Something && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool --no-gui trumps config guiDefault' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	echo branch >expect &&
+	git difftool --no-prompt --no-gui branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'difftool --gui last setting wins' '
 	difftool_test_setup &&
 	: >expect &&

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df
-- 
gitgitgadget

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

[Tao Klerks]

On Tue, Oct 18, 2022 at 8:54 AM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>
> 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.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     RFC: mergetool: new config guiDefault supports auto-toggling gui by
>     DISPLAY
>
>     I'm reasonably comfortable that with this patch we do the right thing,
>     but I'm not sure about yet another remaining implementation detail:
>
>      * After implementing Junio's recommended "fail if defaulting config is
>        consulted and is invalid" flow, there now needs to be a distinction
>        between subshell exit code 1, which was used before and indicates
>        "tool not found or broken; falling back to default" and other
>        (higher) exit codes, which newly mean "something went wrong, stop!".
>        The resulting code looks awkward, I can't tell whether I'm missing a
>        code or even commenting pattern that would make it clearer.
>
>     V3:
>
>      * Simplify C code to use OPT_BOOL with an int rather than a custom
>        option-parsing function with an enum
>      * Fix doc to more extensively use backticks for config keys / values /
>        args
>      * Fix more shell script formatting issues
>      * Change error-handling in mergetool and difftool helpers to exit if
>        defaulting config is invalid
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1381


Hi folks, this v3 never got any feedback - the only reason I had left
it as an RFC was that the error-handling looked a bit awkward, as I
noted above.

Should I resubmit this without the RFC prefix?

Are there any concerns about the change here to better support mixed
GUI/console-only environments?

Thanks,
Tao

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

[Tao Klerks via GitGitGadget]

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.

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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
    
    A few months later, first "real" submission - 4th version wrt original
    draft.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1381

Range-diff vs v3:

 1:  dbb80499218 ! 1:  765abfe7fc4 RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
     @@ Metadata
      Author: Tao Klerks <tao@klerks.biz>
      
       ## Commit message ##
     -    RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
     +    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
      
          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


 Documentation/config/difftool.txt  |  7 ++++
 Documentation/config/mergetool.txt |  7 ++++
 Documentation/git-difftool.txt     | 10 +++---
 Documentation/git-mergetool.txt    |  9 +++---
 builtin/difftool.c                 | 16 ++++++---
 git-difftool--helper.sh            |  5 +++
 git-mergetool--lib.sh              | 40 +++++++++++++++++++++++
 git-mergetool.sh                   |  9 ++++--
 t/t7610-mergetool.sh               | 38 ++++++++++++++++++++++
 t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++
 10 files changed, 179 insertions(+), 14 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index a3f82112102..447c40d85a2 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.guiDefault::
+	Set `true` to use the `diff.guitool` by default (equivalent to specifying
+	the `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`
+	depending on the presence of a `DISPLAY` environment variable value. The
+	default is `false`, where the `--gui` argument must be provided
+	explicitly for the `diff.guitool` to be used.
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index e779a122d8a..56a7eeeffb4 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -85,3 +85,10 @@ mergetool.writeToTemp::
 
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
+
+mergetool.guiDefault::
+	Set `true` to use the `merge.guitool` by default (equivalent to
+	specifying the `--gui` argument), or `auto` to select `merge.guitool`
+	or `merge.tool` depending on the presence of a `DISPLAY` environment
+	variable value. The default is `false`, where the `--gui` argument
+	must be provided explicitly for the `merge.guitool` to be used.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 9d14c3c9f09..ac0ac6fa022 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.
 --[no-]gui::
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
-	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting. If `diff.guitool`
-	is not set, we will fallback in the order of `merge.guitool`,
-	`diff.tool`, `merge.tool` until a tool is found.
+	`diff.guitool` variable instead of `diff.tool`. This may be
+	selected automatically using the configuration variable
+	`difftool.guiDefault`. The `--no-gui` option can be used to
+	override these settings. If `diff.guitool` is not set, we will
+	fallback in the order of `merge.guitool`, `diff.tool`,
+	`merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index c44e205629b..07535f6576e 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.
 	the default merge tool will be read from the configured
 	`merge.guitool` variable instead of `merge.tool`. If
 	`merge.guitool` is not set, we will fallback to the tool
-	configured under `merge.tool`.
+	configured under `merge.tool`. This may be autoselected using
+	the configuration variable `mergetool.guiDefault`.
 
 --no-gui::
-	This overrides a previous `-g` or `--gui` setting and reads the
-	default merge tool will be read from the configured `merge.tool`
-	variable.
+	This overrides a previous `-g` or `--gui` setting or
+	`mergetool.guiDefault` configuration and reads the default merge
+	tool from the configured `merge.tool` variable.
 
 -O<orderfile>::
 	Process files in the order specified in the
diff --git a/builtin/difftool.c b/builtin/difftool.c
index dbbfb19f192..88a00abfbdb 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -684,7 +684,7 @@ static int run_file_diff(int prompt, const char *prefix,
 
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
-	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
+	int use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,
 	    tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
@@ -734,13 +734,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
-	die_for_incompatible_opt3(use_gui_tool, "--gui",
+	die_for_incompatible_opt3(use_gui_tool == 1, "--gui",
 				  !!difftool_cmd, "--tool",
 				  !!extcmd, "--extcmd");
 
-	if (use_gui_tool)
+	/*
+	 * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;
+	 * empty or unset means "use the difftool.guiDefault config or default to
+	 * false".
+	 */
+	if (use_gui_tool == 1)
 		setenv("GIT_MERGETOOL_GUI", "true", 1);
-	else if (difftool_cmd) {
+	else if (use_gui_tool == 0)
+		setenv("GIT_MERGETOOL_GUI", "false", 1);
+
+	if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
 		else
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
+		then
+			exit $subshell_exit_status
+		fi
 	fi
 fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9f99201bcca..1ff26170ffc 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -97,7 +97,42 @@ merge_mode () {
 	test "$TOOL_MODE" = merge
 }
 
+get_gui_default () {
+	if diff_mode
+	then
+		GUI_DEFAULT_KEY="difftool.guiDefault"
+	else
+		GUI_DEFAULT_KEY="mergetool.guiDefault"
+	fi
+	GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get "$GUI_DEFAULT_KEY" | tr 'A-Z' 'a-z')
+	if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto"
+	then
+		if test -n "$DISPLAY"
+		then
+			GUI_DEFAULT=true
+		else
+			GUI_DEFAULT=false
+		fi
+	else
+		GUI_DEFAULT=$(git config --default false --bool --get "$GUI_DEFAULT_KEY")
+		subshell_exit_status=$?
+		if test $subshell_exit_status -ne 0
+		then
+			exit $subshell_exit_status
+		fi
+	fi
+	echo $GUI_DEFAULT
+}
+
 gui_mode () {
+	if test -z "$GIT_MERGETOOL_GUI"
+	then
+		GIT_MERGETOOL_GUI=$(get_gui_default)
+		if test $? -ne 0
+		then
+			exit 2
+		fi
+	fi
 	test "$GIT_MERGETOOL_GUI" = true
 }
 
@@ -467,6 +502,11 @@ get_merge_tool () {
 	is_guessed=false
 	# Check if a merge tool has been configured
 	merge_tool=$(get_configured_merge_tool)
+	subshell_exit_status=$?
+	if test $subshell_exit_status -gt "1"
+	then
+		exit $subshell_exit_status
+	fi
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f751d9cfe20..8a922893f75 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -451,7 +451,7 @@ print_noop_and_exit () {
 
 main () {
 	prompt=$(git config --bool mergetool.prompt)
-	GIT_MERGETOOL_GUI=false
+	GIT_MERGETOOL_GUI=
 	guessed_merge_tool=false
 	orderfile=
 
@@ -511,9 +511,14 @@ main () {
 
 	if test -z "$merge_tool"
 	then
-		if ! merge_tool=$(get_merge_tool)
+		merge_tool=$(get_merge_tool)
+		subshell_exit_status=$?
+		if test $subshell_exit_status = 1
 		then
 			guessed_merge_tool=true
+		elif test $subshell_exit_status -gt 1
+		then
+			exit $subshell_exit_status
 		fi
 	fi
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7b957022f1a..22b3a85b3e9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -860,4 +860,42 @@ test_expect_success 'mergetool hideResolved' '
 	git commit -m "test resolved with mergetool"
 '
 
+test_expect_success 'mergetool with guiDefault' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge main &&
+
+	test_config mergetool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+	yes "" | git mergetool both &&
+	yes "" | git mergetool file1 file1 &&
+
+	DISPLAY= && export DISPLAY &&
+	yes "" | git mergetool file2 "spaced name" &&
+
+	test_config mergetool.guiDefault true &&
+	yes "" | git mergetool subdir/file3 &&
+
+	yes "d" | git mergetool file11 &&
+	yes "d" | git mergetool file12 &&
+	yes "l" | git mergetool submod &&
+
+	echo "gui main updated" >expect &&
+	test_cmp expect file1 &&
+
+	echo "main new" >expect &&
+	test_cmp expect file2 &&
+
+	echo "gui main new sub" >expect &&
+	test_cmp expect subdir/file3 &&
+
+	echo "branch1 submodule" >expect &&
+	test_cmp expect submod/bar &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_done
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 24297e26ca0..59d3847bf87 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '
+	difftool_test_setup &&
+	test_config merge.tool bogus-tool &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault auto &&
+	DISPLAY=SOMETHING && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault Auto &&
+	DISPLAY= && export DISPLAY &&
+
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool with guiDefault true selects gui tool' '
+	difftool_test_setup &&
+	test_config diff.tool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	DISPLAY= && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	DISPLAY=Something && export DISPLAY &&
+	echo branch >expect &&
+	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'difftool --no-gui trumps config guiDefault' '
+	difftool_test_setup &&
+	test_config diff.guitool bogus-tool &&
+	test_config diff.tool test-tool &&
+	test_config difftool.guiDefault true &&
+
+	echo branch >expect &&
+	git difftool --no-prompt --no-gui branch >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'difftool --gui last setting wins' '
 	difftool_test_setup &&
 	: >expect &&

base-commit: d15644fe0226af7ffc874572d968598564a230dd
-- 
gitgitgadget

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

[David Aguilar]

On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:
> 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.


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


> 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

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

[Tao Klerks]

First of all, thank you for reviewing!!

On Tue, Apr 4, 2023 at 11:46 AM David Aguilar <davvid@gmail.com> wrote:
>
> On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:
> > 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.

Background: If you don't set a mergetool at all, git defaults one if
it can. The default you get depends on the DISPLAY variable - if it
looks like you're in a gui context (because we detect that DISPLAY is
set), then we propose a gui tool. Otherwise, we propose a cli tool.

This is particularly valuable for contexts where you might access the
same environment, the same workstation, via multiple contexts -
CLI-only, and GUI. For example, sometimes you connect to the desktop,
and sometimes you SSH into it (without X forwarding). You don't need
to tell git "I want a GUI now please", it figures that out.

Point/problem: As soon as you set "merge.tool", you lose that
autoselection. A reasonable user would assume "there is `merge.tool`,
there is `merge.guitool`, and git knows how to autodetect GUI context
based on the DISPLAY variable... so if I set both `merge.tool` and
`merge.guitool`, I'll get the right one based on context, right? Like
I do when I set neither?"
Unfortunately, that is *not* the case. The gui autodetection only
applies when git is choosing a *defaulted* merge tool, not an
explicitly-defined one. As soon as you specify a tool you want to use,
you need to start *explicitly* telling git whether you want to use a
GUI tool or not. If you get it wrong, an inconvenient or even bad
thing happens: If you're in a GUI context and you forget to say
`--gui`, then you get the CLI tool - not the end of the world, you
might argue. If you're in a non-GUI context and you say `--gui`, you
get a (hopefully intelligible) error about not finding a display, or
worst case, you hang - and git doesn't know what happened, the
messaging has to assume you aborted explicitly.

If there were a way to get git to autoselect "--gui" by presence of
the DISPLAY, as it does for the *default* mergetool, then this kind of
flow would be much more intuitive, usable, etc.

I'm not sure whether I'm explaining better, or just repeating myself,
so I'll stop here :)

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

Correction: Someone that sets `mergetool.guiDefault` to *true* should
get behavior as-if they had written `--gui` on the command-line. They
could also set *false* (current behavior - explicit), or *auto*, and
get different behavior.

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

One reason is because you might choose to do something different in a
*specific* invocation, eg by passing `--no-gui`. This is a common
pattern in git - the config allows you to change the default behavior,
but you can override it on a specific invocation by using the positive
or negative version of the argument.

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

I'm not aware of having read a statement identifying this
*singular*/sole purpose anywhere, but as you state it with authority
(and I see you're the author of a popular git GUI), I'm happy to
accept that this was originally its only purpose. That would certainly
explain the incongruousness of its behavior wrt (lack of)
autodetection of DISPLAY.

That said, this patch does not in any way diminish the validity or
usability of this usecase. If you don't set `mergetool.guiDefault` or
leave it set to false, that purpose will continue to be perfectly
served.

For other users, like me, who prefer to select a GUI mergetool or not
depending on the *availability* of a GUI environment (auto), or
default to gui but allow manually downgrading to a (defaulted or
configured) CLI-only tool, this configuration will allow a simpler,
more coherent, easier UX.

If everything else here makes sense, I will happily update the
abstract/commit message to reflect the proposed broadening of
application / correct my misunderstanding as to the original intended
scope of the setting.

>
> This patch basically makes the `merge.tool` configuration useless,

I don't understand why you say this. Unless I misunderstand something,
this patch does not break, or even change, any existing behavior.

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

(see above - the main usecase is a system you access via both GUI and
CLI contexts, and the secondary usecase is having a default GUI
behavior that you can easily override with `--no-gui`.)

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

Please let me know if it makes more sense now. You could argue that
the value is too limited to warrant the additional complexity
(obviously I would disagree), but hopefully the concrete value is
clearer now.

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

Umm, no: The user wants `merge.tool` to be set to a specific CLI tool,
they want `merge.guitool` to be set to a specific GUI tool, and they
want the GUI tool to be used in GUI contexts, and the CLI tool to be
used when there is no GUI (eg SSH without X forwarding) - like the
behavior they get if they configure neither `merge.tool` nor
`merge.guitool`.

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

Yes, it is special: 1 means "mergetool was guessed/defaulted",
anything greater than 1 means "mergetool determination failed;
aborting".

I was not very happy with this way of passing information around when
I implemented, and commented in the previous version of the patch
https://lore.kernel.org/git/pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com/
as my last doubt as to the proposed/RFC patch:

       After implementing Junio's recommended "fail if defaulting config is
       consulted and is invalid" flow, there now needs to be a distinction
       between subshell exit code 1, which was used before and indicates
       "tool not found or broken; falling back to default" and other
       (higher) exit codes, which newly mean "something went wrong, stop!".
       The resulting code looks awkward, I can't tell whether I'm missing a
       code or even commenting pattern that would make it clearer.

Unfortunately I didn't get any feedback on this, so I went ahead and
resumitted the change as a non-Draft. I'd be very grateful for any
recommendations on how to handle this better.

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

> If you get it wrong, an inconvenient or even bad
> thing happens: If you're in a GUI context and you forget to say
> `--gui`, then you get the CLI tool - not the end of the world, you
> might argue. If you're in a non-GUI context and you say `--gui`, you
> get a (hopefully intelligible) error about not finding a display, or
> worst case, you hang - and git doesn't know what happened, the
> messaging has to assume you aborted explicitly.

The worst case I can think of is if you have a machine that allows a
persistent virtual graphical session (remote desktop).  You can go
there, open a terminal and create a screen session, which captures
the $DISPLAY in effect.  You may disconnect from the screen session
before you disconnect from the remote desktop.  As the remote
desktop is persistent, the $DISPLAY would be valid even after you
disconnect from there.

Then you ssh into the machine and connect to the screen session.  If
you say "xterm &" in that screen session, a new window with a
terminal will open and be shown on that remote display session,
which you cannot see until you reconnect to the remote desktop.
Running "git mergetool --gui" would be the same.

With the proposed patch, the experience would become even worse in
that scenario, I am afraid.  "git mergetool" (without "--[no-]gui")
would detect the presense of $DISPLAY and start a GUI mergetool that
you cannot view or interact with ;-).  You need to explicitly say
"git mergetool --no-gui" in such a case to force use of cli tool.

Even with that caveat, I think the proposed behaviour would be a net
improvement.  I almost always work in non-GUI environment, but I can
see many people work in both and depending on their environment wish
to use the best tool for the situation.

> If there were a way to get git to autoselect "--gui" by presence of
> the DISPLAY, as it does for the *default* mergetool, then this kind of
> flow would be much more intuitive, usable, etc.
>
> I'm not sure whether I'm explaining better, or just repeating myself,
> so I'll stop here :)

Hopefully your explanation was clear enough.  Others may be able to
offer solution better than "look at DISPLAY and choose --[no-]gui
when neither is given from the command line", but I think at least I
understand your motivation behind this change.

Thanks.

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

[David Aguilar]

On Tue, Apr 4, 2023 at 1:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > If you get it wrong, an inconvenient or even bad
> > thing happens: If you're in a GUI context and you forget to say
> > `--gui`, then you get the CLI tool - not the end of the world, you
> > might argue. If you're in a non-GUI context and you say `--gui`, you
> > get a (hopefully intelligible) error about not finding a display, or
> > worst case, you hang - and git doesn't know what happened, the
> > messaging has to assume you aborted explicitly.
>
> The worst case I can think of is if you have a machine that allows a
> persistent virtual graphical session (remote desktop).  You can go
> there, open a terminal and create a screen session, which captures
> the $DISPLAY in effect.  You may disconnect from the screen session
> before you disconnect from the remote desktop.  As the remote
> desktop is persistent, the $DISPLAY would be valid even after you
> disconnect from there.
>
> Then you ssh into the machine and connect to the screen session.  If
> you say "xterm &" in that screen session, a new window with a
> terminal will open and be shown on that remote display session,
> which you cannot see until you reconnect to the remote desktop.
> Running "git mergetool --gui" would be the same.
>
> With the proposed patch, the experience would become even worse in
> that scenario, I am afraid.  "git mergetool" (without "--[no-]gui")
> would detect the presense of $DISPLAY and start a GUI mergetool that
> you cannot view or interact with ;-).  You need to explicitly say
> "git mergetool --no-gui" in such a case to force use of cli tool.
>
> Even with that caveat, I think the proposed behaviour would be a net
> improvement.  I almost always work in non-GUI environment, but I can
> see many people work in both and depending on their environment wish
> to use the best tool for the situation.
>
> > If there were a way to get git to autoselect "--gui" by presence of
> > the DISPLAY, as it does for the *default* mergetool, then this kind of
> > flow would be much more intuitive, usable, etc.
> >
> > I'm not sure whether I'm explaining better, or just repeating myself,
> > so I'll stop here :)
>
> Hopefully your explanation was clear enough.  Others may be able to
> offer solution better than "look at DISPLAY and choose --[no-]gui
> when neither is given from the command line", but I think at least I
> understand your motivation behind this change.
>
> Thanks.

Thanks for the thorough explanation. With that in mind, this all
sounds good to me.
-- 
David