Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
