Use the built-in implementation of the interactive add command by default
cover Use the built-in implementation of the interactive add command by default
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
Signature · 1 line
gitgitgadget
PATCH 1/2 t2016: require the PERL prereq only when necessary
From: Johannes Schindelin <[email protected]>
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).
--- t/t2016-checkout-patch.sh | 42 ++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
t/t2016-checkout-patch.sh +22 -20
@@ -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) -- '
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] t2016: require the PERL prereq only when necessary
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Johannes Schindelin <[email protected]>
The scripted version of the interactive mode of
git addstill requires Perl, but the built-in version does not. Let's only require the PERL prereq if testing the scripted version.
Quoted 7 lines
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 <[email protected]>--- 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.
Quoted 29 lines
t/t2016-checkout-patch.sh +8
-6@@ -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
Hi Junio,
Quoted 17 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 6 lines · writes:
From: Johannes Schindelin <[email protected]>
The scripted version of the interactive mode of
git addstill requires Perl, but the built-in version does not. Let's only require the PERL prereq if testing the scripted version.Quoted 7 lines
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 <[email protected]>--- t/t2016-checkout-patch.sh | 42 ++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
Quoted 6 lines
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
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- 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(-)
Documentation/config/add.txt +3 -3
@@ -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.
builtin/add.c +5 -10
@@ -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)
ci/run-build-and-tests.sh +1 -1
@@ -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
t/README +1 -1
@@ -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).
t/t2016-checkout-patch.sh +1 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] add -i: default to the built-in implementation
Hi Dscho
Quoted 14 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In
9a5315edfdf(Merge branch 'js/patch-mode-in-others-in-c', 2020-02-05), Git acquired a built-in implementation ofgit add's interactive mode that could be turned on via the config optionadd.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 viafeature.experimental. The first version with this change was v2.29.0.
Quoted 6 lines
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.useBuiltinconfiguration 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 37 lines
Signed-off-by Johannes Schindelin <[email protected]>--- 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(-)Documentation/config/add.txt +3
-3@@ -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.builtin/add.c +0
-6@@ -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) &&
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 10 lines
builtin/add.c +5
-4- 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 43 lines
builtin/add.c +0
-0enum add_p_mode mode;if (!patch_mode)ci/run-build-and-tests.sh +1
-1@@ -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=2t/README +1
-1@@ -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).t/t2016-checkout-patch.sh +1
-1@@ -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
Hi Phillip,
Quoted 21 lines · Phillip Wood
Quoted 19 lines · Johannes Schindelin via GitGitGadget
builtin/add.c +0
-5@@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, constchar *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) &&
builtin/add.c +1
-3- experimental) - use_builtin_add_i =1 ; - } + if (use_builtin_add_i < 0 &&
Quoted 8 lines
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 5 lines
builtin/add.c +4
-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
Quoted 10 lines · Johannes Schindelin
Quoted 9 lines
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 6 lines
builtin/add.c +5
-0+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
Quoted 7 lines
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_iwas a Boolean. But it is a tristate, it can also be-1("undecided"). And I wanted to express "if this is not set tofalsespecifically", 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
Quoted 35 lines · Junio C Hamano
Quoted 10 lines · Johannes Schindelin
Quoted 9 lines
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 6 lines
builtin/add.c +5
-0+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 thatQuoted 7 lines
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_iwas a Boolean. But it is a tristate, it can also be-1("undecided"). And I wanted to express "if this is not set tofalsespecifically", 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.
Quoted 3 lines
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
In reply to Junio C HamanoHi Junio,
Quoted 35 lines · Junio C Hamano
Quoted 10 lines · Johannes Schindelin
Quoted 9 lines
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 6 lines
builtin/add.c +5
-0+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 thatQuoted 7 lines
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_iwas a Boolean. But it is a tristate, it can also be-1("undecided"). And I wanted to express "if this is not set tofalsespecifically", 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.
Quoted 2 lines
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
In reply to Johannes Schindelin via GitGitGadgetQuoted 35 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
In
9a5315edfdf(Merge branch 'js/patch-mode-in-others-in-c', 2020-02-05), Git acquired a built-in implementation ofgit add's interactive mode that could be turned on via the config optionadd.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 viafeature.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.useBuiltinconfiguration 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 <[email protected]>--- 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(-)Documentation/config/add.txt +0
-0
Documentation/config/add.txt add.ignore-errors (deprecated):: · line 12
Quoted 9 lines
Documentation/config/add.txt +3
-3@@ -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 14 lines
builtin/add.c +0
-6@@ -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) &&
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 10 lines
builtin/add.c +5
-4- 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
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/add.txt add.ignore-errors (deprecated):: · line 12
Quoted 7 lines
add.interactive.useBuiltin::
Documentation/config/add.txt +3
-3- [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.
Quoted 8 lines
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).
Quoted 8 lines
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 2 lines
builtin/add.c +1
-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.
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
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · writes:
From: Johannes Schindelin <[email protected]>
In
9a5315edfdf(Merge branch 'js/patch-mode-in-others-in-c', 2020-02-05), Git acquired a built-in implementation ofgit add's interactive mode that could be turned on via the config optionadd.interactive.useBuiltin.The first official Git version to support this knob was v2.26.0.
Quoted 7 lines
In
2df2d81ddd0(add -i: use the built-in version when feature.experimental is set, 2020-09-08), this built-in implementation was also enabled viafeature.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 ;-).
Quoted 3 lines
We specifically leave the
add.interactive.useBuiltinconfiguration in place, to give users an "escape hatch" in the unexpected case should they encounter a previously undetected bug in that implementation.
Good thinking.
Quoted 21 lines
add.interactive.useBuiltin::
Documentation/config/add.txt +3
-3- [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.builtin/add.c +0
-6@@ -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) &&
builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 10 lines
builtin/add.c +5
-4- 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.
ci/run-build-and-tests.sh linux-gcc) · line 35
Quoted 13 lines
ci/run-build-and-tests.sh +1
-1@@ -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.
Quoted 18 lines
t/README +1
-1@@ -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).t/t2016-checkout-patch.sh +0
-0
t/t2016-checkout-patch.sh test_description='git checkout --patch' · line 10
Quoted 8 lines
t/t2016-checkout-patch.sh +1
-1@@ -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
Hi Junio,
Quoted 31 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]> writes:
[...]
Quoted 14 lines
builtin/add.c +0
-6@@ -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) &&builtin/add.c int run_add_interactive(const char *revision, const char *patch_mode, · line 245
Quoted 10 lines
builtin/add.c +5
-4- 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.
Quoted 7 lines
if (use_builtin_add_i) {I wondered if these random calls to
git_config_get_X()should be consolidated into the existingadd_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 1/1 fixup! add -i: default to the built-in implementation
In reply to Johannes Schindelin via GitGitGadgetWith the reimplementated "git add -i", two test pieces that used to expect failure now succeed. Mark them as such.
---
* 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(-)
t/t3701-add-interactive.sh +2 -2
@@ -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 &&
Signature · 1 line
2.34.1-373-gb739cc97c5
Re: [PATCH] fixup! add -i: default to the built-in implementation
Hi Junio,
Quoted 10 lines · 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 <[email protected]>---
* 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 30 lines
t/t3701-add-interactive.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/t3701-add-interactive.sh +2
-3@@ -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
Quoted 7 lines · 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.
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
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · 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.
Quoted 11 lines · 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
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!
Quoted 6 lines · Johannes Schindelin via GitGitGadget
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
Hi Carlo,
Quoted 17 lines · Carlo Arenas
Quoted 6 lines · Johannes Schindelin via GitGitGadget
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 implementationSadly 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.
Quoted 8 lines
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
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
Hi Dscho,
Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit :
Quoted 12 lines
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
Quoted 6 lines
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.
Hi Philippe,
Quoted 20 lines · Philippe Blain
Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit :
Quoted 12 lines
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 implementationQuoted 6 lines
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(-)
Quoted 5 lines
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