Hi Junio,
On Thu, 2 Dec 2021, 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.
That forces the reader to perform those mental gymnastics to follow the
reasoning that the tristate now essentially became a Boolean.
I wanted to avoid that unnecessary cognitive load.
Ciao,
Dscho
