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

From: Tao Klerks <tao@klerks.biz>
Date: Oct 16, 2022, 20:19 UTC
Message-ID: <CAPMMpohWE17xk4fF+uQja0jfn53bjgVtg75FKEC1w0Km7U8Dvg@mail.gmail.com>
In-Reply-To: <xmqqczaube8o.fsf@gitster.g>

On Fri, Oct 14, 2022 at 5:45 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

>
> "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.

Show 5 quoted lines

>
>  * 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.

Show 6 quoted lines

>
> 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