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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Dscho

On 30/11/2021 14:14, Johannes Schindelin via GitGitGadget wrote:
> 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.

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.


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

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