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