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

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