git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 1, 2021, 22:34 UTC
Message-ID
<xmqqee6wymx2.fsf@gitster.g>
In-Reply-To
<84824918ae4564a9194a1a55124ee8694f210437.1638281655.git.gitgitgadget@gmail.com>

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

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

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

> We specifically leave the `add.interactive.useBuiltin` configuration in
> place, to give users an "escape hatch" in the unexpected case should
> they encounter a previously undetected bug in that implementation.
Good thinking.
Show 31 quoted lines
>  add.interactive.useBuiltin::
> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> -	implementation of the interactive version of linkgit:git-add[1]
> -	instead of the Perl script version. Is `false` by default.
> +	Set to `false` to fall back to the original Perl implementation of
> +	the interactive version of linkgit:git-add[1] instead of the built-in
> +	version. Is `true` by default.
> diff --git a/builtin/add.c b/builtin/add.c
> index ef6b619c45e..8ef230a345b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -237,17 +237,12 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	int use_builtin_add_i =
>  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>  
> -	if (use_builtin_add_i < 0) {
> -		int experimental;
> -		if (!git_config_get_bool("add.interactive.usebuiltin",
> -					 &use_builtin_add_i))
> -			; /* ok */
> -		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> -			 experimental)
> -			use_builtin_add_i = 1;
> -	}
> +	if (use_builtin_add_i < 0 &&
> +	    git_config_get_bool("add.interactive.usebuiltin",
> +				&use_builtin_add_i))
> +		use_builtin_add_i = 1;
>  
> -	if (use_builtin_add_i == 1) {
> +	if (use_builtin_add_i != 0) {
Nit.
	if (use_builtin_add_i) {

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

Show 13 quoted lines
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index cc62616d806..660ebe8d108 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -29,7 +29,7 @@ linux-gcc)
>  	export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
>  	export GIT_TEST_MULTI_PACK_INDEX=1
>  	export GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1
> -	export GIT_TEST_ADD_I_USE_BUILTIN=1
> +	export GIT_TEST_ADD_I_USE_BUILTIN=0
>  	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
>  	export GIT_TEST_WRITE_REV_INDEX=1
>  	export GIT_TEST_CHECKOUT_WORKERS=2
OK.
Show 26 quoted lines
> diff --git a/t/README b/t/README
> index 29f72354bf1..2c22337d6e7 100644
> --- a/t/README
> +++ b/t/README
> @@ -419,7 +419,7 @@ the --sparse command-line argument.
>  GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
>  by overriding the minimum number of cache entries required per thread.
>  
> -GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when true, enables the
> +GIT_TEST_ADD_I_USE_BUILTIN=<boolean>, when false, disables the
>  built-in version of git add -i. See 'add.interactive.useBuiltin' in
>  git-config(1).
>  
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 71c5a15be00..bc3f69b4b1d 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>  
>  . ./lib-patch-mode.sh
>  
> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN false && ! test_have_prereq PERL
> +if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
>  then
>  	skip_all='skipping interactive add tests, PERL not set'
>  	test_done
Looks good.
Thanks.
Previous: Junio C HamanoNext: Johannes Schindelin
Message 13 of 23 in “Use the built-in implementation of the interactive add command by default”
  1. 0/2 Use the built-in implementation of the interactive add command by defaultJohannes Schindelin via GitGitGadget, Nov 30, 2021
  2. 1/2 t2016: require the PERL prereq only when necessaryJohannes Schindelin via GitGitGadget, Nov 30, 2021
  3. Junio C HamanoDec 1, 2021
  4. Johannes SchindelinDec 2, 2021
  5. 2/2 add -i: default to the built-in implementationJohannes Schindelin via GitGitGadget, Nov 30, 2021
  6. Phillip WoodDec 1, 2021
  7. Johannes SchindelinDec 2, 2021
  8. Junio C HamanoDec 2, 2021
  9. Ramsay JonesDec 2, 2021
  10. Johannes SchindelinDec 10, 2021
  11. Ævar Arnfjörð BjarmasonDec 1, 2021
  12. Junio C HamanoDec 1, 2021
  13. Junio C HamanoDec 1, 2021
  14. Johannes SchindelinDec 2, 2021
  15. fixup! add -i: default to the built-in implementationJunio C Hamano, Dec 9, 2021
  16. Johannes SchindelinDec 10, 2021
  17. Jeff KingNov 30, 2021
  18. Junio C HamanoNov 30, 2021
  19. Ævar Arnfjörð BjarmasonDec 1, 2021
  20. Carlo ArenasDec 1, 2021
  21. Johannes SchindelinDec 2, 2021
  22. Philippe BlainDec 3, 2021
  23. Johannes SchindelinDec 6, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.