[PATCH 0/2] Use the built-in implementation of the interactive add command by default

[PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Johannes Schindelin via GitGitGadget]

Over two years ago, Slavica Đukić participated in the Outreachy project,
starting to implement a built-in version of the interactive git add command.
A little over a year ago, Git turned on that mode whenever users were
running with feature.experimental = true.

It is time to declare this implementation robust, to use it by default, and
to start deprecating the scripted implementation.

Johannes Schindelin (2):
  t2016: require the PERL prereq only when necessary
  add -i: default to the built-in implementation

 Documentation/config/add.txt |  6 +++---
 builtin/add.c                | 15 +++++--------
 ci/run-build-and-tests.sh    |  2 +-
 t/README                     |  2 +-
 t/t2016-checkout-patch.sh    | 42 +++++++++++++++++++-----------------
 5 files changed, 32 insertions(+), 35 deletions(-)


base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1087%2Fdscho%2Fdefault-to-builtin-add-p-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1087/dscho/default-to-builtin-add-p-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1087
-- 
gitgitgadget

[PATCH 1/2] t2016: require the PERL prereq only when necessary

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The scripted version of the interactive mode of `git add` still requires
Perl, but the built-in version does not. Let's only require the PERL
prereq if testing the scripted version.

This addresses a long-standing NEEDSWORK added in 35166b1fb54 (t2016:
add a NEEDSWORK about the PERL prerequisite, 2020-10-07).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t2016-checkout-patch.sh | 42 ++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index abfd586c32b..71c5a15be00 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -4,7 +4,13 @@ test_description='git checkout --patch'
 
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
+then
+	skip_all='skipping interactive add tests, PERL not set'
+	test_done
+fi
+
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
@@ -18,44 +24,40 @@ test_expect_success PERL 'setup' '
 
 # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
 
-# NEEDSWORK: Since the builtin add-p is used when $GIT_TEST_ADD_I_USE_BUILTIN
-# is given, we should replace the PERL prerequisite with an ADD_I prerequisite
-# which first checks if $GIT_TEST_ADD_I_USE_BUILTIN is defined before checking
-# PERL.
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n n | git checkout -p &&
 	verify_saved_state bar &&
 	verify_saved_state dir/foo
 '
 
-test_expect_success PERL 'git checkout -p' '
+test_expect_success 'git checkout -p' '
 	test_write_lines n y | git checkout -p &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git checkout -p with staged changes' '
+test_expect_success 'git checkout -p with staged changes' '
 	set_state dir/foo work index &&
 	test_write_lines n y | git checkout -p &&
 	verify_saved_state bar &&
 	verify_state dir/foo index index
 '
 
-test_expect_success PERL 'git checkout -p HEAD with NO staged changes: abort' '
+test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n y n | git checkout -p HEAD &&
 	verify_saved_state bar &&
 	verify_saved_state dir/foo
 '
 
-test_expect_success PERL 'git checkout -p HEAD with NO staged changes: apply' '
+test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
 	test_write_lines n y y | git checkout -p HEAD &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git checkout -p HEAD with change already staged' '
+test_expect_success 'git checkout -p HEAD with change already staged' '
 	set_state dir/foo index index &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines n y n | git checkout -p HEAD &&
@@ -63,21 +65,21 @@ test_expect_success PERL 'git checkout -p HEAD with change already staged' '
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git checkout -p HEAD^...' '
+test_expect_success 'git checkout -p HEAD^...' '
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines n y n | git checkout -p HEAD^... &&
 	verify_saved_state bar &&
 	verify_state dir/foo parent parent
 '
 
-test_expect_success PERL 'git checkout -p HEAD^' '
+test_expect_success 'git checkout -p HEAD^' '
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines n y n | git checkout -p HEAD^ &&
 	verify_saved_state bar &&
 	verify_state dir/foo parent parent
 '
 
-test_expect_success PERL 'git checkout -p handles deletion' '
+test_expect_success 'git checkout -p handles deletion' '
 	set_state dir/foo work index &&
 	rm dir/foo &&
 	test_write_lines n y | git checkout -p &&
@@ -90,28 +92,28 @@ test_expect_success PERL 'git checkout -p handles deletion' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'path limiting works: dir' '
+test_expect_success 'path limiting works: dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git checkout -p dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: -- dir' '
+test_expect_success 'path limiting works: -- dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git checkout -p -- dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
+test_expect_success 'path limiting works: HEAD^ -- dir' '
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | git checkout -p HEAD^ -- dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo parent parent
 '
 
-test_expect_success PERL 'path limiting works: foo inside dir' '
+test_expect_success 'path limiting works: foo inside dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | (cd dir && git checkout -p foo) &&
@@ -119,11 +121,11 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
-test_expect_success PERL 'empty tree can be handled' '
+test_expect_success 'empty tree can be handled' '
 	test_when_finished "git reset --hard" &&
 	git checkout -p $(test_oid empty_tree) --
 '
-- 
gitgitgadget

Re: [PATCH 1/2] t2016: require the PERL prereq only when necessary

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The scripted version of the interactive mode of `git add` still requires
> Perl, but the built-in version does not. Let's only require the PERL
> prereq if testing the scripted version.
>
> This addresses a long-standing NEEDSWORK added in 35166b1fb54 (t2016:
> add a NEEDSWORK about the PERL prerequisite, 2020-10-07).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t2016-checkout-patch.sh | 42 ++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)

Good.  I suspect there may not be too many developers who lack PERL
prerequisite around here, so it is not like the built-in version was
not sufficiently tested in developers' environment without this
patch, but it is nice to see us move in this direction.  Of course,
when we remove the non-builtin version, we'd further be able to lose
these prerequisites.

> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index abfd586c32b..71c5a15be00 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,13 @@ test_description='git checkout --patch'
>  
>  . ./lib-patch-mode.sh
>  
> -test_expect_success PERL 'setup' '
> +if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
> +then
> +	skip_all='skipping interactive add tests, PERL not set'
> +	test_done
> +fi
> +
> +test_expect_success 'setup' '
>  	mkdir dir &&
>  	echo parent > dir/foo &&
>  	echo dummy > bar &&
> @@ -18,44 +24,40 @@ test_expect_success PERL 'setup' '
>  
>  # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
>  
> -# NEEDSWORK: Since the builtin add-p is used when $GIT_TEST_ADD_I_USE_BUILTIN
> -# is given, we should replace the PERL prerequisite with an ADD_I prerequisite
> -# which first checks if $GIT_TEST_ADD_I_USE_BUILTIN is defined before checking
> -# PERL.
> -test_expect_success PERL 'saying "n" does nothing' '
> +test_expect_success 'saying "n" does nothing' '

Re: [PATCH 1/2] t2016: require the PERL prereq only when necessary

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The scripted version of the interactive mode of `git add` still requires
> > Perl, but the built-in version does not. Let's only require the PERL
> > prereq if testing the scripted version.
> >
> > This addresses a long-standing NEEDSWORK added in 35166b1fb54 (t2016:
> > add a NEEDSWORK about the PERL prerequisite, 2020-10-07).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t2016-checkout-patch.sh | 42 ++++++++++++++++++++-------------------
> >  1 file changed, 22 insertions(+), 20 deletions(-)
>
> Good.  I suspect there may not be too many developers who lack PERL
> prerequisite around here, so it is not like the built-in version was
> not sufficiently tested in developers' environment without this
> patch, but it is nice to see us move in this direction.  Of course,
> when we remove the non-builtin version, we'd further be able to lose
> these prerequisites.

It is probably true that few developers miss the Perl prerequisite, in
particular given that you cannot even run Git's tests without having a
working Perl.

However, we have seen time and time again that developers do not really
run the test suite. For some, it takes way too long time (for some, 10
minutes is too long, but I remember that Jeff Hostetler reported a 5h
running time, and let's not forget about Cygwin and NonStop) and others
simply won't bother. So my focus lies more with the CI/PR builds. And
there, Windows specifically runs with NO_PERL. And you guessed it, to save
time.

Ciao,
Dscho

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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) {
 		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
-- 
gitgitgadget

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

[Phillip Wood]

Hi Dscho

On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <johannes.schindelin@gmx.de>
> ---
>   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
> 

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Ramsay Jones]

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

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

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Dec 2021, 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.

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

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

[Ævar Arnfjörð Bjarmason]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
>  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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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

[PATCH] fixup! add -i: default to the built-in implementation

[Junio C Hamano]

With the reimplementated "git add -i", two test pieces that used to
expect failure now succeed.  Mark them as such.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Yes, I know this is on hold until some issues in the "add -i"
   reimplementation on MacOS are resolved, but as I am getting tired
   of seeing "TODO PASSED" when I rebuild and test 'seen', I'll
   queue this band-aid at the tip of this topic.

 t/t3701-add-interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 207714655f..1effc3f419 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -500,7 +500,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
 	! grep "^+15" actual
 '
 
-test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
+test_expect_success 'split hunk "add -p (no, yes, edit)"' '
 	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
 	git reset &&
 	# test sequence is s(plit), n(o), y(es), e(dit)
@@ -524,7 +524,7 @@ test_expect_success 'split hunk with incomplete line at end' '
 	test_must_fail git grep --cached before
 '
 
-test_expect_failure 'edit, adding lines to the first hunk' '
+test_expect_success 'edit, adding lines to the first hunk' '
 	test_write_lines 10 11 20 30 40 50 51 60 >test &&
 	git reset &&
 	tr _ " " >patch <<-EOF &&
-- 
2.34.1-373-gb739cc97c5

Re: [PATCH] fixup! add -i: default to the built-in implementation

[Johannes Schindelin]

Hi Junio,

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

> With the reimplementated "git add -i", two test pieces that used to
> expect failure now succeed.  Mark them as such.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Yes, I know this is on hold until some issues in the "add -i"
>    reimplementation on MacOS are resolved, but as I am getting tired
>    of seeing "TODO PASSED" when I rebuild and test 'seen', I'll
>    queue this band-aid at the tip of this topic.

As long as we ship the Perl version as an escape hatch, and as long as
that version does not pass those two test cases, your patch is premature.

I would expect the `linux-gcc` job (or whatever it is called for the next
few hours) to fail with this fixup!, as a consequence of running t3701
with the scripted version of `add -i`.

Ciao,
Dscho

>
>  t/t3701-add-interactive.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 207714655f..1effc3f419 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -500,7 +500,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
>  	! grep "^+15" actual
>  '
>
> -test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
> +test_expect_success 'split hunk "add -p (no, yes, edit)"' '
>  	test_write_lines 5 10 20 21 30 31 40 50 60 >test &&
>  	git reset &&
>  	# test sequence is s(plit), n(o), y(es), e(dit)
> @@ -524,7 +524,7 @@ test_expect_success 'split hunk with incomplete line at end' '
>  	test_must_fail git grep --cached before
>  '
>
> -test_expect_failure 'edit, adding lines to the first hunk' '
> +test_expect_success 'edit, adding lines to the first hunk' '
>  	test_write_lines 10 11 20 30 40 50 51 60 >test &&
>  	git reset &&
>  	tr _ " " >patch <<-EOF &&
> --
> 2.34.1-373-gb739cc97c5
>
>

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Jeff King]

On Tue, Nov 30, 2021 at 02:14:13PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Over two years ago, Slavica Đukić participated in the Outreachy project,
> starting to implement a built-in version of the interactive git add command.
> A little over a year ago, Git turned on that mode whenever users were
> running with feature.experimental = true.
> 
> It is time to declare this implementation robust, to use it by default, and
> to start deprecating the scripted implementation.

Yay. I agree it is time.

It's still possible there are bugs that feature.experimental folks
missed, but at some point we need to flip this switch to get the
exposure to find those bugs. Doing it early in a cycle makes sense.

The patches themselves look good to me. I look forward to dropping the
perl version entirely, just to reduce the duplicated code, but I think
your approach of leaving it as an escape hatch for now makes sense in
the shorter term.

-Peff

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Over two years ago, Slavica Đukić participated in the Outreachy project,
> starting to implement a built-in version of the interactive git add command.
> A little over a year ago, Git turned on that mode whenever users were
> running with feature.experimental = true.
>
> It is time to declare this implementation robust, to use it by default, and
> to start deprecating the scripted implementation.

Yes, it is time to use it by default to expose any remaining bugs
that those with feature.experimental set failed to catch.
Unfortunately, we do not catch issues real users would encounter in
any "opt-in" feature, until it becomes "opt-out" and unconfigured
users are exposed to it.  This is true even in the presense of large
corporations that expose their internal users to versions based on
'next' regularly.

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Ævar Arnfjörð Bjarmason]

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

> Over two years ago, Slavica Đukić participated in the Outreachy project,
> starting to implement a built-in version of the interactive git add command.
> A little over a year ago, Git turned on that mode whenever users were
> running with feature.experimental = true.
>
> It is time to declare this implementation robust, to use it by default, and
> to start deprecating the scripted implementation.
>
> Johannes Schindelin (2):
>   t2016: require the PERL prereq only when necessary
>   add -i: default to the built-in implementation

I'm very happy to see this. I left some minor nits on 2/2[1], but
with/without those suggested changes this LGTM.

I was a tad surprised that feature.experimental=false doesn't disable
this anymore, but after looking into it a bit that's how we should be
doing this. I.e. the life cycle for these has been

    opt in setting [&& experimental] -> opt-out setting [&& !experimental] -> remove opt-out

If you're intending to re-roll anyway I think a brief mention of that
being intended & correct would be nice.

I.e. I went looking down that rabbit hole since there was no mention of
it in the commit message, and wondered if it was intentional & correct,
which I then found it is (well, correct, but I'm assuming also
intentional).

Thanks!

1. https://lore.kernel.org/git/211201.86pmqgbful.gmgdl@evledraar.gmail.com/

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Carlo Arenas]

On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> It is time to declare this implementation robust, to use it by default, and
> to start deprecating the scripted implementation.
>
> Johannes Schindelin (2):
>   t2016: require the PERL prereq only when necessary
>   add -i: default to the built-in implementation

Sadly this implementation has a few bugs that still need fixing, with
at least one IMHO being a showstopper.

The way macOS implements stdin (through a device) it will always
timeout in poll(), so escape keys that are left in the unread buffer
and that could match some of the entries will result in the wrong
entry being selected.

I have a series[1] that reimplements this and that seemed to work fine
in my tests while making the code simpler, but that I didn't
prioritize (and wanted to clean up further) since I wanted to
prioritize the EDITOR fixes in the same area.

Carlo

[1] https://github.com/git/git/pull/1150

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Johannes Schindelin]

Hi Carlo,

On Wed, 1 Dec 2021, Carlo Arenas wrote:

> On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > It is time to declare this implementation robust, to use it by default, and
> > to start deprecating the scripted implementation.
> >
> > Johannes Schindelin (2):
> >   t2016: require the PERL prereq only when necessary
> >   add -i: default to the built-in implementation
>
> Sadly this implementation has a few bugs that still need fixing, with
> at least one IMHO being a showstopper.
>
> The way macOS implements stdin (through a device) it will always
> timeout in poll(), so escape keys that are left in the unread buffer
> and that could match some of the entries will result in the wrong
> entry being selected.
>
> I have a series[1] that reimplements this and that seemed to work fine
> in my tests while making the code simpler, but that I didn't
> prioritize (and wanted to clean up further) since I wanted to
> prioritize the EDITOR fixes in the same area.
>
> Carlo
>
> [1] https://github.com/git/git/pull/1150

Thank you for pointing that out. I agree both with prioritizing your macOS
patches, and with prioritizing the editor patches before that. Please just
let me know when would be a good time to move forward with this here patch
series.

Thank you,
Dscho

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Philippe Blain]

Hi Dscho,

Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit :
> Over two years ago, Slavica Đukić participated in the Outreachy project,
> starting to implement a built-in version of the interactive git add command.
> A little over a year ago, Git turned on that mode whenever users were
> running with feature.experimental = true.
> 
> It is time to declare this implementation robust, to use it by default, and
> to start deprecating the scripted implementation.
> 
> Johannes Schindelin (2):
>    t2016: require the PERL prereq only when necessary
>    add -i: default to the built-in implementation
> 
>   Documentation/config/add.txt |  6 +++---
>   builtin/add.c                | 15 +++++--------
>   ci/run-build-and-tests.sh    |  2 +-
>   t/README                     |  2 +-
>   t/t2016-checkout-patch.sh    | 42 +++++++++++++++++++-----------------
>   5 files changed, 32 insertions(+), 35 deletions(-)

I just noticed that 'INSTALL' mentions that Perl is needed for 'git add interactive'
et al, so maybe we would want to tweak the wording a bit in there when switch the default
to the C version ?

Cheers,
Philippe.

Re: [PATCH 0/2] Use the built-in implementation of the interactive add command by default

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

Hi Philippe,

On Fri, 3 Dec 2021, Philippe Blain wrote:

> Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit :
> > Over two years ago, Slavica Đukić participated in the Outreachy project,
> > starting to implement a built-in version of the interactive git add command.
> > A little over a year ago, Git turned on that mode whenever users were
> > running with feature.experimental = true.
> >
> > It is time to declare this implementation robust, to use it by default, and
> > to start deprecating the scripted implementation.
> >
> > Johannes Schindelin (2):
> >    t2016: require the PERL prereq only when necessary
> >    add -i: default to the built-in implementation
> >
> >   Documentation/config/add.txt |  6 +++---
> >   builtin/add.c                | 15 +++++--------
> >   ci/run-build-and-tests.sh    |  2 +-
> >   t/README                     |  2 +-
> >   t/t2016-checkout-patch.sh    | 42 +++++++++++++++++++-----------------
> >   5 files changed, 32 insertions(+), 35 deletions(-)
>
> I just noticed that 'INSTALL' mentions that Perl is needed for 'git add
> interactive'
> et al, so maybe we would want to tweak the wording a bit in there when switch
> the default
> to the C version ?

Not yet. Only once we remove `git-add--interactive.perl`.

Thanks,
Dscho