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

Commit Message

Johannes Schindelin Nov. 30, 2021, 2:14 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In 9a5315edfdf (Merge branch 'js/patch-mode-in-others-in-c',
2020-02-05), Git acquired a built-in implementation of `git add`'s
interactive mode that could be turned on via the config option
`add.interactive.useBuiltin`.

The first official Git version to support this knob was v2.26.0.

In 2df2d81ddd0 (add -i: use the built-in version when
feature.experimental is set, 2020-09-08), this built-in implementation
was also enabled via `feature.experimental`. The first version with this
change was v2.29.0.

More than a year (and very few bug reports) later, it is time to declare
the built-in implementation mature and to turn it on by default.

We specifically leave the `add.interactive.useBuiltin` configuration in
place, to give users an "escape hatch" in the unexpected case should
they encounter a previously undetected bug in that implementation.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/config/add.txt |  6 +++---
 builtin/add.c                | 15 +++++----------
 ci/run-build-and-tests.sh    |  2 +-
 t/README                     |  2 +-
 t/t2016-checkout-patch.sh    |  2 +-
 5 files changed, 11 insertions(+), 16 deletions(-)

Comments

Phillip Wood Dec. 1, 2021, 11:20 a.m. UTC | #1

Hi Dscho

On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> In 9a5315edfdf (Merge branch 'js/patch-mode-in-others-in-c',
> 2020-02-05), Git acquired a built-in implementation of `git add`'s
> interactive mode that could be turned on via the config option
> `add.interactive.useBuiltin`.
> 
> The first official Git version to support this knob was v2.26.0.
> 
> In 2df2d81ddd0 (add -i: use the built-in version when
> feature.experimental is set, 2020-09-08), this built-in implementation
> was also enabled via `feature.experimental`. The first version with this
> change was v2.29.0.
> 
> More than a year (and very few bug reports) later, it is time to declare
> the built-in implementation mature and to turn it on by default.
> 
> We specifically leave the `add.interactive.useBuiltin` configuration in
> place, to give users an "escape hatch" in the unexpected case should
> they encounter a previously undetected bug in that implementation.

Thanks for doing this, I agree it is time to switch over - it feels like 
it is quite a while since anyone reported a bug with the C version. Both 
patches look good to me. I've left one minor comment below but it is not 
worth re-rolling just for that. Thanks Slavica for your work on this, 
it's great to have it converted to C.


> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>   Documentation/config/add.txt |  6 +++---
>   builtin/add.c                | 15 +++++----------
>   ci/run-build-and-tests.sh    |  2 +-
>   t/README                     |  2 +-
>   t/t2016-checkout-patch.sh    |  2 +-
>   5 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
> index c9f748f81cb..3e859f34197 100644
> --- a/Documentation/config/add.txt
> +++ b/Documentation/config/add.txt
> @@ -7,6 +7,6 @@ add.ignore-errors (deprecated)::
>   	variables.
>   
>   add.interactive.useBuiltin::
> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> -	implementation of the interactive version of linkgit:git-add[1]
> -	instead of the Perl script version. Is `false` by default.
> +	Set to `false` to fall back to the original Perl implementation of
> +	the interactive version of linkgit:git-add[1] instead of the built-in
> +	version. Is `true` by default.
> diff --git a/builtin/add.c b/builtin/add.c
> index ef6b619c45e..8ef230a345b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>   	int use_builtin_add_i =
>   		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>   
> -	if (use_builtin_add_i < 0) {
> -		int experimental;
> -		if (!git_config_get_bool("add.interactive.usebuiltin",
> -					 &use_builtin_add_i))
> -			; /* ok */
> -		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> -			 experimental)
> -			use_builtin_add_i = 1;
> -	}
> +	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

Best Wishes

Phillip

>   		enum add_p_mode mode;
>   
>   		if (!patch_mode)
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index cc62616d806..660ebe8d108 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -29,7 +29,7 @@ linux-gcc)
>   	export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
>   	export GIT_TEST_MULTI_PACK_INDEX=1
>   	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
> -	export GIT_TEST_ADD_I_USE_BUILTIN=1
> +	export GIT_TEST_ADD_I_USE_BUILTIN=0
>   	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>   	export GIT_TEST_WRITE_REV_INDEX=1
>   	export GIT_TEST_CHECKOUT_WORKERS=2
> diff --git a/t/README b/t/README
> index 29f72354bf1..2c22337d6e7 100644
> --- a/t/README
> +++ b/t/README
> @@ -419,7 +419,7 @@ the --sparse command-line argument.
>   GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
>   by overriding the minimum number of cache entries required per thread.
>   
> -GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
> +GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when false, disables the
>   built-in version of git add -i. See 'add.interactive.useBuiltin' in
>   git-config(1).
>   
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 71c5a15be00..bc3f69b4b1d 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>   
>   . ./lib-patch-mode.sh
>   
> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
> +if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
>   then
>   	skip_all='skipping interactive add tests, PERL not set'
>   	test_done
>

Ævar Arnfjörð Bjarmason Dec. 1, 2021, 1:37 p.m. UTC | #2

On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
>
> In 9a5315edfdf (Merge branch 'js/patch-mode-in-others-in-c',
> 2020-02-05), Git acquired a built-in implementation of `git add`'s
> interactive mode that could be turned on via the config option
> `add.interactive.useBuiltin`.
>
> The first official Git version to support this knob was v2.26.0.
>
> In 2df2d81ddd0 (add -i: use the built-in version when
> feature.experimental is set, 2020-09-08), this built-in implementation
> was also enabled via `feature.experimental`. The first version with this
> change was v2.29.0.
>
> More than a year (and very few bug reports) later, it is time to declare
> the built-in implementation mature and to turn it on by default.
>
> We specifically leave the `add.interactive.useBuiltin` configuration in
> place, to give users an "escape hatch" in the unexpected case should
> they encounter a previously undetected bug in that implementation.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  Documentation/config/add.txt |  6 +++---
>  builtin/add.c                | 15 +++++----------
>  ci/run-build-and-tests.sh    |  2 +-
>  t/README                     |  2 +-
>  t/t2016-checkout-patch.sh    |  2 +-
>  5 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
> index c9f748f81cb..3e859f34197 100644
> --- a/Documentation/config/add.txt
> +++ b/Documentation/config/add.txt
> @@ -7,6 +7,6 @@ add.ignore-errors (deprecated)::
>  	variables.
>  
>  add.interactive.useBuiltin::
> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> -	implementation of the interactive version of linkgit:git-add[1]
> -	instead of the Perl script version. Is `false` by default.
> +	Set to `false` to fall back to the original Perl implementation of
> +	the interactive version of linkgit:git-add[1] instead of the built-in
> +	version. Is `true` by default.

I think this would be a bit better if we just stole the version you
added for stash.useBuiltin entirely. I.e. from your 336ad8424cb (stash:
document stash.useBuiltin, 2019-05-14), with the relevant s/shell
script/Perl/g etc. replaced.

I.e. that version encouraged users to report any bugs, because we were
really going to remove it soon, as we then did for rebase.useBuiltin in
9bcde4d5314 (rebase: remove transitory rebase.useBuiltin setting & env,
2021-03-23).

The wording in the opening paragraph is also a bit more to the point
there, i.e. calling it "legacy" rather than "original [...]
implementation".

(I notice that the stash.useBuiltin is still there in-tree, hrm...)

> diff --git a/builtin/add.c b/builtin/add.c
> index ef6b619c45e..8ef230a345b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	int use_builtin_add_i =
>  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>  
> -	if (use_builtin_add_i < 0) {
> -		int experimental;
> -		if (!git_config_get_bool("add.interactive.usebuiltin",
> -					 &use_builtin_add_i))
> -			; /* ok */
> -		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> -			 experimental)
> -			use_builtin_add_i = 1;
> -	}
> +	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) {

Style/idiom: This should just be "if (use_builtin_add_i)".

I.e. before we cared about not catching -1 here, but now that it's true
by default we don't care about the distinction between -1 or 1 anymore,
we just want it not to be 0 here.

Junio C Hamano Dec. 1, 2021, 10:34 p.m. UTC | #3

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> In 9a5315edfdf (Merge branch 'js/patch-mode-in-others-in-c',
> 2020-02-05), Git acquired a built-in implementation of `git add`'s
> interactive mode that could be turned on via the config option
> `add.interactive.useBuiltin`.
>
> The first official Git version to support this knob was v2.26.0.
>
> In 2df2d81ddd0 (add -i: use the built-in version when
> feature.experimental is set, 2020-09-08), this built-in implementation
> was also enabled via `feature.experimental`. The first version with this
> change was v2.29.0.
>
> More than a year (and very few bug reports) later, it is time to declare
> the built-in implementation mature and to turn it on by default.

I think that declaration is a bit premature.  I do agree that by now
we should have killed as many bugs as we could without unleashing it
tot he general public by keeping it behind the experimental flag, and
we should move forward by turn it on by default.  In a month or two
after this change hits a stable release of major distros, we can learn
that the implementation was mature, but not until then ;-).


> We specifically leave the `add.interactive.useBuiltin` configuration in
> place, to give users an "escape hatch" in the unexpected case should
> they encounter a previously undetected bug in that implementation.

Good thinking.

>  add.interactive.useBuiltin::
> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> -	implementation of the interactive version of linkgit:git-add[1]
> -	instead of the Perl script version. Is `false` by default.
> +	Set to `false` to fall back to the original Perl implementation of
> +	the interactive version of linkgit:git-add[1] instead of the built-in
> +	version. Is `true` by default.
> diff --git a/builtin/add.c b/builtin/add.c
> index ef6b619c45e..8ef230a345b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	int use_builtin_add_i =
>  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>  
> -	if (use_builtin_add_i < 0) {
> -		int experimental;
> -		if (!git_config_get_bool("add.interactive.usebuiltin",
> -					 &use_builtin_add_i))
> -			; /* ok */
> -		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> -			 experimental)
> -			use_builtin_add_i = 1;
> -	}
> +	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) {

Nit.

	if (use_builtin_add_i) {

I wondered if these random calls to git_config_get_X() should be
consolidated into the existing add_config() callback, but this
conditional will go away hopefully in a few releases, so it probably
is not worth it.  Inheriting the way the original code grabbed the
configuration variables is good enough for our purpose here.

> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index cc62616d806..660ebe8d108 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -29,7 +29,7 @@ linux-gcc)
>  	export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
>  	export GIT_TEST_MULTI_PACK_INDEX=1
>  	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
> -	export GIT_TEST_ADD_I_USE_BUILTIN=1
> +	export GIT_TEST_ADD_I_USE_BUILTIN=0
>  	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>  	export GIT_TEST_WRITE_REV_INDEX=1
>  	export GIT_TEST_CHECKOUT_WORKERS=2

OK.

> diff --git a/t/README b/t/README
> index 29f72354bf1..2c22337d6e7 100644
> --- a/t/README
> +++ b/t/README
> @@ -419,7 +419,7 @@ the --sparse command-line argument.
>  GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
>  by overriding the minimum number of cache entries required per thread.
>  
> -GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
> +GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when false, disables the
>  built-in version of git add -i. See 'add.interactive.useBuiltin' in
>  git-config(1).
>  
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 71c5a15be00..bc3f69b4b1d 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>  
>  . ./lib-patch-mode.sh
>  
> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
> +if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
>  then
>  	skip_all='skipping interactive add tests, PERL not set'
>  	test_done

Looks good.

Thanks.

Junio C Hamano Dec. 1, 2021, 10:41 p.m. UTC | #4

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

>>  add.interactive.useBuiltin::
>> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
>> -	implementation of the interactive version of linkgit:git-add[1]
>> -	instead of the Perl script version. Is `false` by default.
>> +	Set to `false` to fall back to the original Perl implementation of
>> +	the interactive version of linkgit:git-add[1] instead of the built-in
>> +	version. Is `true` by default.
>
> I think this would be a bit better if we just stole the version you
> added for stash.useBuiltin entirely. I.e. from your 336ad8424cb (stash:
> document stash.useBuiltin, 2019-05-14), with the relevant s/shell
> script/Perl/g etc. replaced.
>
> I.e. that version encouraged users to report any bugs, because we were
> really going to remove it soon, as we then did for rebase.useBuiltin in
> 9bcde4d5314 (rebase: remove transitory rebase.useBuiltin setting & env,
> 2021-03-23).
>
> The wording in the opening paragraph is also a bit more to the point
> there, i.e. calling it "legacy" rather than "original [...]
> implementation".

After reading the description on stash.useBuiltin, I'd agree with
you, except I do not see the distinction between "original" vs
"legacy".  I very much like the way the last paragraph for
"stash.useBuiltin" delivers the right message.

    +If you find some reason to set this option to `false`, other than
    +one-off testing, you should report the behavior difference as a bug in
    +Git (see https://git-scm.com/community for details).


>> -	if (use_builtin_add_i == 1) {
>> +	if (use_builtin_add_i != 0) {
>
> Style/idiom: This should just be "if (use_builtin_add_i)".
>
> I.e. before we cared about not catching -1 here, but now that it's true
> by default we don't care about the distinction between -1 or 1 anymore,
> we just want it not to be 0 here.

Yes, if we are redoing this step anyway, we should lose the explicit
comparison with zero here.

Johannes Schindelin Dec. 2, 2021, 3:01 p.m. UTC | #5

Hi Phillip,

On Wed, 1 Dec 2021, Phillip Wood wrote:

> On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
>
> > diff --git a/builtin/add.c b/builtin/add.c
> > index ef6b619c45e..8ef230a345b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const
> > char *patch_mode,
> >    int use_builtin_add_i =
> >     git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
> >   -	if (use_builtin_add_i < 0) {
> > -		int experimental;
> > -		if (!git_config_get_bool("add.interactive.usebuiltin",
> > -					 &use_builtin_add_i))
> > -			; /* ok */
> > -		else if (!git_config_get_bool("feature.experimental",
> > &experimental) &&
> > -			 experimental)
> > -			use_builtin_add_i = 1;
> > -	}
> > +	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.

Ciao,
Dscho

Junio C Hamano Dec. 2, 2021, 4:58 p.m. UTC | #6

Johannes Schindelin <[e-mail]> 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.

Johannes Schindelin Dec. 2, 2021, 5:30 p.m. UTC | #7

Hi Junio,

On Wed, 1 Dec 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> [...]
> > diff --git a/builtin/add.c b/builtin/add.c
> > index ef6b619c45e..8ef230a345b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> >  	int use_builtin_add_i =
> >  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
> >
> > -	if (use_builtin_add_i < 0) {
> > -		int experimental;
> > -		if (!git_config_get_bool("add.interactive.usebuiltin",
> > -					 &use_builtin_add_i))
> > -			; /* ok */
> > -		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> > -			 experimental)
> > -			use_builtin_add_i = 1;
> > -	}
> > +	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) {
>
> Nit.
>
> 	if (use_builtin_add_i) {
>
> I wondered if these random calls to git_config_get_X() should be
> consolidated into the existing add_config() callback, but this
> conditional will go away hopefully in a few releases, so it probably
> is not worth it.  Inheriting the way the original code grabbed the
> configuration variables is good enough for our purpose here.

As I said in my reply to Phillip, I found it misleading to skip the `!= 0`
because we are catching both the `== 1` as well as the `== -1` here.

Ciao,
Dscho

Ramsay Jones Dec. 2, 2021, 5:43 p.m. UTC | #8

On 02/12/2021 16:58, Junio C Hamano wrote:
> Johannes Schindelin <[e-mail]> 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

Johannes Schindelin Dec. 10, 2021, 10:59 p.m. UTC | #9

Hi Junio,

On Thu, 2 Dec 2021, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> 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

Patch

diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
index c9f748f81cb..3e859f34197 100644
--- a/Documentation/config/add.txt
+++ b/Documentation/config/add.txt
@@ -7,6 +7,6 @@  add.ignore-errors (deprecated)::
 	variables.
 
 add.interactive.useBuiltin::
-	[EXPERIMENTAL] Set to `true` to use the experimental built-in
-	implementation of the interactive version of linkgit:git-add[1]
-	instead of the Perl script version. Is `false` by default.
+	Set to `false` to fall back to the original Perl implementation of
+	the interactive version of linkgit:git-add[1] instead of the built-in
+	version. Is `true` by default.
diff --git a/builtin/add.c b/builtin/add.c
index ef6b619c45e..8ef230a345b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -237,17 +237,12 @@  int run_add_interactive(const char *revision, const char *patch_mode,
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
 
-	if (use_builtin_add_i < 0) {
-		int experimental;
-		if (!git_config_get_bool("add.interactive.usebuiltin",
-					 &use_builtin_add_i))
-			; /* ok */
-		else if (!git_config_get_bool("feature.experimental", &experimental) &&
-			 experimental)
-			use_builtin_add_i = 1;
-	}
+	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) {
 		enum add_p_mode mode;
 
 		if (!patch_mode)
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index cc62616d806..660ebe8d108 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -29,7 +29,7 @@  linux-gcc)
 	export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
 	export GIT_TEST_MULTI_PACK_INDEX=1
 	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
-	export GIT_TEST_ADD_I_USE_BUILTIN=1
+	export GIT_TEST_ADD_I_USE_BUILTIN=0
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 	export GIT_TEST_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2
diff --git a/t/README b/t/README
index 29f72354bf1..2c22337d6e7 100644
--- a/t/README
+++ b/t/README
@@ -419,7 +419,7 @@  the --sparse command-line argument.
 GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 
-GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
+GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when false, disables the
 built-in version of git add -i. See 'add.interactive.useBuiltin' in
 git-config(1).
 
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 71c5a15be00..bc3f69b4b1d 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -4,7 +4,7 @@  test_description='git checkout --patch'
 
 . ./lib-patch-mode.sh
 
-if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
+if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
 then
 	skip_all='skipping interactive add tests, PERL not set'
 	test_done