mergetool: new config guiDefault supports auto-toggling gui by DISPLAY #1381
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 2be3e23: |
TaoK
force-pushed
the
tao-mergetool-autogui
branch
from
2be3e23
to
796ce96
Compare
|
There are issues in commit 796ce96: |
TaoK
force-pushed
the
tao-mergetool-autogui
branch
2 times, most recently
from
844bb56
to
4e34953
Compare
|
Note to self: there was an error in the previous build run, and it was in a file touched in this merge request, but I simply can't understand how this error could be caused by the command that apparently caused it / failed! (and only in the one OSX build... and only with --no-symlinks???) I might be misunderstanding the logs in some important way, but the claim is that a command I've triggered a new build, but am tempted to claim it's a bit-flip error (which just happened to hit the test I updated...? Hmm...) https://github.com/gitgitgadget/git/actions/runs/3232738504/jobs/5293785327 |
|
/submit |
|
Submitted as pull.1381.git.1665590389045.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Tao Klerks wrote (reply to this): On Wed, Oct 12, 2022 at 5:59 PM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tao Klerks <tao@klerks.biz>
>
> When no merge.tool or diff.tool is configured or manually selected, the
> selection of a default tool is sensitive to the DISPLAY variable; in a
> GUI session a gui-specific tool will be proposed if found, and
> otherwise a terminal-based one. This "GUI-optimizing" behavior is
> important because a GUI can make a huge difference to a user's ability
> to understand and correctly complete a non-trivial conflicting merge.
>
Quick clarification - this was intended to be an RFC.
I forgot that with GitGitGadget at the moment (until we do something
to explicitly support RFCs) a single-commit patch needs the *commit*
to have the RFC prefix, instead of the Merge Request, as the commit's
subject is used instead of the merge request's subject in a
single-commit patch.
I will make sure any later RFC versions have the prefix as appropriate. |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Tao Klerks <tao@klerks.biz>
>
> When no merge.tool or diff.tool is configured or manually selected, the
> selection of a default tool is sensitive to the DISPLAY variable; in a
> GUI session a gui-specific tool will be proposed if found, and
> otherwise a terminal-based one. This "GUI-optimizing" behavior is
> important because a GUI can make a huge difference to a user's ability
> to understand and correctly complete a non-trivial conflicting merge.
>
> Some time ago the merge.guitool and diff.guitool config options were
> introduced to enable users to configure both a GUI tool, and a non-GUI
> tool (with fallback if no GUI tool configured), in the same environment.
>
> Unfortunately, the --gui argument introduced to support the selection of
> the guitool is still explicit. When using configured tools, there is no
> equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
> environment" behavior.
>
> Introduce new configuration options, difftool.guiDefault and
> mergetool.guiDefault, supporting a special value "auto" which causes the
> corresponding tool or guitool to be selected depending on the presence of a
> non-empty DISPLAY value. Also support "true" to say "default to the guitool
> (unless --no-gui is passed on the commandline)", and "false" as the previous
> default behavior when these new configuration options are not specified.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
> RFC: mergetool: new config guiDefault supports auto-toggling gui by
> DISPLAY
This somehow felt somewhat familiar, so I had to go the list archive
to find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,
which seems to be the previous discussion.
It would have been much easier if you gave readers the original
context that inspired this design.
|
|
On the Git mailing list, Tao Klerks wrote (reply to this): On Wed, Oct 12, 2022 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> This somehow felt somewhat familiar, so I had to go the list archive
> to find https://lore.kernel.org/git/xmqqmtb8jsej.fsf@gitster.g/,
> which seems to be the previous discussion.
>
> It would have been much easier if you gave readers the original
> context that inspired this design.
>
My apologies, I should have included this link in the commit message -
I tried to "fix it" by reviving the thread in question, but clearly
too little too late. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +enum difftool_gui_mode {
> + GUI_DISABLED = -1,
> + GUI_BY_CONFIG = 0,
> + GUI_ENABLED = 1
> +};
> +
> +static int difftool_opt_gui(const struct option *opt,
> + const char *optarg, int unset)
> +{
> + enum difftool_gui_mode *mode;
> + mode = opt->value;
> +
> + BUG_ON_OPT_ARG(optarg);
> +
> + if (unset)
> + *mode = GUI_DISABLED;
> + else
> + *mode = GUI_ENABLED;
> + return 0;
> +}
> +
> int cmd_difftool(int argc, const char **argv, const char *prefix)
> {
> - int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> - tool_help = 0, no_index = 0;
> + int dir_diff = 0, prompt = -1, symlinks = 0, tool_help = 0,
> + no_index = 0;
> + enum difftool_gui_mode gui_mode;
This is left uninitialized ...
> static char *difftool_cmd = NULL, *extcmd = NULL;
> struct option builtin_difftool_options[] = {
> - OPT_BOOL('g', "gui", &use_gui_tool,
> - N_("use `diff.guitool` instead of `diff.tool`")),
> + OPT_CALLBACK_F('g', "gui", &gui_mode, NULL,
> + N_("use `diff.guitool` instead of `diff.tool`"),
> + PARSE_OPT_NOARG, difftool_opt_gui),
... and its address is used here. When "--no-gui" or "--gui" option
is given, the callback function will fill either _DISABLED or _ENABLED
to it.
But without any command line argument? Isn't gui_mode variable ...
> OPT_BOOL('d', "dir-diff", &dir_diff,
> N_("perform a full-directory diff")),
> OPT_SET_INT_F('y', "no-prompt", &prompt,
> @@ -732,13 +755,16 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> } else if (dir_diff)
> die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
>
... still uninitialized here? The old use_gui_tool was initialized
to 0 so it wouldn't have had this problem.
> - die_for_incompatible_opt3(use_gui_tool, "--gui",
> + die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
> !!difftool_cmd, "--tool",
> !!extcmd, "--extcmd");
>
> - if (use_gui_tool)
> + if (gui_mode == GUI_ENABLED)
> setenv("GIT_MERGETOOL_GUI", "true", 1);
I suspect that there is no need to introduce a enum. The flow would
probably be
* git_config(difftool_config) would learn to parse the .guiDefault
option and initialize use_gui_tool to -1 when set to "auto" (and
to 0 with "false", to 1 with "true").
* Call to parse_options() then overwrites use_gui_tool with either
0 or 1 when --no-gui or --gui is given.
* After parse_options() returns, use_gui_tool can be examined and
when it is 0 or 1, then nothing need to change. The current code
before this patch is doing what the user wants when an explicit
command line option is given.
* When use_gui_tool is -1, we need a new code that sets it to
either 0 or 1 depending on the running environment.
But what is curious is that nothing in C code even looks at .guiDefault
configuration, so I am not sure why you would even need to change
anything in builtin/difftool.c file at all.
Puzzled.
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 9f99201bcca..8dbd04e5c5c 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -97,7 +97,33 @@ merge_mode () {
> test "$TOOL_MODE" = merge
> }
>
> +get_gui_default () {
> + if diff_mode
> + then
> + GUI_DEFAULT_KEY="difftool.guiDefault"
> + else
> + GUI_DEFAULT_KEY="mergetool.guiDefault"
> + fi
> + GUI_DEFAULT_CONFIG_LCASE=$(git config --default false --get $GUI_DEFAULT_KEY | tr '[:upper:]' '[:lower:]')
Avoid [:class:] when 'A-Z' 'a-z' is sufficient. Easier to read and
you do not even need to worry about portability that way.
> + if [ "$GUI_DEFAULT_CONFIG_LCASE" = "auto" ]
> + then
> + if [ -n "$DISPLAY" ]
> + then
> + GUI_DEFAULT=true
> + else
> + GUI_DEFAULT=false
> + fi
> + else
> + GUI_DEFAULT=$(git config --default false --bool --get $GUI_DEFAULT_KEY)
> + fi
> + echo $GUI_DEFAULT
> +}
Check Documentation/CodingGuidelines with special attention to the
"for shell scripts" section? |
|
On the Git mailing list, Tao Klerks wrote (reply to this): On Thu, Oct 13, 2022 at 11:58 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > + enum difftool_gui_mode gui_mode;
>
> This is left uninitialized ...
>
This is clearly a bug, and you've just solved a mystery for me. At one
point one test behaved "crazily", reporting an error as though --gui
had been specified in conflict with another option, despite it not
having been specified at all! I was unable to reproduce this behavior
in any way, and uncomfortably declared it must have been a "bit-flip"
error. Now I understand why, and how this class of error can occur in
C. Thank you for spotting this.
I had incorrectly assumed that it would be implicitly initialized to
0, meaning GUI_BY_CONFIG. Even if that had been true, it was a mistake
on my part to not be explicit about this default value - I failed to
use the enum for its intended purpose of making the code easy to
understand.
>
> [...] Isn't gui_mode variable ...
> ... still uninitialized here? The old use_gui_tool was initialized
> to 0 so it wouldn't have had this problem.
Absolutely. This meant that *occasionally*, it would look like --gui
or --no-gui was specified even though it was not. Most of the time the
uninitialized value did not exactly match 1 or -1, so it looked like
things were working correctly, even though they were doing so
"accidentally".
>
> > - die_for_incompatible_opt3(use_gui_tool, "--gui",
> > + die_for_incompatible_opt3(gui_mode == GUI_ENABLED, "--gui",
> > !!difftool_cmd, "--tool",
> > !!extcmd, "--extcmd");
> >
> > - if (use_gui_tool)
> > + if (gui_mode == GUI_ENABLED)
> > setenv("GIT_MERGETOOL_GUI", "true", 1);
>
> I suspect that there is no need to introduce a enum.
An enum is unnecessary, I only added it in order for a tristate
"gui_mode" value to be legible.
Case in point, you proposed that -1 could mean "_BY_CONFIG", whereas I
(possibly against all prior logic in this codebase) assumed it would
make more sense to have -1 be "_FALSE" in this case; the enum helps
avoid any confusion. If you find the enum has a nontrivial cost or
fails to improve understandability I am happy to remove it and use
simple int values instead.
> The flow would
> probably be
>
> * git_config(difftool_config) would learn to parse the .guiDefault
> option and initialize use_gui_tool to -1 when set to "auto" (and
> to 0 with "false", to 1 with "true").
>
> * Call to parse_options() then overwrites use_gui_tool with either
> 0 or 1 when --no-gui or --gui is given.
>
> * After parse_options() returns, use_gui_tool can be examined and
> when it is 0 or 1, then nothing need to change. The current code
> before this patch is doing what the user wants when an explicit
> command line option is given.
>
> * When use_gui_tool is -1, we need a new code that sets it to
> either 0 or 1 depending on the running environment.
>
> But what is curious is that nothing in C code even looks at .guiDefault
> configuration, so I am not sure why you would even need to change
> anything in builtin/difftool.c file at all.
>
> Puzzled.
>
There are two interesting things here I guess:
1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
OPT_BOOL arg type?
Because OPT_BOOL always overwrites with either true or false, so in
order to have use_gui_tool retain its "_BY_CONFIG" value in the
absence of an arg, the explicit arg handler function is required.
2. Why is the "auto" case not explicitly addressed in this code at all?
Because both difftool and mergetool use the same tool-finding logic in
git-mergetool--lib.sh. The C code is just a (complex) "shell", setting
the environment such that git-mergetool--lib.sh knows what to do. Wrt
the gui decision, the implementation here has it set
"GIT_MERGETOOL_GUI" to "true" or "false" if the user has made an
explicit decision, and if it is blank or unset that means "use the
config". The tristate signal in difftool.c is "GUI, NO_GUI or
USE_CONFIG". It would be possible to parse the config in C and change
the tristate signal being passed down to instead be "GUI, NO_GUI or
AUTO", but I believe it would make things more complicated.
I understand "interesting things that need to be explained" are
something one tries to avoid in the source code - I will see if adding
a couple of well-placed comments makes things cleaner.
>
> Avoid [:class:] when 'A-Z' 'a-z' is sufficient. Easier to read and
> you do not even need to worry about portability that way.
>
Sounds good!
>
> Check Documentation/CodingGuidelines with special attention to the
> "for shell scripts" section?
Will do.
As always, thank you for taking the time to look at this! V2 coming soon. |
TaoK
force-pushed
the
tao-mergetool-autogui
branch
from
4e34953
to
70d0e0d
Compare
|
/submit |
|
Submitted as pull.1381.v2.git.1665734440009.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): "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? |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Tao Klerks <tao@klerks.biz> writes:
> 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
> OPT_BOOL arg type?
> Because OPT_BOOL always overwrites with either true or false, so in
> order to have use_gui_tool retain its "_BY_CONFIG" value in the
> absence of an arg, the explicit arg handler function is required.
That is exactly why you want to initialize the variable to something
other than 0 or 1 (say "-1") and use that as "not explicitly
specified from the command line" aka _BY_CONFIG value, isn't it?
And once you do so, you do not need callback and you do not need
enum. The original "int" can take tri-state, (unspecified = -1,
false = 0, true = 1).
Am I missing something? |
|
On the Git mailing list, Tao Klerks wrote (reply to this): 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! |
|
On the Git mailing list, Tao Klerks wrote (reply to this): On Fri, Oct 14, 2022 at 11:11 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > 1. Why add an OPT_CALLBACK_F arg handler, rather than keeping a simple
> > OPT_BOOL arg type?
> > Because OPT_BOOL always overwrites with either true or false, so in
> > order to have use_gui_tool retain its "_BY_CONFIG" value in the
> > absence of an arg, the explicit arg handler function is required.
>
> That is exactly why you want to initialize the variable to something
> other than 0 or 1 (say "-1") and use that as "not explicitly
> specified from the command line" aka _BY_CONFIG value, isn't it?
>
> And once you do so, you do not need callback and you do not need
> enum. The original "int" can take tri-state, (unspecified = -1,
> false = 0, true = 1).
>
> Am I missing something?
Heh, I guess one of us must be, and it's probably me :)
My understanding, from ad-hoc testing, is that using OPT_BOOL causes
the target variable ("use_gui_tool" in this case) to *always* be set
to either "1" or "0", regardless of whether the user sets the main
flag, the --no form of the flag, or sets nothing at all. As far as I
can tell, OPT_BOOL never leaves the original value, so it cannot be
used to track tri-state "true, false or not specified" logic. Maybe I
made a mistake somewhere, and that's not really or always how it
behaves.
I can re-test, and/or try to dig into the option parsing logic to
understand how it works - the time I tried, I got hopelessly lost and
was relieved to find that OPT_CALLBACK_F existed and seemed to be used
for exactly these kinds of use cases. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Tao Klerks <tao@klerks.biz> writes:
> My understanding, from ad-hoc testing, is that using OPT_BOOL causes
> the target variable ("use_gui_tool" in this case) to *always* be set
> to either "1" or "0", regardless of whether the user sets the main
> flag, the --no form of the flag, or sets nothing at all.
You are saying "git gc --keep-largest-pack" is broken ;-)
OPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there
is no "set to this default value if no command line option is given"
involved. At least I do not recall allowing such a code in.
Thanks. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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 the Git mailing list, Tao Klerks wrote (reply to this): On Fri, Oct 14, 2022 at 5:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > My understanding, from ad-hoc testing, is that using OPT_BOOL causes
> > the target variable ("use_gui_tool" in this case) to *always* be set
> > to either "1" or "0", regardless of whether the user sets the main
> > flag, the --no form of the flag, or sets nothing at all.
>
> You are saying "git gc --keep-largest-pack" is broken ;-)
Now that I look at that, that does appear to have been implied by what
I was saying, yes!
>
> OPT_BOOL() and OPT_BOOL_F() are OPT_SET_INT() in disguise and there
> is no "set to this default value if no command line option is given"
> involved. At least I do not recall allowing such a code in.
>
I can confirm that it works the way you expected. I was sure I had
experienced such behavior during my early attempts at tristate
handling here, but in retrospect I was very much mistaken, not only
because --keep-largest-pack is not broken, but also because I just
ripped out all the arg-parsing function and enum stuff, and it all
works fine with -1, 0 and 1 as you pointed out it should. I don't know
exactly how I messed up my early testing, but I don't suppose it
matters.
Substantial simplification in the next update.
Thanks! |
|
On the Git mailing list, Tao Klerks wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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.
|
TaoK
force-pushed
the
tao-mergetool-autogui
branch
2 times, most recently
from
327019f
to
dbb8049
Compare
|
/submit |
TaoK
force-pushed
the
tao-mergetool-autogui
branch
from
dbb8049
to
c3cfc24
Compare
TaoK
changed the title
TaoK
force-pushed
the
tao-mergetool-autogui
branch
from
c3cfc24
to
765abfe
Compare
|
/submit |
|
Submitted as pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, David Aguilar wrote (reply to this): On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <tao@klerks.biz>
>
> When no merge.tool or diff.tool is configured or manually selected, the
> selection of a default tool is sensitive to the DISPLAY variable; in a
> GUI session a gui-specific tool will be proposed if found, and
> otherwise a terminal-based one. This "GUI-optimizing" behavior is
> important because a GUI can make a huge difference to a user's ability
> to understand and correctly complete a non-trivial conflicting merge.
>
> Some time ago the merge.guitool and diff.guitool config options were
> introduced to enable users to configure both a GUI tool, and a non-GUI
> tool (with fallback if no GUI tool configured), in the same environment.
>
> Unfortunately, the --gui argument introduced to support the selection of
> the guitool is still explicit. When using configured tools, there is no
> equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
> environment" behavior.
I'm not sure what problem this is solving. Please help me understand.
This commit seems to be talking about the behavior w.r.t. the DISPLAY
variable and also about automatically enabling `--gui` automatically.
Regarding `--gui`: Someone that configures `mergetool.guiDefault`
means that all of their invocations should be as-if they had written
`--gui` on the command-line.
So.. if you find that you always use `--gui` then why are
you not configuring `merge.tool` directly and instead configuring
`merge.guitool`?
The purpose of `--gui` is to provide an override for use by tools (such
as Git Cola) to execute an optional, 2nd merge tool separate from that
which the user might configure for command-line use.
This patch basically makes the `merge.tool` configuration useless, so I
don't understand why someone would want to configure this option rather
than just configuring `merge.tool` directly, leading to an overall
simpler and easier to understand system.
If we add this boolean then it becomes harder to understand.
I'm not sure it really provides any additional value to the user.
From my vantage point, the user is better off configuring `merge.tool`
directly rather than the tool becoming more complex.
That said, I probably don't understand and I'd be happy to be
enlightened.
> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration
> options, difftool.guiDefault and mergetool.guiDefault, supporting a special
> value "auto" which causes the corresponding tool or guitool to be selected
> depending on the presence of a non-empty DISPLAY value. Also support "true"
> to say "default to the guitool (unless --no-gui is passed on the
> commandline)", and "false" as the previous default behavior when these new
> configuration options are not specified.
The rationale here is slightly surprising to me as well.
Apparently the user cannot be bothered to configure `merge.tool`
(because we care about this DISPLAY behavior) yet the user is expected
to configure `merge.guiDefault` instead?
I think it'd be better to encourage users to go ahead and configure
`merge.tool`, but again, I'm probably missing context around why.
Small patch note below...
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 992124cc67c..e4e820e6809 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -75,6 +75,11 @@ then
> merge_tool="$GIT_DIFF_TOOL"
> else
> merge_tool="$(get_merge_tool)"
> + subshell_exit_status=$?
> + if test $subshell_exit_status -gt 1
Is exit status 1 special here?
Should this be "if test $subshell_exit_status != 0" instead?
--
David |
|
User |
|
On the Git mailing list, Tao Klerks wrote (reply to this): First of all, thank you for reviewing!!
On Tue, Apr 4, 2023 at 11:46 AM David Aguilar <davvid@gmail.com> wrote:
>
> On Sat, Mar 18, 2023 at 03:27:43PM +0000, Tao Klerks via GitGitGadget wrote:
> > From: Tao Klerks <tao@klerks.biz>
> >
> > When no merge.tool or diff.tool is configured or manually selected, the
> > selection of a default tool is sensitive to the DISPLAY variable; in a
> > GUI session a gui-specific tool will be proposed if found, and
> > otherwise a terminal-based one. This "GUI-optimizing" behavior is
> > important because a GUI can make a huge difference to a user's ability
> > to understand and correctly complete a non-trivial conflicting merge.
> >
> > Some time ago the merge.guitool and diff.guitool config options were
> > introduced to enable users to configure both a GUI tool, and a non-GUI
> > tool (with fallback if no GUI tool configured), in the same environment.
> >
> > Unfortunately, the --gui argument introduced to support the selection of
> > the guitool is still explicit. When using configured tools, there is no
> > equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
> > environment" behavior.
>
> I'm not sure what problem this is solving. Please help me understand.
> This commit seems to be talking about the behavior w.r.t. the DISPLAY
> variable and also about automatically enabling `--gui` automatically.
Background: If you don't set a mergetool at all, git defaults one if
it can. The default you get depends on the DISPLAY variable - if it
looks like you're in a gui context (because we detect that DISPLAY is
set), then we propose a gui tool. Otherwise, we propose a cli tool.
This is particularly valuable for contexts where you might access the
same environment, the same workstation, via multiple contexts -
CLI-only, and GUI. For example, sometimes you connect to the desktop,
and sometimes you SSH into it (without X forwarding). You don't need
to tell git "I want a GUI now please", it figures that out.
Point/problem: As soon as you set "merge.tool", you lose that
autoselection. A reasonable user would assume "there is `merge.tool`,
there is `merge.guitool`, and git knows how to autodetect GUI context
based on the DISPLAY variable... so if I set both `merge.tool` and
`merge.guitool`, I'll get the right one based on context, right? Like
I do when I set neither?"
Unfortunately, that is *not* the case. The gui autodetection only
applies when git is choosing a *defaulted* merge tool, not an
explicitly-defined one. As soon as you specify a tool you want to use,
you need to start *explicitly* telling git whether you want to use a
GUI tool or not. If you get it wrong, an inconvenient or even bad
thing happens: If you're in a GUI context and you forget to say
`--gui`, then you get the CLI tool - not the end of the world, you
might argue. If you're in a non-GUI context and you say `--gui`, you
get a (hopefully intelligible) error about not finding a display, or
worst case, you hang - and git doesn't know what happened, the
messaging has to assume you aborted explicitly.
If there were a way to get git to autoselect "--gui" by presence of
the DISPLAY, as it does for the *default* mergetool, then this kind of
flow would be much more intuitive, usable, etc.
I'm not sure whether I'm explaining better, or just repeating myself,
so I'll stop here :)
>
> Regarding `--gui`: Someone that configures `mergetool.guiDefault`
> means that all of their invocations should be as-if they had written
> `--gui` on the command-line.
Correction: Someone that sets `mergetool.guiDefault` to *true* should
get behavior as-if they had written `--gui` on the command-line. They
could also set *false* (current behavior - explicit), or *auto*, and
get different behavior.
>
> So.. if you find that you always use `--gui` then why are
> you not configuring `merge.tool` directly and instead configuring
> `merge.guitool`?
One reason is because you might choose to do something different in a
*specific* invocation, eg by passing `--no-gui`. This is a common
pattern in git - the config allows you to change the default behavior,
but you can override it on a specific invocation by using the positive
or negative version of the argument.
>
> The purpose of `--gui` is to provide an override for use by tools (such
> as Git Cola) to execute an optional, 2nd merge tool separate from that
> which the user might configure for command-line use.
I'm not aware of having read a statement identifying this
*singular*/sole purpose anywhere, but as you state it with authority
(and I see you're the author of a popular git GUI), I'm happy to
accept that this was originally its only purpose. That would certainly
explain the incongruousness of its behavior wrt (lack of)
autodetection of DISPLAY.
That said, this patch does not in any way diminish the validity or
usability of this usecase. If you don't set `mergetool.guiDefault` or
leave it set to false, that purpose will continue to be perfectly
served.
For other users, like me, who prefer to select a GUI mergetool or not
depending on the *availability* of a GUI environment (auto), or
default to gui but allow manually downgrading to a (defaulted or
configured) CLI-only tool, this configuration will allow a simpler,
more coherent, easier UX.
If everything else here makes sense, I will happily update the
abstract/commit message to reflect the proposed broadening of
application / correct my misunderstanding as to the original intended
scope of the setting.
>
> This patch basically makes the `merge.tool` configuration useless,
I don't understand why you say this. Unless I misunderstand something,
this patch does not break, or even change, any existing behavior.
> so I
> don't understand why someone would want to configure this option rather
> than just configuring `merge.tool` directly, leading to an overall
> simpler and easier to understand system.
(see above - the main usecase is a system you access via both GUI and
CLI contexts, and the secondary usecase is having a default GUI
behavior that you can easily override with `--no-gui`.)
>
> If we add this boolean then it becomes harder to understand.
> I'm not sure it really provides any additional value to the user.
>
> From my vantage point, the user is better off configuring `merge.tool`
> directly rather than the tool becoming more complex.
>
> That said, I probably don't understand and I'd be happy to be
> enlightened.
Please let me know if it makes more sense now. You could argue that
the value is too limited to warrant the additional complexity
(obviously I would disagree), but hopefully the concrete value is
clearer now.
>
>
> > As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration
> > options, difftool.guiDefault and mergetool.guiDefault, supporting a special
> > value "auto" which causes the corresponding tool or guitool to be selected
> > depending on the presence of a non-empty DISPLAY value. Also support "true"
> > to say "default to the guitool (unless --no-gui is passed on the
> > commandline)", and "false" as the previous default behavior when these new
> > configuration options are not specified.
>
> The rationale here is slightly surprising to me as well.
>
> Apparently the user cannot be bothered to configure `merge.tool`
> (because we care about this DISPLAY behavior) yet the user is expected
> to configure `merge.guiDefault` instead?
Umm, no: The user wants `merge.tool` to be set to a specific CLI tool,
they want `merge.guitool` to be set to a specific GUI tool, and they
want the GUI tool to be used in GUI contexts, and the CLI tool to be
used when there is no GUI (eg SSH without X forwarding) - like the
behavior they get if they configure neither `merge.tool` nor
`merge.guitool`.
>
> I think it'd be better to encourage users to go ahead and configure
> `merge.tool`, but again, I'm probably missing context around why.
>
>
> Small patch note below...
>
>
> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> > index 992124cc67c..e4e820e6809 100755
> > --- a/git-difftool--helper.sh
> > +++ b/git-difftool--helper.sh
> > @@ -75,6 +75,11 @@ then
> > merge_tool="$GIT_DIFF_TOOL"
> > else
> > merge_tool="$(get_merge_tool)"
> > + subshell_exit_status=$?
> > + if test $subshell_exit_status -gt 1
>
> Is exit status 1 special here?
> Should this be "if test $subshell_exit_status != 0" instead?
Yes, it is special: 1 means "mergetool was guessed/defaulted",
anything greater than 1 means "mergetool determination failed;
aborting".
I was not very happy with this way of passing information around when
I implemented, and commented in the previous version of the patch
https://lore.kernel.org/git/pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com/
as my last doubt as to the proposed/RFC patch:
After implementing Junio's recommended "fail if defaulting config is
consulted and is invalid" flow, there now needs to be a distinction
between subshell exit code 1, which was used before and indicates
"tool not found or broken; falling back to default" and other
(higher) exit codes, which newly mean "something went wrong, stop!".
The resulting code looks awkward, I can't tell whether I'm missing a
code or even commenting pattern that would make it clearer.
Unfortunately I didn't get any feedback on this, so I went ahead and
resumitted the change as a non-Draft. I'd be very grateful for any
recommendations on how to handle this better. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Tao Klerks <tao@klerks.biz> writes:
> If you get it wrong, an inconvenient or even bad
> thing happens: If you're in a GUI context and you forget to say
> `--gui`, then you get the CLI tool - not the end of the world, you
> might argue. If you're in a non-GUI context and you say `--gui`, you
> get a (hopefully intelligible) error about not finding a display, or
> worst case, you hang - and git doesn't know what happened, the
> messaging has to assume you aborted explicitly.
The worst case I can think of is if you have a machine that allows a
persistent virtual graphical session (remote desktop). You can go
there, open a terminal and create a screen session, which captures
the $DISPLAY in effect. You may disconnect from the screen session
before you disconnect from the remote desktop. As the remote
desktop is persistent, the $DISPLAY would be valid even after you
disconnect from there.
Then you ssh into the machine and connect to the screen session. If
you say "xterm &" in that screen session, a new window with a
terminal will open and be shown on that remote display session,
which you cannot see until you reconnect to the remote desktop.
Running "git mergetool --gui" would be the same.
With the proposed patch, the experience would become even worse in
that scenario, I am afraid. "git mergetool" (without "--[no-]gui")
would detect the presense of $DISPLAY and start a GUI mergetool that
you cannot view or interact with ;-). You need to explicitly say
"git mergetool --no-gui" in such a case to force use of cli tool.
Even with that caveat, I think the proposed behaviour would be a net
improvement. I almost always work in non-GUI environment, but I can
see many people work in both and depending on their environment wish
to use the best tool for the situation.
> If there were a way to get git to autoselect "--gui" by presence of
> the DISPLAY, as it does for the *default* mergetool, then this kind of
> flow would be much more intuitive, usable, etc.
>
> I'm not sure whether I'm explaining better, or just repeating myself,
> so I'll stop here :)
Hopefully your explanation was clear enough. Others may be able to
offer solution better than "look at DISPLAY and choose --[no-]gui
when neither is given from the command line", but I think at least I
understand your motivation behind this change.
Thanks. |
|
On the Git mailing list, David Aguilar wrote (reply to this): On Tue, Apr 4, 2023 at 1:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > If you get it wrong, an inconvenient or even bad
> > thing happens: If you're in a GUI context and you forget to say
> > `--gui`, then you get the CLI tool - not the end of the world, you
> > might argue. If you're in a non-GUI context and you say `--gui`, you
> > get a (hopefully intelligible) error about not finding a display, or
> > worst case, you hang - and git doesn't know what happened, the
> > messaging has to assume you aborted explicitly.
>
> The worst case I can think of is if you have a machine that allows a
> persistent virtual graphical session (remote desktop). You can go
> there, open a terminal and create a screen session, which captures
> the $DISPLAY in effect. You may disconnect from the screen session
> before you disconnect from the remote desktop. As the remote
> desktop is persistent, the $DISPLAY would be valid even after you
> disconnect from there.
>
> Then you ssh into the machine and connect to the screen session. If
> you say "xterm &" in that screen session, a new window with a
> terminal will open and be shown on that remote display session,
> which you cannot see until you reconnect to the remote desktop.
> Running "git mergetool --gui" would be the same.
>
> With the proposed patch, the experience would become even worse in
> that scenario, I am afraid. "git mergetool" (without "--[no-]gui")
> would detect the presense of $DISPLAY and start a GUI mergetool that
> you cannot view or interact with ;-). You need to explicitly say
> "git mergetool --no-gui" in such a case to force use of cli tool.
>
> Even with that caveat, I think the proposed behaviour would be a net
> improvement. I almost always work in non-GUI environment, but I can
> see many people work in both and depending on their environment wish
> to use the best tool for the situation.
>
> > If there were a way to get git to autoselect "--gui" by presence of
> > the DISPLAY, as it does for the *default* mergetool, then this kind of
> > flow would be much more intuitive, usable, etc.
> >
> > I'm not sure whether I'm explaining better, or just repeating myself,
> > so I'll stop here :)
>
> Hopefully your explanation was clear enough. Others may be able to
> offer solution better than "look at DISPLAY and choose --[no-]gui
> when neither is given from the command line", but I think at least I
> understand your motivation behind this change.
>
> Thanks.
Thanks for the thorough explanation. With that in mind, this all
sounds good to me.
--
David |
|
This branch is now known as |
|
This patch series was integrated into seen via git@efbb2b2. |
|
There was a status update in the "New Topics" section about the branch "git mergetool" and "git difftool" learns a new configuration guiDefault to optionally favor configured guitool over non-gui-tool automatically when $DISPLAY is set. Will merge to 'next'. source: <pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@bea9cd3. |
|
This patch series was integrated into seen via git@266ccfa. |
|
This patch series was integrated into seen via git@810bbf9. |
|
This patch series was integrated into next via git@2284325. |
|
This patch series was integrated into seen via git@045218f. |
|
There was a status update in the "Cooking" section about the branch "git mergetool" and "git difftool" learns a new configuration guiDefault to optionally favor configured guitool over non-gui-tool automatically when $DISPLAY is set. Will merge to 'master'. source: <pull.1381.v4.git.1679153263217.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@a4d220a. |
|
This patch series was integrated into seen via git@4d825cb. |
|
This patch series was integrated into seen via git@89d80fc. |
|
This patch series was integrated into seen via git@9d8370d. |
|
This patch series was integrated into master via git@9d8370d. |
|
This patch series was integrated into next via git@9d8370d. |
|
Closed via 9d8370d. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few months later, first "real" submission - 4th version wrt original draft.
cc: Tao Klerks tao@klerks.biz
cc: Eric Sunshine sunshine@sunshineco.com
cc: David Aguilar davvid@gmail.com