git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Tao Klerks <tao@klerks.biz>
To: Junio C Hamano <gitster@pobox.com>
Cc: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY
Date: Fri, 14 Oct 2022 06:53:03 +0200	[thread overview]
Message-ID: <CAPMMpoigKVzhXu1WqSrx13MP1jR0J+ajkwRVVtY_LwHYAoWeTg@mail.gmail.com> (raw)
In-Reply-To: <xmqqedvbcrnn.fsf@gitster.g>

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.

  reply	other threads:[~2022-10-14  4:53 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-12 15:59 [PATCH] mergetool: new config guiDefault supports auto-toggling gui by DISPLAY Tao Klerks via GitGitGadget
2022-10-12 16:08 ` Tao Klerks
2022-10-12 18:12 ` Junio C Hamano
2022-10-13  6:49   ` Tao Klerks
2022-10-13 21:58 ` Junio C Hamano
2022-10-14  4:53   ` Tao Klerks [this message]
2022-10-14  9:10     ` Junio C Hamano
2022-10-14 12:32       ` Tao Klerks
2022-10-14 15:37         ` Junio C Hamano
2022-10-16 20:07           ` Tao Klerks
2022-10-14  8:00 ` [PATCH v2] RFC: " Tao Klerks via GitGitGadget
2022-10-14  8:24   ` Eric Sunshine
2022-10-14  9:11     ` Tao Klerks
2022-10-14 15:45   ` Junio C Hamano
2022-10-16 20:19     ` Tao Klerks
2022-10-17  5:50       ` Junio C Hamano
2022-10-18  6:54   ` [PATCH v3] " Tao Klerks via GitGitGadget
2023-02-17 10:59     ` Tao Klerks
2023-03-18 15:27     ` [PATCH v4] " Tao Klerks via GitGitGadget
2023-04-04  9:46       ` David Aguilar
2023-04-04 14:50         ` Tao Klerks
2023-04-04 20:35           ` Junio C Hamano
2023-04-06  3:33             ` David Aguilar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPMMpoigKVzhXu1WqSrx13MP1jR0J+ajkwRVVtY_LwHYAoWeTg@mail.gmail.com \
    --to=tao@klerks.biz \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).