threads / patch / 49832

v2, 2 partsrebase.useBuiltin doc & test mode

Subject: [PATCH v2 0/2] rebase.useBuiltin doc & test mode

## tl;dr

9 messages between Nov 14, 2018 and Nov 16, 2018. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Nov 14, 2018, 09:15 UTC · lore

I was missing a "=" in the t/README docs in 2/2 in v1. Also move the docs around slightly so this & my gettext series don't have conflicts.

*** BLURB HERE ***
Ævar Arnfjörð Bjarmason (2):
  rebase doc: document rebase.useBuiltin
  tests: add a special setup where rebase.useBuiltin is off
 Documentation/config/rebase.txt | 14 ++++++++++++++
 builtin/rebase.c                |  5 ++++-
 t/README                        |  4 ++++
 3 files changed, 22 insertions(+), 1 deletion(-)
Range-diff:
1:  a5508195c6 ! 1:  ca87aacbfa tests: add a special setup where rebase.useBuiltin is off
    @@ -29,12 +29,13 @@
      --- a/t/README
      +++ b/t/README
     @@
    - index to be written after every 'git repack' command, and overrides the
    - 'core.multiPackIndex' setting to true.
    + GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
    + by overriding the minimum number of cache entries required per thread.
      
    -+GIT_TEST_REBASE_USE_BUILTIN<boolean>, when false, disables the builtin
    -+version of git-rebase. See 'rebase.useBuiltin' in git-config(1).
    ++GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when false, disables the
    ++builtin version of git-rebase. See 'rebase.useBuiltin' in
    ++git-config(1).
     +
    - Naming Tests
    - ------------
    - 
    + GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
    + of the index for the whole test suite by bypassing the default number of
    + cache entries and thread minimums. Setting this to 1 will make the
-- 
2.19.1.1182.g4ecb1133ce
Ævar Arnfjörð Bjarmason· Nov 14, 2018, 09:15 UTC · lore

[PATCH v2 1/2] rebase doc: document rebase.useBuiltin

The rebase.useBuiltin variable introduced in 55071ea248 ("rebase: start implementing it as a builtin", 2018-08-07) was turned on by default in 5541bd5b8f ("rebase: default to using the builtin rebase", 2018-08-08), but had no documentation.

Let's document it so that users who run into any stability issues with the C rewrite know there's an escape hatch[1], and make it clear that needing to turn off builtin rebase means you've found a bug in git.

1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/rebase.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to Documentation/config/rebase.txt +14 −0
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 42e1ba7575..f079bf6b7e 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,3 +1,17 @@
+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.
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.

-- 
2.19.1.1182.g4ecb1133ce
Johannes Schindelin· Nov 14, 2018, 13:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin

Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
> The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> start implementing it as a builtin", 2018-08-07) was turned on by
> default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> 2018-08-08), but had no documentation.
> 
> Let's document it so that users who run into any stability issues with
> the C rewrite know there's an escape hatch[1], and make it clear that
> needing to turn off builtin rebase means you've found a bug in git.
> 
> 1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Makes sense.

Thanks, Dscho

Show 30 quoted lines
> ---
>  Documentation/config/rebase.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 42e1ba7575..f079bf6b7e 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,3 +1,17 @@
> +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.
> +
>  rebase.stat::
>  	Whether to show a diffstat of what changed upstream since the last
>  	rebase. False by default.
> -- 
> 2.19.1.1182.g4ecb1133ce
> 
>
Junio C Hamano· Nov 16, 2018, 03:40 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> start implementing it as a builtin", 2018-08-07) was turned on by
> default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> 2018-08-08), but had no documentation.

I actually thought that everybody understood that this was merely an aid to be used during the development of the feature and never meant to be given to the end users.

With my devil's advocate hat on, how much do we trust it as an escape hatch? After all, the codepath to hide the "rebase in C" implementation and use the scripted version were never in 'master' (or 'next' for that matter) with this variable turned off, so I am reasonably sure it had no serious exposure to real world usage.

Having said that, assuming that the switching back to scripted version works correctly and assuming that we want to expose this to end users, I think the patch text makes sense.

Thanks.
Show 33 quoted lines
> Let's document it so that users who run into any stability issues with
> the C rewrite know there's an escape hatch[1], and make it clear that
> needing to turn off builtin rebase means you've found a bug in git.
>
> 1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config/rebase.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 42e1ba7575..f079bf6b7e 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,3 +1,17 @@
> +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.
> +
>  rebase.stat::
>  	Whether to show a diffstat of what changed upstream since the last
>  	rebase. False by default.
Johannes Schindelin· Nov 16, 2018, 09:35 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin

Hi Junio,
On Fri, 16 Nov 2018, Junio C Hamano wrote:
Show 10 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> > start implementing it as a builtin", 2018-08-07) was turned on by
> > default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> > 2018-08-08), but had no documentation.
> 
> I actually thought that everybody understood that this was merely an
> aid to be used during the development of the feature and never meant
> to be given to the end users.

It may have been from git.git's point of view, but from Git for Windows' point of view it was always meant to be a real feature flag.

> With my devil's advocate hat on, how much do we trust it as an
> escape hatch?

As you know, only a fraction of the bug reports about the built-in rebase came in from Git for Windows: the autostash-with-submodules bug and the perf-regression one. By my counting that is 2 out of 5 bugs coming in via that route.

One of the reasons for that was that the built-in rebase that was shipped in Git for Windows v2.19.1 was marked as experimental.

And the way I could mark it experimental was by flipping the default to executing the scripted version: https://github.com/git-for-windows/git/commit/cff1a96cfe (you will note that I added the same escape hatch for `git stash` by adding back `git-stash.sh` as `git-legacy-stash.sh` and imitating the same dance as for built-in `rebase`, and I also added back the scripted `git-rebase--interactive.sh` for use by `git-legacy-rebase.sh`).

Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and
if a user installed Git for Windows with the experimental built-in rebase,
it was set to `true` in the system config.

There was not a single complaint about the scripted `git rebase` being broken in any way.

So we *do* have some real-world testing of that feature. (Obviously I have no numbers about Git for Windows' usage, apart from download numbers, and they do not say how many users opted in and how many did not, but Git for Windows v2.19.1 was downloaded more than 2.7 million times so far and I think it is safe to assume that some percentage tested that feature.)

> After all, the codepath to hide the "rebase in C" implementation and use
> the scripted version were never in 'master' (or 'next' for that matter)
> with this variable turned off, so I am reasonably sure it had no serious
> exposure to real world usage.
See above for a counter-argument.
> Having said that, assuming that the switching back to scripted
> version works correctly and assuming that we want to expose this to
> end users, I think the patch text makes sense.
Indeed.

I would still love to see the built-in rebase to be used by default in v2.20.0, and I am reasonably sure that the escape hatch works (because, as I told you above, it worked in the reverse, making the built-in rebase an opt-in in Git for Windows v2.19.1).

Ciao, Dscho

Junio C Hamano· Nov 16, 2018, 09:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and
> if a user installed Git for Windows with the experimental built-in rebase,
> it was set to `true` in the system config.

Oh, that changes the picture entirely. If that was what was shipped to Windows users for 2.19.X, then I'd say we should be able to trust the switch well enough. I just somehow thought that everybody in the Windows land has been using the -in-C version.

Show 10 quoted lines
>> Having said that, assuming that the switching back to scripted
>> version works correctly and assuming that we want to expose this to
>> end users, I think the patch text makes sense.
>
> Indeed.
>
> I would still love to see the built-in rebase to be used by default in
> v2.20.0, and I am reasonably sure that the escape hatch works (because, as
> I told you above, it worked in the reverse, making the built-in rebase an
> opt-in in Git for Windows v2.19.1).
Good.  That makes things a lot simpler.
Thanks.
Ævar Arnfjörð Bjarmason· Nov 14, 2018, 09:15 UTC · lore

[PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off

Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent to running with rebase.useBuiltin=false. This is needed to spot that we're not introducing any regressions in the legacy rebase version while we're carrying both it and the new builtin version.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 5 ++++-
 t/README         | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)
Show changes to 2 files +8 −1

builtin/rebase.c, t/README

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..68ad8c1149 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf out = STRBUF_INIT;
-	int ret;
+	int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
+
+	if (env != -1)
+		return env;
 
 	argv_array_pushl(&cp.args,
 			 "config", "--bool", "rebase.usebuiltin", NULL);
diff --git a/t/README b/t/README
index 242497455f..3df5d12e46 100644
--- a/t/README
+++ b/t/README
@@ -339,6 +339,10 @@ for the index version specified.  Can be set to any valid version
 GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 
+GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when false, disables the
+builtin version of git-rebase. See 'rebase.useBuiltin' in
+git-config(1).
+
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
 of the index for the whole test suite by bypassing the default number of
 cache entries and thread minimums. Setting this to 1 will make the

-- 
2.19.1.1182.g4ecb1133ce
Johannes Schindelin· Nov 14, 2018, 13:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off

Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 10 quoted lines
> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
> to running with rebase.useBuiltin=false. This is needed to spot that
> we're not introducing any regressions in the legacy rebase version
> while we're carrying both it and the new builtin version.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/rebase.c | 5 ++++-
>  t/README         | 4 ++++
>  2 files changed, 8 insertions(+), 1 deletion(-)

I am slightly surprised not to see any ci/ change in this diffstat. Did you mean to add a test axis for Travis, or not?

Ciao, Dscho

Show 36 quoted lines
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..68ad8c1149 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf out = STRBUF_INIT;
> -	int ret;
> +	int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
> +
> +	if (env != -1)
> +		return env;
>  
>  	argv_array_pushl(&cp.args,
>  			 "config", "--bool", "rebase.usebuiltin", NULL);
> diff --git a/t/README b/t/README
> index 242497455f..3df5d12e46 100644
> --- a/t/README
> +++ b/t/README
> @@ -339,6 +339,10 @@ for the index version specified.  Can be set to any valid version
>  GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
>  by overriding the minimum number of cache entries required per thread.
>  
> +GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when false, disables the
> +builtin version of git-rebase. See 'rebase.useBuiltin' in
> +git-config(1).
> +
>  GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading
>  of the index for the whole test suite by bypassing the default number of
>  cache entries and thread minimums. Setting this to 1 will make the
> -- 
> 2.19.1.1182.g4ecb1133ce
> 
>
Ævar Arnfjörð Bjarmason· Nov 14, 2018, 13:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off

On Wed, Nov 14 2018, Johannes Schindelin wrote:
Show 17 quoted lines
> Hi Ævar,
>
> On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
>> to running with rebase.useBuiltin=false. This is needed to spot that
>> we're not introducing any regressions in the legacy rebase version
>> while we're carrying both it and the new builtin version.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/rebase.c | 5 ++++-
>>  t/README         | 4 ++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> I am slightly surprised not to see any ci/ change in this diffstat. Did
> you mean to add a test axis for Travis, or not?

No, but that's a logical follow-up by someone more familiar with the CI setup. I'm using this so I can test versions of "next" when building my own package.

← back to recent threads