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
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 1, 2021, 13:37 UTC
Message-ID
<211201.86pmqgbful.gmgdl@evledraar.gmail.com>
In-Reply-To
<84824918ae4564a9194a1a55124ee8694f210437.1638281655.git.gitgitgadget@gmail.com>
On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote:
Show 44 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.
>
> We specifically leave the `add.interactive.useBuiltin` configuration in
> place, to give users an "escape hatch" in the unexpected case should
> they encounter a previously undetected bug in that implementation.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/config/add.txt |  6 +++---
>  builtin/add.c                | 15 +++++----------
>  ci/run-build-and-tests.sh    |  2 +-
>  t/README                     |  2 +-
>  t/t2016-checkout-patch.sh    |  2 +-
>  5 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt
> index c9f748f81cb..3e859f34197 100644
> --- a/Documentation/config/add.txt
> +++ b/Documentation/config/add.txt
> @@ -7,6 +7,6 @@ add.ignore-errors (deprecated)::
>  	variables.
>  
>  add.interactive.useBuiltin::
> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
> -	implementation of the interactive version of linkgit:git-add[1]
> -	instead of the Perl script version. Is `false` by default.
> +	Set to `false` to fall back to the original Perl implementation of
> +	the interactive version of linkgit:git-add[1] instead of the built-in
> +	version. Is `true` by default.

I think this would be a bit better if we just stole the version you added for stash.useBuiltin entirely. I.e. from your 336ad8424cb (stash: document stash.useBuiltin, 2019-05-14), with the relevant s/shell script/Perl/g etc. replaced.

I.e. that version encouraged users to report any bugs, because we were really going to remove it soon, as we then did for rebase.useBuiltin in 9bcde4d5314 (rebase: remove transitory rebase.useBuiltin setting & env, 2021-03-23).

The wording in the opening paragraph is also a bit more to the point there, i.e. calling it "legacy" rather than "original [...] implementation".

(I notice that the stash.useBuiltin is still there in-tree, hrm...)
Show 24 quoted lines
> 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.

Previous: Johannes SchindelinNext: Junio C Hamano
Message 11 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.