| Message ID | pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY | expand |
"On Fri, Oct 14, 2022 at 4:07 AM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote: > [...] > As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration > options, difftool.guiDefault and mergetool.guiDefault, supporting a special > value "auto" which causes the corresponding tool or guitool to be selected > depending on the presence of a non-empty DISPLAY value. Also support "true" > to say "default to the guitool (unless --no-gui is passed on the > commandline)", and "false" as the previous default behavior when these new > configuration options are not specified. > > Signed-off-by: Tao Klerks <tao@klerks.biz> > --- > diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt > @@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details. > +difftool.guiDefault:: > + Set 'true' to use the diff.guitool by default (equivalent to specifying > + the "--gui" argument), or "auto" to select diff.guitool or diff.tool > + depending on the presence of a DISPLAY environment variable value. The > + default is 'false', where the "--gui" argument must be provided > + explicitly for the diff.guitool to be used. Let's use backticks rather than double-quotes to ensure that these get typeset similar to other documentation; i.e. `--gui`, `auto`, `diff.guitool`, `diff.tool`, `DISPLAY`. > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > @@ -85,3 +85,10 @@ mergetool.writeToTemp:: > +mergetool.guiDefault:: > + Set 'true' to use the merge.guitool by default (equivalent to > + specifying the "--gui" argument), or "auto" to select merge.guitool > + or merge.tool depending on the presence of a DISPLAY environment > + variable value. The default is 'false', where the "--gui" argument > + must be provided explicitly for the merge.guitool to be used. Ditto. > diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt > @@ -97,10 +97,12 @@ instead. `--no-symlinks` is the default on Windows. > --[no-]gui:: > When 'git-difftool' is invoked with the `-g` or `--gui` option > the default diff tool will be read from the configured > + `diff.guitool` variable instead of `diff.tool`. This may be > + autoselected using the configuration variable > + `difftool.guiDefault`. The `--no-gui` option can be used to > + override these settings. If `diff.guitool` is not set, we will > + fallback in the order of `merge.guitool`, `diff.tool`, > + `merge.tool` until a tool is found. Correct use of backticks here. Good. Probably want: s/autoselected/auto-selected/ or /.../selected automatically/ > diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt > @@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited. > + configured under `merge.tool`. This may be autoselected using > + the configuration variable `mergetool.guiDefault`. Ditto: "autoselected" > --no-gui:: > + This overrides a previous `-g` or `--gui` setting or > + `mergetool.guiDefault` configuration and reads the default merge > + tool from the configured `merge.tool` variable. Backticks; good. > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > @@ -97,7 +97,33 @@ merge_mode () { > +get_gui_default () { > + if diff_mode > + then > + GUI_DEFAULT_KEY="difftool.guiDefault" > + else > + GUI_DEFAULT_KEY="mergetool.guiDefault" > + fi > + GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY | tr 'A-Z' 'a-z') Too many spaces before pipe symbol. Nit: It doesn't matter in this case, but you could safeguard against (possible?) future problems by using double-quotes in `--get "$GUI_DEFAULT_KEY"`. > + if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto" > + then > + if test -n "$DISPLAY" > + then > + GUI_DEFAULT=true > + else > + GUI_DEFAULT=false > + fi > + else > + GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY) Ditto: `--get "$GUI_DEFAULT_KEY"` > + fi > + echo $GUI_DEFAULT > +} > + > gui_mode () { > + if [ -z "$GIT_MERGETOOL_GUI" ] Style: if test -z "$GIT_MERGETOOL_GUI" > + then > + GIT_MERGETOOL_GUI=$(get_gui_default) > + fi > test "$GIT_MERGETOOL_GUI" = true > } > > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh > @@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' ' > +test_expect_success 'mergetool with guiDefault' ' > + test_config mergetool.guiDefault true && > + yes "" | git mergetool subdir/file3 && > + > + yes "d" | git mergetool file11 && > + yes "d" | git mergetool file12 && > + yes "l" | git mergetool submod && > + > + > + echo "gui main updated" >expect && > + test_cmp expect file1 && Accidental double-spacing?
On Fri, Oct 14, 2022 at 10:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > backticks > selected automatically > spaces > test Ack on all these, my apologies, and thank you!
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > * In git-mergetool--lib.sh the way I implemented the "auto" special > value means that if you put an arbitrary value in the config, eg the > typo "uato", you get an error about it being an invalid boolean > config value; is that OK? Is there a better way to handle "boolean or > special value" config validation? Are there any examples? I think the ideal behaviour would be: * Unless running difftool and difftool.guiChoice has a wrong value, or running mergetool and mergetool.guiChoice has a wrong value, we should not even complain. * If the command line says --gui or --no-gui that makes the setting irrelevant, it is OK for us to give a warning to remind the user that they may want to fix the spelling of the variable, but otherwise go ahead and perform the action as they asked us to. * If the command line lacks --gui or --no-gui, we do need to have a usable value in the configuration, and we should error out without spawning either gui or no-gui tool backend. It may be usable without the second one and always fail difftool and mergetool until the setting gets fixed, but that is less than ideal. We do allow less than ideal code in, as long as it is an improvement over the status quo, and its presence does not make it harder to later get closer to the ideal. Thanks.
On Fri, Oct 14, 2022 at 5:45 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > * In git-mergetool--lib.sh the way I implemented the "auto" special > > value means that if you put an arbitrary value in the config, eg the > > typo "uato", you get an error about it being an invalid boolean > > config value; is that OK? Is there a better way to handle "boolean or > > special value" config validation? Are there any examples? > > I think the ideal behaviour would be: > > * Unless running difftool and difftool.guiChoice has a wrong value, > or running mergetool and mergetool.guiChoice has a wrong value, > we should not even complain. This is how it behaves right now, yes. > > * If the command line says --gui or --no-gui that makes the setting > irrelevant, it is OK for us to give a warning to remind the user > that they may want to fix the spelling of the variable, but > otherwise go ahead and perform the action as they asked us to. In the current implementation, there is no warning if the choice has been explicitly made - there is no reason to check the configured default under such circumstances. > > * If the command line lacks --gui or --no-gui, we do need to have a > usable value in the configuration, and we should error out > without spawning either gui or no-gui tool backend. This is *not* the current behavior - currently an error is printed, but execution continues with a no-gui default. I will correct this. > > It may be usable without the second one and always fail difftool and > mergetool until the setting gets fixed, but that is less than ideal. > We do allow less than ideal code in, as long as it is an improvement > over the status quo, and its presence does not make it harder to > later get closer to the ideal. Thanks for the logic table, sounds good. However, my concern was with the precise form of the error if a configured value is neither trueish, not falseish, nor a case-tolerant form of "auto": under such circumstances, instead of a dedicated error message along the lines of "fatal: bad config value 'autod' for 'difftool.guidefault' - must be boolean or 'auto'", we get the *default* error message for *boolean* config settings: "fatal: bad boolean config value 'autod' for 'difftool.guidefault'". This can of course be corrected, I just need to work out how, and exactly what form the error message should then take. Should we be elaborating an error message for this specific tristate config value, or is it "normal" fail with a slightly-misleading message suggesting it is a simple boolean config in such cases? Thanks, Tao
Tao Klerks <tao@klerks.biz> writes: >> * If the command line says --gui or --no-gui that makes the setting >> irrelevant, it is OK for us to give a warning to remind the user >> that they may want to fix the spelling of the variable, but >> otherwise go ahead and perform the action as they asked us to. > > In the current implementation, there is no warning if the choice has > been explicitly made - there is no reason to check the configured > default under such circumstances. Yeah, not making any noise is better. I only meant that if the implementation warns in this irrelevant case, it is OK (aka "acceptable, not necessarily desired") as long as it does not stop. >> * If the command line lacks --gui or --no-gui, we do need to have a >> usable value in the configuration, and we should error out >> without spawning either gui or no-gui tool backend. > > This is *not* the current behavior - currently an error is printed, > but execution continues with a no-gui default. I will correct this. Sounds good. > 'difftool.guidefault' - must be boolean or 'auto'", we get the > *default* error message for *boolean* config settings: "fatal: bad > boolean config value 'autod' for 'difftool.guidefault'". Yeah, I do not think it is a problem, and it is not misleading, as long as the user knows how to ask further information with "git difftool --help" and the help page says what the acceptable values are other than Boolean yes/no. Thanks.
diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt index a3f82112102..8c2f7460be4 100644 --- a/Documentation/config/difftool.txt +++ b/Documentation/config/difftool.txt @@ -34,3 +34,10 @@ See the `--trust-exit-code` option in linkgit:git-difftool[1] for more details. difftool.prompt:: Prompt before each invocation of the diff tool. + +difftool.guiDefault:: + Set 'true' to use the diff.guitool by default (equivalent to specifying + the "--gui" argument), or "auto" to select diff.guitool or diff.tool + depending on the presence of a DISPLAY environment variable value. The + default is 'false', where the "--gui" argument must be provided + explicitly for the diff.guitool to be used. diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 90b38097002..bc1a85abc95 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -85,3 +85,10 @@ mergetool.writeToTemp:: mergetool.prompt:: Prompt before each invocation of the merge resolution program. + +mergetool.guiDefault:: + Set 'true' to use the merge.guitool by default (equivalent to + specifying the "--gui" argument), or "auto" to select merge.guitool + or merge.tool depending on the presence of a DISPLAY environment + variable value. The default is 'false', where the "--gui" argument + must be provided explicitly for the merge.guitool to be used. diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt index 9d14c3c9f09..a09dfb072d5 100644 --- a/Documentation/git-difftool.txt +++ b/Documentation/git-difftool.txt @@ -97,10 +97,12 @@ instead. `--no-symlinks` is the default on Windows. --[no-]gui:: When 'git-difftool' is invoked with the `-g` or `--gui` option the default diff tool will be read from the configured - `diff.guitool` variable instead of `diff.tool`. The `--no-gui` - option can be used to override this setting. If `diff.guitool` - is not set, we will fallback in the order of `merge.guitool`, - `diff.tool`, `merge.tool` until a tool is found. + `diff.guitool` variable instead of `diff.tool`. This may be + autoselected using the configuration variable + `difftool.guiDefault`. The `--no-gui` option can be used to + override these settings. If `diff.guitool` is not set, we will + fallback in the order of `merge.guitool`, `diff.tool`, + `merge.tool` until a tool is found. --[no-]trust-exit-code:: 'git-difftool' invokes a diff tool individually on each file. diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index c44e205629b..07535f6576e 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -85,12 +85,13 @@ success of the resolution after the custom tool has exited. the default merge tool will be read from the configured `merge.guitool` variable instead of `merge.tool`. If `merge.guitool` is not set, we will fallback to the tool - configured under `merge.tool`. + configured under `merge.tool`. This may be autoselected using + the configuration variable `mergetool.guiDefault`. --no-gui:: - This overrides a previous `-g` or `--gui` setting and reads the - default merge tool will be read from the configured `merge.tool` - variable. + This overrides a previous `-g` or `--gui` setting or + `mergetool.guiDefault` configuration and reads the default merge + tool from the configured `merge.tool` variable. -O<orderfile>:: Process files in the order specified in the diff --git a/builtin/difftool.c b/builtin/difftool.c index 4b10ad1a369..374bb7df34f 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -680,14 +680,37 @@ static int run_file_diff(int prompt, const char *prefix, return run_command(child); } +enum difftool_gui_mode { + GUI_BY_CONFIG = -1, + GUI_DISABLED = 0, + GUI_ENABLED = 1 +}; + +static int difftool_opt_gui(const struct option *opt, + const char *optarg, int unset) +{ + enum difftool_gui_mode *mode; + mode = opt->value; + + BUG_ON_OPT_ARG(optarg); + + if (unset) + *mode = GUI_DISABLED; + else + *mode = GUI_ENABLED; + return 0; +} + int cmd_difftool(int argc, const char **argv, const char *prefix) { - int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0, - tool_help = 0, no_index = 0; + int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0, + no_index = 0; + enum difftool_gui_mode gui_mode = GUI_BY_CONFIG; static char *difftool_cmd = NULL, *extcmd = NULL; struct option builtin_difftool_options[] = { - OPT_BOOL('g', "gui", &use_gui_tool, - N_("use `diff.guitool` instead of `diff.tool`")), + OPT_CALLBACK_F('g', "gui", &gui_mode, NULL, + N_("use `diff.guitool` instead of `diff.tool`"), + PARSE_OPT_NOARG, difftool_opt_gui), OPT_BOOL('d', "dir-diff", &dir_diff, N_("perform a full-directory diff")), OPT_SET_INT_F('y', "no-prompt", &prompt, @@ -732,13 +755,21 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) } else if (dir_diff) die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index"); - die_for_incompatible_opt3(use_gui_tool, "--gui", + die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui", !!difftool_cmd, "--tool", !!extcmd, "--extcmd"); - if (use_gui_tool) + /* + * Explicitly specified GUI option is forwarded to git-mergetool--lib.sh; + * empty or unset means "use the difftool.guiDefault config or default to + * false". + */ + if (gui_mode == GUI_ENABLED) setenv("GIT_MERGETOOL_GUI", "true", 1); - else if (difftool_cmd) { + else if (gui_mode == GUI_DISABLED) + setenv("GIT_MERGETOOL_GUI", "false", 1); + + if (difftool_cmd) { if (*difftool_cmd) setenv("GIT_DIFF_TOOL", difftool_cmd, 1); else diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 9f99201bcca..ff08eb7ae93 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -97,7 +97,33 @@ merge_mode () { test "$TOOL_MODE" = merge } +get_gui_default () { + if diff_mode + then + GUI_DEFAULT_KEY="difftool.guiDefault" + else + GUI_DEFAULT_KEY="mergetool.guiDefault" + fi + GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY | tr 'A-Z' 'a-z') + if test "$GUI_DEFAULT_CONFIG_LCASE" = "auto" + then + if test -n "$DISPLAY" + then + GUI_DEFAULT=true + else + GUI_DEFAULT=false + fi + else + GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY) + fi + echo $GUI_DEFAULT +} + gui_mode () { + if [ -z "$GIT_MERGETOOL_GUI" ] + then + GIT_MERGETOOL_GUI=$(get_gui_default) + fi test "$GIT_MERGETOOL_GUI" = true } diff --git a/git-mergetool.sh b/git-mergetool.sh index f751d9cfe20..f510ff9ea70 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -451,7 +451,7 @@ print_noop_and_exit () { main () { prompt=$(git config --bool mergetool.prompt) - GIT_MERGETOOL_GUI=false + GIT_MERGETOOL_GUI= guessed_merge_tool=false orderfile= diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 8cc64729adb..3a807fd681c 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -860,4 +860,43 @@ test_expect_success 'mergetool hideResolved' ' git commit -m "test resolved with mergetool" ' +test_expect_success 'mergetool with guiDefault' ' + test_config merge.guitool myguitool && + test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" && + test_config mergetool.myguitool.trustExitCode true && + test_when_finished "git reset --hard" && + git checkout -b test$test_count branch1 && + git submodule update -N && + test_must_fail git merge main && + + test_config mergetool.guiDefault auto && + DISPLAY=SOMETHING && export DISPLAY && + yes "" | git mergetool both && + yes "" | git mergetool file1 file1 && + + DISPLAY= && export DISPLAY && + yes "" | git mergetool file2 "spaced name" && + + test_config mergetool.guiDefault true && + yes "" | git mergetool subdir/file3 && + + yes "d" | git mergetool file11 && + yes "d" | git mergetool file12 && + yes "l" | git mergetool submod && + + + echo "gui main updated" >expect && + test_cmp expect file1 && + + echo "main new" >expect && + test_cmp expect file2 && + + echo "gui main new sub" >expect && + test_cmp expect subdir/file3 && + + echo "branch1 submodule" >expect && + test_cmp expect submod/bar && + git commit -m "branch1 resolved with mergetool" +' + test_done diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 096456292c0..1e47d3c05bd 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -155,6 +155,58 @@ test_expect_success 'difftool honors --gui' ' test_cmp expect actual ' +test_expect_success 'difftool with guiDefault auto selects gui tool when there is DISPLAY' ' + difftool_test_setup && + test_config merge.tool bogus-tool && + test_config diff.tool bogus-tool && + test_config diff.guitool test-tool && + test_config difftool.guiDefault auto && + DISPLAY=SOMETHING && export DISPLAY && + + echo branch >expect && + git difftool --no-prompt branch >actual && + test_cmp expect actual +' +test_expect_success 'difftool with guiDefault auto selects regular tool when no DISPLAY' ' + difftool_test_setup && + test_config diff.guitool bogus-tool && + test_config diff.tool test-tool && + test_config difftool.guiDefault Auto && + DISPLAY= && export DISPLAY && + + echo branch >expect && + git difftool --no-prompt branch >actual && + test_cmp expect actual +' + +test_expect_success 'difftool with guiDefault true selects gui tool' ' + difftool_test_setup && + test_config diff.tool bogus-tool && + test_config diff.guitool test-tool && + test_config difftool.guiDefault true && + + DISPLAY= && export DISPLAY && + echo branch >expect && + git difftool --no-prompt branch >actual && + test_cmp expect actual && + + DISPLAY=Something && export DISPLAY && + echo branch >expect && + git difftool --no-prompt branch >actual && + test_cmp expect actual +' + +test_expect_success 'difftool --no-gui trumps config guiDefault' ' + difftool_test_setup && + test_config diff.guitool bogus-tool && + test_config diff.tool test-tool && + test_config difftool.guiDefault true && + + echo branch >expect && + git difftool --no-prompt --no-gui branch >actual && + test_cmp expect actual +' + test_expect_success 'difftool --gui last setting wins' ' difftool_test_setup && : >expect &&