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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 02/12/2021 16:58, Junio C Hamano wrote:
> 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.
> 

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