{"thread":{"id":"30293","subject":"[PATCH] rebase -i: avoid checking out $branch when possible","startedAt":"2012-04-20T15:05:10Z","lastAt":"2012-05-18T15:25:45Z","messageCount":9,"participants":["Thomas Rast","Junio C Hamano","Johannes Sixt","Shezan Baig"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"189760","messageId":"fee3225e29915e1b61e29a5d2fe37db20fa4b596.1334933837.git.trast@student.ethz.ch","threadId":"30293","inReplyTo":null,"subject":"[PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-04-20T15:05:10Z","receivedAt":"2012-04-20T15:05:10Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The command\n\n git rebase [-i] [--onto $onto] $base $branch\n\nis defined to be equivalent to\n\n git checkout $branch && git rebase [-i] [--onto $onto] $base\n\nHowever, we do not have to actually perform the checkout. The rebase\nstarts building on top of $base (or $onto, if given). The tree\n_state_ (not diff) of $branch is irrelevant. Actually performing the\ncheckout has some downsides: $branch may potentially be way out of\ntouch with HEAD, and thus e.g. trigger a full rebuild in a timestamp-\nbased build system, even if $base..$branch only edits the README.\n\nIn the event that $branch is already up-to-date w.r.t. $base, we still\nhave to check out, however. git-rebase.sh has had the corresponding\nlazy-checkout logic since approximately forever (0cb0664, rebase\n[--onto O] A B: omit needless checkout, 2008-03-15).\n\nThis logic has also been used for interactive since Martin's\nrefactoring around cc1453e (rebase: factor out call to pre-rebase\nhook, 2011-02-06). However, an unconditional checkout was carried\nover into the new interactive rebase code. Remove it. HEAD is only\nused in the rev-parse invocation immediately after, which is easy\nenough to fix.\n\nNote that this does change the state of the repository if something\nbad happens and we 'die'. Noninteractive rebase already behaves the\nsame way. The catch here is that \"there is nothing to rebase\", as\nwell as \"the user cleared the TODO file\", both go through an error\npath. We need to ensure that a checkout happens in these cases, to\nkeep it equivalent to checkout&&rebase.\n\nNoticed-by: Shezan Baig \nSigned-off-by: Thomas Rast \n---\n\nI was a bit torn on whether I should abort with checkout, or without\nit. The manual clearly states that rebase \"will perform an automatic\ngit checkout before doing anything else\", which mandates at\nleast *trying* the checkout in the error path, hence this version.\n\nHowever, in contrived cases this can lead to strange behavior. For\nexample, a checkout conflict with a file in the worktree may prevent\nthe abort path from working correctly, even though going through with\nthe rebase itself may succeed.\n\n git-rebase--interactive.sh | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2e13258..3b40c2a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -163,6 +163,15 @@ die_abort () {\n \tdie \"$1\"\n }\n \n+die_abort_with_checkout () {\n+\tif test -n \"$switch_to\"\n+\tthen\n+\t\tgit checkout \"$switch_to\" -- ||\n+\t\t\tdie_abort \"$1, and failed to check out $switch_to\"\n+\tfi\n+\tdie_abort \"$1\"\n+}\n+\n has_action () {\n \tsane_grep '^[^#]' \"$1\" >/dev/null\n }\n@@ -728,13 +737,7 @@ git var GIT_COMMITTER_IDENT >/dev/null ||\n \n comment_for_reflog start\n \n-if test ! -z \"$switch_to\"\n-then\n-\toutput git checkout \"$switch_to\" -- ||\n-\t\tdie \"Could not checkout $switch_to\"\n-fi\n-\n-orig_head=$(git rev-parse --verify HEAD) || die \"No HEAD?\"\n+orig_head=$(git rev-parse --verify \"${switch_to:-HEAD}\") || die \"No HEAD?\"\n mkdir \"$state_dir\" || die \"Could not create temporary $state_dir\"\n \n : > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n@@ -854,14 +857,14 @@ cat >> \"$todo\" << EOF\n EOF\n \n has_action \"$todo\" ||\n-\tdie_abort \"Nothing to do\"\n+\tdie_abort_with_checkout \"Nothing to do\"\n \n cp \"$todo\" \"$todo\".backup\n git_sequence_editor \"$todo\" ||\n-\tdie_abort \"Could not execute editor\"\n+\tdie_abort_with_checkout \"Could not execute editor\"\n \n has_action \"$todo\" ||\n-\tdie_abort \"Nothing to do\"\n+\tdie_abort_with_checkout \"Nothing to do\"\n \n test -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\n \n-- \n1.7.10.323.g7b65b\n"},{"id":"189762","messageId":"xmqqwr5atn89.fsf@junio.mtv.corp.google.com","threadId":"30293","inReplyTo":"fee3225e29915e1b61e29a5d2fe37db20fa4b596.1334933837.git.trast@student.ethz.ch","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-20T15:52:06Z","receivedAt":"2012-04-20T15:52:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> I was a bit torn on whether I should abort with checkout, or without\n> it. The manual clearly states that rebase \"will perform an automatic\n> git checkout before doing anything else\", which mandates at\n> least *trying* the checkout in the error path, hence this version.\n>\n> However, in contrived cases this can lead to strange behavior. For\n> example, a checkout conflict with a file in the worktree may prevent\n> the abort path from working correctly, even though going through with\n> the rebase itself may succeed.\n\nGiven all that contortion, is it even worth doing this?\n"},{"id":"189764","messageId":"87lilqjstf.fsf@thomas.inf.ethz.ch","threadId":"30293","inReplyTo":"xmqqwr5atn89.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-04-20T16:01:32Z","receivedAt":"2012-04-20T16:01:32Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> I was a bit torn on whether I should abort with checkout, or without\n>> it. The manual clearly states that rebase \"will perform an automatic\n>> git checkout before doing anything else\", which mandates at\n>> least *trying* the checkout in the error path, hence this version.\n>>\n>> However, in contrived cases this can lead to strange behavior. For\n>> example, a checkout conflict with a file in the worktree may prevent\n>> the abort path from working correctly, even though going through with\n>> the rebase itself may succeed.\n>\n> Given all that contortion, is it even worth doing this?\n\nWell, the logic isn't new; 0cb0664 already does the same. It just never\ncarried over to interactive rebase.\n\nAs to whether the whole thing is worth it: if you rebase all your topics\nagainst master regularly, and 'make test' on each, this patch may speed\nthat up greatly if you are careful about using a branch argument for\nrebase.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"189765","messageId":"4F9192AE.5050304@kdbg.org","threadId":"30293","inReplyTo":"fee3225e29915e1b61e29a5d2fe37db20fa4b596.1334933837.git.trast@student.ethz.ch","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-04-20T16:45:34Z","receivedAt":"2012-04-20T16:45:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.04.2012 17:05, schrieb Thomas Rast:\n> The command\n> \n> git rebase [-i] [--onto $onto] $base $branch\n> \n> is defined to be equivalent to\n> \n> git checkout $branch && git rebase [-i] [--onto $onto] $base\n> \n> However, we do not have to actually perform the checkout. The rebase\n> starts building on top of $base (or $onto, if given). The tree\n> _state_ (not diff) of $branch is irrelevant. Actually performing the\n> checkout has some downsides: $branch may potentially be way out of\n> touch with HEAD, and thus e.g. trigger a full rebuild in a timestamp-\n> based build system, even if $base..$branch only edits the README.\n\nThanks, this is very much appreciated. I like it (because it would\nprevent lots of unneeded rebuilds in my use-cases).\n\n> cp \"$todo\" \"$todo\".backup\n> git_sequence_editor \"$todo\" ||\n> -\tdie_abort \"Could not execute editor\"\n> +\tdie_abort_with_checkout \"Could not execute editor\"\n\nI don't think that we want to checkout here, even if the docs say we do.\n\n-- Hannes\n"},{"id":"189775","messageId":"xmqqmx66mamc.fsf@junio.mtv.corp.google.com","threadId":"30293","inReplyTo":"87lilqjstf.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-20T20:06:19Z","receivedAt":"2012-04-20T20:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Rast writes:\n>>\n>>> I was a bit torn on whether I should abort with checkout, or without\n>>> it. The manual clearly states that rebase \"will perform an automatic\n>>> git checkout before doing anything else\", which mandates at\n>>> least *trying* the checkout in the error path, hence this version.\n>>>\n>>> However, in contrived cases this can lead to strange behavior. For\n>>> example, a checkout conflict with a file in the worktree may prevent\n>>> the abort path from working correctly, even though going through with\n>>> the rebase itself may succeed.\n>>\n>> Given all that contortion, is it even worth doing this?\n>\n> Well, the logic isn't new; 0cb0664 already does the same. It just never\n> carried over to interactive rebase.\n\nOK.\n\nThe discrepancy in the \"abort\" case may come only in the three cases:\n\n - EDITOR is pointing at something funny; it is not worth introducing\n any backward incompatibility to optimize for this case, so\n abort-with-checkout is the right thing to do here. One less thing we\n have to worry about (but see the third point below).\n\n - It turns out that everything is already contained and there is\n nothing to apply, i.e. after this sequence:\n\n\tgit checkout branch\n\tgit checkout $onto_or_merge_base_between_base_and_branch\n\n we find out that \"git cherry $onto_or_merge_base branch\" is empty.\n\n Because you will be one commit ahead of $onto_or_merge_base if \"git\n cherry\" were to give one commit to be replayed, I think it is\n logically correct if you stayed at the $onto_or_merge_base without\n checking out . In other words, abort-with-checkout is not\n ideal for this case; we would want to just abort in this case.\n\n - The user told us not to do the rebase by making insn sheet empty. In\n other words, this is \"aborting the entire rebase\", so ideally it\n should come back to the state before the user ran \"git rebase\"\n command (i.e. where she was before we switched to ).\n\n I do not think this ideal behaviour is something neither batch or\n interactive rebase has traditionally implemented, but I can see how\n we can sell this as a bugfix to the end users.\n"},{"id":"191555","messageId":"CANMpiOQPefL_FNocLJ=pZyAb1hZ7bEvLk6frU3xme6VQrSDD2A@mail.gmail.com","threadId":"30293","inReplyTo":"xmqqmx66mamc.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Shezan Baig","fromEmail":"shezbaig.wk@gmail.com","sentAt":"2012-05-15T16:26:29Z","receivedAt":"2012-05-15T16:26:29Z","isPatch":true,"sender":{"key":"shezbaig.wk@gmail.com","avatar":null},"body":"Hi there,\nJust wondering if this patch is going to be available in an upcoming\nversion of git?\nThanks,\nShezan\n\n\nOn Fri, Apr 20, 2012 at 4:06 PM, Junio C Hamano wrote:\n> Thomas Rast writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> Thomas Rast writes:\n>>>\n>>>> I was a bit torn on whether I should abort with checkout, or without\n>>>> it.  The manual clearly states that rebase \"will perform an automatic\n>>>> git checkout before doing anything else\", which mandates at\n>>>> least *trying* the checkout in the error path, hence this version.\n>>>>\n>>>> However, in contrived cases this can lead to strange behavior.  For\n>>>> example, a checkout conflict with a file in the worktree may prevent\n>>>> the abort path from working correctly, even though going through with\n>>>> the rebase itself may succeed.\n>>>\n>>> Given all that contortion, is it even worth doing this?\n>>\n>> Well, the logic isn't new; 0cb0664 already does the same.  It just never\n>> carried over to interactive rebase.\n>\n> OK.\n>\n> The discrepancy in the \"abort\" case may come only in the three cases:\n>\n>  - EDITOR is pointing at something funny; it is not worth introducing\n>   any backward incompatibility to optimize for this case, so\n>   abort-with-checkout is the right thing to do here.  One less thing we\n>   have to worry about (but see the third point below).\n>\n>  - It turns out that everything is already contained and there is\n>   nothing to apply, i.e. after this sequence:\n>\n>        git checkout branch\n>        git checkout $onto_or_merge_base_between_base_and_branch\n>\n>   we find out that \"git cherry $onto_or_merge_base branch\" is empty.\n>\n>   Because you will be one commit ahead of $onto_or_merge_base if \"git\n>   cherry\" were to give one commit to be replayed, I think it is\n>   logically correct if you stayed at the $onto_or_merge_base without\n>   checking out .  In other words, abort-with-checkout is not\n>   ideal for this case; we would want to just abort in this case.\n>\n>  - The user told us not to do the rebase by making insn sheet empty.  In\n>   other words, this is \"aborting the entire rebase\", so ideally it\n>   should come back to the state before the user ran \"git rebase\"\n>   command (i.e. where she was before we switched to ).\n>\n>   I do not think this ideal behaviour is something neither batch or\n>   interactive rebase has traditionally implemented, but I can see how\n>   we can sell this as a bugfix to the end users.\n>\n"},{"id":"191558","messageId":"7vwr4de5j8.fsf@alter.siamese.dyndns.org","threadId":"30293","inReplyTo":"CANMpiOQPefL_FNocLJ=pZyAb1hZ7bEvLk6frU3xme6VQrSDD2A@mail.gmail.com","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-15T17:08:11Z","receivedAt":"2012-05-15T17:08:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shezan Baig writes:\n\n> Just wondering if this patch is going to be available in an upcoming\n> version of git?\n\nAs we can see from the exchange you quoted, I do not think we have nailed\nthe details of desired behaviour in the updated code down.\n\nThomas, how would you want to proceed?\n"},{"id":"191649","messageId":"87obplhp12.fsf@thomas.inf.ethz.ch","threadId":"30293","inReplyTo":"7vwr4de5j8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-05-18T08:27:53Z","receivedAt":"2012-05-18T08:27:53Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Shezan Baig writes:\n>\n>> Just wondering if this patch is going to be available in an upcoming\n>> version of git?\n>\n> As we can see from the exchange you quoted, I do not think we have nailed\n> the details of desired behaviour in the updated code down.\n>\n> Thomas, how would you want to proceed?\n\nHrm. You wrote:\n\n> The discrepancy in the \"abort\" case may come only in the three cases:\n> \n> - EDITOR is pointing at something funny, [let's do the checkout to be\n> safe for scripts]\n\nAgreed.\n\n> - The user told us not to do the rebase by making insn sheet empty. In\n> other words, this is \"aborting the entire rebase\", so ideally it\n> should come back to the state before the user ran \"git rebase\"\n> command (i.e. where she was before we switched to ).\n> \n> I do not think this ideal behaviour is something neither batch or\n> interactive rebase has traditionally implemented, but I can see how\n> we can sell this as a bugfix to the end users.\n\nThat's a convincing argument, so let's make it so.\n\n> - It turns out that everything is already contained and there is\n> nothing to apply, i.e. after this sequence:\n> \n> \tgit checkout branch\n> \tgit checkout $onto_or_merge_base_between_base_and_branch\n> \n> we find out that \"git cherry $onto_or_merge_base branch\" is empty.\n\nIs there a command missing here? This alone does not make them the\nsame, perhaps you meant some resetting.\n\nI'll assume that you meant a case where the user is *not* on branch, but\nbase/onto is an ancestor of the branch, e.g., in a sequence\n\n git checkout base\n git reset --hard branch~5\n # now branch..base is empty\n git rebase base branch\n\nthe rebase will not do anything unless forced. Similarly for the case\nwhere base==branch. And then\n\n> Because you will be one commit ahead of $onto_or_merge_base if \"git\n> cherry\" were to give one commit to be replayed, I think it is\n> logically correct if you stayed at the $onto_or_merge_base without\n> checking out . In other words, abort-with-checkout is not\n> ideal for this case; we would want to just abort in this case.\n\nThis would be hard to sell, because a script that runs\n\n git rebase base branch\n\ncould reasonably assume that after the rebase, the current branch is\n'branch', unless some error occurred. And we'd be breaking that.\n\nOr am I missing the point of your reasoning?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"191664","messageId":"xmqqaa154ikm.fsf@junio.mtv.corp.google.com","threadId":"30293","inReplyTo":"87obplhp12.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] rebase -i: avoid checking out $branch when possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-18T15:25:45Z","receivedAt":"2012-05-18T15:25:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n>> - The user told us not to do the rebase by making insn sheet empty. In\n>> other words, this is \"aborting the entire rebase\", so ideally it\n>> should come back to the state before the user ran \"git rebase\"\n>> command (i.e. where she was before we switched to ).\n>> \n>> I do not think this ideal behaviour is something neither batch or\n>> interactive rebase has traditionally implemented, but I can see how\n>> we can sell this as a bugfix to the end users.\n>\n> That's a convincing argument, so let's make it so.\n>\n>> - It turns out that everything is already contained and there is\n>> nothing to apply, i.e. after this sequence:\n>> \n>> \tgit checkout branch\n>> \tgit checkout $onto_or_merge_base_between_base_and_branch\n>> \n>> we find out that \"git cherry $onto_or_merge_base branch\" is empty.\n>\n> Is there a command missing here? This alone does not make them the\n> same, perhaps you meant some resetting.\n\nActually that was phrased rather poorly. At least \"after this sequence\"\nshould have been \"during this sequence inside the implementation of\n'rebase -i'\". In other words, the scenario is still where the user\ntyped:\n\n git rebase [-i] [--onto $onto] $base $branch\n\nwhich is defined to be equivalent to\n\n git checkout $branch && git rebase [-i] [--onto $onto] $base\n\nThe first half of the equivalent command succeeds. We are on $branch.\nThe second half finds there is nothing to be done.\n\nI think the current code (and probably with your patch) does this in\nsuch a case:\n\nhas_action \"$todo\" ||\n\tdie_abort \"Nothing to do\"\n\nand the die happens on $branch before we detach (iow, between the two\ncheckouts), and the user should end up on $branch. Unlike the case\nwhere the user told us to abort the entire rebase, which we agreed that\nwe should come back to the original branch above, I think in this case\nwe should not nullify the first \"checkout $branch\".\n"}]}