On Thu, Oct 13, 2022 at 11:58 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
