Re: [PATCH v2] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY

[Tao Klerks <tao@klerks.biz>]

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