Re: [PATCH 2/2] add -i: default to the built-in implementation

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Wed, 1 Dec 2021, Phillip Wood wrote:

> On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
>
> > diff --git a/builtin/add.c b/builtin/add.c
> > index ef6b619c45e..8ef230a345b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const
> > char *patch_mode,
> >    int use_builtin_add_i =
> >     git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
> >   -	if (use_builtin_add_i < 0) {
> > -		int experimental;
> > -		if (!git_config_get_bool("add.interactive.usebuiltin",
> > -					 &use_builtin_add_i))
> > -			; /* ok */
> > -		else if (!git_config_get_bool("feature.experimental",
> > &experimental) &&
> > -			 experimental)
> > -			use_builtin_add_i = 1;
> > -	}
> > +	if (use_builtin_add_i < 0 &&
> > +	    git_config_get_bool("add.interactive.usebuiltin",
> > +				&use_builtin_add_i))
> > +		use_builtin_add_i = 1;
> >   -	if (use_builtin_add_i == 1) {
> > +	if (use_builtin_add_i != 0) {
>
> This could be simplified to "if (use_builtin_add_i)" but don't re-roll just
> for that

I was actually considering this, given that Git's coding practice suggests
precisely the form you suggested.

However, in this instance I found that form misleading: it would read to
me as if `use_builtin_add_i` was a Boolean. But it is a tristate, it can
also be `-1` ("undecided"). And I wanted to express "if this is not set to
`false` specifically", therefore I ended up with my proposal.

Ciao,
Dscho