{"thread":{"id":"58611","subject":"[PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","startedAt":"2022-10-12T15:59:57Z","lastAt":"2023-04-06T03:34:19Z","messageCount":23,"participants":["Tao Klerks via GitGitGadget","Tao Klerks","Junio C Hamano","Eric Sunshine","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"464722","messageId":"pull.1381.git.1665590389045.gitgitgadget@gmail.com","threadId":"58611","inReplyTo":null,"subject":"[PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-12T15:59:48Z","receivedAt":"2022-10-12T15:59:57Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nWhen no merge.tool or diff.tool is configured or manually selected, the\nselection of a default tool is sensitive to the DISPLAY variable; in a\nGUI session a gui-specific tool will be proposed if found, and\notherwise a terminal-based one. This \"GUI-optimizing\" behavior is\nimportant because a GUI can make a huge difference to a user's ability\nto understand and correctly complete a non-trivial conflicting merge.\n\nSome time ago the merge.guitool and diff.guitool config options were\nintroduced to enable users to configure both a GUI tool, and a non-GUI\ntool (with fallback if no GUI tool configured), in the same environment.\n\nUnfortunately, the --gui argument introduced to support the selection of\nthe guitool is still explicit. When using configured tools, there is no\nequivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\nenvironment\" behavior.\n\nIntroduce new configuration options, difftool.guiDefault and\nmergetool.guiDefault, supporting a special value \"auto\" which causes the\ncorresponding tool or guitool to be selected depending on the presence of a\nnon-empty DISPLAY value. Also support \"true\" to say \"default to the guitool\n(unless --no-gui is passed on the commandline)\", and \"false\" as the previous\ndefault behavior when these new configuration options are not specified.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    RFC: mergetool: new config guiDefault supports auto-toggling gui by\n    DISPLAY\n    \n    I'm confortable that with this patch we do the right thing, but I'm not\n    sure about a couple of specific code patterns and implementation\n    choices:\n    \n     * In difftool.c, I added a new enum and arg validation function, but\n       didn't know where in the file to place them. Is there any guidance on\n       this that I missed?\n    \n     * In git-mergetool--lib.sh, I used an awkward \"tr\" call to make the\n       config value case-insensitive (to support \"Auto\" for example); Would\n       relying on bash 4's case conversion support be better?\n    \n     * In git-mergetool--lib.sh the way I implemented the \"auto\" special\n       value means that if you put an arbitrary value in the config, eg the\n       typo \"uato\", you get an error about it being an invalid boolean\n       config value; is that OK? Is there a better way to handle \"boolean or\n       special value\" config validation? Are there any examples?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1381\n\n Documentation/config/difftool.txt  |  7 +++++\n Documentation/config/mergetool.txt |  7 +++++\n Documentation/git-difftool.txt     | 10 +++++---\n Documentation/git-mergetool.txt    |  9 ++++---\n builtin/difftool.c                 | 40 ++++++++++++++++++++++++-----\n git-mergetool--lib.sh              | 26 +++++++++++++++++++\n git-mergetool.sh                   |  2 +-\n t/t7610-mergetool.sh               | 39 ++++++++++++++++++++++++++++\n t/t7800-difftool.sh                | 41 ++++++++++++++++++++++++++++++\n 9 files changed, 165 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex a3f82112102..8c2f7460be4 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.guiDefault::\n+\tSet 'true' to use the diff.guitool by default (equivalent to specifying\n+\tthe \"--gui\" argument), or \"auto\" to select diff.guitool or diff.tool\n+\tdepending on the presence of a DISPLAY environment variable value. The\n+\tdefault is 'false', where the \"--gui\" argument must be provided\n+\texplicitly for the diff.guitool to be used.\ndiff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt\nindex 90b38097002..bc1a85abc95 100644\n--- a/Documentation/config/mergetool.txt\n+++ b/Documentation/config/mergetool.txt\n@@ -85,3 +85,10 @@ mergetool.writeToTemp::\n \n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n+\n+mergetool.guiDefault::\n+\tSet 'true' to use the merge.guitool by default (equivalent to\n+\tspecifying the \"--gui\" argument), or \"auto\" to select merge.guitool\n+\tor merge.tool depending on the presence of a DISPLAY environment\n+\tvariable value. The default is 'false', where the \"--gui\" argument\n+\tmust be provided explicitly for the merge.guitool to be used.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 9d14c3c9f09..a09dfb072d5 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.\n --[no-]gui::\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n-\t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting. If `diff.guitool`\n-\tis not set, we will fallback in the order of `merge.guitool`,\n-\t`diff.tool`, `merge.tool` until a tool is found.\n+\t`diff.guitool` variable instead of `diff.tool`. This may be\n+\tautoselected using the configuration variable\n+\t`difftool.guiDefault`. The `--no-gui` option can be used to\n+\toverride these settings. If `diff.guitool` is not set, we will\n+\tfallback in the order of `merge.guitool`, `diff.tool`,\n+\t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex c44e205629b..07535f6576e 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.\n \tthe default merge tool will be read from the configured\n \t`merge.guitool` variable instead of `merge.tool`. If\n \t`merge.guitool` is not set, we will fallback to the tool\n-\tconfigured under `merge.tool`.\n+\tconfigured under `merge.tool`. This may be autoselected using\n+\tthe configuration variable `mergetool.guiDefault`.\n \n --no-gui::\n-\tThis overrides a previous `-g` or `--gui` setting and reads the\n-\tdefault merge tool will be read from the configured `merge.tool`\n-\tvariable.\n+\tThis overrides a previous `-g` or `--gui` setting or\n+\t`mergetool.guiDefault` configuration and reads the default merge\n+\ttool from the configured `merge.tool` variable.\n \n -O<orderfile>::\n \tProcess files in the order specified in the\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a369..2a7d5ec5f42 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -680,14 +680,37 @@ static int run_file_diff(int prompt, const char *prefix,\n \treturn run_command(child);\n }\n \n+enum difftool_gui_mode {\n+\tGUI_DISABLED = -1,\n+\tGUI_BY_CONFIG = 0,\n+\tGUI_ENABLED = 1\n+};\n+\n+static int difftool_opt_gui(const struct option *opt,\n+\t\t\t      const char *optarg, int unset)\n+{\n+\tenum difftool_gui_mode *mode;\n+\tmode = opt->value;\n+\n+\tBUG_ON_OPT_ARG(optarg);\n+\n+\tif (unset)\n+\t\t*mode = GUI_DISABLED;\n+\telse\n+\t\t*mode = GUI_ENABLED;\n+\treturn 0;\n+}\n+\n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n-\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t    tool_help = 0, no_index = 0;\n+\tint dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,\n+\t    no_index = 0;\n+\tenum difftool_gui_mode gui_mode;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n-\t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n-\t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n+\t\tOPT_CALLBACK_F('g', \"gui\", &gui_mode, NULL,\n+\t\t\t       N_(\"use `diff.guitool` instead of `diff.tool`\"),\n+\t\t\t       PARSE_OPT_NOARG, difftool_opt_gui),\n \t\tOPT_BOOL('d', \"dir-diff\", &dir_diff,\n \t\t\t N_(\"perform a full-directory diff\")),\n \t\tOPT_SET_INT_F('y', \"no-prompt\", &prompt,\n@@ -732,13 +755,16 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t} else if (dir_diff)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n \n-\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n+\tdie_for_incompatible_opt3(gui_mode == GUI_ENABLED, \"--gui\",\n \t\t\t\t  !!difftool_cmd, \"--tool\",\n \t\t\t\t  !!extcmd, \"--extcmd\");\n \n-\tif (use_gui_tool)\n+\tif (gui_mode == GUI_ENABLED)\n \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n-\telse if (difftool_cmd) {\n+\telse if (gui_mode == GUI_DISABLED)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"false\", 1);\n+\n+\tif (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n \t\telse\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9f99201bcca..8dbd04e5c5c 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -97,7 +97,33 @@ merge_mode () {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+get_gui_default () {\n+\tif diff_mode\n+\tthen\n+\t\tGUI_DEFAULT_KEY=\"difftool.guiDefault\"\n+\telse\n+\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n+\tfi\n+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')\n+\tif [ \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\" ]\n+\tthen\n+\t\tif [ -n \"$DISPLAY\" ]\n+\t\tthen\n+\t\t\tGUI_DEFAULT=true\n+\t\telse\n+\t\t\tGUI_DEFAULT=false\n+\t\tfi\n+\telse\n+\t\tGUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)\n+\tfi\n+\techo $GUI_DEFAULT\n+}\n+\n gui_mode () {\n+\tif [ -z \"$GIT_MERGETOOL_GUI\" ]\n+\tthen\n+\t\tGIT_MERGETOOL_GUI=$(get_gui_default)\n+\tfi\n \ttest \"$GIT_MERGETOOL_GUI\" = true\n }\n \ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex f751d9cfe20..f510ff9ea70 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -451,7 +451,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tGIT_MERGETOOL_GUI=false\n+\tGIT_MERGETOOL_GUI=\n \tguessed_merge_tool=false\n \torderfile=\n \ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 8cc64729adb..3a807fd681c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '\n \tgit commit -m \"test resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with guiDefault' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge main &&\n+\n+\ttest_config mergetool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\tyes \"\" | git mergetool both &&\n+\tyes \"\" | git mergetool file1 file1 &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\tyes \"\" | git mergetool file2 \"spaced name\" &&\n+\n+\ttest_config mergetool.guiDefault true &&\n+\tyes \"\" | git mergetool subdir/file3 &&\n+\n+\tyes \"d\" | git mergetool file11 &&\n+\tyes \"d\" | git mergetool file12 &&\n+\tyes \"l\" | git mergetool submod &&\n+\n+\n+\techo \"gui main updated\" >expect &&\n+\ttest_cmp expect file1 &&\n+\n+\techo \"main new\" >expect &&\n+\ttest_cmp expect file2 &&\n+\n+\techo \"gui main new sub\" >expect &&\n+\ttest_cmp expect subdir/file3 &&\n+\n+\techo \"branch1 submodule\" >expect &&\n+\ttest_cmp expect submod/bar &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_done\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 096456292c0..ef785c55747 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -155,6 +155,47 @@ test_expect_success 'difftool honors --gui' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault auto &&\n+\tDISPLAY= && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool with guiDefault true selects gui tool' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tDISPLAY=Something && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'difftool --gui last setting wins' '\n \tdifftool_test_setup &&\n \t: >expect &&\n\nbase-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df\n-- \ngitgitgadget\n"},{"id":"464725","messageId":"CAPMMpoiQe+9YEQC4mHwTNt4QGBawKxYv5s0v4k+dkjqtKw6NKg@mail.gmail.com","threadId":"58611","inReplyTo":"pull.1381.git.1665590389045.gitgitgadget@gmail.com","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-12T16:08:06Z","receivedAt":"2022-10-12T16:08:21Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Oct 12, 2022 at 5:59 PM Tao Klerks via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Tao Klerks <tao@klerks.biz>\n>\n> When no merge.tool or diff.tool is configured or manually selected, the\n> selection of a default tool is sensitive to the DISPLAY variable; in a\n> GUI session a gui-specific tool will be proposed if found, and\n> otherwise a terminal-based one. This \"GUI-optimizing\" behavior is\n> important because a GUI can make a huge difference to a user's ability\n> to understand and correctly complete a non-trivial conflicting merge.\n>\n\nQuick clarification - this was intended to be an RFC.\n\nI forgot that with GitGitGadget at the moment (until we do something\nto explicitly support RFCs) a single-commit patch needs the *commit*\nto have the RFC prefix, instead of the Merge Request, as the commit's\nsubject is used instead of the merge request's subject in a\nsingle-commit patch.\n\nI will make sure any later RFC versions have the prefix as appropriate.\n"},{"id":"464734","messageId":"xmqqpmewlxmo.fsf@gitster.g","threadId":"58611","inReplyTo":"pull.1381.git.1665590389045.gitgitgadget@gmail.com","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-12T18:12:15Z","receivedAt":"2022-10-12T18:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Tao Klerks <tao@klerks.biz>\n>\n> When no merge.tool or diff.tool is configured or manually selected, the\n> selection of a default tool is sensitive to the DISPLAY variable; in a\n> GUI session a gui-specific tool will be proposed if found, and\n> otherwise a terminal-based one. This \"GUI-optimizing\" behavior is\n> important because a GUI can make a huge difference to a user's ability\n> to understand and correctly complete a non-trivial conflicting merge.\n>\n> Some time ago the merge.guitool and diff.guitool config options were\n> introduced to enable users to configure both a GUI tool, and a non-GUI\n> tool (with fallback if no GUI tool configured), in the same environment.\n>\n> Unfortunately, the --gui argument introduced to support the selection of\n> the guitool is still explicit. When using configured tools, there is no\n> equivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\n> environment\" behavior.\n>\n> Introduce new configuration options, difftool.guiDefault and\n> mergetool.guiDefault, supporting a special value \"auto\" which causes the\n> corresponding tool or guitool to be selected depending on the presence of a\n> non-empty DISPLAY value. Also support \"true\" to say \"default to the guitool\n> (unless --no-gui is passed on the commandline)\", and \"false\" as the previous\n> default behavior when these new configuration options are not specified.\n>\n> Signed-off-by: Tao Klerks <tao@klerks.biz>\n> ---\n>     RFC: mergetool: new config guiDefault supports auto-toggling gui by\n>     DISPLAY\n\nThis somehow felt somewhat familiar, so I had to go the list archive\nto find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,\nwhich seems to be the previous discussion.\n\nIt would have been much easier if you gave readers the original\ncontext that inspired this design.\n\n"},{"id":"464800","messageId":"CAPMMpojZksy9mvf9kg1qsaUKv6ivc7NPTW7NAaHTrM0D0wQ=Mg@mail.gmail.com","threadId":"58611","inReplyTo":"xmqqpmewlxmo.fsf@gitster.g","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-13T06:49:04Z","receivedAt":"2022-10-13T06:49:53Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Oct 12, 2022 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> This somehow felt somewhat familiar, so I had to go the list archive\n> to find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,\n> which seems to be the previous discussion.\n>\n> It would have been much easier if you gave readers the original\n> context that inspired this design.\n>\n\nMy apologies, I should have included this link in the commit message -\nI tried to \"fix it\" by reviving the thread in question, but clearly\ntoo little too late.\n"},{"id":"464899","messageId":"xmqqedvbcrnn.fsf@gitster.g","threadId":"58611","inReplyTo":"pull.1381.git.1665590389045.gitgitgadget@gmail.com","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-13T21:58:20Z","receivedAt":"2022-10-13T21:58:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +enum difftool_gui_mode {\n> +\tGUI_DISABLED = -1,\n> +\tGUI_BY_CONFIG = 0,\n> +\tGUI_ENABLED = 1\n> +};\n> +\n> +static int difftool_opt_gui(const struct option *opt,\n> +\t\t\t      const char *optarg, int unset)\n> +{\n> +\tenum difftool_gui_mode *mode;\n> +\tmode = opt->value;\n> +\n> +\tBUG_ON_OPT_ARG(optarg);\n> +\n> +\tif (unset)\n> +\t\t*mode = GUI_DISABLED;\n> +\telse\n> +\t\t*mode = GUI_ENABLED;\n> +\treturn 0;\n> +}\n> +\n>  int cmd_difftool(int argc, const char **argv, const char *prefix)\n>  {\n> -\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n> -\t    tool_help = 0, no_index = 0;\n> +\tint dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,\n> +\t    no_index = 0;\n> +\tenum difftool_gui_mode gui_mode;\n\nThis is left uninitialized ...\n\n\n>  \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n>  \tstruct option builtin_difftool_options[] = {\n> -\t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n> -\t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n> +\t\tOPT_CALLBACK_F('g', \"gui\", &gui_mode, NULL,\n> +\t\t\t       N_(\"use `diff.guitool` instead of `diff.tool`\"),\n> +\t\t\t       PARSE_OPT_NOARG, difftool_opt_gui),\n\n... and its address is used here.  When \"--no-gui\" or \"--gui\" option\nis given, the callback function will fill either _DISABLED or _ENABLED\nto it.\n\nBut without any command line argument?  Isn't gui_mode variable ...\n\n>  \t\tOPT_BOOL('d', \"dir-diff\", &dir_diff,\n>  \t\t\t N_(\"perform a full-directory diff\")),\n>  \t\tOPT_SET_INT_F('y', \"no-prompt\", &prompt,\n> @@ -732,13 +755,16 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n>  \t} else if (dir_diff)\n>  \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n>  \n\n... still uninitialized here?  The old use_gui_tool was initialized\nto 0 so it wouldn't have had this problem.\n\n> -\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n> +\tdie_for_incompatible_opt3(gui_mode == GUI_ENABLED, \"--gui\",\n>  \t\t\t\t  !!difftool_cmd, \"--tool\",\n>  \t\t\t\t  !!extcmd, \"--extcmd\");\n>  \n> -\tif (use_gui_tool)\n> +\tif (gui_mode == GUI_ENABLED)\n>  \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n\nI suspect that there is no need to introduce a enum. The flow would\nprobably be\n\n * git_config(difftool_config) would learn to parse the .guiDefault\n   option and initialize use_gui_tool to -1 when set to \"auto\" (and\n   to 0 with \"false\", to 1 with \"true\").\n\n * Call to parse_options() then overwrites use_gui_tool with either\n   0 or 1 when --no-gui or --gui is given.\n\n * After parse_options() returns, use_gui_tool can be examined and\n   when it is 0 or 1, then nothing need to change.  The current code\n   before this patch is doing what the user wants when an explicit\n   command line option is given.\n\n * When use_gui_tool is -1, we need a new code that sets it to\n   either 0 or 1 depending on the running environment.\n\nBut what is curious is that nothing in C code even looks at .guiDefault\nconfiguration, so I am not sure why you would even need to change\nanything in builtin/difftool.c file at all.\n\nPuzzled.\n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 9f99201bcca..8dbd04e5c5c 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -97,7 +97,33 @@ merge_mode () {\n>  \ttest \"$TOOL_MODE\" = merge\n>  }\n>  \n> +get_gui_default () {\n> +\tif diff_mode\n> +\tthen\n> +\t\tGUI_DEFAULT_KEY=\"difftool.guiDefault\"\n> +\telse\n> +\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n> +\tfi\n> +\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')\n\nAvoid [:class:] when 'A-Z' 'a-z' is sufficient.  Easier to read and\nyou do not even need to worry about portability that way.\n\n> +\tif [ \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\" ]\n> +\tthen\n> +\t\tif [ -n \"$DISPLAY\" ]\n> +\t\tthen\n> +\t\t\tGUI_DEFAULT=true\n> +\t\telse\n> +\t\t\tGUI_DEFAULT=false\n> +\t\tfi\n> +\telse\n> +\t\tGUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)\n> +\tfi\n> +\techo $GUI_DEFAULT\n> +}\n\nCheck Documentation/CodingGuidelines with special attention to the\n\"for shell scripts\" section?\n"},{"id":"464907","messageId":"CAPMMpoigKVzhXu1WqSrx13MP1jR0J+ajkwRVVtY_LwHYAoWeTg@mail.gmail.com","threadId":"58611","inReplyTo":"xmqqedvbcrnn.fsf@gitster.g","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-14T04:53:03Z","receivedAt":"2022-10-14T04:53:19Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Thu, Oct 13, 2022 at 11:58 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > +     enum difftool_gui_mode gui_mode;\n>\n> This is left uninitialized ...\n>\n\nThis is clearly a bug, and you've just solved a mystery for me. At one\npoint one test behaved \"crazily\", reporting an error as though --gui\nhad been specified in conflict with another option, despite it not\nhaving been specified at all! I was unable to reproduce this behavior\nin any way, and uncomfortably declared it must have been a \"bit-flip\"\nerror. Now I understand why, and how this class of error can occur in\nC. Thank you for spotting this.\n\nI had incorrectly assumed that it would be implicitly initialized to\n0, meaning GUI_BY_CONFIG. Even if that had been true, it was a mistake\non my part to not be explicit about this default value - I failed to\nuse the enum for its intended purpose of making the code easy to\nunderstand.\n\n>\n> [...] Isn't gui_mode variable ...\n> ... still uninitialized here?  The old use_gui_tool was initialized\n> to 0 so it wouldn't have had this problem.\n\nAbsolutely. This meant that *occasionally*, it would look like --gui\nor --no-gui was specified even though it was not. Most of the time the\nuninitialized value did not exactly match 1 or -1, so it looked like\nthings were working correctly, even though they were doing so\n\"accidentally\".\n\n>\n> > -     die_for_incompatible_opt3(use_gui_tool, \"--gui\",\n> > +     die_for_incompatible_opt3(gui_mode == GUI_ENABLED, \"--gui\",\n> >                                 !!difftool_cmd, \"--tool\",\n> >                                 !!extcmd, \"--extcmd\");\n> >\n> > -     if (use_gui_tool)\n> > +     if (gui_mode == GUI_ENABLED)\n> >               setenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n>\n> I suspect that there is no need to introduce a enum.\n\nAn enum is unnecessary, I only added it in order for a tristate\n\"gui_mode\" value to be legible.\n\nCase in point, you proposed that -1 could mean \"_BY_CONFIG\", whereas I\n(possibly against all prior logic in this codebase) assumed it would\nmake more sense to have -1 be \"_FALSE\" in this case; the enum helps\navoid any confusion. If you find the enum has a nontrivial cost or\nfails to improve understandability I am happy to remove it and use\nsimple int values instead.\n\n> The flow would\n> probably be\n>\n>  * git_config(difftool_config) would learn to parse the .guiDefault\n>    option and initialize use_gui_tool to -1 when set to \"auto\" (and\n>    to 0 with \"false\", to 1 with \"true\").\n>\n>  * Call to parse_options() then overwrites use_gui_tool with either\n>    0 or 1 when --no-gui or --gui is given.\n>\n>  * After parse_options() returns, use_gui_tool can be examined and\n>    when it is 0 or 1, then nothing need to change.  The current code\n>    before this patch is doing what the user wants when an explicit\n>    command line option is given.\n>\n>  * When use_gui_tool is -1, we need a new code that sets it to\n>    either 0 or 1 depending on the running environment.\n>\n> But what is curious is that nothing in C code even looks at .guiDefault\n> configuration, so I am not sure why you would even need to change\n> anything in builtin/difftool.c file at all.\n>\n> Puzzled.\n>\n\nThere are two interesting things here I guess:\n\n1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple\nOPT_BOOL arg type?\nBecause OPT_BOOL always overwrites with either true or false, so in\norder to have use_gui_tool retain its \"_BY_CONFIG\" value in the\nabsence of an arg, the explicit arg handler function is required.\n\n2. Why is the \"auto\" case not explicitly addressed in this code at all?\nBecause both difftool and mergetool use the same tool-finding logic in\ngit-mergetool--lib.sh. The C code is just a (complex) \"shell\", setting\nthe environment such that git-mergetool--lib.sh knows what to do. Wrt\nthe gui decision, the implementation here has it set\n\"GIT_MERGETOOL_GUI\" to \"true\" or \"false\" if the user has made an\nexplicit decision, and if it is blank or unset that means \"use the\nconfig\". The tristate signal in difftool.c is \"GUI, NO_GUI or\nUSE_CONFIG\". It would be possible to parse the config in C and change\nthe tristate signal being passed down to instead be \"GUI, NO_GUI or\nAUTO\", but I believe it would make things more complicated.\n\nI understand \"interesting things that need to be explained\" are\nsomething one tries to avoid in the source code - I will see if adding\na couple of well-placed comments makes things cleaner.\n\n>\n> Avoid [:class:] when 'A-Z' 'a-z' is sufficient.  Easier to read and\n> you do not even need to worry about portability that way.\n>\n\nSounds good!\n\n>\n> Check Documentation/CodingGuidelines with special attention to the\n> \"for shell scripts\" section?\n\nWill do.\n\nAs always, thank you for taking the time to look at this! V2 coming soon.\n"},{"id":"464911","messageId":"pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com","threadId":"58611","inReplyTo":"pull.1381.git.1665590389045.gitgitgadget@gmail.com","subject":"[PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:00:39Z","receivedAt":"2022-10-14T08:00:51Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nWhen no merge.tool or diff.tool is configured or manually selected, the\nselection of a default tool is sensitive to the DISPLAY variable; in a\nGUI session a gui-specific tool will be proposed if found, and\notherwise a terminal-based one. This \"GUI-optimizing\" behavior is\nimportant because a GUI can make a huge difference to a user's ability\nto understand and correctly complete a non-trivial conflicting merge.\n\nSome time ago the merge.guitool and diff.guitool config options were\nintroduced to enable users to configure both a GUI tool, and a non-GUI\ntool (with fallback if no GUI tool configured), in the same environment.\n\nUnfortunately, the --gui argument introduced to support the selection of\nthe guitool is still explicit. When using configured tools, there is no\nequivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\nenvironment\" behavior.\n\nAs proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\noptions, difftool.guiDefault and mergetool.guiDefault, supporting a special\nvalue \"auto\" which causes the corresponding tool or guitool to be selected\ndepending on the presence of a non-empty DISPLAY value. Also support \"true\"\nto say \"default to the guitool (unless --no-gui is passed on the\ncommandline)\", and \"false\" as the previous default behavior when these new\nconfiguration options are not specified.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    RFC: mergetool: new config guiDefault supports auto-toggling gui by\n    DISPLAY\n    \n    I'm reasonably comfortable that with this patch we do the right thing,\n    but I'm not sure about one remaining implementation choice:\n    \n     * In git-mergetool--lib.sh the way I implemented the \"auto\" special\n       value means that if you put an arbitrary value in the config, eg the\n       typo \"uato\", you get an error about it being an invalid boolean\n       config value; is that OK? Is there a better way to handle \"boolean or\n       special value\" config validation? Are there any examples?\n    \n    V2:\n    \n     * Fix uninitialized variable that caused buggy behavior and lack of\n       clarity\n     * Fix enum values to better match Junio's expectations\n     * Add comment about relationship between C and shell scripts\n     * Fix \"tr\" call to use simple ANSI ranges rather than character classes\n     * Fix shell test to use \"test\" rather than brackets\n     * Add better test coverage by using case-inconsistent \"Auto\" in test\n     * Add test to validate command-line override of configured default in\n       difftool\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1381\n\nRange-diff vs v1:\n\n 1:  4e3495343db ! 1:  70d0e0d09fa mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n     @@ Metadata\n      Author: Tao Klerks <tao@klerks.biz>\n      \n       ## Commit message ##\n     -    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n     +    RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n      \n          When no merge.tool or diff.tool is configured or manually selected, the\n          selection of a default tool is sensitive to the DISPLAY variable; in a\n     @@ Commit message\n          equivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\n          environment\" behavior.\n      \n     -    Introduce new configuration options, difftool.guiDefault and\n     -    mergetool.guiDefault, supporting a special value \"auto\" which causes the\n     -    corresponding tool or guitool to be selected depending on the presence of a\n     -    non-empty DISPLAY value. Also support \"true\" to say \"default to the guitool\n     -    (unless --no-gui is passed on the commandline)\", and \"false\" as the previous\n     -    default behavior when these new configuration options are not specified.\n     +    As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\n     +    options, difftool.guiDefault and mergetool.guiDefault, supporting a special\n     +    value \"auto\" which causes the corresponding tool or guitool to be selected\n     +    depending on the presence of a non-empty DISPLAY value. Also support \"true\"\n     +    to say \"default to the guitool (unless --no-gui is passed on the\n     +    commandline)\", and \"false\" as the previous default behavior when these new\n     +    configuration options are not specified.\n      \n          Signed-off-by: Tao Klerks <tao@klerks.biz>\n      \n     @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,\n       }\n       \n      +enum difftool_gui_mode {\n     -+\tGUI_DISABLED = -1,\n     -+\tGUI_BY_CONFIG = 0,\n     ++\tGUI_BY_CONFIG = -1,\n     ++\tGUI_DISABLED = 0,\n      +\tGUI_ENABLED = 1\n      +};\n      +\n     @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,\n      -\t    tool_help = 0, no_index = 0;\n      +\tint dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,\n      +\t    no_index = 0;\n     -+\tenum difftool_gui_mode gui_mode;\n     ++\tenum difftool_gui_mode gui_mode = GUI_BY_CONFIG;\n       \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n       \tstruct option builtin_difftool_options[] = {\n      -\t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr\n       \t\t\t\t  !!extcmd, \"--extcmd\");\n       \n      -\tif (use_gui_tool)\n     ++\t/*\n     ++\t * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;\n     ++\t * empty or unset means \"use the difftool.guiDefault config or default to\n     ++\t * false\".\n     ++\t */\n      +\tif (gui_mode == GUI_ENABLED)\n       \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n      -\telse if (difftool_cmd) {\n     @@ git-mergetool--lib.sh: merge_mode () {\n      +\telse\n      +\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n      +\tfi\n     -+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr '[:upper:]' '[:lower:]')\n     -+\tif [ \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\" ]\n     ++\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')\n     ++\tif test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n      +\tthen\n     -+\t\tif [ -n \"$DISPLAY\" ]\n     ++\t\tif test -n \"$DISPLAY\"\n      +\t\tthen\n      +\t\t\tGUI_DEFAULT=true\n      +\t\telse\n     @@ t/t7800-difftool.sh: test_expect_success 'difftool honors --gui' '\n      +\tdifftool_test_setup &&\n      +\ttest_config diff.guitool bogus-tool &&\n      +\ttest_config diff.tool test-tool &&\n     -+\ttest_config difftool.guiDefault auto &&\n     ++\ttest_config difftool.guiDefault Auto &&\n      +\tDISPLAY= && export DISPLAY &&\n      +\n      +\techo branch >expect &&\n     @@ t/t7800-difftool.sh: test_expect_success 'difftool honors --gui' '\n      +\tgit difftool --no-prompt branch >actual &&\n      +\ttest_cmp expect actual\n      +'\n     ++\n     ++test_expect_success 'difftool --no-gui trumps config guiDefault' '\n     ++\tdifftool_test_setup &&\n     ++\ttest_config diff.guitool bogus-tool &&\n     ++\ttest_config diff.tool test-tool &&\n     ++\ttest_config difftool.guiDefault true &&\n     ++\n     ++\techo branch >expect &&\n     ++\tgit difftool --no-prompt --no-gui branch >actual &&\n     ++\ttest_cmp expect actual\n     ++'\n      +\n       test_expect_success 'difftool --gui last setting wins' '\n       \tdifftool_test_setup &&\n\n\n Documentation/config/difftool.txt  |  7 ++++\n Documentation/config/mergetool.txt |  7 ++++\n Documentation/git-difftool.txt     | 10 +++---\n Documentation/git-mergetool.txt    |  9 +++---\n builtin/difftool.c                 | 45 ++++++++++++++++++++++----\n git-mergetool--lib.sh              | 26 +++++++++++++++\n git-mergetool.sh                   |  2 +-\n t/t7610-mergetool.sh               | 39 ++++++++++++++++++++++\n t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++\n 9 files changed, 181 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex a3f82112102..8c2f7460be4 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.guiDefault::\n+\tSet 'true' to use the diff.guitool by default (equivalent to specifying\n+\tthe \"--gui\" argument), or \"auto\" to select diff.guitool or diff.tool\n+\tdepending on the presence of a DISPLAY environment variable value. The\n+\tdefault is 'false', where the \"--gui\" argument must be provided\n+\texplicitly for the diff.guitool to be used.\ndiff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt\nindex 90b38097002..bc1a85abc95 100644\n--- a/Documentation/config/mergetool.txt\n+++ b/Documentation/config/mergetool.txt\n@@ -85,3 +85,10 @@ mergetool.writeToTemp::\n \n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n+\n+mergetool.guiDefault::\n+\tSet 'true' to use the merge.guitool by default (equivalent to\n+\tspecifying the \"--gui\" argument), or \"auto\" to select merge.guitool\n+\tor merge.tool depending on the presence of a DISPLAY environment\n+\tvariable value. The default is 'false', where the \"--gui\" argument\n+\tmust be provided explicitly for the merge.guitool to be used.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 9d14c3c9f09..a09dfb072d5 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.\n --[no-]gui::\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n-\t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting. If `diff.guitool`\n-\tis not set, we will fallback in the order of `merge.guitool`,\n-\t`diff.tool`, `merge.tool` until a tool is found.\n+\t`diff.guitool` variable instead of `diff.tool`. This may be\n+\tautoselected using the configuration variable\n+\t`difftool.guiDefault`. The `--no-gui` option can be used to\n+\toverride these settings. If `diff.guitool` is not set, we will\n+\tfallback in the order of `merge.guitool`, `diff.tool`,\n+\t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex c44e205629b..07535f6576e 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.\n \tthe default merge tool will be read from the configured\n \t`merge.guitool` variable instead of `merge.tool`. If\n \t`merge.guitool` is not set, we will fallback to the tool\n-\tconfigured under `merge.tool`.\n+\tconfigured under `merge.tool`. This may be autoselected using\n+\tthe configuration variable `mergetool.guiDefault`.\n \n --no-gui::\n-\tThis overrides a previous `-g` or `--gui` setting and reads the\n-\tdefault merge tool will be read from the configured `merge.tool`\n-\tvariable.\n+\tThis overrides a previous `-g` or `--gui` setting or\n+\t`mergetool.guiDefault` configuration and reads the default merge\n+\ttool from the configured `merge.tool` variable.\n \n -O<orderfile>::\n \tProcess files in the order specified in the\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a369..374bb7df34f 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -680,14 +680,37 @@ static int run_file_diff(int prompt, const char *prefix,\n \treturn run_command(child);\n }\n \n+enum difftool_gui_mode {\n+\tGUI_BY_CONFIG = -1,\n+\tGUI_DISABLED = 0,\n+\tGUI_ENABLED = 1\n+};\n+\n+static int difftool_opt_gui(const struct option *opt,\n+\t\t\t      const char *optarg, int unset)\n+{\n+\tenum difftool_gui_mode *mode;\n+\tmode = opt->value;\n+\n+\tBUG_ON_OPT_ARG(optarg);\n+\n+\tif (unset)\n+\t\t*mode = GUI_DISABLED;\n+\telse\n+\t\t*mode = GUI_ENABLED;\n+\treturn 0;\n+}\n+\n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n-\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t    tool_help = 0, no_index = 0;\n+\tint dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,\n+\t    no_index = 0;\n+\tenum difftool_gui_mode gui_mode = GUI_BY_CONFIG;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n-\t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n-\t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n+\t\tOPT_CALLBACK_F('g', \"gui\", &gui_mode, NULL,\n+\t\t\t       N_(\"use `diff.guitool` instead of `diff.tool`\"),\n+\t\t\t       PARSE_OPT_NOARG, difftool_opt_gui),\n \t\tOPT_BOOL('d', \"dir-diff\", &dir_diff,\n \t\t\t N_(\"perform a full-directory diff\")),\n \t\tOPT_SET_INT_F('y', \"no-prompt\", &prompt,\n@@ -732,13 +755,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t} else if (dir_diff)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n \n-\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n+\tdie_for_incompatible_opt3(gui_mode == GUI_ENABLED, \"--gui\",\n \t\t\t\t  !!difftool_cmd, \"--tool\",\n \t\t\t\t  !!extcmd, \"--extcmd\");\n \n-\tif (use_gui_tool)\n+\t/*\n+\t * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;\n+\t * empty or unset means \"use the difftool.guiDefault config or default to\n+\t * false\".\n+\t */\n+\tif (gui_mode == GUI_ENABLED)\n \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n-\telse if (difftool_cmd) {\n+\telse if (gui_mode == GUI_DISABLED)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"false\", 1);\n+\n+\tif (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n \t\telse\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9f99201bcca..ff08eb7ae93 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -97,7 +97,33 @@ merge_mode () {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+get_gui_default () {\n+\tif diff_mode\n+\tthen\n+\t\tGUI_DEFAULT_KEY=\"difftool.guiDefault\"\n+\telse\n+\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n+\tfi\n+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')\n+\tif test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n+\tthen\n+\t\tif test -n \"$DISPLAY\"\n+\t\tthen\n+\t\t\tGUI_DEFAULT=true\n+\t\telse\n+\t\t\tGUI_DEFAULT=false\n+\t\tfi\n+\telse\n+\t\tGUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)\n+\tfi\n+\techo $GUI_DEFAULT\n+}\n+\n gui_mode () {\n+\tif [ -z \"$GIT_MERGETOOL_GUI\" ]\n+\tthen\n+\t\tGIT_MERGETOOL_GUI=$(get_gui_default)\n+\tfi\n \ttest \"$GIT_MERGETOOL_GUI\" = true\n }\n \ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex f751d9cfe20..f510ff9ea70 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -451,7 +451,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tGIT_MERGETOOL_GUI=false\n+\tGIT_MERGETOOL_GUI=\n \tguessed_merge_tool=false\n \torderfile=\n \ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 8cc64729adb..3a807fd681c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '\n \tgit commit -m \"test resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with guiDefault' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge main &&\n+\n+\ttest_config mergetool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\tyes \"\" | git mergetool both &&\n+\tyes \"\" | git mergetool file1 file1 &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\tyes \"\" | git mergetool file2 \"spaced name\" &&\n+\n+\ttest_config mergetool.guiDefault true &&\n+\tyes \"\" | git mergetool subdir/file3 &&\n+\n+\tyes \"d\" | git mergetool file11 &&\n+\tyes \"d\" | git mergetool file12 &&\n+\tyes \"l\" | git mergetool submod &&\n+\n+\n+\techo \"gui main updated\" >expect &&\n+\ttest_cmp expect file1 &&\n+\n+\techo \"main new\" >expect &&\n+\ttest_cmp expect file2 &&\n+\n+\techo \"gui main new sub\" >expect &&\n+\ttest_cmp expect subdir/file3 &&\n+\n+\techo \"branch1 submodule\" >expect &&\n+\ttest_cmp expect submod/bar &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_done\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 096456292c0..1e47d3c05bd 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault Auto &&\n+\tDISPLAY= && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool with guiDefault true selects gui tool' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tDISPLAY=Something && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool --no-gui trumps config guiDefault' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --no-gui branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'difftool --gui last setting wins' '\n \tdifftool_test_setup &&\n \t: >expect &&\n\nbase-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df\n-- \ngitgitgadget\n"},{"id":"464914","messageId":"CAPig+cSPWsiKBU6hgvP_BO7n23nLf7Q-59ZbO4fSL0idcw2qzA@mail.gmail.com","threadId":"58611","inReplyTo":"pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-14T08:24:16Z","receivedAt":"2022-10-14T08:24:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"On Fri, Oct 14, 2022 at 4:07 AM Tao Klerks via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> [...]\n> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\n> options, difftool.guiDefault and mergetool.guiDefault, supporting a special\n> value \"auto\" which causes the corresponding tool or guitool to be selected\n> depending on the presence of a non-empty DISPLAY value. Also support \"true\"\n> to say \"default to the guitool (unless --no-gui is passed on the\n> commandline)\", and \"false\" as the previous default behavior when these new\n> configuration options are not specified.\n>\n> Signed-off-by: Tao Klerks <tao@klerks.biz>\n> ---\n> diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\n> @@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.\n> +difftool.guiDefault::\n> +       Set 'true' to use the diff.guitool by default (equivalent to specifying\n> +       the \"--gui\" argument), or \"auto\" to select diff.guitool or diff.tool\n> +       depending on the presence of a DISPLAY environment variable value. The\n> +       default is 'false', where the \"--gui\" argument must be provided\n> +       explicitly for the diff.guitool to be used.\n\nLet's use backticks rather than double-quotes to ensure that these get\ntypeset similar to other documentation; i.e. `--gui`, `auto`,\n`diff.guitool`, `diff.tool`, `DISPLAY`.\n\n> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt\n> @@ -85,3 +85,10 @@ mergetool.writeToTemp::\n> +mergetool.guiDefault::\n> +       Set 'true' to use the merge.guitool by default (equivalent to\n> +       specifying the \"--gui\" argument), or \"auto\" to select merge.guitool\n> +       or merge.tool depending on the presence of a DISPLAY environment\n> +       variable value. The default is 'false', where the \"--gui\" argument\n> +       must be provided explicitly for the merge.guitool to be used.\n\nDitto.\n\n> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\n> @@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.\n>  --[no-]gui::\n>         When 'git-difftool' is invoked with the `-g` or `--gui` option\n>         the default diff tool will be read from the configured\n> +       `diff.guitool` variable instead of `diff.tool`. This may be\n> +       autoselected using the configuration variable\n> +       `difftool.guiDefault`. The `--no-gui` option can be used to\n> +       override these settings. If `diff.guitool` is not set, we will\n> +       fallback in the order of `merge.guitool`, `diff.tool`,\n> +       `merge.tool` until a tool is found.\n\nCorrect use of backticks here. Good.\n\nProbably want: s/autoselected/auto-selected/ or /.../selected automatically/\n\n> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\n> @@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.\n> +       configured under `merge.tool`. This may be autoselected using\n> +       the configuration variable `mergetool.guiDefault`.\n\nDitto: \"autoselected\"\n\n>  --no-gui::\n> +       This overrides a previous `-g` or `--gui` setting or\n> +       `mergetool.guiDefault` configuration and reads the default merge\n> +       tool from the configured `merge.tool` variable.\n\nBackticks; good.\n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> @@ -97,7 +97,33 @@ merge_mode () {\n> +get_gui_default () {\n> +       if diff_mode\n> +       then\n> +               GUI_DEFAULT_KEY=\"difftool.guiDefault\"\n> +       else\n> +               GUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n> +       fi\n> +       GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')\n\nToo many spaces before pipe symbol.\n\nNit: It doesn't matter in this case, but you could safeguard against\n(possible?) future problems by using double-quotes in `--get\n\"$GUI_DEFAULT_KEY\"`.\n\n> +       if test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n> +       then\n> +               if test -n \"$DISPLAY\"\n> +               then\n> +                       GUI_DEFAULT=true\n> +               else\n> +                       GUI_DEFAULT=false\n> +               fi\n> +       else\n> +               GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)\n\nDitto: `--get \"$GUI_DEFAULT_KEY\"`\n\n> +       fi\n> +       echo $GUI_DEFAULT\n> +}\n> +\n>  gui_mode () {\n> +       if [ -z \"$GIT_MERGETOOL_GUI\" ]\n\nStyle: if test -z \"$GIT_MERGETOOL_GUI\"\n\n> +       then\n> +               GIT_MERGETOOL_GUI=$(get_gui_default)\n> +       fi\n>         test \"$GIT_MERGETOOL_GUI\" = true\n>  }\n>\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> @@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' '\n> +test_expect_success 'mergetool with guiDefault' '\n> +       test_config mergetool.guiDefault true &&\n> +       yes \"\" | git mergetool subdir/file3 &&\n> +\n> +       yes \"d\" | git mergetool file11 &&\n> +       yes \"d\" | git mergetool file12 &&\n> +       yes \"l\" | git mergetool submod &&\n> +\n> +\n> +       echo \"gui main updated\" >expect &&\n> +       test_cmp expect file1 &&\n\nAccidental double-spacing?\n"},{"id":"464928","messageId":"xmqqsfjqbwil.fsf@gitster.g","threadId":"58611","inReplyTo":"CAPMMpoigKVzhXu1WqSrx13MP1jR0J+ajkwRVVtY_LwHYAoWeTg@mail.gmail.com","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T09:10:58Z","receivedAt":"2022-10-14T09:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple\n> OPT_BOOL arg type?\n> Because OPT_BOOL always overwrites with either true or false, so in\n> order to have use_gui_tool retain its \"_BY_CONFIG\" value in the\n> absence of an arg, the explicit arg handler function is required.\n\nThat is exactly why you want to initialize the variable to something\nother than 0 or 1 (say \"-1\") and use that as \"not explicitly\nspecified from the command line\" aka _BY_CONFIG value, isn't it?\n\nAnd once you do so, you do not need callback and you do not need\nenum.  The original \"int\" can take tri-state, (unspecified = -1,\nfalse = 0, true = 1).\n\nAm I missing something?\n"},{"id":"464929","messageId":"CAPMMpogj50aWGdvs_gAJJvft+TOQXKvZm7+Y5yDgrCavjNvABA@mail.gmail.com","threadId":"58611","inReplyTo":"CAPig+cSPWsiKBU6hgvP_BO7n23nLf7Q-59ZbO4fSL0idcw2qzA@mail.gmail.com","subject":"Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-14T09:11:31Z","receivedAt":"2022-10-14T09:11:47Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Oct 14, 2022 at 10:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> backticks\n> selected automatically\n> spaces\n> test\n\nAck on all these, my apologies, and thank you!\n"},{"id":"464935","messageId":"CAPMMpoiwnLyqzBBubkgSpjyJQqSeEVdoZy72FAwGDbMgg9UXxg@mail.gmail.com","threadId":"58611","inReplyTo":"xmqqsfjqbwil.fsf@gitster.g","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-14T12:32:47Z","receivedAt":"2022-10-14T12:33:03Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Oct 14, 2022 at 11:11 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple\n> > OPT_BOOL arg type?\n> > Because OPT_BOOL always overwrites with either true or false, so in\n> > order to have use_gui_tool retain its \"_BY_CONFIG\" value in the\n> > absence of an arg, the explicit arg handler function is required.\n>\n> That is exactly why you want to initialize the variable to something\n> other than 0 or 1 (say \"-1\") and use that as \"not explicitly\n> specified from the command line\" aka _BY_CONFIG value, isn't it?\n>\n> And once you do so, you do not need callback and you do not need\n> enum.  The original \"int\" can take tri-state, (unspecified = -1,\n> false = 0, true = 1).\n>\n> Am I missing something?\n\nHeh, I guess one of us must be, and it's probably me :)\n\nMy understanding, from ad-hoc testing, is that using OPT_BOOL causes\nthe target variable (\"use_gui_tool\" in this case) to *always* be set\nto either \"1\" or \"0\", regardless of whether the user sets the main\nflag, the --no form of the flag, or sets nothing at all. As far as I\ncan tell, OPT_BOOL never leaves the original value, so it cannot be\nused to track tri-state \"true, false or not specified\" logic. Maybe I\nmade a mistake somewhere, and that's not really or always how it\nbehaves.\n\nI can re-test, and/or try to dig into the option parsing logic to\nunderstand how it works - the time I tried, I got hopelessly lost and\nwas relieved to find that OPT_CALLBACK_F existed and seemed to be used\nfor exactly these kinds of use cases.\n"},{"id":"464949","messageId":"xmqqilkmbelx.fsf@gitster.g","threadId":"58611","inReplyTo":"CAPMMpoiwnLyqzBBubkgSpjyJQqSeEVdoZy72FAwGDbMgg9UXxg@mail.gmail.com","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T15:37:46Z","receivedAt":"2022-10-14T15:37:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> My understanding, from ad-hoc testing, is that using OPT_BOOL causes\n> the target variable (\"use_gui_tool\" in this case) to *always* be set\n> to either \"1\" or \"0\", regardless of whether the user sets the main\n> flag, the --no form of the flag, or sets nothing at all.\n\nYou are saying \"git gc --keep-largest-pack\" is broken ;-)\n\nOPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there\nis no \"set to this default value if no command line option is given\"\ninvolved.  At least I do not recall allowing such a code in.\n\nThanks.\n"},{"id":"464961","messageId":"xmqqczaube8o.fsf@gitster.g","threadId":"58611","inReplyTo":"pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T15:45:43Z","receivedAt":"2022-10-14T15:45:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>      * In git-mergetool--lib.sh the way I implemented the \"auto\" special\n>        value means that if you put an arbitrary value in the config, eg the\n>        typo \"uato\", you get an error about it being an invalid boolean\n>        config value; is that OK? Is there a better way to handle \"boolean or\n>        special value\" config validation? Are there any examples?\n\nI think the ideal behaviour would be:\n\n * Unless running difftool and difftool.guiChoice has a wrong value,\n   or running mergetool and mergetool.guiChoice has a wrong value,\n   we should not even complain.\n\n * If the command line says --gui or --no-gui that makes the setting\n   irrelevant, it is OK for us to give a warning to remind the user\n   that they may want to fix the spelling of the variable, but\n   otherwise go ahead and perform the action as they asked us to.\n\n * If the command line lacks --gui or --no-gui, we do need to have a\n   usable value in the configuration, and we should error out\n   without spawning either gui or no-gui tool backend.\n\nIt may be usable without the second one and always fail difftool and\nmergetool until the setting gets fixed, but that is less than ideal.\nWe do allow less than ideal code in, as long as it is an improvement\nover the status quo, and its presence does not make it harder to\nlater get closer to the ideal.\n\nThanks.\n"},{"id":"465044","messageId":"CAPMMpohBOk2eQMbMxJhjr7cezrhMxVfaS--Aq8jJ1e-LQbEjrw@mail.gmail.com","threadId":"58611","inReplyTo":"xmqqilkmbelx.fsf@gitster.g","subject":"Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-16T20:07:38Z","receivedAt":"2022-10-16T20:07:55Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Oct 14, 2022 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > My understanding, from ad-hoc testing, is that using OPT_BOOL causes\n> > the target variable (\"use_gui_tool\" in this case) to *always* be set\n> > to either \"1\" or \"0\", regardless of whether the user sets the main\n> > flag, the --no form of the flag, or sets nothing at all.\n>\n> You are saying \"git gc --keep-largest-pack\" is broken ;-)\n\nNow that I look at that, that does appear to have been implied by what\nI was saying, yes!\n\n>\n> OPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there\n> is no \"set to this default value if no command line option is given\"\n> involved.  At least I do not recall allowing such a code in.\n>\n\nI can confirm that it works the way you expected. I was sure I had\nexperienced such behavior during my early attempts at tristate\nhandling here, but in retrospect I was very much mistaken, not only\nbecause --keep-largest-pack is not broken, but also because I just\nripped out all the arg-parsing function and enum stuff, and it all\nworks fine with -1, 0 and 1 as you pointed out it should. I don't know\nexactly how I messed up my early testing, but I don't suppose it\nmatters.\n\nSubstantial simplification in the next update.\n\nThanks!\n"},{"id":"465045","messageId":"CAPMMpohWE17xk4fF+uQja0jfn53bjgVtg75FKEC1w0Km7U8Dvg@mail.gmail.com","threadId":"58611","inReplyTo":"xmqqczaube8o.fsf@gitster.g","subject":"Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-10-16T20:19:22Z","receivedAt":"2022-10-16T20:19:39Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Oct 14, 2022 at 5:45 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> >      * In git-mergetool--lib.sh the way I implemented the \"auto\" special\n> >        value means that if you put an arbitrary value in the config, eg the\n> >        typo \"uato\", you get an error about it being an invalid boolean\n> >        config value; is that OK? Is there a better way to handle \"boolean or\n> >        special value\" config validation? Are there any examples?\n>\n> I think the ideal behaviour would be:\n>\n>  * Unless running difftool and difftool.guiChoice has a wrong value,\n>    or running mergetool and mergetool.guiChoice has a wrong value,\n>    we should not even complain.\n\nThis is how it behaves right now, yes.\n\n>\n>  * If the command line says --gui or --no-gui that makes the setting\n>    irrelevant, it is OK for us to give a warning to remind the user\n>    that they may want to fix the spelling of the variable, but\n>    otherwise go ahead and perform the action as they asked us to.\n\nIn the current implementation, there is no warning if the choice has\nbeen explicitly made - there is no reason to check the configured\ndefault under such circumstances.\n\n>\n>  * If the command line lacks --gui or --no-gui, we do need to have a\n>    usable value in the configuration, and we should error out\n>    without spawning either gui or no-gui tool backend.\n\nThis is *not* the current behavior - currently an error is printed,\nbut execution continues with a no-gui default. I will correct this.\n\n>\n> It may be usable without the second one and always fail difftool and\n> mergetool until the setting gets fixed, but that is less than ideal.\n> We do allow less than ideal code in, as long as it is an improvement\n> over the status quo, and its presence does not make it harder to\n> later get closer to the ideal.\n\nThanks for the logic table, sounds good.\n\nHowever, my concern was with the precise form of the error if a\nconfigured value is neither trueish, not falseish, nor a case-tolerant\nform of \"auto\": under such circumstances, instead of a dedicated error\nmessage along the lines of \"fatal: bad config value 'autod' for\n'difftool.guidefault' - must be boolean or 'auto'\", we get the\n*default* error message for *boolean* config settings: \"fatal: bad\nboolean config value 'autod' for 'difftool.guidefault'\". This can of\ncourse be corrected, I just need to work out how, and exactly what\nform the error message should then take.\n\nShould we be elaborating an error message for this specific tristate\nconfig value, or is it \"normal\" fail with a slightly-misleading\nmessage suggesting it is a simple boolean config in such cases?\n\nThanks,\nTao\n"},{"id":"465066","messageId":"xmqqa65v2e2l.fsf@gitster.g","threadId":"58611","inReplyTo":"CAPMMpohWE17xk4fF+uQja0jfn53bjgVtg75FKEC1w0Km7U8Dvg@mail.gmail.com","subject":"Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-17T05:50:58Z","receivedAt":"2022-10-17T05:51:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n>>  * If the command line says --gui or --no-gui that makes the setting\n>>    irrelevant, it is OK for us to give a warning to remind the user\n>>    that they may want to fix the spelling of the variable, but\n>>    otherwise go ahead and perform the action as they asked us to.\n>\n> In the current implementation, there is no warning if the choice has\n> been explicitly made - there is no reason to check the configured\n> default under such circumstances.\n\nYeah, not making any noise is better.  I only meant that if the\nimplementation warns in this irrelevant case, it is OK (aka\n\"acceptable, not necessarily desired\") as long as it does not stop.\n\n>>  * If the command line lacks --gui or --no-gui, we do need to have a\n>>    usable value in the configuration, and we should error out\n>>    without spawning either gui or no-gui tool backend.\n>\n> This is *not* the current behavior - currently an error is printed,\n> but execution continues with a no-gui default. I will correct this.\n\nSounds good.\n\n> 'difftool.guidefault' - must be boolean or 'auto'\", we get the\n> *default* error message for *boolean* config settings: \"fatal: bad\n> boolean config value 'autod' for 'difftool.guidefault'\".\n\nYeah, I do not think it is a problem, and it is not misleading, as\nlong as the user knows how to ask further information with \"git\ndifftool --help\" and the help page says what the acceptable values\nare other than Boolean yes/no.\n\nThanks.\n\n"},{"id":"465175","messageId":"pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com","threadId":"58611","inReplyTo":"pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com","subject":"[PATCH v3] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-18T06:54:46Z","receivedAt":"2022-10-18T06:54:57Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nWhen no merge.tool or diff.tool is configured or manually selected, the\nselection of a default tool is sensitive to the DISPLAY variable; in a\nGUI session a gui-specific tool will be proposed if found, and\notherwise a terminal-based one. This \"GUI-optimizing\" behavior is\nimportant because a GUI can make a huge difference to a user's ability\nto understand and correctly complete a non-trivial conflicting merge.\n\nSome time ago the merge.guitool and diff.guitool config options were\nintroduced to enable users to configure both a GUI tool, and a non-GUI\ntool (with fallback if no GUI tool configured), in the same environment.\n\nUnfortunately, the --gui argument introduced to support the selection of\nthe guitool is still explicit. When using configured tools, there is no\nequivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\nenvironment\" behavior.\n\nAs proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\noptions, difftool.guiDefault and mergetool.guiDefault, supporting a special\nvalue \"auto\" which causes the corresponding tool or guitool to be selected\ndepending on the presence of a non-empty DISPLAY value. Also support \"true\"\nto say \"default to the guitool (unless --no-gui is passed on the\ncommandline)\", and \"false\" as the previous default behavior when these new\nconfiguration options are not specified.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    RFC: mergetool: new config guiDefault supports auto-toggling gui by\n    DISPLAY\n    \n    I'm reasonably comfortable that with this patch we do the right thing,\n    but I'm not sure about yet another remaining implementation detail:\n    \n     * After implementing Junio's recommended \"fail if defaulting config is\n       consulted and is invalid\" flow, there now needs to be a distinction\n       between subshell exit code 1, which was used before and indicates\n       \"tool not found or broken; falling back to default\" and other\n       (higher) exit codes, which newly mean \"something went wrong, stop!\".\n       The resulting code looks awkward, I can't tell whether I'm missing a\n       code or even commenting pattern that would make it clearer.\n    \n    V3:\n    \n     * Simplify C code to use OPT_BOOL with an int rather than a custom\n       option-parsing function with an enum\n     * Fix doc to more extensively use backticks for config keys / values /\n       args\n     * Fix more shell script formatting issues\n     * Change error-handling in mergetool and difftool helpers to exit if\n       defaulting config is invalid\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1381\n\nRange-diff vs v2:\n\n 1:  70d0e0d09fa ! 1:  dbb80499218 RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n     @@ Documentation/config/difftool.txt: See the `--trust-exit-code` option in linkgit\n       \tPrompt before each invocation of the diff tool.\n      +\n      +difftool.guiDefault::\n     -+\tSet 'true' to use the diff.guitool by default (equivalent to specifying\n     -+\tthe \"--gui\" argument), or \"auto\" to select diff.guitool or diff.tool\n     -+\tdepending on the presence of a DISPLAY environment variable value. The\n     -+\tdefault is 'false', where the \"--gui\" argument must be provided\n     -+\texplicitly for the diff.guitool to be used.\n     ++\tSet `true` to use the `diff.guitool` by default (equivalent to specifying\n     ++\tthe `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`\n     ++\tdepending on the presence of a `DISPLAY` environment variable value. The\n     ++\tdefault is `false`, where the `--gui` argument must be provided\n     ++\texplicitly for the `diff.guitool` to be used.\n      \n       ## Documentation/config/mergetool.txt ##\n      @@ Documentation/config/mergetool.txt: mergetool.writeToTemp::\n     @@ Documentation/config/mergetool.txt: mergetool.writeToTemp::\n       \tPrompt before each invocation of the merge resolution program.\n      +\n      +mergetool.guiDefault::\n     -+\tSet 'true' to use the merge.guitool by default (equivalent to\n     -+\tspecifying the \"--gui\" argument), or \"auto\" to select merge.guitool\n     -+\tor merge.tool depending on the presence of a DISPLAY environment\n     -+\tvariable value. The default is 'false', where the \"--gui\" argument\n     -+\tmust be provided explicitly for the merge.guitool to be used.\n     ++\tSet `true` to use the `merge.guitool` by default (equivalent to\n     ++\tspecifying the `--gui` argument), or `auto` to select `merge.guitool`\n     ++\tor `merge.tool` depending on the presence of a `DISPLAY` environment\n     ++\tvariable value. The default is `false`, where the `--gui` argument\n     ++\tmust be provided explicitly for the `merge.guitool` to be used.\n      \n       ## Documentation/git-difftool.txt ##\n      @@ Documentation/git-difftool.txt: instead.  `--no-symlinks` is the default on Windows.\n     @@ Documentation/git-difftool.txt: instead.  `--no-symlinks` is the default on Wind\n      -\tis not set, we will fallback in the order of `merge.guitool`,\n      -\t`diff.tool`, `merge.tool` until a tool is found.\n      +\t`diff.guitool` variable instead of `diff.tool`. This may be\n     -+\tautoselected using the configuration variable\n     ++\tselected automatically using the configuration variable\n      +\t`difftool.guiDefault`. The `--no-gui` option can be used to\n      +\toverride these settings. If `diff.guitool` is not set, we will\n      +\tfallback in the order of `merge.guitool`, `diff.tool`,\n     @@ Documentation/git-mergetool.txt: success of the resolution after the custom tool\n      \n       ## builtin/difftool.c ##\n      @@ builtin/difftool.c: static int run_file_diff(int prompt, const char *prefix,\n     - \treturn run_command(child);\n     - }\n       \n     -+enum difftool_gui_mode {\n     -+\tGUI_BY_CONFIG = -1,\n     -+\tGUI_DISABLED = 0,\n     -+\tGUI_ENABLED = 1\n     -+};\n     -+\n     -+static int difftool_opt_gui(const struct option *opt,\n     -+\t\t\t      const char *optarg, int unset)\n     -+{\n     -+\tenum difftool_gui_mode *mode;\n     -+\tmode = opt->value;\n     -+\n     -+\tBUG_ON_OPT_ARG(optarg);\n     -+\n     -+\tif (unset)\n     -+\t\t*mode = GUI_DISABLED;\n     -+\telse\n     -+\t\t*mode = GUI_ENABLED;\n     -+\treturn 0;\n     -+}\n     -+\n       int cmd_difftool(int argc, const char **argv, const char *prefix)\n       {\n      -\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n     --\t    tool_help = 0, no_index = 0;\n     -+\tint dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,\n     -+\t    no_index = 0;\n     -+\tenum difftool_gui_mode gui_mode = GUI_BY_CONFIG;\n     ++\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n     + \t    tool_help = 0, no_index = 0;\n       \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n       \tstruct option builtin_difftool_options[] = {\n     --\t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n     --\t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n     -+\t\tOPT_CALLBACK_F('g', \"gui\", &gui_mode, NULL,\n     -+\t\t\t       N_(\"use `diff.guitool` instead of `diff.tool`\"),\n     -+\t\t\t       PARSE_OPT_NOARG, difftool_opt_gui),\n     - \t\tOPT_BOOL('d', \"dir-diff\", &dir_diff,\n     - \t\t\t N_(\"perform a full-directory diff\")),\n     - \t\tOPT_SET_INT_F('y', \"no-prompt\", &prompt,\n      @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *prefix)\n       \t} else if (dir_diff)\n       \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n       \n      -\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n     -+\tdie_for_incompatible_opt3(gui_mode == GUI_ENABLED, \"--gui\",\n     ++\tdie_for_incompatible_opt3(use_gui_tool == 1, \"--gui\",\n       \t\t\t\t  !!difftool_cmd, \"--tool\",\n       \t\t\t\t  !!extcmd, \"--extcmd\");\n       \n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr\n      +\t * empty or unset means \"use the difftool.guiDefault config or default to\n      +\t * false\".\n      +\t */\n     -+\tif (gui_mode == GUI_ENABLED)\n     ++\tif (use_gui_tool == 1)\n       \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n      -\telse if (difftool_cmd) {\n     -+\telse if (gui_mode == GUI_DISABLED)\n     ++\telse if (use_gui_tool == 0)\n      +\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"false\", 1);\n      +\n      +\tif (difftool_cmd) {\n     @@ builtin/difftool.c: int cmd_difftool(int argc, const char **argv, const char *pr\n       \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n       \t\telse\n      \n     + ## git-difftool--helper.sh ##\n     +@@ git-difftool--helper.sh: then\n     + \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n     + \telse\n     + \t\tmerge_tool=\"$(get_merge_tool)\"\n     ++\t\tsubshell_exit_status=$?\n     ++\t\tif test $subshell_exit_status -gt 1\n     ++\t\tthen\n     ++\t\t\texit $subshell_exit_status\n     ++\t\tfi\n     + \tfi\n     + fi\n     + \n     +\n       ## git-mergetool--lib.sh ##\n      @@ git-mergetool--lib.sh: merge_mode () {\n       \ttest \"$TOOL_MODE\" = merge\n     @@ git-mergetool--lib.sh: merge_mode () {\n      +\telse\n      +\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n      +\tfi\n     -+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY  | tr 'A-Z' 'a-z')\n     ++\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get \"$GUI_DEFAULT_KEY\" | tr 'A-Z' 'a-z')\n      +\tif test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n      +\tthen\n      +\t\tif test -n \"$DISPLAY\"\n     @@ git-mergetool--lib.sh: merge_mode () {\n      +\t\t\tGUI_DEFAULT=false\n      +\t\tfi\n      +\telse\n     -+\t\tGUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)\n     ++\t\tGUI_DEFAULT=$(git config --default false --bool --get \"$GUI_DEFAULT_KEY\")\n     ++\t\tsubshell_exit_status=$?\n     ++\t\tif test $subshell_exit_status -ne 0\n     ++\t\tthen\n     ++\t\t\texit $subshell_exit_status\n     ++\t\tfi\n      +\tfi\n      +\techo $GUI_DEFAULT\n      +}\n      +\n       gui_mode () {\n     -+\tif [ -z \"$GIT_MERGETOOL_GUI\" ]\n     ++\tif test -z \"$GIT_MERGETOOL_GUI\"\n      +\tthen\n      +\t\tGIT_MERGETOOL_GUI=$(get_gui_default)\n     ++\t\tif test $? -ne 0\n     ++\t\tthen\n     ++\t\t\texit 2\n     ++\t\tfi\n      +\tfi\n       \ttest \"$GIT_MERGETOOL_GUI\" = true\n       }\n       \n     +@@ git-mergetool--lib.sh: get_merge_tool () {\n     + \tis_guessed=false\n     + \t# Check if a merge tool has been configured\n     + \tmerge_tool=$(get_configured_merge_tool)\n     ++\tsubshell_exit_status=$?\n     ++\tif test $subshell_exit_status -gt \"1\"\n     ++\tthen\n     ++\t\texit $subshell_exit_status\n     ++\tfi\n     + \t# Try to guess an appropriate merge tool if no tool has been set.\n     + \tif test -z \"$merge_tool\"\n     + \tthen\n      \n       ## git-mergetool.sh ##\n      @@ git-mergetool.sh: print_noop_and_exit () {\n     @@ git-mergetool.sh: print_noop_and_exit () {\n       \tguessed_merge_tool=false\n       \torderfile=\n       \n     +@@ git-mergetool.sh: main () {\n     + \n     + \tif test -z \"$merge_tool\"\n     + \tthen\n     +-\t\tif ! merge_tool=$(get_merge_tool)\n     ++\t\tmerge_tool=$(get_merge_tool)\n     ++\t\tsubshell_exit_status=$?\n     ++\t\tif test $subshell_exit_status = 1\n     + \t\tthen\n     + \t\t\tguessed_merge_tool=true\n     ++\t\telif test $subshell_exit_status -gt 1\n     ++\t\tthen\n     ++\t\t\texit $subshell_exit_status\n     + \t\tfi\n     + \tfi\n     + \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n      \n       ## t/t7610-mergetool.sh ##\n      @@ t/t7610-mergetool.sh: test_expect_success 'mergetool hideResolved' '\n     @@ t/t7610-mergetool.sh: test_expect_success 'mergetool hideResolved' '\n      +\tyes \"d\" | git mergetool file12 &&\n      +\tyes \"l\" | git mergetool submod &&\n      +\n     -+\n      +\techo \"gui main updated\" >expect &&\n      +\ttest_cmp expect file1 &&\n      +\n\n\n Documentation/config/difftool.txt  |  7 ++++\n Documentation/config/mergetool.txt |  7 ++++\n Documentation/git-difftool.txt     | 10 +++---\n Documentation/git-mergetool.txt    |  9 +++---\n builtin/difftool.c                 | 16 ++++++---\n git-difftool--helper.sh            |  5 +++\n git-mergetool--lib.sh              | 40 +++++++++++++++++++++++\n git-mergetool.sh                   |  9 ++++--\n t/t7610-mergetool.sh               | 38 ++++++++++++++++++++++\n t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++\n 10 files changed, 179 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex a3f82112102..447c40d85a2 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.guiDefault::\n+\tSet `true` to use the `diff.guitool` by default (equivalent to specifying\n+\tthe `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`\n+\tdepending on the presence of a `DISPLAY` environment variable value. The\n+\tdefault is `false`, where the `--gui` argument must be provided\n+\texplicitly for the `diff.guitool` to be used.\ndiff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt\nindex 90b38097002..a615bf24187 100644\n--- a/Documentation/config/mergetool.txt\n+++ b/Documentation/config/mergetool.txt\n@@ -85,3 +85,10 @@ mergetool.writeToTemp::\n \n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n+\n+mergetool.guiDefault::\n+\tSet `true` to use the `merge.guitool` by default (equivalent to\n+\tspecifying the `--gui` argument), or `auto` to select `merge.guitool`\n+\tor `merge.tool` depending on the presence of a `DISPLAY` environment\n+\tvariable value. The default is `false`, where the `--gui` argument\n+\tmust be provided explicitly for the `merge.guitool` to be used.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 9d14c3c9f09..ac0ac6fa022 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.\n --[no-]gui::\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n-\t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting. If `diff.guitool`\n-\tis not set, we will fallback in the order of `merge.guitool`,\n-\t`diff.tool`, `merge.tool` until a tool is found.\n+\t`diff.guitool` variable instead of `diff.tool`. This may be\n+\tselected automatically using the configuration variable\n+\t`difftool.guiDefault`. The `--no-gui` option can be used to\n+\toverride these settings. If `diff.guitool` is not set, we will\n+\tfallback in the order of `merge.guitool`, `diff.tool`,\n+\t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex c44e205629b..07535f6576e 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.\n \tthe default merge tool will be read from the configured\n \t`merge.guitool` variable instead of `merge.tool`. If\n \t`merge.guitool` is not set, we will fallback to the tool\n-\tconfigured under `merge.tool`.\n+\tconfigured under `merge.tool`. This may be autoselected using\n+\tthe configuration variable `mergetool.guiDefault`.\n \n --no-gui::\n-\tThis overrides a previous `-g` or `--gui` setting and reads the\n-\tdefault merge tool will be read from the configured `merge.tool`\n-\tvariable.\n+\tThis overrides a previous `-g` or `--gui` setting or\n+\t`mergetool.guiDefault` configuration and reads the default merge\n+\ttool from the configured `merge.tool` variable.\n \n -O<orderfile>::\n \tProcess files in the order specified in the\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a369..2b89c5dc865 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -682,7 +682,7 @@ static int run_file_diff(int prompt, const char *prefix,\n \n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n-\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n+\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n \t    tool_help = 0, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n@@ -732,13 +732,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t} else if (dir_diff)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n \n-\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n+\tdie_for_incompatible_opt3(use_gui_tool == 1, \"--gui\",\n \t\t\t\t  !!difftool_cmd, \"--tool\",\n \t\t\t\t  !!extcmd, \"--extcmd\");\n \n-\tif (use_gui_tool)\n+\t/*\n+\t * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;\n+\t * empty or unset means \"use the difftool.guiDefault config or default to\n+\t * false\".\n+\t */\n+\tif (use_gui_tool == 1)\n \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n-\telse if (difftool_cmd) {\n+\telse if (use_gui_tool == 0)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"false\", 1);\n+\n+\tif (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n \t\telse\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 992124cc67c..e4e820e6809 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -75,6 +75,11 @@ then\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n \t\tmerge_tool=\"$(get_merge_tool)\"\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status -gt 1\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n+\t\tfi\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9f99201bcca..1ff26170ffc 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -97,7 +97,42 @@ merge_mode () {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+get_gui_default () {\n+\tif diff_mode\n+\tthen\n+\t\tGUI_DEFAULT_KEY=\"difftool.guiDefault\"\n+\telse\n+\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n+\tfi\n+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get \"$GUI_DEFAULT_KEY\" | tr 'A-Z' 'a-z')\n+\tif test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n+\tthen\n+\t\tif test -n \"$DISPLAY\"\n+\t\tthen\n+\t\t\tGUI_DEFAULT=true\n+\t\telse\n+\t\t\tGUI_DEFAULT=false\n+\t\tfi\n+\telse\n+\t\tGUI_DEFAULT=$(git config --default false --bool --get \"$GUI_DEFAULT_KEY\")\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status -ne 0\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n+\t\tfi\n+\tfi\n+\techo $GUI_DEFAULT\n+}\n+\n gui_mode () {\n+\tif test -z \"$GIT_MERGETOOL_GUI\"\n+\tthen\n+\t\tGIT_MERGETOOL_GUI=$(get_gui_default)\n+\t\tif test $? -ne 0\n+\t\tthen\n+\t\t\texit 2\n+\t\tfi\n+\tfi\n \ttest \"$GIT_MERGETOOL_GUI\" = true\n }\n \n@@ -467,6 +502,11 @@ get_merge_tool () {\n \tis_guessed=false\n \t# Check if a merge tool has been configured\n \tmerge_tool=$(get_configured_merge_tool)\n+\tsubshell_exit_status=$?\n+\tif test $subshell_exit_status -gt \"1\"\n+\tthen\n+\t\texit $subshell_exit_status\n+\tfi\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex f751d9cfe20..8a922893f75 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -451,7 +451,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tGIT_MERGETOOL_GUI=false\n+\tGIT_MERGETOOL_GUI=\n \tguessed_merge_tool=false\n \torderfile=\n \n@@ -511,9 +511,14 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\tif ! merge_tool=$(get_merge_tool)\n+\t\tmerge_tool=$(get_merge_tool)\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status = 1\n \t\tthen\n \t\t\tguessed_merge_tool=true\n+\t\telif test $subshell_exit_status -gt 1\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n \t\tfi\n \tfi\n \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 8cc64729adb..ba0d2ae76de 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -860,4 +860,42 @@ test_expect_success 'mergetool hideResolved' '\n \tgit commit -m \"test resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with guiDefault' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge main &&\n+\n+\ttest_config mergetool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\tyes \"\" | git mergetool both &&\n+\tyes \"\" | git mergetool file1 file1 &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\tyes \"\" | git mergetool file2 \"spaced name\" &&\n+\n+\ttest_config mergetool.guiDefault true &&\n+\tyes \"\" | git mergetool subdir/file3 &&\n+\n+\tyes \"d\" | git mergetool file11 &&\n+\tyes \"d\" | git mergetool file12 &&\n+\tyes \"l\" | git mergetool submod &&\n+\n+\techo \"gui main updated\" >expect &&\n+\ttest_cmp expect file1 &&\n+\n+\techo \"main new\" >expect &&\n+\ttest_cmp expect file2 &&\n+\n+\techo \"gui main new sub\" >expect &&\n+\ttest_cmp expect subdir/file3 &&\n+\n+\techo \"branch1 submodule\" >expect &&\n+\ttest_cmp expect submod/bar &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_done\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 096456292c0..1e47d3c05bd 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault Auto &&\n+\tDISPLAY= && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool with guiDefault true selects gui tool' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tDISPLAY=Something && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool --no-gui trumps config guiDefault' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --no-gui branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'difftool --gui last setting wins' '\n \tdifftool_test_setup &&\n \t: >expect &&\n\nbase-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df\n-- \ngitgitgadget\n"},{"id":"472251","messageId":"CAPMMpoi8mbqSAYMbhgYRj0UTjxHnGy50Z3HKP6fOaDj7AcQ=mA@mail.gmail.com","threadId":"58611","inReplyTo":"pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-02-17T10:59:34Z","receivedAt":"2023-02-17T11:00:18Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Oct 18, 2022 at 8:54 AM Tao Klerks via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Tao Klerks <tao@klerks.biz>\n>\n> When no merge.tool or diff.tool is configured or manually selected, the\n> selection of a default tool is sensitive to the DISPLAY variable; in a\n> GUI session a gui-specific tool will be proposed if found, and\n> otherwise a terminal-based one. This \"GUI-optimizing\" behavior is\n> important because a GUI can make a huge difference to a user's ability\n> to understand and correctly complete a non-trivial conflicting merge.\n>\n> Some time ago the merge.guitool and diff.guitool config options were\n> introduced to enable users to configure both a GUI tool, and a non-GUI\n> tool (with fallback if no GUI tool configured), in the same environment.\n>\n> Unfortunately, the --gui argument introduced to support the selection of\n> the guitool is still explicit. When using configured tools, there is no\n> equivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\n> environment\" behavior.\n>\n> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\n> options, difftool.guiDefault and mergetool.guiDefault, supporting a special\n> value \"auto\" which causes the corresponding tool or guitool to be selected\n> depending on the presence of a non-empty DISPLAY value. Also support \"true\"\n> to say \"default to the guitool (unless --no-gui is passed on the\n> commandline)\", and \"false\" as the previous default behavior when these new\n> configuration options are not specified.\n>\n> Signed-off-by: Tao Klerks <tao@klerks.biz>\n> ---\n>     RFC: mergetool: new config guiDefault supports auto-toggling gui by\n>     DISPLAY\n>\n>     I'm reasonably comfortable that with this patch we do the right thing,\n>     but I'm not sure about yet another remaining implementation detail:\n>\n>      * After implementing Junio's recommended \"fail if defaulting config is\n>        consulted and is invalid\" flow, there now needs to be a distinction\n>        between subshell exit code 1, which was used before and indicates\n>        \"tool not found or broken; falling back to default\" and other\n>        (higher) exit codes, which newly mean \"something went wrong, stop!\".\n>        The resulting code looks awkward, I can't tell whether I'm missing a\n>        code or even commenting pattern that would make it clearer.\n>\n>     V3:\n>\n>      * Simplify C code to use OPT_BOOL with an int rather than a custom\n>        option-parsing function with an enum\n>      * Fix doc to more extensively use backticks for config keys / values /\n>        args\n>      * Fix more shell script formatting issues\n>      * Change error-handling in mergetool and difftool helpers to exit if\n>        defaulting config is invalid\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1381\n\n\nHi folks, this v3 never got any feedback - the only reason I had left\nit as an RFC was that the error-handling looked a bit awkward, as I\nnoted above.\n\nShould I resubmit this without the RFC prefix?\n\nAre there any concerns about the change here to better support mixed\nGUI/console-only environments?\n\nThanks,\nTao\n"},{"id":"473683","messageId":"pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com","threadId":"58611","inReplyTo":"pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com","subject":"[PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-18T15:27:43Z","receivedAt":"2023-03-18T15:27:53Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nWhen no merge.tool or diff.tool is configured or manually selected, the\nselection of a default tool is sensitive to the DISPLAY variable; in a\nGUI session a gui-specific tool will be proposed if found, and\notherwise a terminal-based one. This \"GUI-optimizing\" behavior is\nimportant because a GUI can make a huge difference to a user's ability\nto understand and correctly complete a non-trivial conflicting merge.\n\nSome time ago the merge.guitool and diff.guitool config options were\nintroduced to enable users to configure both a GUI tool, and a non-GUI\ntool (with fallback if no GUI tool configured), in the same environment.\n\nUnfortunately, the --gui argument introduced to support the selection of\nthe guitool is still explicit. When using configured tools, there is no\nequivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\nenvironment\" behavior.\n\nAs proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\noptions, difftool.guiDefault and mergetool.guiDefault, supporting a special\nvalue \"auto\" which causes the corresponding tool or guitool to be selected\ndepending on the presence of a non-empty DISPLAY value. Also support \"true\"\nto say \"default to the guitool (unless --no-gui is passed on the\ncommandline)\", and \"false\" as the previous default behavior when these new\nconfiguration options are not specified.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n    \n    A few months later, first \"real\" submission - 4th version wrt original\n    draft.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1381\n\nRange-diff vs v3:\n\n 1:  dbb80499218 ! 1:  765abfe7fc4 RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n     @@ Metadata\n      Author: Tao Klerks <tao@klerks.biz>\n      \n       ## Commit message ##\n     -    RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n     +    mergetool: new config guiDefault supports auto-toggling gui by DISPLAY\n      \n          When no merge.tool or diff.tool is configured or manually selected, the\n          selection of a default tool is sensitive to the DISPLAY variable; in a\n\n\n Documentation/config/difftool.txt  |  7 ++++\n Documentation/config/mergetool.txt |  7 ++++\n Documentation/git-difftool.txt     | 10 +++---\n Documentation/git-mergetool.txt    |  9 +++---\n builtin/difftool.c                 | 16 ++++++---\n git-difftool--helper.sh            |  5 +++\n git-mergetool--lib.sh              | 40 +++++++++++++++++++++++\n git-mergetool.sh                   |  9 ++++--\n t/t7610-mergetool.sh               | 38 ++++++++++++++++++++++\n t/t7800-difftool.sh                | 52 ++++++++++++++++++++++++++++++\n 10 files changed, 179 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex a3f82112102..447c40d85a2 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details.\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.guiDefault::\n+\tSet `true` to use the `diff.guitool` by default (equivalent to specifying\n+\tthe `--gui` argument), or `auto` to select `diff.guitool` or `diff.tool`\n+\tdepending on the presence of a `DISPLAY` environment variable value. The\n+\tdefault is `false`, where the `--gui` argument must be provided\n+\texplicitly for the `diff.guitool` to be used.\ndiff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt\nindex e779a122d8a..56a7eeeffb4 100644\n--- a/Documentation/config/mergetool.txt\n+++ b/Documentation/config/mergetool.txt\n@@ -85,3 +85,10 @@ mergetool.writeToTemp::\n \n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n+\n+mergetool.guiDefault::\n+\tSet `true` to use the `merge.guitool` by default (equivalent to\n+\tspecifying the `--gui` argument), or `auto` to select `merge.guitool`\n+\tor `merge.tool` depending on the presence of a `DISPLAY` environment\n+\tvariable value. The default is `false`, where the `--gui` argument\n+\tmust be provided explicitly for the `merge.guitool` to be used.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 9d14c3c9f09..ac0ac6fa022 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -97,10 +97,12 @@ instead.  `--no-symlinks` is the default on Windows.\n --[no-]gui::\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n-\t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting. If `diff.guitool`\n-\tis not set, we will fallback in the order of `merge.guitool`,\n-\t`diff.tool`, `merge.tool` until a tool is found.\n+\t`diff.guitool` variable instead of `diff.tool`. This may be\n+\tselected automatically using the configuration variable\n+\t`difftool.guiDefault`. The `--no-gui` option can be used to\n+\toverride these settings. If `diff.guitool` is not set, we will\n+\tfallback in the order of `merge.guitool`, `diff.tool`,\n+\t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex c44e205629b..07535f6576e 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited.\n \tthe default merge tool will be read from the configured\n \t`merge.guitool` variable instead of `merge.tool`. If\n \t`merge.guitool` is not set, we will fallback to the tool\n-\tconfigured under `merge.tool`.\n+\tconfigured under `merge.tool`. This may be autoselected using\n+\tthe configuration variable `mergetool.guiDefault`.\n \n --no-gui::\n-\tThis overrides a previous `-g` or `--gui` setting and reads the\n-\tdefault merge tool will be read from the configured `merge.tool`\n-\tvariable.\n+\tThis overrides a previous `-g` or `--gui` setting or\n+\t`mergetool.guiDefault` configuration and reads the default merge\n+\ttool from the configured `merge.tool` variable.\n \n -O<orderfile>::\n \tProcess files in the order specified in the\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex dbbfb19f192..88a00abfbdb 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -684,7 +684,7 @@ static int run_file_diff(int prompt, const char *prefix,\n \n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n-\tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n+\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n \t    tool_help = 0, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n@@ -734,13 +734,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t} else if (dir_diff)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n \n-\tdie_for_incompatible_opt3(use_gui_tool, \"--gui\",\n+\tdie_for_incompatible_opt3(use_gui_tool == 1, \"--gui\",\n \t\t\t\t  !!difftool_cmd, \"--tool\",\n \t\t\t\t  !!extcmd, \"--extcmd\");\n \n-\tif (use_gui_tool)\n+\t/*\n+\t * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh;\n+\t * empty or unset means \"use the difftool.guiDefault config or default to\n+\t * false\".\n+\t */\n+\tif (use_gui_tool == 1)\n \t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n-\telse if (difftool_cmd) {\n+\telse if (use_gui_tool == 0)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"false\", 1);\n+\n+\tif (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n \t\telse\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 992124cc67c..e4e820e6809 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -75,6 +75,11 @@ then\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n \t\tmerge_tool=\"$(get_merge_tool)\"\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status -gt 1\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n+\t\tfi\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9f99201bcca..1ff26170ffc 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -97,7 +97,42 @@ merge_mode () {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+get_gui_default () {\n+\tif diff_mode\n+\tthen\n+\t\tGUI_DEFAULT_KEY=\"difftool.guiDefault\"\n+\telse\n+\t\tGUI_DEFAULT_KEY=\"mergetool.guiDefault\"\n+\tfi\n+\tGUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get \"$GUI_DEFAULT_KEY\" | tr 'A-Z' 'a-z')\n+\tif test \"$GUI_DEFAULT_CONFIG_LCASE\" = \"auto\"\n+\tthen\n+\t\tif test -n \"$DISPLAY\"\n+\t\tthen\n+\t\t\tGUI_DEFAULT=true\n+\t\telse\n+\t\t\tGUI_DEFAULT=false\n+\t\tfi\n+\telse\n+\t\tGUI_DEFAULT=$(git config --default false --bool --get \"$GUI_DEFAULT_KEY\")\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status -ne 0\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n+\t\tfi\n+\tfi\n+\techo $GUI_DEFAULT\n+}\n+\n gui_mode () {\n+\tif test -z \"$GIT_MERGETOOL_GUI\"\n+\tthen\n+\t\tGIT_MERGETOOL_GUI=$(get_gui_default)\n+\t\tif test $? -ne 0\n+\t\tthen\n+\t\t\texit 2\n+\t\tfi\n+\tfi\n \ttest \"$GIT_MERGETOOL_GUI\" = true\n }\n \n@@ -467,6 +502,11 @@ get_merge_tool () {\n \tis_guessed=false\n \t# Check if a merge tool has been configured\n \tmerge_tool=$(get_configured_merge_tool)\n+\tsubshell_exit_status=$?\n+\tif test $subshell_exit_status -gt \"1\"\n+\tthen\n+\t\texit $subshell_exit_status\n+\tfi\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex f751d9cfe20..8a922893f75 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -451,7 +451,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tGIT_MERGETOOL_GUI=false\n+\tGIT_MERGETOOL_GUI=\n \tguessed_merge_tool=false\n \torderfile=\n \n@@ -511,9 +511,14 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\tif ! merge_tool=$(get_merge_tool)\n+\t\tmerge_tool=$(get_merge_tool)\n+\t\tsubshell_exit_status=$?\n+\t\tif test $subshell_exit_status = 1\n \t\tthen\n \t\t\tguessed_merge_tool=true\n+\t\telif test $subshell_exit_status -gt 1\n+\t\tthen\n+\t\t\texit $subshell_exit_status\n \t\tfi\n \tfi\n \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 7b957022f1a..22b3a85b3e9 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -860,4 +860,42 @@ test_expect_success 'mergetool hideResolved' '\n \tgit commit -m \"test resolved with mergetool\"\n '\n \n+test_expect_success 'mergetool with guiDefault' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge main &&\n+\n+\ttest_config mergetool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\tyes \"\" | git mergetool both &&\n+\tyes \"\" | git mergetool file1 file1 &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\tyes \"\" | git mergetool file2 \"spaced name\" &&\n+\n+\ttest_config mergetool.guiDefault true &&\n+\tyes \"\" | git mergetool subdir/file3 &&\n+\n+\tyes \"d\" | git mergetool file11 &&\n+\tyes \"d\" | git mergetool file12 &&\n+\tyes \"l\" | git mergetool submod &&\n+\n+\techo \"gui main updated\" >expect &&\n+\ttest_cmp expect file1 &&\n+\n+\techo \"main new\" >expect &&\n+\ttest_cmp expect file2 &&\n+\n+\techo \"gui main new sub\" >expect &&\n+\ttest_cmp expect subdir/file3 &&\n+\n+\techo \"branch1 submodule\" >expect &&\n+\ttest_cmp expect submod/bar &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_done\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 24297e26ca0..59d3847bf87 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config merge.tool bogus-tool &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault auto &&\n+\tDISPLAY=SOMETHING && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault Auto &&\n+\tDISPLAY= && export DISPLAY &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool with guiDefault true selects gui tool' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\tDISPLAY= && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tDISPLAY=Something && export DISPLAY &&\n+\techo branch >expect &&\n+\tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'difftool --no-gui trumps config guiDefault' '\n+\tdifftool_test_setup &&\n+\ttest_config diff.guitool bogus-tool &&\n+\ttest_config diff.tool test-tool &&\n+\ttest_config difftool.guiDefault true &&\n+\n+\techo branch >expect &&\n+\tgit difftool --no-prompt --no-gui branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'difftool --gui last setting wins' '\n \tdifftool_test_setup &&\n \t: >expect &&\n\nbase-commit: d15644fe0226af7ffc874572d968598564a230dd\n-- \ngitgitgadget\n"},{"id":"474766","messageId":"ZCvx/REMF69xkXJg@gmail.com","threadId":"58611","inReplyTo":"pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2023-04-04T09:46:37Z","receivedAt":"2023-04-04T09:47:05Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:\n> From: Tao Klerks <tao@klerks.biz>\n> \n> When no merge.tool or diff.tool is configured or manually selected, the\n> selection of a default tool is sensitive to the DISPLAY variable; in a\n> GUI session a gui-specific tool will be proposed if found, and\n> otherwise a terminal-based one. This \"GUI-optimizing\" behavior is\n> important because a GUI can make a huge difference to a user's ability\n> to understand and correctly complete a non-trivial conflicting merge.\n> \n> Some time ago the merge.guitool and diff.guitool config options were\n> introduced to enable users to configure both a GUI tool, and a non-GUI\n> tool (with fallback if no GUI tool configured), in the same environment.\n> \n> Unfortunately, the --gui argument introduced to support the selection of\n> the guitool is still explicit. When using configured tools, there is no\n> equivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\n> environment\" behavior.\n\nI'm not sure what problem this is solving. Please help me understand.\nThis commit seems to be talking about the behavior w.r.t. the DISPLAY\nvariable and also about automatically enabling `--gui` automatically.\n\nRegarding `--gui`: Someone that configures `mergetool.guiDefault`\nmeans that all of their invocations should be as-if they had written\n`--gui` on the command-line.\n\nSo.. if you find that you always use `--gui` then why are\nyou not configuring `merge.tool` directly and instead configuring\n`merge.guitool`?\n\nThe purpose of `--gui` is to provide an override for use by tools (such\nas Git Cola) to execute an optional, 2nd merge tool separate from that\nwhich the user might configure for command-line use.\n\nThis patch basically makes the `merge.tool` configuration useless, so I\ndon't understand why someone would want to configure this option rather\nthan just configuring `merge.tool` directly, leading to an overall\nsimpler and easier to understand system.\n\nIf we add this boolean then it becomes harder to understand.\nI'm not sure it really provides any additional value to the user.\n\nFrom my vantage point, the user is better off configuring `merge.tool`\ndirectly rather than the tool becoming more complex.\n\nThat said, I probably don't understand and I'd be happy to be\nenlightened.\n\n\n> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\n> options, difftool.guiDefault and mergetool.guiDefault, supporting a special\n> value \"auto\" which causes the corresponding tool or guitool to be selected\n> depending on the presence of a non-empty DISPLAY value. Also support \"true\"\n> to say \"default to the guitool (unless --no-gui is passed on the\n> commandline)\", and \"false\" as the previous default behavior when these new\n> configuration options are not specified.\n\nThe rationale here is slightly surprising to me as well.\n\nApparently the user cannot be bothered to configure `merge.tool`\n(because we care about this DISPLAY behavior) yet the user is expected\nto configure `merge.guiDefault` instead?\n\nI think it'd be better to encourage users to go ahead and configure\n`merge.tool`, but again, I'm probably missing context around why.\n\n\nSmall patch note below...\n\n\n> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> index 992124cc67c..e4e820e6809 100755\n> --- a/git-difftool--helper.sh\n> +++ b/git-difftool--helper.sh\n> @@ -75,6 +75,11 @@ then\n>  \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n>  \telse\n>  \t\tmerge_tool=\"$(get_merge_tool)\"\n> +\t\tsubshell_exit_status=$?\n> +\t\tif test $subshell_exit_status -gt 1\n\nIs exit status 1 special here?\nShould this be \"if test $subshell_exit_status != 0\" instead?\n-- \nDavid\n"},{"id":"474779","messageId":"CAPMMpohtL2dG_Ody5zALhS5kOuLLTTRw7L0vc6SnpWym+usppg@mail.gmail.com","threadId":"58611","inReplyTo":"ZCvx/REMF69xkXJg@gmail.com","subject":"Re: [PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-04-04T14:50:31Z","receivedAt":"2023-04-04T14:50:48Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"First of all, thank you for reviewing!!\n\nOn Tue, Apr 4, 2023 at 11:46 AM David Aguilar <davvid@gmail.com> wrote:\n>\n> On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:\n> > From: Tao Klerks <tao@klerks.biz>\n> >\n> > When no merge.tool or diff.tool is configured or manually selected, the\n> > selection of a default tool is sensitive to the DISPLAY variable; in a\n> > GUI session a gui-specific tool will be proposed if found, and\n> > otherwise a terminal-based one. This \"GUI-optimizing\" behavior is\n> > important because a GUI can make a huge difference to a user's ability\n> > to understand and correctly complete a non-trivial conflicting merge.\n> >\n> > Some time ago the merge.guitool and diff.guitool config options were\n> > introduced to enable users to configure both a GUI tool, and a non-GUI\n> > tool (with fallback if no GUI tool configured), in the same environment.\n> >\n> > Unfortunately, the --gui argument introduced to support the selection of\n> > the guitool is still explicit. When using configured tools, there is no\n> > equivalent of the no-tool-configured \"propose a GUI tool if we are in a GUI\n> > environment\" behavior.\n>\n> I'm not sure what problem this is solving. Please help me understand.\n> This commit seems to be talking about the behavior w.r.t. the DISPLAY\n> variable and also about automatically enabling `--gui` automatically.\n\nBackground: If you don't set a mergetool at all, git defaults one if\nit can. The default you get depends on the DISPLAY variable - if it\nlooks like you're in a gui context (because we detect that DISPLAY is\nset), then we propose a gui tool. Otherwise, we propose a cli tool.\n\nThis is particularly valuable for contexts where you might access the\nsame environment, the same workstation, via multiple contexts -\nCLI-only, and GUI. For example, sometimes you connect to the desktop,\nand sometimes you SSH into it (without X forwarding). You don't need\nto tell git \"I want a GUI now please\", it figures that out.\n\nPoint/problem: As soon as you set \"merge.tool\", you lose that\nautoselection. A reasonable user would assume \"there is `merge.tool`,\nthere is `merge.guitool`, and git knows how to autodetect GUI context\nbased on the DISPLAY variable... so if I set both `merge.tool` and\n`merge.guitool`, I'll get the right one based on context, right? Like\nI do when I set neither?\"\nUnfortunately, that is *not* the case. The gui autodetection only\napplies when git is choosing a *defaulted* merge tool, not an\nexplicitly-defined one. As soon as you specify a tool you want to use,\nyou need to start *explicitly* telling git whether you want to use a\nGUI tool or not. If you get it wrong, an inconvenient or even bad\nthing happens: If you're in a GUI context and you forget to say\n`--gui`, then you get the CLI tool - not the end of the world, you\nmight argue. If you're in a non-GUI context and you say `--gui`, you\nget a (hopefully intelligible) error about not finding a display, or\nworst case, you hang - and git doesn't know what happened, the\nmessaging has to assume you aborted explicitly.\n\nIf there were a way to get git to autoselect \"--gui\" by presence of\nthe DISPLAY, as it does for the *default* mergetool, then this kind of\nflow would be much more intuitive, usable, etc.\n\nI'm not sure whether I'm explaining better, or just repeating myself,\nso I'll stop here :)\n\n>\n> Regarding `--gui`: Someone that configures `mergetool.guiDefault`\n> means that all of their invocations should be as-if they had written\n> `--gui` on the command-line.\n\nCorrection: Someone that sets `mergetool.guiDefault` to *true* should\nget behavior as-if they had written `--gui` on the command-line. They\ncould also set *false* (current behavior - explicit), or *auto*, and\nget different behavior.\n\n>\n> So.. if you find that you always use `--gui` then why are\n> you not configuring `merge.tool` directly and instead configuring\n> `merge.guitool`?\n\nOne reason is because you might choose to do something different in a\n*specific* invocation, eg by passing `--no-gui`. This is a common\npattern in git - the config allows you to change the default behavior,\nbut you can override it on a specific invocation by using the positive\nor negative version of the argument.\n\n>\n> The purpose of `--gui` is to provide an override for use by tools (such\n> as Git Cola) to execute an optional, 2nd merge tool separate from that\n> which the user might configure for command-line use.\n\nI'm not aware of having read a statement identifying this\n*singular*/sole purpose anywhere, but as you state it with authority\n(and I see you're the author of a popular git GUI), I'm happy to\naccept that this was originally its only purpose. That would certainly\nexplain the incongruousness of its behavior wrt (lack of)\nautodetection of DISPLAY.\n\nThat said, this patch does not in any way diminish the validity or\nusability of this usecase. If you don't set `mergetool.guiDefault` or\nleave it set to false, that purpose will continue to be perfectly\nserved.\n\nFor other users, like me, who prefer to select a GUI mergetool or not\ndepending on the *availability* of a GUI environment (auto), or\ndefault to gui but allow manually downgrading to a (defaulted or\nconfigured) CLI-only tool, this configuration will allow a simpler,\nmore coherent, easier UX.\n\nIf everything else here makes sense, I will happily update the\nabstract/commit message to reflect the proposed broadening of\napplication / correct my misunderstanding as to the original intended\nscope of the setting.\n\n>\n> This patch basically makes the `merge.tool` configuration useless,\n\nI don't understand why you say this. Unless I misunderstand something,\nthis patch does not break, or even change, any existing behavior.\n\n> so I\n> don't understand why someone would want to configure this option rather\n> than just configuring `merge.tool` directly, leading to an overall\n> simpler and easier to understand system.\n\n(see above - the main usecase is a system you access via both GUI and\nCLI contexts, and the secondary usecase is having a default GUI\nbehavior that you can easily override with `--no-gui`.)\n\n>\n> If we add this boolean then it becomes harder to understand.\n> I'm not sure it really provides any additional value to the user.\n>\n> From my vantage point, the user is better off configuring `merge.tool`\n> directly rather than the tool becoming more complex.\n>\n> That said, I probably don't understand and I'd be happy to be\n> enlightened.\n\nPlease let me know if it makes more sense now. You could argue that\nthe value is too limited to warrant the additional complexity\n(obviously I would disagree), but hopefully the concrete value is\nclearer now.\n\n>\n>\n> > As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration\n> > options, difftool.guiDefault and mergetool.guiDefault, supporting a special\n> > value \"auto\" which causes the corresponding tool or guitool to be selected\n> > depending on the presence of a non-empty DISPLAY value. Also support \"true\"\n> > to say \"default to the guitool (unless --no-gui is passed on the\n> > commandline)\", and \"false\" as the previous default behavior when these new\n> > configuration options are not specified.\n>\n> The rationale here is slightly surprising to me as well.\n>\n> Apparently the user cannot be bothered to configure `merge.tool`\n> (because we care about this DISPLAY behavior) yet the user is expected\n> to configure `merge.guiDefault` instead?\n\nUmm, no: The user wants `merge.tool` to be set to a specific CLI tool,\nthey want `merge.guitool` to be set to a specific GUI tool, and they\nwant the GUI tool to be used in GUI contexts, and the CLI tool to be\nused when there is no GUI (eg SSH without X forwarding) - like the\nbehavior they get if they configure neither `merge.tool` nor\n`merge.guitool`.\n\n>\n> I think it'd be better to encourage users to go ahead and configure\n> `merge.tool`, but again, I'm probably missing context around why.\n>\n>\n> Small patch note below...\n>\n>\n> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> > index 992124cc67c..e4e820e6809 100755\n> > --- a/git-difftool--helper.sh\n> > +++ b/git-difftool--helper.sh\n> > @@ -75,6 +75,11 @@ then\n> >               merge_tool=\"$GIT_DIFF_TOOL\"\n> >       else\n> >               merge_tool=\"$(get_merge_tool)\"\n> > +             subshell_exit_status=$?\n> > +             if test $subshell_exit_status -gt 1\n>\n> Is exit status 1 special here?\n> Should this be \"if test $subshell_exit_status != 0\" instead?\n\nYes, it is special: 1 means \"mergetool was guessed/defaulted\",\nanything greater than 1 means \"mergetool determination failed;\naborting\".\n\nI was not very happy with this way of passing information around when\nI implemented, and commented in the previous version of the patch\nhttps://lore.kernel.org/git/pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com/\nas my last doubt as to the proposed/RFC patch:\n\n       After implementing Junio's recommended \"fail if defaulting config is\n       consulted and is invalid\" flow, there now needs to be a distinction\n       between subshell exit code 1, which was used before and indicates\n       \"tool not found or broken; falling back to default\" and other\n       (higher) exit codes, which newly mean \"something went wrong, stop!\".\n       The resulting code looks awkward, I can't tell whether I'm missing a\n       code or even commenting pattern that would make it clearer.\n\nUnfortunately I didn't get any feedback on this, so I went ahead and\nresumitted the change as a non-Draft. I'd be very grateful for any\nrecommendations on how to handle this better.\n"},{"id":"474810","messageId":"xmqq355fz8pe.fsf@gitster.g","threadId":"58611","inReplyTo":"CAPMMpohtL2dG_Ody5zALhS5kOuLLTTRw7L0vc6SnpWym+usppg@mail.gmail.com","subject":"Re: [PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-04T20:35:57Z","receivedAt":"2023-04-04T20:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> If you get it wrong, an inconvenient or even bad\n> thing happens: If you're in a GUI context and you forget to say\n> `--gui`, then you get the CLI tool - not the end of the world, you\n> might argue. If you're in a non-GUI context and you say `--gui`, you\n> get a (hopefully intelligible) error about not finding a display, or\n> worst case, you hang - and git doesn't know what happened, the\n> messaging has to assume you aborted explicitly.\n\nThe worst case I can think of is if you have a machine that allows a\npersistent virtual graphical session (remote desktop).  You can go\nthere, open a terminal and create a screen session, which captures\nthe $DISPLAY in effect.  You may disconnect from the screen session\nbefore you disconnect from the remote desktop.  As the remote\ndesktop is persistent, the $DISPLAY would be valid even after you\ndisconnect from there.\n\nThen you ssh into the machine and connect to the screen session.  If\nyou say \"xterm &\" in that screen session, a new window with a\nterminal will open and be shown on that remote display session,\nwhich you cannot see until you reconnect to the remote desktop.\nRunning \"git mergetool --gui\" would be the same.\n\nWith the proposed patch, the experience would become even worse in\nthat scenario, I am afraid.  \"git mergetool\" (without \"--[no-]gui\")\nwould detect the presense of $DISPLAY and start a GUI mergetool that\nyou cannot view or interact with ;-).  You need to explicitly say\n\"git mergetool --no-gui\" in such a case to force use of cli tool.\n\nEven with that caveat, I think the proposed behaviour would be a net\nimprovement.  I almost always work in non-GUI environment, but I can\nsee many people work in both and depending on their environment wish\nto use the best tool for the situation.\n\n> If there were a way to get git to autoselect \"--gui\" by presence of\n> the DISPLAY, as it does for the *default* mergetool, then this kind of\n> flow would be much more intuitive, usable, etc.\n>\n> I'm not sure whether I'm explaining better, or just repeating myself,\n> so I'll stop here :)\n\nHopefully your explanation was clear enough.  Others may be able to\noffer solution better than \"look at DISPLAY and choose --[no-]gui\nwhen neither is given from the command line\", but I think at least I\nunderstand your motivation behind this change.\n\nThanks.\n"},{"id":"474903","messageId":"CAJDDKr53xaMyTVWOwBT=sjKuRf78SkNCcfdS99sRF3-CPV4=Lg@mail.gmail.com","threadId":"58611","inReplyTo":"xmqq355fz8pe.fsf@gitster.g","subject":"Re: [PATCH v4] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2023-04-06T03:33:37Z","receivedAt":"2023-04-06T03:34:19Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Apr 4, 2023 at 1:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > If you get it wrong, an inconvenient or even bad\n> > thing happens: If you're in a GUI context and you forget to say\n> > `--gui`, then you get the CLI tool - not the end of the world, you\n> > might argue. If you're in a non-GUI context and you say `--gui`, you\n> > get a (hopefully intelligible) error about not finding a display, or\n> > worst case, you hang - and git doesn't know what happened, the\n> > messaging has to assume you aborted explicitly.\n>\n> The worst case I can think of is if you have a machine that allows a\n> persistent virtual graphical session (remote desktop).  You can go\n> there, open a terminal and create a screen session, which captures\n> the $DISPLAY in effect.  You may disconnect from the screen session\n> before you disconnect from the remote desktop.  As the remote\n> desktop is persistent, the $DISPLAY would be valid even after you\n> disconnect from there.\n>\n> Then you ssh into the machine and connect to the screen session.  If\n> you say \"xterm &\" in that screen session, a new window with a\n> terminal will open and be shown on that remote display session,\n> which you cannot see until you reconnect to the remote desktop.\n> Running \"git mergetool --gui\" would be the same.\n>\n> With the proposed patch, the experience would become even worse in\n> that scenario, I am afraid.  \"git mergetool\" (without \"--[no-]gui\")\n> would detect the presense of $DISPLAY and start a GUI mergetool that\n> you cannot view or interact with ;-).  You need to explicitly say\n> \"git mergetool --no-gui\" in such a case to force use of cli tool.\n>\n> Even with that caveat, I think the proposed behaviour would be a net\n> improvement.  I almost always work in non-GUI environment, but I can\n> see many people work in both and depending on their environment wish\n> to use the best tool for the situation.\n>\n> > If there were a way to get git to autoselect \"--gui\" by presence of\n> > the DISPLAY, as it does for the *default* mergetool, then this kind of\n> > flow would be much more intuitive, usable, etc.\n> >\n> > I'm not sure whether I'm explaining better, or just repeating myself,\n> > so I'll stop here :)\n>\n> Hopefully your explanation was clear enough.  Others may be able to\n> offer solution better than \"look at DISPLAY and choose --[no-]gui\n> when neither is given from the command line\", but I think at least I\n> understand your motivation behind this change.\n>\n> Thanks.\n\nThanks for the thorough explanation. With that in mind, this all\nsounds good to me.\n-- \nDavid\n"}]}