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
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(-)
@@ -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_successPERL'saying "n" does nothing''+test_expect_success'saying "n" does nothing''set_and_save_statedir/fooworkhead&&test_write_linesnn|gitcheckout-p&&verify_saved_statebar&&verify_saved_statedir/foo'-test_expect_successPERL'git checkout -p''+test_expect_success'git checkout -p''test_write_linesny|gitcheckout-p&&verify_saved_statebar&&verify_statedir/fooheadhead'-test_expect_successPERL'git checkout -p with staged changes''+test_expect_success'git checkout -p with staged changes''set_statedir/fooworkindex&&test_write_linesny|gitcheckout-p&&verify_saved_statebar&&verify_statedir/fooindexindex'-test_expect_successPERL'git checkout -p HEAD with NO staged changes: abort''+test_expect_success'git checkout -p HEAD with NO staged changes: abort''set_and_save_statedir/fooworkhead&&test_write_linesnyn|gitcheckout-pHEAD&&verify_saved_statebar&&verify_saved_statedir/foo'-test_expect_successPERL'git checkout -p HEAD with NO staged changes: apply''+test_expect_success'git checkout -p HEAD with NO staged changes: apply''test_write_linesnyy|gitcheckout-pHEAD&&verify_saved_statebar&&verify_statedir/fooheadhead'-test_expect_successPERL'git checkout -p HEAD with change already staged''+test_expect_success'git checkout -p HEAD with change already staged''set_statedir/fooindexindex&&# the third n is to get out in case it mistakenly does not applytest_write_linesnyn|gitcheckout-pHEAD&&
@@ -63,21 +65,21 @@ test_expect_success PERL 'git checkout -p HEAD with change already staged' 'verify_statedir/fooheadhead'-test_expect_successPERL'git checkout -p HEAD^...''+test_expect_success'git checkout -p HEAD^...''# the third n is to get out in case it mistakenly does not applytest_write_linesnyn|gitcheckout-pHEAD^...&&verify_saved_statebar&&verify_statedir/fooparentparent'-test_expect_successPERL'git checkout -p HEAD^''+test_expect_success'git checkout -p HEAD^''# the third n is to get out in case it mistakenly does not applytest_write_linesnyn|gitcheckout-pHEAD^&&verify_saved_statebar&&verify_statedir/fooparentparent'-test_expect_successPERL'git checkout -p handles deletion''+test_expect_success'git checkout -p handles deletion''set_statedir/fooworkindex&&rmdir/foo&&test_write_linesny|gitcheckout-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_successPERL'path limiting works: dir''+test_expect_success'path limiting works: dir''set_statedir/fooworkhead&&test_write_linesyn|gitcheckout-pdir&&verify_saved_statebar&&verify_statedir/fooheadhead'-test_expect_successPERL'path limiting works: -- dir''+test_expect_success'path limiting works: -- dir''set_statedir/fooworkhead&&test_write_linesyn|gitcheckout-p--dir&&verify_saved_statebar&&verify_statedir/fooheadhead'-test_expect_successPERL'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 applytest_write_linesynn|gitcheckout-pHEAD^--dir&&verify_saved_statebar&&verify_statedir/fooparentparent'-test_expect_successPERL'path limiting works: foo inside dir''+test_expect_success'path limiting works: foo inside dir''set_statedir/fooworkhead&&# the third n is to get out in case it mistakenly does not applytest_write_linesynn|(cddir&&gitcheckout-pfoo)&&
@@ -119,11 +121,11 @@ test_expect_success PERL 'path limiting works: foo inside dir' 'verify_statedir/fooheadhead'-test_expect_successPERL'none of this moved HEAD''+test_expect_success'none of this moved HEAD''verify_saved_head'-test_expect_successPERL'empty tree can be handled''+test_expect_success'empty tree can be handled''test_when_finished"git reset --hard"&&gitcheckout-p$(test_oidempty_tree)--'
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(-)
@@ -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.
@@ -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).
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
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.
@@ -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.
@@ -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).
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(-)
@@ -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...)
@@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,intuse_builtin_add_i=git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN",-1);-if(use_builtin_add_i<0){-intexperimental;-if(!git_config_get_bool("add.interactive.usebuiltin",-&use_builtin_add_i))-;/* ok */-elseif(!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.
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/
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
@@ -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
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
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.
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
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(-)
@@ -524,7 +524,7 @@ test_expect_success 'split hunk with incomplete line at end' 'test_must_failgitgrep--cachedbefore'-test_expect_failure'edit, adding lines to the first hunk''+test_expect_success'edit, adding lines to the first hunk''test_write_lines1011203040505160>test&&gitreset&&tr_" ">patch<<-EOF&&
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
@@ -524,7 +524,7 @@ test_expect_success 'split hunk with incomplete line at end' 'test_must_failgitgrep--cachedbefore'-test_expect_failure'edit, adding lines to the first hunk''+test_expect_success'edit, adding lines to the first hunk''test_write_lines1011203040505160>test&&gitreset&&tr_" ">patch<<-EOF&&--