On 02/12/2021 16:58, Junio C Hamano wrote:
> 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.
>
Just an FYI - I had t3701-add-interactive.sh show:
# 2 known breakage(s) vanished; please update test(s)
on Linux tonight (tests #45 and #47).
I assumed, with little (well, any) thought, that these vanishing
breakages are due to this 'js/use-builtin-add-i' branch.
Just ignore me (and apologies in advance), if this is not the case! ;-)
ATB,
Ramsay Jones
