{"thread":{"id":"52596","subject":"Unreliable 'git rebase --onto'","startedAt":"2020-01-08T21:44:05Z","lastAt":"2020-02-05T14:31:35Z","messageCount":22,"participants":["Eugeniu Rosca","SZEDER Gábor","Elijah Newren","Alban Gruin","Junio C Hamano","Andrei Rybak","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389507","messageId":"20200108214349.GA17624@lxhi-065.adit-jv.com","threadId":"52596","inReplyTo":null,"subject":"Unreliable 'git rebase --onto'","fromName":"Eugeniu Rosca","fromEmail":"erosca@de.adit-jv.com","sentAt":"2020-01-08T21:43:49Z","receivedAt":"2020-01-08T21:44:05Z","isPatch":false,"sender":{"key":"erosca@de.adit-jv.com","avatar":null},"body":"Hello Git community,\n\nBelow is a simple reproduction scenario for what looks to be a bug (?)\nin 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).\n\nI would appreciate your confirmation of the misbehavior.\nIf the behavior is correct/expected, I would appreciate some feedback\nhow to avoid it in future, since it occurs with the default parameters.\n\n1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n\n2. ### Cherry pick an upstream commit, to contrast the results with\n 'git rebase --onto':\n $ git checkout -b v4.18-cherry-pick v4.18\n $ git cherry-pick 463fa44eec2fef50\n Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c\n warning: inexact rename detection was skipped due to too many files.\n warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.\n [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend\n Author: Evan Green \n Date: Wed Oct 2 14:00:21 2019 -0700\n 1 file changed, 4 insertions(+)\n\n3. ### In spite of the warning, the result matches the original commit:\n $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)\n\n4. ### Now, backport the same commit via 'git rebase --onto'\n $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n First, rewinding head to replay your work on top of it...\n Applying: Input: atmel_mxt_ts - disable IRQ across suspend\n\n5. ### The result is different:\n $ git branch v4.18-rebase-onto\n $ git diff v4.18-cherry-pick v4.18-rebase-onto\n\ndiff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c\nindex b45958e89cc5..2345b587662b 100644\n--- a/drivers/input/touchscreen/atmel_mxt_ts.c\n+++ b/drivers/input/touchscreen/atmel_mxt_ts.c\n@@ -3139,8 +3139,6 @@ static int __maybe_unused mxt_suspend(struct device *dev)\n \n \tmutex_unlock(&input_dev->mutex);\n \n-\tdisable_irq(data->irq);\n-\n \treturn 0;\n }\n \n@@ -3162,6 +3160,8 @@ static int __maybe_unused mxt_resume(struct device *dev)\n \n \tmutex_unlock(&input_dev->mutex);\n \n+\tdisable_irq(data->irq);\n+\n \treturn 0;\n }\n\n\nIn a nutshell, purely from user's perspective:\n - I get a warning from 'git cherry pick', with perfect results\n - I get no warning from 'git rebase --onto', with wrong results\n\nDoes git still behave expectedly? TIA!\n\n-- \nBest Regards,\nEugeniu\n"},{"id":"389509","messageId":"20200108223557.GE32750@szeder.dev","threadId":"52596","inReplyTo":"20200108214349.GA17624@lxhi-065.adit-jv.com","subject":"Re: Unreliable 'git rebase --onto'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-01-08T22:35:57Z","receivedAt":"2020-01-08T22:36:04Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jan 08, 2020 at 10:43:49PM +0100, Eugeniu Rosca wrote:\n> Hello Git community,\n> \n> Below is a simple reproduction scenario for what looks to be a bug (?)\n> in 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).\n> \n> I would appreciate your confirmation of the misbehavior.\n> If the behavior is correct/expected, I would appreciate some feedback\n> how to avoid it in future, since it occurs with the default parameters.\n> \n> 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n> \n> 2. ### Cherry pick an upstream commit, to contrast the results with\n> 'git rebase --onto':\n> $ git checkout -b v4.18-cherry-pick v4.18\n> $ git cherry-pick 463fa44eec2fef50\n> Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c\n> warning: inexact rename detection was skipped due to too many files.\n> warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.\n> [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend\n> Author: Evan Green \n> Date: Wed Oct 2 14:00:21 2019 -0700\n> 1 file changed, 4 insertions(+)\n> \n> 3. ### In spite of the warning, the result matches the original commit:\n> $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)\n> \n> 4. ### Now, backport the same commit via 'git rebase --onto'\n> $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> First, rewinding head to replay your work on top of it...\n> Applying: Input: atmel_mxt_ts - disable IRQ across suspend\n> \n> 5. ### The result is different:\n> $ git branch v4.18-rebase-onto\n> $ git diff v4.18-cherry-pick v4.18-rebase-onto\n> \n> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c\n> index b45958e89cc5..2345b587662b 100644\n> --- a/drivers/input/touchscreen/atmel_mxt_ts.c\n> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c\n> @@ -3139,8 +3139,6 @@ static int __maybe_unused mxt_suspend(struct device *dev)\n> \n> \tmutex_unlock(&input_dev->mutex);\n> \n> -\tdisable_irq(data->irq);\n> -\n> \treturn 0;\n> }\n> \n> @@ -3162,6 +3160,8 @@ static int __maybe_unused mxt_resume(struct device *dev)\n> \n> \tmutex_unlock(&input_dev->mutex);\n> \n> +\tdisable_irq(data->irq);\n> +\n> \treturn 0;\n> }\n> \n> \n> In a nutshell, purely from user's perspective:\n> - I get a warning from 'git cherry pick', with perfect results\n> - I get no warning from 'git rebase --onto', with wrong results\n> \n> Does git still behave expectedly? TIA!\n\nThis is a known issue with the 'am' backend of 'git rebase'.\n\nThe good news is that work is already well under way to change the\ndefault backend from 'am' to 'merge', which will solve this issue.\nFrom the log message of aa523de170 (rebase: change the default backend\nfrom \"am\" to \"merge\", 2019-12-24):\n\n The am-backend drops information and thus limits what we can do:\n [...]\n * reduction in context from only having a few lines beyond those\n changed means that when context lines are non-unique we can apply\n patches incorrectly.[2]\n [...]\n [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>\n\nAlas, there is unexpected bad news: with that commit the runtime of\nyour 'git rebase --onto' command goes from <1sec to over 50secs.\nCc-ing Elijah, author of that patch...\n\n"},{"id":"389513","messageId":"CABPp-BHsy75UGm4wTOP2_AYik_dZi-_BxtAn-hyi-ZrNRRWGuw@mail.gmail.com","threadId":"52596","inReplyTo":"20200108223557.GE32750@szeder.dev","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-09T00:55:46Z","receivedAt":"2020-01-09T00:56:00Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"user 0m9.644s\nsys 0m3.620s\nOn Wed, Jan 8, 2020 at 2:36 PM SZEDER Gábor wrote:\n>\n> On Wed, Jan 08, 2020 at 10:43:49PM +0100, Eugeniu Rosca wrote:\n> > Hello Git community,\n> >\n> > Below is a simple reproduction scenario for what looks to be a bug (?)\n> > in 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).\n> >\n> > I would appreciate your confirmation of the misbehavior.\n> > If the behavior is correct/expected, I would appreciate some feedback\n> > how to avoid it in future, since it occurs with the default parameters.\n> >\n> > 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n> >\n> > 2. ### Cherry pick an upstream commit, to contrast the results with\n> > 'git rebase --onto':\n> > $ git checkout -b v4.18-cherry-pick v4.18\n> > $ git cherry-pick 463fa44eec2fef50\n> > Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c\n> > warning: inexact rename detection was skipped due to too many files.\n> > warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.\n\nLots of renames...\n\n> > [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend\n> > Author: Evan Green \n> > Date: Wed Oct 2 14:00:21 2019 -0700\n> > 1 file changed, 4 insertions(+)\n> >\n> > 3. ### In spite of the warning, the result matches the original commit:\n> > $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)\n> >\n> > 4. ### Now, backport the same commit via 'git rebase --onto'\n> > $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> > First, rewinding head to replay your work on top of it...\n> > Applying: Input: atmel_mxt_ts - disable IRQ across suspend\n> >\n> > 5. ### The result is different:\n> > $ git branch v4.18-rebase-onto\n> > $ git diff v4.18-cherry-pick v4.18-rebase-onto\n> >\n> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c\n> > index b45958e89cc5..2345b587662b 100644\n> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c\n> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c\n> > @@ -3139,8 +3139,6 @@ static int __maybe_unused mxt_suspend(struct device *dev)\n> >\n> > mutex_unlock(&input_dev->mutex);\n> >\n> > - disable_irq(data->irq);\n> > -\n> > return 0;\n> > }\n> >\n> > @@ -3162,6 +3160,8 @@ static int __maybe_unused mxt_resume(struct device *dev)\n> >\n> > mutex_unlock(&input_dev->mutex);\n> >\n> > + disable_irq(data->irq);\n> > +\n> > return 0;\n> > }\n> >\n> >\n> > In a nutshell, purely from user's perspective:\n> > - I get a warning from 'git cherry pick', with perfect results\n> > - I get no warning from 'git rebase --onto', with wrong results\n> >\n> > Does git still behave expectedly? TIA!\n>\n> This is a known issue with the 'am' backend of 'git rebase'.\n>\n> The good news is that work is already well under way to change the\n> default backend from 'am' to 'merge', which will solve this issue.\n> From the log message of aa523de170 (rebase: change the default backend\n> from \"am\" to \"merge\", 2019-12-24):\n>\n> The am-backend drops information and thus limits what we can do:\n> [...]\n> * reduction in context from only having a few lines beyond those\n> changed means that when context lines are non-unique we can apply\n> patches incorrectly.[2]\n> [...]\n> [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>\n>\n> Alas, there is unexpected bad news: with that commit the runtime of\n> your 'git rebase --onto' command goes from <1sec to over 50secs.\n> Cc-ing Elijah, author of that patch...\n\nI see slowdown, but not nearly as big as you report:\n\n$ git checkout -b v4.18-cherry-pick v4.18\n$ time git cherry-pick 463fa44eec2fef50\nAuto-merging drivers/input/touchscreen/atmel_mxt_ts.c\nwarning: inexact rename detection was skipped due to too many files.\nwarning: you may want to set your merge.renamelimit variable to at\nleast 7216 and retry the command.\n[v4.18-cherry-pick 88d39cdf3e80] Input: atmel_mxt_ts - disable IRQ\nacross suspend\n Author: Evan Green \n Date: Wed Oct 2 14:00:21 2019 -0700\n 1 file changed, 4 insertions(+)\n\nreal 0m1.110s\nuser 0m0.956s\nsys 0m0.284s\n$ time git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef5\nFirst, rewinding head to replay your work on top of it...\nApplying: Input: atmel_mxt_ts - disable IRQ across suspend\n\nreal 0m1.643s\nuser 0m1.296s\nsys 0m0.264s\n$ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\nwarning: inexact rename detection was skipped due to too many files.\nwarning: you may want to set your merge.renamelimit variable to at\nleast 7216 and retry the command.\nSuccessfully rebased and updated detached HEAD.\n\nreal 0m13.305s\nuser 0m9.644s\nsys 0m3.620s\n\n\n\n\nInterestingly, turning off rename detection only speeds it up a little bit:\n$ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~\n463fa44eec2fef50\nSuccessfully rebased and updated detached HEAD.\n\nreal 0m11.955s\nuser 0m8.732s\nsys 0m3.424s\n\n\nThis is an interesting testcase; I'm going to try to find some time to\ndig in further.\n"},{"id":"389531","messageId":"20200109105307.GA1349@lxhi-065.adit-jv.com","threadId":"52596","inReplyTo":"CABPp-BHsyMOz+hi7EYoAnAWfzms7FRfwqCoarnu8H+vyDoN6SQ@mail.gmail.com","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Eugeniu Rosca","fromEmail":"erosca@de.adit-jv.com","sentAt":"2020-01-09T10:53:07Z","receivedAt":"2020-01-09T10:53:19Z","isPatch":false,"sender":{"key":"erosca@de.adit-jv.com","avatar":null},"body":"Hi Elijah, hi Szeder,\n\nOn Wed, Jan 08, 2020 at 02:06:22PM -0800, Elijah Newren wrote:\n> This looks like a known bug in rebase, in particular in the am-backend that\n> rebase uses by default. If I'm correct that it's just a context region\n> issue, then this is the same bug that was recently discussed at\n> https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/.\n> The current plan is to switch the default over to the merge backend (the\n> same machinery that cherry-pick uses), which doesn't suffer from the same\n> shortcomings (you can see the current work being done in this area at\n> https://lore.kernel.org/git/pull.679.v3.git.git.1577217299.gitgitgadget@gmail.com/\n> ).\n\nThank you for your feedback and references, here and in [*].\n\nOnce hit by this or similar issues, I think there is high chance for\npeople to go through the same feelings as described by Pavel in [**]:\n\n ---\n That's so scary that I'm going to stop using \"git rebase\" for now.\n ---\n\nSome years ago I was hit by 'git merge' producing slightly different\nresults compared to 'git rebase --onto' and 'git cherry-pick A..B'\n(maybe I can come up with a reproduction scenario for that too).\n\nSince then, I usually contrast the outcomes of merging, cherry-picking\nand rebasing, to make sure they match, but that's painful and\ntime-consuming.\n\n> In the mean time, you can pass the -m flag to rebase to avoid these types\n> of problems. In fact, if you could retry with -m you may be able to\n> confirm whether it's the same issue.\n\nIndeed, neither `git rebase -m` nor `git rebase -i` exhibit the problem.\n\n[*] https://lore.kernel.org/git/CABPp-BHsy75UGm4wTOP2_AYik_dZi-_BxtAn-hyi-ZrNRRWGuw@mail.gmail.com/T/#m1cbf80ef56c260a626146d61291d7fbabd108f1b\n[**] https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/\n\nThanks again.\n\n-- \nBest Regards,\nEugeniu\n"},{"id":"389532","messageId":"20200109111306.GB1349@lxhi-065.adit-jv.com","threadId":"52596","inReplyTo":"20200108223557.GE32750@szeder.dev","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Eugeniu Rosca","fromEmail":"erosca@de.adit-jv.com","sentAt":"2020-01-09T11:13:06Z","receivedAt":"2020-01-09T11:13:16Z","isPatch":false,"sender":{"key":"erosca@de.adit-jv.com","avatar":null},"body":"Hi Szeder,\n\nOn Wed, Jan 08, 2020 at 11:35:57PM +0100, SZEDER Gábor wrote:\n> This is a known issue with the 'am' backend of 'git rebase'.\n> \n> The good news is that work is already well under way to change the\n> default backend from 'am' to 'merge', which will solve this issue.\n> From the log message of aa523de170 (rebase: change the default backend\n> from \"am\" to \"merge\", 2019-12-24):\n> \n> The am-backend drops information and thus limits what we can do:\n> [...]\n> * reduction in context from only having a few lines beyond those\n> changed means that when context lines are non-unique we can apply\n> patches incorrectly.[2]\n> [...]\n> [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>\n> \n> Alas, there is unexpected bad news: with that commit the runtime of\n> your 'git rebase --onto' command goes from <1sec to over 50secs.\n> Cc-ing Elijah, author of that patch...\n\n[$.02] I would personally take the route of regaining users' trust in\n'git rebase' first, with fixing the performance penalty later on.\n\nI was quite impressed by the recent 2.24.0 performance improvements,\nwhich tells there might be room for improvement for `git rebase` too,\nonce it is fixed.\n\n-- \nBest Regards,\nEugeniu\n"},{"id":"389534","messageId":"20200109150332.GF32750@szeder.dev","threadId":"52596","inReplyTo":"CABPp-BHsy75UGm4wTOP2_AYik_dZi-_BxtAn-hyi-ZrNRRWGuw@mail.gmail.com","subject":"Re: Unreliable 'git rebase --onto'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-01-09T15:03:32Z","receivedAt":"2020-01-09T15:03:39Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jan 08, 2020 at 04:55:46PM -0800, Elijah Newren wrote:\n> > Alas, there is unexpected bad news: with that commit the runtime of\n> > your 'git rebase --onto' command goes from <1sec to over 50secs.\n> > Cc-ing Elijah, author of that patch...\n> \n> I see slowdown, but not nearly as big as you report:\n\nThe linux repo is big, my notebook is small, the poor thing :)\n\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> warning: inexact rename detection was skipped due to too many files.\n> warning: you may want to set your merge.renamelimit variable to at\n> least 7216 and retry the command.\n> Successfully rebased and updated detached HEAD.\n> \n> real 0m13.305s\n> user 0m9.644s\n> sys 0m3.620s\n\n> Interestingly, turning off rename detection only speeds it up a little bit:\n> $ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~\n> 463fa44eec2fef50\n> Successfully rebased and updated detached HEAD.\n> \n> real 0m11.955s\n> user 0m8.732s\n> sys 0m3.424s\n> \n> \n> This is an interesting testcase; I'm going to try to find some time to\n> dig in further.\n\nThe culprits are two seemingly unnecessary back-and-forth checkouts.\n\nI didn't realize I could use 'git rebase -m', so ran some tests with\nit, and turns out that the slowdown started with 68aa495b59 (rebase:\nimplement --merge via the interactive machinery, 2018-12-11), where\nthe runtime suddenly went from <1.5s to 45+s.\n\nRunning 'git rebase -i --onto ' is just as slow,\nand it appears that it has always been (the oldest I tried was\nv1.8.0), and it spends a long time both before and after popping up\nthe editor for the rebase instructions. That's highly suspicious, so:\n\n $ git log --oneline -1\n 94710cac0ef4 (HEAD, tag: v4.18) Linux 4.18\n $ git rebase -i --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n hint: Waiting for your editor to close the file... \n # Hit ctrl-z in the editor\n $ git log --oneline -1\n 463fa44eec2f (HEAD) Input: atmel_mxt_ts - disable IRQ across suspend\n\nOh.\n\nSo 'git rebase -i' apparently checks out the tip commit of the\nto-be-rebased revision range before invoking the editor for the rebase\ninstructions, only to check out the --onto commit (i.e. the commit\nwe've started from!) to apply the selected commit on top.\n\nAnd indeed those two checkouts account for all the wasted runtime:\n\n $ time { git checkout 463fa44eec2fef50 && git checkout v4.18 ; }\n Updating files: 100% (49483/49483), done.\n Previous HEAD position was 94710cac0ef4 Linux 4.18\n HEAD is now at 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend\n Updating files: 100% (49483/49483), done.\n Previous HEAD position was 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend\n HEAD is now at 94710cac0ef4 Linux 4.18\n \n real 0m48.801s\n user 0m13.963s\n sys 0m5.114s\n\n"},{"id":"389536","messageId":"CABPp-BEQn83MJKLdOh+9BUWsOeD5seP9Zf8hbVhkAPCOaivHaw@mail.gmail.com","threadId":"52596","inReplyTo":"20200109150332.GF32750@szeder.dev","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-09T17:53:30Z","receivedAt":"2020-01-09T17:53:43Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Thu, Jan 9, 2020 at 7:03 AM SZEDER Gábor wrote:\n>\n> On Wed, Jan 08, 2020 at 04:55:46PM -0800, Elijah Newren wrote:\n> > > Alas, there is unexpected bad news: with that commit the runtime of\n> > > your 'git rebase --onto' command goes from <1sec to over 50secs.\n> > > Cc-ing Elijah, author of that patch...\n> >\n> > I see slowdown, but not nearly as big as you report:\n>\n> The linux repo is big, my notebook is small, the poor thing :)\n\nIt went to just over 64secs on my home laptop (older and with spinny\ndisks), so yeah, a big difference from my work machine which has an\nSSD.\n\n> > $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> > warning: inexact rename detection was skipped due to too many files.\n> > warning: you may want to set your merge.renamelimit variable to at\n> > least 7216 and retry the command.\n> > Successfully rebased and updated detached HEAD.\n> >\n> > real 0m13.305s\n> > user 0m9.644s\n> > sys 0m3.620s\n>\n> > Interestingly, turning off rename detection only speeds it up a little bit:\n> > $ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~\n> > 463fa44eec2fef50\n> > Successfully rebased and updated detached HEAD.\n> >\n> > real 0m11.955s\n> > user 0m8.732s\n> > sys 0m3.424s\n> >\n> >\n> > This is an interesting testcase; I'm going to try to find some time to\n> > dig in further.\n>\n> The culprits are two seemingly unnecessary back-and-forth checkouts.\n>\n> I didn't realize I could use 'git rebase -m', so ran some tests with\n> it, and turns out that the slowdown started with 68aa495b59 (rebase:\n> implement --merge via the interactive machinery, 2018-12-11), where\n> the runtime suddenly went from <1.5s to 45+s.\n>\n> Running 'git rebase -i --onto ' is just as slow,\n> and it appears that it has always been (the oldest I tried was\n> v1.8.0), and it spends a long time both before and after popping up\n> the editor for the rebase instructions. That's highly suspicious, so:\n>\n> $ git log --oneline -1\n> 94710cac0ef4 (HEAD, tag: v4.18) Linux 4.18\n> $ git rebase -i --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> hint: Waiting for your editor to close the file...\n> # Hit ctrl-z in the editor\n> $ git log --oneline -1\n> 463fa44eec2f (HEAD) Input: atmel_mxt_ts - disable IRQ across suspend\n>\n> Oh.\n>\n> So 'git rebase -i' apparently checks out the tip commit of the\n> to-be-rebased revision range before invoking the editor for the rebase\n> instructions, only to check out the --onto commit (i.e. the commit\n> we've started from!) to apply the selected commit on top.\n>\n> And indeed those two checkouts account for all the wasted runtime:\n>\n> $ time { git checkout 463fa44eec2fef50 && git checkout v4.18 ; }\n> Updating files: 100% (49483/49483), done.\n> Previous HEAD position was 94710cac0ef4 Linux 4.18\n> HEAD is now at 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend\n> Updating files: 100% (49483/49483), done.\n> Previous HEAD position was 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend\n> HEAD is now at 94710cac0ef4 Linux 4.18\n>\n> real 0m48.801s\n> user 0m13.963s\n> sys 0m5.114s\n\nOh, cool, sounds like you're already investigating and found the problem.\n"},{"id":"389537","messageId":"CABPp-BFiDNb18m8geTCxKLXg0fOd0DS1dWRVWCfnTG0suwGRHg@mail.gmail.com","threadId":"52596","inReplyTo":"20200109105307.GA1349@lxhi-065.adit-jv.com","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-09T18:05:52Z","receivedAt":"2020-01-09T18:06:05Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca wrote:\n>\n> Hi Elijah, hi Szeder,\n>\n> On Wed, Jan 08, 2020 at 02:06:22PM -0800, Elijah Newren wrote:\n> > This looks like a known bug in rebase, in particular in the am-backend that\n> > rebase uses by default. If I'm correct that it's just a context region\n> > issue, then this is the same bug that was recently discussed at\n> > https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/.\n> > The current plan is to switch the default over to the merge backend (the\n> > same machinery that cherry-pick uses), which doesn't suffer from the same\n> > shortcomings (you can see the current work being done in this area at\n> > https://lore.kernel.org/git/pull.679.v3.git.git.1577217299.gitgitgadget@gmail.com/\n> > ).\n>\n> Thank you for your feedback and references, here and in [*].\n>\n> Once hit by this or similar issues, I think there is high chance for\n> people to go through the same feelings as described by Pavel in [**]:\n>\n> ---\n> That's so scary that I'm going to stop using \"git rebase\" for now.\n> ---\n\nYep, I understand; that kind of feeling is why I wanted to jump in and\ntry to help fix it. I want merge/rebase/cherry-pick to be reliable.\n\n> Some years ago I was hit by 'git merge' producing slightly different\n> results compared to 'git rebase --onto' and 'git cherry-pick A..B'\n> (maybe I can come up with a reproduction scenario for that too).\n\nIf you can, I'd be interested to see it and take a look. I'd normally\nassume it was just some case where A..B included \"evil\" merge commits\n(merge commits that made additional changes not part of the actual\nmerging) since rebasing or cherry-picking such a range would exclude\nthe merge commits and thus drop those changes -- but you identified a\nreal bug with the default rebase backend so I'm interested to see if\nyou happen to have more bugs I should know about.\n\n>\n> Since then, I usually contrast the outcomes of merging, cherry-picking\n> and rebasing, to make sure they match, but that's painful and\n> time-consuming.\n>\n> > In the mean time, you can pass the -m flag to rebase to avoid these types\n> > of problems. In fact, if you could retry with -m you may be able to\n> > confirm whether it's the same issue.\n>\n> Indeed, neither `git rebase -m` nor `git rebase -i` exhibit the problem.\n\nThat's good news.\n\nUnfortunately, you should note that git-2.25 is going to have the same\nbug you reported; there are still some loose ends with my series to\nmake -m the default, and the 2.25 release is expected within a few\ndays, so my change of default won't happen until 2.26. (That series\nwould have needed to be completed several weeks ago for it to go into\n2.25).\n"},{"id":"389559","messageId":"20200110000603.GA19040@erosca","threadId":"52596","inReplyTo":"CABPp-BFiDNb18m8geTCxKLXg0fOd0DS1dWRVWCfnTG0suwGRHg@mail.gmail.com","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Eugeniu Rosca","fromEmail":"roscaeugeniu@gmail.com","sentAt":"2020-01-10T00:06:03Z","receivedAt":"2020-01-10T00:08:05Z","isPatch":false,"sender":{"key":"roscaeugeniu@gmail.com","avatar":null},"body":"Hi Elijah,\n\nOn Thu, Jan 09, 2020 at 10:05:52AM -0800, Elijah Newren wrote:\n> On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca wrote:\n> > Some years ago I was hit by 'git merge' producing slightly different\n> > results compared to 'git rebase --onto' and 'git cherry-pick A..B'\n> > (maybe I can come up with a reproduction scenario for that too).\n> \n> If you can, I'd be interested to see it and take a look. I'd normally\n> assume it was just some case where A..B included \"evil\" merge commits\n> (merge commits that made additional changes not part of the actual\n> merging) since rebasing or cherry-picking such a range would exclude\n> the merge commits and thus drop those changes -- but you identified a\n> real bug with the default rebase backend so I'm interested to see if\n> you happen to have more bugs I should know about.\n\nHere is a _simplified_ scenario to get a totally unexpected result from\n'git merge' (initially reproduced years ago, but still happening on\n2.25.0.rc2):\n\n ## Preparation\n0. git --version\n git version 2.25.0.rc2\n1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n2. git remote add linux-stable https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git\n3. git fetch linux-stable\n\n # Reproduction\n4. git checkout f7a8e38f07a1\n5. git merge --no-edit e18da11fc0f959\n ## Merge v4.4.3 commit\n https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e18da11fc0f959\n which is a linux-stable backport of vanilla v4.5-rc1 commit\n https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7a8e38f07a1\n the latter being checked out at step 4.\n\n6. git show HEAD\n ## Inspect the _automatic_ conflict resolution performed by git in\n drivers/mtd/nand/nand_base.c. Git decided to integrate e18da11fc0f959\n alongside f7a8e38f07a1, while essentially they are the same commit.\n We end up with two times commit f7a8e38f07a1.\n\nWhat do you think about that?\n\n> Unfortunately, you should note that git-2.25 is going to have the same\n> bug you reported; there are still some loose ends with my series to\n> make -m the default, and the 2.25 release is expected within a few\n> days, so my change of default won't happen until 2.26. (That series\n> would have needed to be completed several weeks ago for it to go into\n> 2.25).\n\nThanks for this piece of information and for the time/effort spent!\n\n-- \nBest Regards,\nEugeniu\n"},{"id":"389563","messageId":"CABPp-BHvJHpSJT7sdFwfNcPn_sOXwJi3=o14qjZS3M8Rzcxe2A@mail.gmail.com","threadId":"52596","inReplyTo":"20200110000603.GA19040@erosca","subject":"Re: Unreliable 'git rebase --onto'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-10T02:35:27Z","receivedAt":"2020-01-10T02:35:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Eugeniu,\n\nOn Thu, Jan 9, 2020 at 4:06 PM Eugeniu Rosca wrote:\n>\n> Hi Elijah,\n>\n> On Thu, Jan 09, 2020 at 10:05:52AM -0800, Elijah Newren wrote:\n> > On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca wrote:\n> > > Some years ago I was hit by 'git merge' producing slightly different\n> > > results compared to 'git rebase --onto' and 'git cherry-pick A..B'\n> > > (maybe I can come up with a reproduction scenario for that too).\n> >\n> > If you can, I'd be interested to see it and take a look. I'd normally\n> > assume it was just some case where A..B included \"evil\" merge commits\n> > (merge commits that made additional changes not part of the actual\n> > merging) since rebasing or cherry-picking such a range would exclude\n> > the merge commits and thus drop those changes -- but you identified a\n> > real bug with the default rebase backend so I'm interested to see if\n> > you happen to have more bugs I should know about.\n>\n> Here is a _simplified_ scenario to get a totally unexpected result from\n> 'git merge' (initially reproduced years ago, but still happening on\n> 2.25.0.rc2):\n>\n> ## Preparation\n> 0. git --version\n> git version 2.25.0.rc2\n> 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n> 2. git remote add linux-stable https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git\n> 3. git fetch linux-stable\n>\n> # Reproduction\n> 4. git checkout f7a8e38f07a1\n> 5. git merge --no-edit e18da11fc0f959\n> ## Merge v4.4.3 commit\n> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e18da11fc0f959\n> which is a linux-stable backport of vanilla v4.5-rc1 commit\n> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7a8e38f07a1\n> the latter being checked out at step 4.\n>\n> 6. git show HEAD\n> ## Inspect the _automatic_ conflict resolution performed by git in\n> drivers/mtd/nand/nand_base.c. Git decided to integrate e18da11fc0f959\n> alongside f7a8e38f07a1, while essentially they are the same commit.\n> We end up with two times commit f7a8e38f07a1.\n>\n> What do you think about that?\n\nOoh, interesting case; thanks for sending it along. I think this is\nthe same as https://lore.kernel.org/git/20190816184051.GB13894@sigill.intra.peff.net/\n, which struck git itself not that long back. It didn't do any actual\nharm, though, it was just surprising. I'm not familiar with the xdiff\npart of the codebase, so I don't know if this is a heuristic thing, or\nsomething more along the lines of the diff3 issues mentioned at\nhttps://www.cis.upenn.edu/~bcpierce/papers/diff3-short.pdf. I read up\non this area a little bit a few months ago and I'd like to dig more at\nthe diff3 stuff in general, but it may be a little while. If you see\nmore issues like this, though, I'm definitely interested in saving and\ncataloging them for when I get back to this.\n\nElijah\n"},{"id":"390195","messageId":"20200121191857.23047-1-alban.gruin@gmail.com","threadId":"52596","inReplyTo":"20200109150332.GF32750@szeder.dev","subject":"[PATCH v1] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-01-21T19:18:57Z","receivedAt":"2020-01-21T19:21:18Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"One of the first things done by the interactive rebase is to make a todo\nlist. This requires knowledge of the commit range to rebase. To get\nthe oid of the last commit of the range, the tip of the branch to rebase\nis checked out with prepare_branch_to_be_rebased(), then the oid of the\nHEAD is read. On big repositories, it's a performance penalty: the user\nmay have to wait before editing the todo list while git is extracting the\nbranch silently (because git-checkout is silenced here). After this,\nthe head of the branch is not even modified.\n\nSince we already have the oid of the tip of the branch in\n`opts->orig_head', it's useless to switch to this commit.\n\nThis removes the call to prepare_branch_to_be_rebased() in\ndo_interactive_rebase(), and adds a `orig_head' parameter to\nget_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\nis no longer used.\n\nThis introduces a visible change: as we do not switch on the tip of the\nbranch to rebase, no reflog entry is created at the beginning of the\nrebase for it.\n\nReported-by: SZEDER Gábor \nSigned-off-by: Alban Gruin \n---\n\nNotes:\n Improvements brought by this patch:\n \n Before:\n \n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n \n real 0m8,940s\n user 0m6,830s\n sys 0m2,121s\n \n After:\n \n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n \n real 0m1,834s\n user 0m0,916s\n sys 0m0,206s\n \n Both tests have been performed on a 5400 RPM SATA III hard drive.\n\n builtin/rebase.c | 18 +++++-------------\n sequencer.c | 14 --------------\n sequencer.h | 3 ---\n 3 files changed, 5 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8081741f8a..6154ad8fa5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t const char **head_hash,\n+\t\t\t struct object_id *orig_head, const char **head_hash,\n \t\t\t char **revisions, char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n-\tstruct object_id orig_head;\n-\n-\tif (get_oid(\"HEAD\", &orig_head))\n-\t\treturn error(_(\"no HEAD?\"));\n \n-\t*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n+\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n \t\t\t\t\t\t *head_hash);\n \n-\tshorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n+\tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n \tif (upstream) {\n \t\tconst char *shortrev;\n@@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct replay_opts replay = get_replay_opts(opts);\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n-\tif (prepare_branch_to_be_rebased(the_repository, &replay,\n-\t\t\t\t\t opts->switch_to))\n-\t\treturn -1;\n-\n-\tif (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n-\t\t\t\t&revisions, &shortrevisions))\n+\tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n+\t\t\t\t&head_hash, &revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..4dc245d7ec 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \treturn ret;\n }\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit)\n-{\n-\tconst char *action;\n-\n-\tif (commit && *commit) {\n-\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n-\t\tif (run_git_checkout(r, opts, commit, action))\n-\t\t\treturn error(_(\"could not checkout %s\"), commit);\n-\t}\n-\n-\treturn 0;\n-}\n-\n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n \t\t\t const char *orig_head)\ndiff --git a/sequencer.h b/sequencer.h\nindex 9f9ae291e3..74f1e2673e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit);\n-\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(struct repository *repo,\n-- \n2.24.1\n\n"},{"id":"390198","messageId":"CABPp-BEMZS4b_iYqP8nw0Oegfdx4DQadSwp00mXKPiaV58Pbpw@mail.gmail.com","threadId":"52596","inReplyTo":"20200121191857.23047-1-alban.gruin@gmail.com","subject":"Re: [PATCH v1] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-21T20:07:20Z","receivedAt":"2020-01-21T20:07:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Alban,\n\n// Adding Phillip and Johannes since they know the sequencer internals\nvery well.\n\nOn Tue, Jan 21, 2020 at 11:21 AM Alban Gruin wrote:\n>\n> One of the first things done by the interactive rebase is to make a todo\n> list. This requires knowledge of the commit range to rebase. To get\n> the oid of the last commit of the range, the tip of the branch to rebase\n> is checked out with prepare_branch_to_be_rebased(), then the oid of the\n> HEAD is read. On big repositories, it's a performance penalty: the user\n> may have to wait before editing the todo list while git is extracting the\n> branch silently (because git-checkout is silenced here). After this,\n> the head of the branch is not even modified.\n>\n> Since we already have the oid of the tip of the branch in\n> `opts->orig_head', it's useless to switch to this commit.\n>\n> This removes the call to prepare_branch_to_be_rebased() in\n> do_interactive_rebase(), and adds a `orig_head' parameter to\n> get_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\n> is no longer used.\n>\n> This introduces a visible change: as we do not switch on the tip of the\n> branch to rebase, no reflog entry is created at the beginning of the\n> rebase for it.\n\nOh, sweet, thanks for digging in. I had also dug in just after the\nreport, but not quite far enough as I still had failing tests and I\nwas feeling a bit stretched thin on other projects so I punted hoping\nthat SZEDER would post something. Looks like the orig_head thing was\nprobably what I was missing.\n\nI was a little surprised that there wasn't any regression test that\nneeded to be modified, as it reminded me of a previous conversation\nabout excessive work in the interactive backend[1], but after looking\nit up that was apparently about too many calls to commit rather than\ntoo many calls to checkout.\n\n[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1811121614190.39@tvgsbejvaqbjf.bet/\n\n> Reported-by: SZEDER Gábor \n> Signed-off-by: Alban Gruin \n> ---\n>\n> Notes:\n> Improvements brought by this patch:\n>\n> Before:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m8,940s\n> user 0m6,830s\n> sys 0m2,121s\n>\n> After:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m1,834s\n> user 0m0,916s\n> sys 0m0,206s\n\nNice...do we want to mention this in the commit message proper too?\n\n>\n> Both tests have been performed on a 5400 RPM SATA III hard drive.\n>\n> builtin/rebase.c | 18 +++++-------------\n> sequencer.c | 14 --------------\n> sequencer.h | 3 ---\n> 3 files changed, 5 insertions(+), 30 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 8081741f8a..6154ad8fa5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n> }\n>\n> static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n> - const char **head_hash,\n> + struct object_id *orig_head, const char **head_hash,\n> char **revisions, char **shortrevisions)\n> {\n> struct commit *base_rev = upstream ? upstream : onto;\n> const char *shorthead;\n> - struct object_id orig_head;\n> -\n> - if (get_oid(\"HEAD\", &orig_head))\n> - return error(_(\"no HEAD?\"));\n>\n> - *head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n> + *head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n> *revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n> *head_hash);\n>\n> - shorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n> + shorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n>\n> if (upstream) {\n> const char *shortrev;\n> @@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> struct replay_opts replay = get_replay_opts(opts);\n> struct string_list commands = STRING_LIST_INIT_DUP;\n>\n> - if (prepare_branch_to_be_rebased(the_repository, &replay,\n> - opts->switch_to))\n> - return -1;\n> -\n> - if (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n> - &revisions, &shortrevisions))\n> + if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n> + &head_hash, &revisions, &shortrevisions))\n> return -1;\n>\n> if (init_basic_state(&replay,\n> diff --git a/sequencer.c b/sequencer.c\n> index b9dbf1adb0..4dc245d7ec 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n> return ret;\n> }\n>\n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> - const char *commit)\n> -{\n> - const char *action;\n> -\n> - if (commit && *commit) {\n> - action = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> - if (run_git_checkout(r, opts, commit, action))\n> - return error(_(\"could not checkout %s\"), commit);\n> - }\n> -\n> - return 0;\n> -}\n> -\n> static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> const char *onto_name, const struct object_id *onto,\n> const char *orig_head)\n> diff --git a/sequencer.h b/sequencer.h\n> index 9f9ae291e3..74f1e2673e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n> const struct commit *current_head,\n> const struct object_id *new_head);\n>\n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> - const char *commit);\n> -\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(struct repository *repo,\n> --\n> 2.24.1\n\nThe code looks reasonable to me, but I'm still not completely familiar\nwith all the rebase and sequencer code so I'm hoping Phillip or\nJohannes can give a thumbs up.\n\nThanks for digging into this and figuring out the bits that I missed\nwhen I tried.\n\n\nElijah\n"},{"id":"390241","messageId":"xmqqa76f6xzt.fsf@gitster-ct.c.googlers.com","threadId":"52596","inReplyTo":"20200121191857.23047-1-alban.gruin@gmail.com","subject":"Re: [PATCH v1] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-22T20:24:06Z","receivedAt":"2020-01-22T20:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This introduces a visible change: as we do not switch on the tip of the\n> branch to rebase, no reflog entry is created at the beginning of the\n> rebase for it.\n\nFortunately, this does not mean \"git log @{-1}..\" during a rebase\nstarts to behave differently. If it were the case, that would have\nbeen a bad regression, but that is not the case.\n\nIf you omit one reflog entry, you are creating TWO changes. The\ndetaching of the HEAD to the \"onto\" commit would record, just like\nany other reflog entry, would record from which commit we detached\nHEAD from. That reflog entry will also be different, as you'd be\nswitching from a different commit. \n\nI am not sure what the implication of this difference will be in\npractice, though, but it must be smaller than the effect of the\nmissing reflog entry discussed earlier.\n"},{"id":"390243","messageId":"xmqq5zh36wx1.fsf@gitster-ct.c.googlers.com","threadId":"52596","inReplyTo":"20200121191857.23047-1-alban.gruin@gmail.com","subject":"Re: [PATCH v1] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-22T20:47:22Z","receivedAt":"2020-01-22T20:47:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> One of the first things done by the interactive rebase is to make a todo\n> list. This requires knowledge of the commit range to rebase. To get\n> the oid of the last commit of the range, the tip of the branch to rebase\n> is checked out with prepare_branch_to_be_rebased(), then the oid of the\n> HEAD is read. On big repositories, it's a performance penalty: the user\n> may have to wait before editing the todo list while git is extracting the\n> branch silently (because git-checkout is silenced here). After this,\n> the head of the branch is not even modified.\n\nHmph. One curious thing in the above is why this is specific to\n\"rebase -i\". The need to know the commit range to rebase is shared\nacross any rebase backend, and it would be the most natural to parse\nthe optional second argument (i.e. the branch or the commit to\nrebase) before builtin/rebase.c dispatches to a specific rebase\nbackend, wouldn't it? So, the question is why a normal \"rebase\"\ndoes not need the same fix?\n\nIf the answer is \"rebase in general was fine without extra checkout,\nbut 'rebase -i' was doing an unnecessary checkout\" (or any other\nanswer) that is something that would help future readers to record\nin the commit log message.\n\nThanks.\n\n\n"},{"id":"390361","messageId":"4142ab55-3311-4be2-2173-5cdacafb17f3@gmail.com","threadId":"52596","inReplyTo":"xmqq5zh36wx1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-01-24T14:45:11Z","receivedAt":"2020-01-24T14:45:25Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 22/01/2020 à 21:47, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> \n>> One of the first things done by the interactive rebase is to make a todo\n>> list. This requires knowledge of the commit range to rebase. To get\n>> the oid of the last commit of the range, the tip of the branch to rebase\n>> is checked out with prepare_branch_to_be_rebased(), then the oid of the\n>> HEAD is read. On big repositories, it's a performance penalty: the user\n>> may have to wait before editing the todo list while git is extracting the\n>> branch silently (because git-checkout is silenced here). After this,\n>> the head of the branch is not even modified.\n> \n> Hmph. One curious thing in the above is why this is specific to\n> \"rebase -i\". The need to know the commit range to rebase is shared\n> across any rebase backend, and it would be the most natural to parse\n> the optional second argument (i.e. the branch or the commit to\n> rebase) before builtin/rebase.c dispatches to a specific rebase\n> backend, wouldn't it? So, the question is why a normal \"rebase\"\n> does not need the same fix?\n> \n\nThat's a problem shared by all rebases using the sequencer, so -m and -r\nare also affected by this. `am' is not.\n\n> If the answer is \"rebase in general was fine without extra checkout,\n> but 'rebase -i' was doing an unnecessary checkout\" (or any other\n> answer) that is something that would help future readers to record\n> in the commit log message.\n> \n\nSo yes, the answer is that the am backend does not perform this\ncheckout, unlike all others rebases.\n\nI will resend this patch very soon.\n\n> Thanks.\n> \n> \n\nCheers,\nAlban\n\n"},{"id":"390362","messageId":"20200124144545.12984-1-alban.gruin@gmail.com","threadId":"52596","inReplyTo":"20200121191857.23047-1-alban.gruin@gmail.com","subject":"[PATCH v2] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-01-24T14:45:45Z","receivedAt":"2020-01-24T14:46:10Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"One of the first things done when using a sequencer-based\nrebase (ie. `rebase -i', `rebase -r', or `rebase -m') is to make a todo\nlist. This requires knowledge of the commit range to rebase. To get\nthe oid of the last commit of the range, the tip of the branch to rebase\nis checked out with prepare_branch_to_be_rebased(), then the oid of the\nhead is read. After this, the tip of the branch is not even modified.\n\nOn big repositories, it's a performance penalty: with `rebase -i', the\nuser may have to wait before editing the todo list while git is\nextracting the branch silently, and \"quiet\" rebases will be slower than\n`am'.\n\nSince we already have the oid of the tip of the branch in\n`opts->orig_head', it's useless to switch to this commit.\n\nThis removes the call to prepare_branch_to_be_rebased() in\ndo_interactive_rebase(), and adds a `orig_head' parameter to\nget_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\nis no longer used.\n\nThis introduces a visible change: as we do not switch on the tip of the\nbranch to rebase, no reflog entry is created at the beginning of the\nrebase for it.\n\nUnscientific performance measurements, performed on linux.git, are as\nfollow:\n\n Before this patch:\n\n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n\n real 0m8,940s\n user 0m6,830s\n sys 0m2,121s\n\n After this patch:\n\n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n\n real 0m1,834s\n user 0m0,916s\n sys 0m0,206s\n\nReported-by: SZEDER Gábor \nSigned-off-by: Alban Gruin \n---\n\nNotes:\n Changes since v1:\n \n - The first version of the commit message talked specifically about\n `rebase -i', but this problem is common to all sequencer-based\n rebases. The first paragraph has been reworded to clear up the\n confusion.\n \n - Included benchmarks in the commit message, as suggested by Elijah\n Newren.\n \n The code did not change.\n\n builtin/rebase.c | 18 +++++-------------\n sequencer.c | 14 --------------\n sequencer.h | 3 ---\n 3 files changed, 5 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8081741f8a..6154ad8fa5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t const char **head_hash,\n+\t\t\t struct object_id *orig_head, const char **head_hash,\n \t\t\t char **revisions, char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n-\tstruct object_id orig_head;\n-\n-\tif (get_oid(\"HEAD\", &orig_head))\n-\t\treturn error(_(\"no HEAD?\"));\n \n-\t*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n+\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n \t\t\t\t\t\t *head_hash);\n \n-\tshorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n+\tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n \tif (upstream) {\n \t\tconst char *shortrev;\n@@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct replay_opts replay = get_replay_opts(opts);\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n-\tif (prepare_branch_to_be_rebased(the_repository, &replay,\n-\t\t\t\t\t opts->switch_to))\n-\t\treturn -1;\n-\n-\tif (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n-\t\t\t\t&revisions, &shortrevisions))\n+\tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n+\t\t\t\t&head_hash, &revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..4dc245d7ec 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \treturn ret;\n }\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit)\n-{\n-\tconst char *action;\n-\n-\tif (commit && *commit) {\n-\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n-\t\tif (run_git_checkout(r, opts, commit, action))\n-\t\t\treturn error(_(\"could not checkout %s\"), commit);\n-\t}\n-\n-\treturn 0;\n-}\n-\n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n \t\t\t const char *orig_head)\ndiff --git a/sequencer.h b/sequencer.h\nindex 9f9ae291e3..74f1e2673e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit);\n-\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(struct repository *repo,\n-- \n2.24.1\n\n"},{"id":"390363","messageId":"9221098c-cba4-5db5-5870-bf6af721c448@gmail.com","threadId":"52596","inReplyTo":"20200124144545.12984-1-alban.gruin@gmail.com","subject":"Re: [PATCH v2] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-01-24T14:55:43Z","receivedAt":"2020-01-24T14:55:50Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Le 24/01/2020 à 15:45, Alban Gruin a écrit :\n> One of the first things done when using a sequencer-based\n> rebase (ie. `rebase -i', `rebase -r', or `rebase -m') is to make a todo\n> list. This requires knowledge of the commit range to rebase. To get\n> the oid of the last commit of the range, the tip of the branch to rebase\n> is checked out with prepare_branch_to_be_rebased(), then the oid of the\n> head is read. After this, the tip of the branch is not even modified.\n> \n> On big repositories, it's a performance penalty: with `rebase -i', the\n> user may have to wait before editing the todo list while git is\n> extracting the branch silently, and \"quiet\" rebases will be slower than\n> `am'.\n> \n> Since we already have the oid of the tip of the branch in\n> `opts->orig_head', it's useless to switch to this commit.\n> \n> This removes the call to prepare_branch_to_be_rebased() in\n> do_interactive_rebase(), and adds a `orig_head' parameter to\n> get_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\n> is no longer used.\n> \n> This introduces a visible change: as we do not switch on the tip of the\n> branch to rebase, no reflog entry is created at the beginning of the\n> rebase for it.\n> \n> Unscientific performance measurements, performed on linux.git, are as\n> follow:\n> \n> Before this patch:\n> \n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> \n> real 0m8,940s\n> user 0m6,830s\n> sys 0m2,121s\n> \n> After this patch:\n> \n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n> \n> real 0m1,834s\n> user 0m0,916s\n> sys 0m0,206s\n> \n> Reported-by: SZEDER Gábor \n> Signed-off-by: Alban Gruin \n> ---\n> \n\nForget this patch, I forgot to clearly say that the `am' backend is not\naffected.\n\n> Notes:\n> Changes since v1:\n> \n> - The first version of the commit message talked specifically about\n> `rebase -i', but this problem is common to all sequencer-based\n> rebases. The first paragraph has been reworded to clear up the\n> confusion.\n> \n> - Included benchmarks in the commit message, as suggested by Elijah\n> Newren.\n> \n> The code did not change.\n> \n> builtin/rebase.c | 18 +++++-------------\n> sequencer.c | 14 --------------\n> sequencer.h | 3 ---\n> 3 files changed, 5 insertions(+), 30 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 8081741f8a..6154ad8fa5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n> }\n> \n> static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n> -\t\t\t const char **head_hash,\n> +\t\t\t struct object_id *orig_head, const char **head_hash,\n> \t\t\t char **revisions, char **shortrevisions)\n> {\n> \tstruct commit *base_rev = upstream ? upstream : onto;\n> \tconst char *shorthead;\n> -\tstruct object_id orig_head;\n> -\n> -\tif (get_oid(\"HEAD\", &orig_head))\n> -\t\treturn error(_(\"no HEAD?\"));\n> \n> -\t*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n> +\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n> \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n> \t\t\t\t\t\t *head_hash);\n> \n> -\tshorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n> +\tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n> \n> \tif (upstream) {\n> \t\tconst char *shortrev;\n> @@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \tstruct replay_opts replay = get_replay_opts(opts);\n> \tstruct string_list commands = STRING_LIST_INIT_DUP;\n> \n> -\tif (prepare_branch_to_be_rebased(the_repository, &replay,\n> -\t\t\t\t\t opts->switch_to))\n> -\t\treturn -1;\n> -\n> -\tif (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n> -\t\t\t\t&revisions, &shortrevisions))\n> +\tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n> +\t\t\t\t&head_hash, &revisions, &shortrevisions))\n> \t\treturn -1;\n> \n> \tif (init_basic_state(&replay,\n> diff --git a/sequencer.c b/sequencer.c\n> index b9dbf1adb0..4dc245d7ec 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n> \treturn ret;\n> }\n> \n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit)\n> -{\n> -\tconst char *action;\n> -\n> -\tif (commit && *commit) {\n> -\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> -\t\tif (run_git_checkout(r, opts, commit, action))\n> -\t\t\treturn error(_(\"could not checkout %s\"), commit);\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> \t\t\t const char *onto_name, const struct object_id *onto,\n> \t\t\t const char *orig_head)\n> diff --git a/sequencer.h b/sequencer.h\n> index 9f9ae291e3..74f1e2673e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n> \t\t\t const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n> \n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit);\n> -\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(struct repository *repo,\n> \n\n"},{"id":"390364","messageId":"20200124150500.15260-1-alban.gruin@gmail.com","threadId":"52596","inReplyTo":"20200124144545.12984-1-alban.gruin@gmail.com","subject":"[PATCH v3] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-01-24T15:05:00Z","receivedAt":"2020-01-24T15:05:21Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"One of the first things done when using a sequencer-based\nrebase (ie. `rebase -i', `rebase -r', or `rebase -m') is to make a todo\nlist. This requires knowledge of the commit range to rebase. To get\nthe oid of the last commit of the range, the tip of the branch to rebase\nis checked out with prepare_branch_to_be_rebased(), then the oid of the\nhead is read. After this, the tip of the branch is not even modified.\nThe `am' backend, on the other hand, does not check out the branch.\n\nOn big repositories, it's a performance penalty: with `rebase -i', the\nuser may have to wait before editing the todo list while git is\nextracting the branch silently, and \"quiet\" rebases will be slower than\n`am'.\n\nSince we already have the oid of the tip of the branch in\n`opts->orig_head', it's useless to switch to this commit.\n\nThis removes the call to prepare_branch_to_be_rebased() in\ndo_interactive_rebase(), and adds a `orig_head' parameter to\nget_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\nis no longer used.\n\nThis introduces a visible change: as we do not switch on the tip of the\nbranch to rebase, no reflog entry is created at the beginning of the\nrebase for it.\n\nUnscientific performance measurements, performed on linux.git, are as\nfollow:\n\n Before this patch:\n\n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n\n real 0m8,940s\n user 0m6,830s\n sys 0m2,121s\n\n After this patch:\n\n $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n\n real 0m1,834s\n user 0m0,916s\n sys 0m0,206s\n\nReported-by: SZEDER Gábor \nSigned-off-by: Alban Gruin \n---\n\nAdded a line in the first paragraph to make it clear that the `am'\nbackend is not affected.\n\n builtin/rebase.c | 18 +++++-------------\n sequencer.c | 14 --------------\n sequencer.h | 3 ---\n 3 files changed, 5 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8081741f8a..6154ad8fa5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n }\n \n static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n-\t\t\t const char **head_hash,\n+\t\t\t struct object_id *orig_head, const char **head_hash,\n \t\t\t char **revisions, char **shortrevisions)\n {\n \tstruct commit *base_rev = upstream ? upstream : onto;\n \tconst char *shorthead;\n-\tstruct object_id orig_head;\n-\n-\tif (get_oid(\"HEAD\", &orig_head))\n-\t\treturn error(_(\"no HEAD?\"));\n \n-\t*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n+\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n \t\t\t\t\t\t *head_hash);\n \n-\tshorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n+\tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n \n \tif (upstream) {\n \t\tconst char *shortrev;\n@@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \tstruct replay_opts replay = get_replay_opts(opts);\n \tstruct string_list commands = STRING_LIST_INIT_DUP;\n \n-\tif (prepare_branch_to_be_rebased(the_repository, &replay,\n-\t\t\t\t\t opts->switch_to))\n-\t\treturn -1;\n-\n-\tif (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n-\t\t\t\t&revisions, &shortrevisions))\n+\tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n+\t\t\t\t&head_hash, &revisions, &shortrevisions))\n \t\treturn -1;\n \n \tif (init_basic_state(&replay,\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..4dc245d7ec 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \treturn ret;\n }\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit)\n-{\n-\tconst char *action;\n-\n-\tif (commit && *commit) {\n-\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n-\t\tif (run_git_checkout(r, opts, commit, action))\n-\t\t\treturn error(_(\"could not checkout %s\"), commit);\n-\t}\n-\n-\treturn 0;\n-}\n-\n static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t const char *onto_name, const struct object_id *onto,\n \t\t\t const char *orig_head)\ndiff --git a/sequencer.h b/sequencer.h\nindex 9f9ae291e3..74f1e2673e 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n-int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n-\t\t\t\t const char *commit);\n-\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(struct repository *repo,\n-- \n2.24.1\n\n"},{"id":"390368","messageId":"71f3148f-22b2-d8f5-9db5-d17b56e022e9@gmail.com","threadId":"52596","inReplyTo":"20200124144545.12984-1-alban.gruin@gmail.com","subject":"Re: [PATCH v2] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2020-01-24T17:11:17Z","receivedAt":"2020-01-24T17:11:24Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2020-01-24 15:45, Alban Gruin wrote:\n> Notes:\n> Changes since v1:\n> \n> - The first version of the commit message talked specifically about\n> `rebase -i', but this problem is common to all sequencer-based\n> rebases. The first paragraph has been reworded to clear up the\n> confusion.\n\nWould it make sense to update the subject line as well?\n"},{"id":"390373","messageId":"xmqqv9p0206p.fsf@gitster-ct.c.googlers.com","threadId":"52596","inReplyTo":"9221098c-cba4-5db5-5870-bf6af721c448@gmail.com","subject":"Re: [PATCH v2] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-24T18:12:30Z","receivedAt":"2020-01-24T18:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> Forget this patch, I forgot to clearly say that the `am' backend is not\n> affected.\n\nThe phrase \"is not affected\" makes it sound like \"Do not worry, I\nmade sure it is not broken by this patch\", but I do not think that\nis the more important part ;-). \n\nThe shared codepath for all types of rebase before dispatching\nalready knew what commit at the tip of the branch being rebased is,\nbut the sequencer-based backend was doing unnecessary work to figure\nit out again by checking it out. And this patch is about fixing\nthat, isn't it?\n\nSo I do not think singling out 'am' is a good use of readers' time.\nThe first paragraph can be further tweaked why the extra checkout is\nunneeded.\n\n Before dispatching the control to one of the individual rebase\n backends, the shared codepath in \"rebase\" figures out what\n branch is being rebased, because it is necessary to compute the\n range of commits to replay to run any backend. The rebase\n backend based on the sequencer machinery (used for '-i', '-r'\n and '-m') however computed this commit range by actually\n checking out the branch and reading HEAD, which was unnecessary,\n as the working tree is then immediately gets reset to that of\n the commit on which rebased history is built (aka \"onto\"\n commit).\n\nor something along the line, perhaps?\n\nWith this patch applied, the wasteful prepare_branch_to_be_rebased()\nhas no caller, and the patch removes it from sequencer.[ch] as well,\nwhich is very good.\n\n\n"},{"id":"390375","messageId":"xmqqk15g1zc7.fsf@gitster-ct.c.googlers.com","threadId":"52596","inReplyTo":"20200124150500.15260-1-alban.gruin@gmail.com","subject":"Re: [PATCH v3] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-24T18:30:48Z","receivedAt":"2020-01-24T18:30:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> On big repositories, it's a performance penalty: with `rebase -i', the\n> user may have to wait before editing the todo list while git is\n> extracting the branch silently, and \"quiet\" rebases will be slower than\n> `am'.\n>\n> Since we already have the oid of the tip of the branch in\n> `opts->orig_head', it's useless to switch to this commit.\n> ...\n> Before this patch:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m8,940s\n> user 0m6,830s\n> sys 0m2,121s\n>\n> After this patch:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m1,834s\n> user 0m0,916s\n> sys 0m0,206s\n>\n> Reported-by: SZEDER Gábor \n> Signed-off-by: Alban Gruin \n> ---\n\nGood.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index b9dbf1adb0..4dc245d7ec 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n> \treturn ret;\n> }\n> \n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit)\n> -{\n> -\tconst char *action;\n> -\n> -\tif (commit && *commit) {\n> -\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> -\t\tif (run_git_checkout(r, opts, commit, action))\n> -\t\t\treturn error(_(\"could not checkout %s\"), commit);\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> \t\t\t const char *onto_name, const struct object_id *onto,\n> \t\t\t const char *orig_head)\n> diff --git a/sequencer.h b/sequencer.h\n> index 9f9ae291e3..74f1e2673e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n> \t\t\t const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n> \n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit);\n> -\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(struct repository *repo,\n\nNice to see this helper to go.\n\nThanks.\n"},{"id":"391168","messageId":"nycvar.QRO.7.76.6.2002051531000.3718@tvgsbejvaqbjf.bet","threadId":"52596","inReplyTo":"20200124150500.15260-1-alban.gruin@gmail.com","subject":"Re: [PATCH v3] rebase -i: stop checking out the tip of the branch to rebase","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-05T14:31:08Z","receivedAt":"2020-02-05T14:31:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Fri, 24 Jan 2020, Alban Gruin wrote:\n\n> One of the first things done when using a sequencer-based\n> rebase (ie. `rebase -i', `rebase -r', or `rebase -m') is to make a todo\n> list. This requires knowledge of the commit range to rebase. To get\n> the oid of the last commit of the range, the tip of the branch to rebase\n> is checked out with prepare_branch_to_be_rebased(), then the oid of the\n> head is read. After this, the tip of the branch is not even modified.\n> The `am' backend, on the other hand, does not check out the branch.\n>\n> On big repositories, it's a performance penalty: with `rebase -i', the\n> user may have to wait before editing the todo list while git is\n> extracting the branch silently, and \"quiet\" rebases will be slower than\n> `am'.\n>\n> Since we already have the oid of the tip of the branch in\n> `opts->orig_head', it's useless to switch to this commit.\n>\n> This removes the call to prepare_branch_to_be_rebased() in\n> do_interactive_rebase(), and adds a `orig_head' parameter to\n> get_revision_ranges(). prepare_branch_to_be_rebased() is removed as it\n> is no longer used.\n\nAt this point, I am a bit puzzled as a reader: why can we just drop this?\nMy immediate reaction was: isn't this required to switch to a new branch\nwhen `switch_to` is non-`NULL`?\n\nSo I went digging a little. The `prepare_branch_to_be_rebased()` call was\nintroduced in 53bbcfbde7c (rebase -i: implement the main part of\ninteractive rebase as a builtin, 2018-09-27).\n\nAnd looking at the `git-rebase--interactive` part of that patch, it\nbecomes relatively obvious that we inherited this behavior from the shell\nscripting days.\n\n2c58483a598 (rebase -i: rewrite setup_reflog_action() in C, 2018-08-10)\nconverted the `setup_reflog_action` function (which oddly enough not only\nset up the reflog action, but also switched to a new branch if so\nconfigured). That function was introduced in d48f97aa854 (rebase: reindent\nfunction git_rebase__interactive, 2018-03-23), but that was _still_ not\nthe commit that introduced that \"let's check out the upstream\" behavior.\n\nIt goes _all_ the way back to 1b1dce4bae7 (Teach rebase an interactive\nmode, 2007-06-25). Except that back then, it was only done when a branch\nname was provided (`git rebase -i `). So\nit behaved correctly.\n\nThe problem was introduced in 71786f54c41 (rebase: factor out reference\nparsing, 2011-02-06), as it substituted the `if test ! -z \"$1\"` with `if\ntest ! -z \"$switch_to\"`, relying on the command-line parsing of\n`git-rebase.sh`.\n\nBut wait! Wait, wait, wait! `switch_to` is still only set in that\nincantation where we provide a branch name _in addition to_ an upstream\ncommit.\n\nAh, I think I slowly see where this is going. The problem is actually\n2ec33cdd19b (rebase--interactive: don't require what's rebased to be a\nbranch, 2010-03-14) which failed to realize that essentially the entire\n`git checkout` was necessary to accommodate the subsequent call a mere 8\nlines further down from that `checkout`:\n\n\t\tgit symbolic-ref HEAD > \"$DOTEST\"/head-name 2> /dev/null ||\n echo \"detached HEAD\" > \"$DOTEST\"/head-name\n\nSo that 2ec33cdd19b commit could have saved itself a lot of trouble by\nrealizing what the role of that `git checkout` is, and should have pulled\nthat `head-name` logic into that conditional instead of _actually_\nswitching to a new branch.\n\nNow, let's see what the C code does to determine \"head-name\". Indeed, it\nis already handled in the option parsing, in that monster of a function\ncalled `cmd_rebase()`.\n\nAnd yes, I think that `head_name` should also be mentioned in this commit\nmessage, as something like\n\n\tgit rebase -i \n\nshould eventually indeed switch to the specified branch, and this here\npatch does _not_ break that promise.\n\nThis is my only concern with this patch, though, therefore:\n\nAcked-by: Johannes Schindelin \n\nThanks,\nDscho\n\n> This introduces a visible change: as we do not switch on the tip of the\n> branch to rebase, no reflog entry is created at the beginning of the\n> rebase for it.\n>\n> Unscientific performance measurements, performed on linux.git, are as\n> follow:\n>\n> Before this patch:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m8,940s\n> user 0m6,830s\n> sys 0m2,121s\n>\n> After this patch:\n>\n> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50\n>\n> real 0m1,834s\n> user 0m0,916s\n> sys 0m0,206s\n>\n> Reported-by: SZEDER Gábor \n> Signed-off-by: Alban Gruin \n> ---\n>\n> Added a line in the first paragraph to make it clear that the `am'\n> backend is not affected.\n>\n> builtin/rebase.c | 18 +++++-------------\n> sequencer.c | 14 --------------\n> sequencer.h | 3 ---\n> 3 files changed, 5 insertions(+), 30 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 8081741f8a..6154ad8fa5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -246,21 +246,17 @@ static int edit_todo_file(unsigned flags)\n> }\n>\n> static int get_revision_ranges(struct commit *upstream, struct commit *onto,\n> -\t\t\t const char **head_hash,\n> +\t\t\t struct object_id *orig_head, const char **head_hash,\n> \t\t\t char **revisions, char **shortrevisions)\n> {\n> \tstruct commit *base_rev = upstream ? upstream : onto;\n> \tconst char *shorthead;\n> -\tstruct object_id orig_head;\n> -\n> -\tif (get_oid(\"HEAD\", &orig_head))\n> -\t\treturn error(_(\"no HEAD?\"));\n>\n> -\t*head_hash = find_unique_abbrev(&orig_head, GIT_MAX_HEXSZ);\n> +\t*head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);\n> \t*revisions = xstrfmt(\"%s...%s\", oid_to_hex(&base_rev->object.oid),\n> \t\t\t\t\t\t *head_hash);\n>\n> -\tshorthead = find_unique_abbrev(&orig_head, DEFAULT_ABBREV);\n> +\tshorthead = find_unique_abbrev(orig_head, DEFAULT_ABBREV);\n>\n> \tif (upstream) {\n> \t\tconst char *shortrev;\n> @@ -314,12 +310,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n> \tstruct replay_opts replay = get_replay_opts(opts);\n> \tstruct string_list commands = STRING_LIST_INIT_DUP;\n>\n> -\tif (prepare_branch_to_be_rebased(the_repository, &replay,\n> -\t\t\t\t\t opts->switch_to))\n> -\t\treturn -1;\n> -\n> -\tif (get_revision_ranges(opts->upstream, opts->onto, &head_hash,\n> -\t\t\t\t&revisions, &shortrevisions))\n> +\tif (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,\n> +\t\t\t\t&head_hash, &revisions, &shortrevisions))\n> \t\treturn -1;\n>\n> \tif (init_basic_state(&replay,\n> diff --git a/sequencer.c b/sequencer.c\n> index b9dbf1adb0..4dc245d7ec 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3715,20 +3715,6 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n> \treturn ret;\n> }\n>\n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit)\n> -{\n> -\tconst char *action;\n> -\n> -\tif (commit && *commit) {\n> -\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> -\t\tif (run_git_checkout(r, opts, commit, action))\n> -\t\t\treturn error(_(\"could not checkout %s\"), commit);\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> static int checkout_onto(struct repository *r, struct replay_opts *opts,\n> \t\t\t const char *onto_name, const struct object_id *onto,\n> \t\t\t const char *orig_head)\n> diff --git a/sequencer.h b/sequencer.h\n> index 9f9ae291e3..74f1e2673e 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -190,9 +190,6 @@ void commit_post_rewrite(struct repository *r,\n> \t\t\t const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n>\n> -int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n> -\t\t\t\t const char *commit);\n> -\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(struct repository *repo,\n> --\n> 2.24.1\n>\n>\n"}]}