Johannes Schindelin [off-list ref] writes:
quoted
quoted
+ 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.
On 02/12/2021 16:58, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
quoted
+ 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
Hi Junio,
On Thu, 2 Dec 2021, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
quoted
+ 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