Re: [PATCH 2/2] add -i: default to the built-in implementation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 2, 2021, 16:58 UTC
- Message-ID
- <xmqq5ys7vt8t.fsf@gitster.g>
- In-Reply-To
- <nycvar.QRO.7.76.6.2112021557530.63@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 17 quoted lines
>> > + 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.