{"thread":{"id":"49832","subject":"[PATCH v2 0/2] rebase.useBuiltin doc & test mode","startedAt":"2018-11-14T09:15:22Z","lastAt":"2018-11-16T09:58:17Z","messageCount":9,"participants":["Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"363339","messageId":"20181114091506.1452-1-avarab@gmail.com","threadId":"49832","inReplyTo":"0181114090144.31412-1-avarab@gmail.com","subject":"[PATCH v2 0/2] rebase.useBuiltin doc & test mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:15:04Z","receivedAt":"2018-11-14T09:15:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I was missing a \"=\" in the t/README docs in 2/2 in v1. Also move the\ndocs around slightly so this & my gettext series don't have conflicts.\n\n*** BLURB HERE ***\n\nÆvar Arnfjörð Bjarmason (2):\n rebase doc: document rebase.useBuiltin\n tests: add a special setup where rebase.useBuiltin is off\n\n Documentation/config/rebase.txt | 14 ++++++++++++++\n builtin/rebase.c | 5 ++++-\n t/README | 4 ++++\n 3 files changed, 22 insertions(+), 1 deletion(-)\n\nRange-diff:\n1: a5508195c6 ! 1: ca87aacbfa tests: add a special setup where rebase.useBuiltin is off\n @@ -29,12 +29,13 @@\n --- a/t/README\n +++ b/t/README\n @@\n - index to be written after every 'git repack' command, and overrides the\n - 'core.multiPackIndex' setting to true.\n + GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path\n + by overriding the minimum number of cache entries required per thread.\n \n -+GIT_TEST_REBASE_USE_BUILTIN, when false, disables the builtin\n -+version of git-rebase. See 'rebase.useBuiltin' in git-config(1).\n ++GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the\n ++builtin version of git-rebase. See 'rebase.useBuiltin' in\n ++git-config(1).\n +\n - Naming Tests\n - ------------\n - \n + GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading\n + of the index for the whole test suite by bypassing the default number of\n + cache entries and thread minimums. Setting this to 1 will make the\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363340","messageId":"20181114091506.1452-2-avarab@gmail.com","threadId":"49832","inReplyTo":"0181114090144.31412-1-avarab@gmail.com","subject":"[PATCH v2 1/2] rebase doc: document rebase.useBuiltin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:15:05Z","receivedAt":"2018-11-14T09:15:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The rebase.useBuiltin variable introduced in 55071ea248 (\"rebase:\nstart implementing it as a builtin\", 2018-08-07) was turned on by\ndefault in 5541bd5b8f (\"rebase: default to using the builtin rebase\",\n2018-08-08), but had no documentation.\n\nLet's document it so that users who run into any stability issues with\nthe C rewrite know there's an escape hatch[1], and make it clear that\nneeding to turn off builtin rebase means you've found a bug in git.\n\n1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/rebase.txt | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\nindex 42e1ba7575..f079bf6b7e 100644\n--- a/Documentation/config/rebase.txt\n+++ b/Documentation/config/rebase.txt\n@@ -1,3 +1,17 @@\n+rebase.useBuiltin::\n+\tSet to `false` to use the legacy shellscript implementation of\n+\tlinkgit:git-rebase[1]. Is `true` by default, which means use\n+\tthe built-in rewrite of it in C.\n++\n+The C rewrite is first included with Git version 2.20. This option\n+serves an an escape hatch to re-enable the legacy version in case any\n+bugs are found in the rewrite. This option and the shellscript version\n+of linkgit:git-rebase[1] will be removed in some future release.\n++\n+If you find some reason to set this option to `false` other than\n+one-off testing you should report the behavior difference as a bug in\n+git.\n+\n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\n \trebase. False by default.\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363341","messageId":"20181114091506.1452-3-avarab@gmail.com","threadId":"49832","inReplyTo":"0181114090144.31412-1-avarab@gmail.com","subject":"[PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T09:15:06Z","receivedAt":"2018-11-14T09:15:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent\nto running with rebase.useBuiltin=false. This is needed to spot that\nwe're not introducing any regressions in the legacy rebase version\nwhile we're carrying both it and the new builtin version.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rebase.c | 5 ++++-\n t/README | 4 ++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..68ad8c1149 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -48,7 +48,10 @@ static int use_builtin_rebase(void)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf out = STRBUF_INIT;\n-\tint ret;\n+\tint ret, env = git_env_bool(\"GIT_TEST_REBASE_USE_BUILTIN\", -1);\n+\n+\tif (env != -1)\n+\t\treturn env;\n \n \targv_array_pushl(&cp.args,\n \t\t\t \"config\", \"--bool\", \"rebase.usebuiltin\", NULL);\ndiff --git a/t/README b/t/README\nindex 242497455f..3df5d12e46 100644\n--- a/t/README\n+++ b/t/README\n@@ -339,6 +339,10 @@ for the index version specified. Can be set to any valid version\n GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path\n by overriding the minimum number of cache entries required per thread.\n \n+GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the\n+builtin version of git-rebase. See 'rebase.useBuiltin' in\n+git-config(1).\n+\n GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading\n of the index for the whole test suite by bypassing the default number of\n cache entries and thread minimums. Setting this to 1 will make the\n-- \n2.19.1.1182.g4ecb1133ce\n\n"},{"id":"363359","messageId":"nycvar.QRO.7.76.6.1811141451460.39@tvgsbejvaqbjf.bet","threadId":"49832","inReplyTo":"20181114091506.1452-2-avarab@gmail.com","subject":"Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T13:52:05Z","receivedAt":"2018-11-14T13:52:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> The rebase.useBuiltin variable introduced in 55071ea248 (\"rebase:\n> start implementing it as a builtin\", 2018-08-07) was turned on by\n> default in 5541bd5b8f (\"rebase: default to using the builtin rebase\",\n> 2018-08-08), but had no documentation.\n> \n> Let's document it so that users who run into any stability issues with\n> the C rewrite know there's an escape hatch[1], and make it clear that\n> needing to turn off builtin rebase means you've found a bug in git.\n> \n> 1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n\nMakes sense.\n\nThanks,\nDscho\n\n> ---\n> Documentation/config/rebase.txt | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n> \n> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\n> index 42e1ba7575..f079bf6b7e 100644\n> --- a/Documentation/config/rebase.txt\n> +++ b/Documentation/config/rebase.txt\n> @@ -1,3 +1,17 @@\n> +rebase.useBuiltin::\n> +\tSet to `false` to use the legacy shellscript implementation of\n> +\tlinkgit:git-rebase[1]. Is `true` by default, which means use\n> +\tthe built-in rewrite of it in C.\n> ++\n> +The C rewrite is first included with Git version 2.20. This option\n> +serves an an escape hatch to re-enable the legacy version in case any\n> +bugs are found in the rewrite. This option and the shellscript version\n> +of linkgit:git-rebase[1] will be removed in some future release.\n> ++\n> +If you find some reason to set this option to `false` other than\n> +one-off testing you should report the behavior difference as a bug in\n> +git.\n> +\n> rebase.stat::\n> \tWhether to show a diffstat of what changed upstream since the last\n> \trebase. False by default.\n> -- \n> 2.19.1.1182.g4ecb1133ce\n> \n> "},{"id":"363360","messageId":"nycvar.QRO.7.76.6.1811141452520.39@tvgsbejvaqbjf.bet","threadId":"49832","inReplyTo":"20181114091506.1452-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T13:53:29Z","receivedAt":"2018-11-14T13:53:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent\n> to running with rebase.useBuiltin=false. This is needed to spot that\n> we're not introducing any regressions in the legacy rebase version\n> while we're carrying both it and the new builtin version.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/rebase.c | 5 ++++-\n> t/README | 4 ++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n\nI am slightly surprised not to see any ci/ change in this diffstat. Did\nyou mean to add a test axis for Travis, or not?\n\nCiao,\nDscho\n\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0ee06aa363..68ad8c1149 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -48,7 +48,10 @@ static int use_builtin_rebase(void)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tstruct strbuf out = STRBUF_INIT;\n> -\tint ret;\n> +\tint ret, env = git_env_bool(\"GIT_TEST_REBASE_USE_BUILTIN\", -1);\n> +\n> +\tif (env != -1)\n> +\t\treturn env;\n> \n> \targv_array_pushl(&cp.args,\n> \t\t\t \"config\", \"--bool\", \"rebase.usebuiltin\", NULL);\n> diff --git a/t/README b/t/README\n> index 242497455f..3df5d12e46 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -339,6 +339,10 @@ for the index version specified. Can be set to any valid version\n> GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path\n> by overriding the minimum number of cache entries required per thread.\n> \n> +GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the\n> +builtin version of git-rebase. See 'rebase.useBuiltin' in\n> +git-config(1).\n> +\n> GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading\n> of the index for the whole test suite by bypassing the default number of\n> cache entries and thread minimums. Setting this to 1 will make the\n> -- \n> 2.19.1.1182.g4ecb1133ce\n> \n> "},{"id":"363361","messageId":"87tvkj222w.fsf@evledraar.gmail.com","threadId":"49832","inReplyTo":"nycvar.QRO.7.76.6.1811141452520.39@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-14T13:58:15Z","receivedAt":"2018-11-14T13:58:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 14 2018, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent\n>> to running with rebase.useBuiltin=false. This is needed to spot that\n>> we're not introducing any regressions in the legacy rebase version\n>> while we're carrying both it and the new builtin version.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/rebase.c | 5 ++++-\n>> t/README | 4 ++++\n>> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> I am slightly surprised not to see any ci/ change in this diffstat. Did\n> you mean to add a test axis for Travis, or not?\n\nNo, but that's a logical follow-up by someone more familiar with the CI\nsetup. I'm using this so I can test versions of \"next\" when building my\nown package.\n"},{"id":"363460","messageId":"xmqqr2fllmft.fsf@gitster-ct.c.googlers.com","threadId":"49832","inReplyTo":"20181114091506.1452-2-avarab@gmail.com","subject":"Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T03:40:22Z","receivedAt":"2018-11-16T03:40:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The rebase.useBuiltin variable introduced in 55071ea248 (\"rebase:\n> start implementing it as a builtin\", 2018-08-07) was turned on by\n> default in 5541bd5b8f (\"rebase: default to using the builtin rebase\",\n> 2018-08-08), but had no documentation.\n\nI actually thought that everybody understood that this was merely an\naid to be used during the development of the feature and never meant\nto be given to the end users.\n\nWith my devil's advocate hat on, how much do we trust it as an\nescape hatch? After all, the codepath to hide the \"rebase in C\"\nimplementation and use the scripted version were never in 'master'\n(or 'next' for that matter) with this variable turned off, so I am\nreasonably sure it had no serious exposure to real world usage.\n\nHaving said that, assuming that the switching back to scripted\nversion works correctly and assuming that we want to expose this to\nend users, I think the patch text makes sense.\n\nThanks.\n\n> Let's document it so that users who run into any stability issues with\n> the C rewrite know there's an escape hatch[1], and make it clear that\n> needing to turn off builtin rebase means you've found a bug in git.\n>\n> 1. https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config/rebase.txt | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\n> index 42e1ba7575..f079bf6b7e 100644\n> --- a/Documentation/config/rebase.txt\n> +++ b/Documentation/config/rebase.txt\n> @@ -1,3 +1,17 @@\n> +rebase.useBuiltin::\n> +\tSet to `false` to use the legacy shellscript implementation of\n> +\tlinkgit:git-rebase[1]. Is `true` by default, which means use\n> +\tthe built-in rewrite of it in C.\n> ++\n> +The C rewrite is first included with Git version 2.20. This option\n> +serves an an escape hatch to re-enable the legacy version in case any\n> +bugs are found in the rewrite. This option and the shellscript version\n> +of linkgit:git-rebase[1] will be removed in some future release.\n> ++\n> +If you find some reason to set this option to `false` other than\n> +one-off testing you should report the behavior difference as a bug in\n> +git.\n> +\n> rebase.stat::\n> \tWhether to show a diffstat of what changed upstream since the last\n> \trebase. False by default.\n"},{"id":"363489","messageId":"nycvar.QRO.7.76.6.1811161007030.41@tvgsbejvaqbjf.bet","threadId":"49832","inReplyTo":"xmqqr2fllmft.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-16T09:35:39Z","receivedAt":"2018-11-16T09:35:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 16 Nov 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > The rebase.useBuiltin variable introduced in 55071ea248 (\"rebase:\n> > start implementing it as a builtin\", 2018-08-07) was turned on by\n> > default in 5541bd5b8f (\"rebase: default to using the builtin rebase\",\n> > 2018-08-08), but had no documentation.\n> \n> I actually thought that everybody understood that this was merely an\n> aid to be used during the development of the feature and never meant\n> to be given to the end users.\n\nIt may have been from git.git's point of view, but from Git for Windows'\npoint of view it was always meant to be a real feature flag.\n\n> With my devil's advocate hat on, how much do we trust it as an\n> escape hatch?\n\nAs you know, only a fraction of the bug reports about the built-in rebase\ncame in from Git for Windows: the autostash-with-submodules bug and the\nperf-regression one. By my counting that is 2 out of 5 bugs coming in via\nthat route.\n\nOne of the reasons for that was that the built-in rebase that was shipped\nin Git for Windows v2.19.1 was marked as experimental.\n\nAnd the way I could mark it experimental was by flipping the default to\nexecuting the scripted version:\nhttps://github.com/git-for-windows/git/commit/cff1a96cfe (you will note\nthat I added the same escape hatch for `git stash` by adding back\n`git-stash.sh` as `git-legacy-stash.sh` and imitating the same dance as\nfor built-in `rebase`, and I also added back the scripted\n`git-rebase--interactive.sh` for use by `git-legacy-rebase.sh`).\n\nMeaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and\nif a user installed Git for Windows with the experimental built-in rebase,\nit was set to `true` in the system config.\n\nThere was not a single complaint about the scripted `git rebase` being\nbroken in any way.\n\nSo we *do* have some real-world testing of that feature. (Obviously I have\nno numbers about Git for Windows' usage, apart from download numbers, and\nthey do not say how many users opted in and how many did not, but Git for\nWindows v2.19.1 was downloaded more than 2.7 million times so far and I\nthink it is safe to assume that some percentage tested that feature.)\n\n> After all, the codepath to hide the \"rebase in C\" implementation and use\n> the scripted version were never in 'master' (or 'next' for that matter)\n> with this variable turned off, so I am reasonably sure it had no serious\n> exposure to real world usage.\n\nSee above for a counter-argument.\n\n> Having said that, assuming that the switching back to scripted\n> version works correctly and assuming that we want to expose this to\n> end users, I think the patch text makes sense.\n\nIndeed.\n\nI would still love to see the built-in rebase to be used by default in\nv2.20.0, and I am reasonably sure that the escape hatch works (because, as\nI told you above, it worked in the reverse, making the built-in rebase an\nopt-in in Git for Windows v2.19.1).\n\nCiao,\nDscho"},{"id":"363491","messageId":"xmqq1s7ljqdr.fsf@gitster-ct.c.googlers.com","threadId":"49832","inReplyTo":"nycvar.QRO.7.76.6.1811161007030.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T09:58:08Z","receivedAt":"2018-11-16T09:58:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and\n> if a user installed Git for Windows with the experimental built-in rebase,\n> it was set to `true` in the system config.\n\nOh, that changes the picture entirely. If that was what was shipped\nto Windows users for 2.19.X, then I'd say we should be able to trust\nthe switch well enough. I just somehow thought that everybody in\nthe Windows land has been using the -in-C version.\n\n>> Having said that, assuming that the switching back to scripted\n>> version works correctly and assuming that we want to expose this to\n>> end users, I think the patch text makes sense.\n>\n> Indeed.\n>\n> I would still love to see the built-in rebase to be used by default in\n> v2.20.0, and I am reasonably sure that the escape hatch works (because, as\n> I told you above, it worked in the reverse, making the built-in rebase an\n> opt-in in Git for Windows v2.19.1).\n\nGood. That makes things a lot simpler.\n\nThanks.\n"}]}