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

From: Tao Klerks <[email protected]>

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.

---
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(-)

index a3f82112102..8c2f7460be4 100644
@@ -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.

index 90b38097002..bc1a85abc95 100644
@@ -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.

index 9d14c3c9f09..a09dfb072d5 100644
@@ -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.

index c44e205629b..07535f6576e 100644
@@ -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

index 4b10ad1a369..2a7d5ec5f42 100644
@@ -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

index 9f99201bcca..8dbd04e5c5c 100644
@@ -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
 }
 

index f751d9cfe20..f510ff9ea70 100755
@@ -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=
 

index 8cc64729adb..3a807fd681c 100755
@@ -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

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df

gitgitgadget

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.

This somehow felt somewhat familiar, so I had to go the list archive to find https://lore.kernel.org/git/[email protected]/,
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.

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.

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

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.

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.

Sounds good!

Will do.

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

From: Tao Klerks <[email protected]>

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 <[email protected]>, 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.

---
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 <[email protected]>
      
       ## 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 <[email protected]>, 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 <[email protected]>
      
     @@ 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(-)

index a3f82112102..8c2f7460be4 100644
@@ -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.

index 90b38097002..bc1a85abc95 100644
@@ -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.

index 9d14c3c9f09..a09dfb072d5 100644
@@ -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.

index c44e205629b..07535f6576e 100644
@@ -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

index 9f99201bcca..ff08eb7ae93 100644
@@ -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
 }
 

index f751d9cfe20..f510ff9ea70 100755
@@ -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=
 

index 8cc64729adb..3a807fd681c 100755
@@ -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

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df

gitgitgadget

"On Fri, Oct 14, 2022 at 4:07 AM Tao Klerks via GitGitGadget

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?

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

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.

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.

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.

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

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!

This is how it behaves right now, yes.

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.

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

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

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.

Sounds good.

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.

From: Tao Klerks <[email protected]>

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 <[email protected]>, 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.

---
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(-)

index a3f82112102..447c40d85a2 100644
@@ -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.

index 90b38097002..a615bf24187 100644
@@ -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.

index 9d14c3c9f09..ac0ac6fa022 100644
@@ -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.

index c44e205629b..07535f6576e 100644
@@ -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

index 4b10ad1a369..2b89c5dc865 100644
@@ -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

index 992124cc67c..e4e820e6809 100755
@@ -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
 

index 9f99201bcca..1ff26170ffc 100644
@@ -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

index f751d9cfe20..8a922893f75 100755
@@ -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)"

index 8cc64729adb..ba0d2ae76de 100755
@@ -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

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df

gitgitgadget

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

From: Tao Klerks <[email protected]>

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 <[email protected]>, 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.

---
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 <[email protected]>

## 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(-)

index a3f82112102..447c40d85a2 100644
@@ -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.

index e779a122d8a..56a7eeeffb4 100644
@@ -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.

index 9d14c3c9f09..ac0ac6fa022 100644
@@ -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.

index c44e205629b..07535f6576e 100644
@@ -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

index dbbfb19f192..88a00abfbdb 100644
@@ -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

index 992124cc67c..e4e820e6809 100755
@@ -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
 

index 9f99201bcca..1ff26170ffc 100644
@@ -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

index f751d9cfe20..8a922893f75 100755
@@ -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)"

index 7b957022f1a..22b3a85b3e9 100755
@@ -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

base-commit: d15644fe0226af7ffc874572d968598564a230dd

gitgitgadget

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.

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

David

First of all, thank you for reviewing!!

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 :)

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.

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.

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.

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

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

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.

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.

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/[email protected]/
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.

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.

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