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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> > +	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.
>
> I do not think that line of logic is sensible.  The variable starts
> its life as a tristate (i.e. not just bool but can be unknown), and
> the four new lines above the conditional the patch adds is exactly
> about getting rid of the unknown-ness and turning it into a known
> boolean.  After that happens, the variable can safely be used as a
> boolean.  In fact, I view the four lines before it is exactly to
> allow us to do so.
>
> Writing "if not zero" implies that the variable can have a non-zero
> value that is still "unknown" at this point in the code that has to
> be defaulted to "true", which would mean that the "if unset, read
> the config, and if that fails, default to true" logic above is not
> doing its job.  That is a false impression that misleads readers of
> the code.
>
> So, I would say this conditional just should treat the variable as a
> simple boolean.