Re: [PATCH v2] rebase: remove the rebase.useBuiltin setting

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 6625 bytes --]

Hi Ævar,

On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

> Remove the rebase.useBuiltin setting, which was added as an escape
> hatch to disable the builtin version of rebase first released with Git
> 2.20.
> 
> See [1] for the initial implementation of rebase.useBuiltin, and [2]
> and [3] for the documentation and corresponding
> GIT_TEST_REBASE_USE_BUILTIN option.
> 
> Carrying the legacy version is a maintenance burden as seen in
> 7e097e27d3 ("legacy-rebase: backport -C<n> and --whitespace=<option>
> checks", 2018-11-20) and 9aea5e9286 ("rebase: fix regression in
> rebase.useBuiltin=false test mode", 2019-02-13). Since the built-in
> version has been shown to be stable enough let's remove the legacy
> version.

I agree with that reasoning. Elsewhere, a wish cropped up for the `git
stash` command to optionally ignore unmatched globs, and if we go about to
implement this, we will have to implement it in the scripted and the
built-in version. If we can at least avoid that for the `rebase` command,
I think it would make things a bit easier over here.

> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 331d250e04..c747452983 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,16 +1,9 @@
>  rebase.useBuiltin::
> -	Set to `false` to use the legacy shellscript implementation of
> -	linkgit:git-rebase[1]. Is `true` by default, which means use
> -	the built-in rewrite of it in C.
> -+
> -The C rewrite is first included with Git version 2.20. This option
> -serves an an escape hatch to re-enable the legacy version in case any
> -bugs are found in the rewrite. This option and the shellscript version
> -of linkgit:git-rebase[1] will be removed in some future release.
> -+
> -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.
> +	Unused configuration variable. Used between Git version 2.20
> +	and 2.21 as an escape hatch to enable the legacy shellscript
> +	implementation of rebase. Now the built-in rewrite of it in C
> +	is always used. Setting this will emit a warning, to alert any
> +	remaining users that setting this now does nothing.

Do we really need to document this? Why not just remove the entire entry
wholesale; the warning if `rebase.useBuiltin=false` is set will be
informative enough.

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 52114cbf0d..829897a8fe 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1143,21 +1143,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	};
>  	int i;
>  
> -	/*
> -	 * NEEDSWORK: Once the builtin rebase has been tested enough
> -	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
> -	 * can be removed.
> -	 */
> -
> -	if (!use_builtin_rebase()) {
> -		const char *path = mkpath("%s/git-legacy-rebase",
> -					  git_exec_path());
> -
> -		if (sane_execvp(path, (char **)argv) < 0)
> -			die_errno(_("could not exec %s"), path);
> -		else
> -			BUG("sane_execvp() returned???");
> -	}
> +	if (!use_builtin_rebase())
> +		warning(_("The rebase.useBuiltin support has been removed!"));

A couple of thoughts about this:

- `use_builtin_rebase()` spawns a `git config`. This is a pretty expensive
  operation on Windows (even if it might not matter in the big scheme of
  things, as the couple of milliseconds are probably a mere drop on a hot
  stone compared to the I/O incurred by the recursive merge), and it was
  only done in that way to allow for spawning the legacy rebase without
  having touched any global state (such as setting `GIT_*` environment
  variables when a Git directory was discovered).

  Couldn't we rather move this warning into `rebase_config()`?

- The warning should start with a lower-case letter (why don't we have any
  automated linter for this? This is a totally automatable thing that
  could run as part of `make` when `DEVELOPER` is set, maybe just on the
  `git diff HEAD --` part, and maybe even generating a patch that can be
  applied; No human should *ever* need to spend time on such issues).

- That warning should probably talk more specifically about the scripted
  version having been removed, not only the option (which was actually not
  removed, otherwise the user would not see that warning ;-)).

> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 3e73f7584c..0a88eed1db 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -311,4 +311,10 @@ test_expect_success 'rebase--merge.sh and --show-current-patch' '
>  	)
>  '
>  
> +test_expect_success 'rebase -c rebase.useBuiltin=false warning' '
> +	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN= \

Good attention to detail! I would have forgotten to unset that environment
variable.

> +		git -c rebase.useBuiltin=false rebase 2>err &&
> +	test_i18ngrep "rebase.useBuiltin support has been removed" err
> +'
> +
>  test_done
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index b60b11f9f2..1723e1a858 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -149,12 +149,10 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>  
>  test_expect_success 'rebase -x with empty command fails' '
>  	test_when_finished "git rebase --abort ||:" &&
> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> -		git rebase -x "" @ 2>actual &&
> +	test_must_fail env git rebase -x "" @ 2>actual &&
>  	test_write_lines "error: empty exec command" >expected &&
>  	test_i18ncmp expected actual &&
> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> -		git rebase -x " " @ 2>actual &&
> +	test_must_fail env git rebase -x " " @ 2>actual &&
>  	test_i18ncmp expected actual
>  '
>  
> @@ -162,8 +160,7 @@ LF='
>  '
>  test_expect_success 'rebase -x with newline in command fails' '
>  	test_when_finished "git rebase --abort ||:" &&
> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> -		git rebase -x "a${LF}b" @ 2>actual &&
> +	test_must_fail env git rebase -x "a${LF}b" @ 2>actual &&

Not a terribly big deal, but I would have structured the patch (series) by
leaving this change to t3404 as a 2/2, as it is not technically necessary
to include those changes in 1/2 (if your goal is, as mine usually is, to
"go from working state to working state" between commits).

Thank you for keeping on the track with this,
Dscho

>  	test_write_lines "error: exec commands cannot contain newlines" \
>  			 >expected &&
>  	test_i18ncmp expected actual
> -- 
> 2.21.0.360.g471c308f928
> 
>