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

STALE1707d

14 messages, 8 authors, 2021-12-10 · open the first message on its own page

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

From: Johannes Schindelin via GitGitGadget <hidden>
Date: 2021-11-30 14:14:23

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

From: Johannes Schindelin via GitGitGadget <hidden>
Date: 2021-11-30 14:14:26

From: Johannes Schindelin <redacted>

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 <redacted>
---
 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

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

From: Johannes Schindelin via GitGitGadget <hidden>
Date: 2021-11-30 14:14:27

From: Johannes Schindelin <redacted>

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 <redacted>
---
 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 0/2] Use the built-in implementation of the interactive add command by default

From: Jeff King <hidden>
Date: 2021-11-30 20:57:39

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 2/2] add -i: default to the built-in implementation

From: Phillip Wood <hidden>
Date: 2021-12-01 11:20:46

Hi Dscho

On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin <redacted>

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.

quoted hunk
Signed-off-by: Johannes Schindelin <redacted>
---
  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
quoted hunk
  		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

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-12-01 13:43:36

On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote:
quoted hunk
From: Johannes Schindelin <redacted>

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 <redacted>
---
 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...)
quoted hunk
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 0/2] Use the built-in implementation of the interactive add command by default

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-12-01 13:53:39

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

From: Carlo Arenas <hidden>
Date: 2021-12-01 21:24:29

On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget
[off-list ref] 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 2/2] add -i: default to the built-in implementation

From: Johannes Schindelin <hidden>
Date: 2021-12-02 15:02:48

Hi Phillip,

On Wed, 1 Dec 2021, Phillip Wood wrote:
On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
quoted
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 0/2] Use the built-in implementation of the interactive add command by default

From: Johannes Schindelin <hidden>
Date: 2021-12-02 17:33:30

Hi Carlo,

On Wed, 1 Dec 2021, Carlo Arenas wrote:
On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget
[off-list ref] wrote:
quoted
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

From: Philippe Blain <hidden>
Date: 2021-12-03 13:58:41

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

From: Johannes Schindelin <hidden>
Date: 2021-12-06 16:02:34

Hi Philippe,

On Fri, 3 Dec 2021, Philippe Blain wrote:
Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit :
quoted
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

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

From: Junio C Hamano <hidden>
Date: 2021-12-09 04:12:44

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 <redacted>
---

 * 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

From: Johannes Schindelin <hidden>
Date: 2021-12-10 22:55:20

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 <redacted>
---

 * 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
quoted hunk
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help