threads / patch / 30293

patchrebase -i: avoid checking out $branch when possible

Subject: [PATCH] rebase -i: avoid checking out $branch when possible

## tl;dr

9 messages between Apr 20, 2012 and May 18, 2012. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Thomas Rast· Apr 20, 2012, 15:05 UTC · lore
The command
  git rebase [-i] [--onto $onto] $base $branch
is defined to be equivalent to
  git checkout $branch && git rebase [-i] [--onto $onto] $base

However, we do not have to actually perform the checkout. The rebase starts building on top of $base (or $onto, if given). The tree _state_ (not diff) of $branch is irrelevant. Actually performing the checkout has some downsides: $branch may potentially be way out of touch with HEAD, and thus e.g. trigger a full rebuild in a timestamp- based build system, even if $base..$branch only edits the README.

In the event that $branch is already up-to-date w.r.t. $base, we still have to check out, however. git-rebase.sh has had the corresponding lazy-checkout logic since approximately forever (0cb0664, rebase [--onto O] A B: omit needless checkout, 2008-03-15).

This logic has also been used for interactive since Martin's refactoring around cc1453e (rebase: factor out call to pre-rebase hook, 2011-02-06). However, an unconditional checkout was carried over into the new interactive rebase code. Remove it. HEAD is only used in the rev-parse invocation immediately after, which is easy enough to fix.

Note that this does change the state of the repository if something bad happens and we 'die'. Noninteractive rebase already behaves the same way. The catch here is that "there is nothing to rebase", as well as "the user cleared the TODO file", both go through an error path. We need to ensure that a checkout happens in these cases, to keep it equivalent to checkout&&rebase.

Noticed-by: Shezan Baig <shezbaig.wk@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

I was a bit torn on whether I should abort with checkout, or without it. The manual clearly states that rebase "will perform an automatic git checkout <branch> before doing anything else", which mandates at least *trying* the checkout in the error path, hence this version.

However, in contrived cases this can lead to strange behavior. For example, a checkout conflict with a file in the worktree may prevent the abort path from working correctly, even though going through with the rebase itself may succeed.

 git-rebase--interactive.sh |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
Show changes to git-rebase--interactive.sh +13 −10
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2e13258..3b40c2a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -163,6 +163,15 @@ die_abort () {
 	die "$1"
 }
 
+die_abort_with_checkout () {
+	if test -n "$switch_to"
+	then
+		git checkout "$switch_to" -- ||
+			die_abort "$1, and failed to check out $switch_to"
+	fi
+	die_abort "$1"
+}
+
 has_action () {
 	sane_grep '^[^#]' "$1" >/dev/null
 }
@@ -728,13 +737,7 @@ git var GIT_COMMITTER_IDENT >/dev/null ||
 
 comment_for_reflog start
 
-if test ! -z "$switch_to"
-then
-	output git checkout "$switch_to" -- ||
-		die "Could not checkout $switch_to"
-fi
-
-orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
+orig_head=$(git rev-parse --verify "${switch_to:-HEAD}") || die "No HEAD?"
 mkdir "$state_dir" || die "Could not create temporary $state_dir"
 
 : > "$state_dir"/interactive || die "Could not mark as interactive"
@@ -854,14 +857,14 @@ cat >> "$todo" << EOF
 EOF
 
 has_action "$todo" ||
-	die_abort "Nothing to do"
+	die_abort_with_checkout "Nothing to do"
 
 cp "$todo" "$todo".backup
 git_sequence_editor "$todo" ||
-	die_abort "Could not execute editor"
+	die_abort_with_checkout "Could not execute editor"
 
 has_action "$todo" ||
-	die_abort "Nothing to do"
+	die_abort_with_checkout "Nothing to do"
 
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 

-- 
1.7.10.323.g7b65b
Junio C Hamano· Apr 20, 2012, 15:52 UTC · re: Thomas Rast · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Thomas Rast <trast@student.ethz.ch> writes:
Show 9 quoted lines
> I was a bit torn on whether I should abort with checkout, or without
> it.  The manual clearly states that rebase "will perform an automatic
> git checkout <branch> before doing anything else", which mandates at
> least *trying* the checkout in the error path, hence this version.
>
> However, in contrived cases this can lead to strange behavior.  For
> example, a checkout conflict with a file in the worktree may prevent
> the abort path from working correctly, even though going through with
> the rebase itself may succeed.
Given all that contortion, is it even worth doing this?
Thomas Rast· Apr 20, 2012, 16:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> I was a bit torn on whether I should abort with checkout, or without
>> it.  The manual clearly states that rebase "will perform an automatic
>> git checkout <branch> before doing anything else", which mandates at
>> least *trying* the checkout in the error path, hence this version.
>>
>> However, in contrived cases this can lead to strange behavior.  For
>> example, a checkout conflict with a file in the worktree may prevent
>> the abort path from working correctly, even though going through with
>> the rebase itself may succeed.
>
> Given all that contortion, is it even worth doing this?

Well, the logic isn't new; 0cb0664 already does the same. It just never carried over to interactive rebase.

As to whether the whole thing is worth it: if you rebase all your topics against master regularly, and 'make test' on each, this patch may speed that up greatly if you are careful about using a branch argument for rebase.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Apr 20, 2012, 20:06 UTC · re: Thomas Rast · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Thomas Rast <trast@student.ethz.ch> writes:
Show 18 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> I was a bit torn on whether I should abort with checkout, or without
>>> it.  The manual clearly states that rebase "will perform an automatic
>>> git checkout <branch> before doing anything else", which mandates at
>>> least *trying* the checkout in the error path, hence this version.
>>>
>>> However, in contrived cases this can lead to strange behavior.  For
>>> example, a checkout conflict with a file in the worktree may prevent
>>> the abort path from working correctly, even though going through with
>>> the rebase itself may succeed.
>>
>> Given all that contortion, is it even worth doing this?
>
> Well, the logic isn't new; 0cb0664 already does the same.  It just never
> carried over to interactive rebase.
OK.
The discrepancy in the "abort" case may come only in the three cases:
 - EDITOR is pointing at something funny; it is not worth introducing
   any backward incompatibility to optimize for this case, so
   abort-with-checkout is the right thing to do here.  One less thing we
   have to worry about (but see the third point below).
 - It turns out that everything is already contained and there is
   nothing to apply, i.e. after this sequence:
	git checkout branch
	git checkout $onto_or_merge_base_between_base_and_branch
   we find out that "git cherry $onto_or_merge_base branch" is empty.
   Because you will be one commit ahead of $onto_or_merge_base if "git
   cherry" were to give one commit to be replayed, I think it is
   logically correct if you stayed at the $onto_or_merge_base without
   checking out <branch>.  In other words, abort-with-checkout is not
   ideal for this case; we would want to just abort in this case.
 - The user told us not to do the rebase by making insn sheet empty.  In
   other words, this is "aborting the entire rebase", so ideally it
   should come back to the state before the user ran "git rebase"
   command (i.e. where she was before we switched to <branch>).
   I do not think this ideal behaviour is something neither batch or
   interactive rebase has traditionally implemented, but I can see how
   we can sell this as a bugfix to the end users.
Shezan Baig· May 15, 2012, 16:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Hi there, Just wondering if this patch is going to be available in an upcoming version of git? Thanks, Shezan

On Fri, Apr 20, 2012 at 4:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 53 quoted lines
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Thomas Rast <trast@student.ethz.ch> writes:
>>>
>>>> I was a bit torn on whether I should abort with checkout, or without
>>>> it.  The manual clearly states that rebase "will perform an automatic
>>>> git checkout <branch> before doing anything else", which mandates at
>>>> least *trying* the checkout in the error path, hence this version.
>>>>
>>>> However, in contrived cases this can lead to strange behavior.  For
>>>> example, a checkout conflict with a file in the worktree may prevent
>>>> the abort path from working correctly, even though going through with
>>>> the rebase itself may succeed.
>>>
>>> Given all that contortion, is it even worth doing this?
>>
>> Well, the logic isn't new; 0cb0664 already does the same.  It just never
>> carried over to interactive rebase.
>
> OK.
>
> The discrepancy in the "abort" case may come only in the three cases:
>
>  - EDITOR is pointing at something funny; it is not worth introducing
>   any backward incompatibility to optimize for this case, so
>   abort-with-checkout is the right thing to do here.  One less thing we
>   have to worry about (but see the third point below).
>
>  - It turns out that everything is already contained and there is
>   nothing to apply, i.e. after this sequence:
>
>        git checkout branch
>        git checkout $onto_or_merge_base_between_base_and_branch
>
>   we find out that "git cherry $onto_or_merge_base branch" is empty.
>
>   Because you will be one commit ahead of $onto_or_merge_base if "git
>   cherry" were to give one commit to be replayed, I think it is
>   logically correct if you stayed at the $onto_or_merge_base without
>   checking out <branch>.  In other words, abort-with-checkout is not
>   ideal for this case; we would want to just abort in this case.
>
>  - The user told us not to do the rebase by making insn sheet empty.  In
>   other words, this is "aborting the entire rebase", so ideally it
>   should come back to the state before the user ran "git rebase"
>   command (i.e. where she was before we switched to <branch>).
>
>   I do not think this ideal behaviour is something neither batch or
>   interactive rebase has traditionally implemented, but I can see how
>   we can sell this as a bugfix to the end users.
>
Junio C Hamano· May 15, 2012, 17:08 UTC · re: Shezan Baig · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Shezan Baig <shezbaig.wk@gmail.com> writes:
> Just wondering if this patch is going to be available in an upcoming
> version of git?

As we can see from the exchange you quoted, I do not think we have nailed the details of desired behaviour in the updated code down.

Thomas, how would you want to proceed?
Thomas Rast· May 18, 2012, 08:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Shezan Baig <shezbaig.wk@gmail.com> writes:
>
>> Just wondering if this patch is going to be available in an upcoming
>> version of git?
>
> As we can see from the exchange you quoted, I do not think we have nailed
> the details of desired behaviour in the updated code down.
>
> Thomas, how would you want to proceed?
Hrm.  You wrote:
> The discrepancy in the "abort" case may come only in the three cases:
> 
>  - EDITOR is pointing at something funny, [let's do the checkout to be
>    safe for scripts]
Agreed.
Show 8 quoted lines
>  - The user told us not to do the rebase by making insn sheet empty.  In
>    other words, this is "aborting the entire rebase", so ideally it
>    should come back to the state before the user ran "git rebase"
>    command (i.e. where she was before we switched to <branch>).
> 
>    I do not think this ideal behaviour is something neither batch or
>    interactive rebase has traditionally implemented, but I can see how
>    we can sell this as a bugfix to the end users.
That's a convincing argument, so let's make it so.
Show 7 quoted lines
>  - It turns out that everything is already contained and there is
>    nothing to apply, i.e. after this sequence:
> 
> 	git checkout branch
> 	git checkout $onto_or_merge_base_between_base_and_branch
> 
>    we find out that "git cherry $onto_or_merge_base branch" is empty.

Is there a command missing here? This alone does not make them the same, perhaps you meant some resetting.

I'll assume that you meant a case where the user is *not* on branch, but base/onto is an ancestor of the branch, e.g., in a sequence

  git checkout base
  git reset --hard branch~5
  # now branch..base is empty
  git rebase base branch

the rebase will not do anything unless forced. Similarly for the case where base==branch. And then

Show 5 quoted lines
>    Because you will be one commit ahead of $onto_or_merge_base if "git
>    cherry" were to give one commit to be replayed, I think it is
>    logically correct if you stayed at the $onto_or_merge_base without
>    checking out <branch>.  In other words, abort-with-checkout is not
>    ideal for this case; we would want to just abort in this case.
This would be hard to sell, because a script that runs
  git rebase base branch

could reasonably assume that after the rebase, the current branch is 'branch', unless some error occurred. And we'd be breaking that.

Or am I missing the point of your reasoning?
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· May 18, 2012, 15:25 UTC · re: Thomas Rast · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Thomas Rast <trast@inf.ethz.ch> writes:
Show 21 quoted lines
>>  - The user told us not to do the rebase by making insn sheet empty.  In
>>    other words, this is "aborting the entire rebase", so ideally it
>>    should come back to the state before the user ran "git rebase"
>>    command (i.e. where she was before we switched to <branch>).
>> 
>>    I do not think this ideal behaviour is something neither batch or
>>    interactive rebase has traditionally implemented, but I can see how
>>    we can sell this as a bugfix to the end users.
>
> That's a convincing argument, so let's make it so.
>
>>  - It turns out that everything is already contained and there is
>>    nothing to apply, i.e. after this sequence:
>> 
>> 	git checkout branch
>> 	git checkout $onto_or_merge_base_between_base_and_branch
>> 
>>    we find out that "git cherry $onto_or_merge_base branch" is empty.
>
> Is there a command missing here?  This alone does not make them the
> same, perhaps you meant some resetting.

Actually that was phrased rather poorly. At least "after this sequence" should have been "during this sequence inside the implementation of 'rebase -i'". In other words, the scenario is still where the user typed:

  git rebase [-i] [--onto $onto] $base $branch
which is defined to be equivalent to
  git checkout $branch && git rebase [-i] [--onto $onto] $base

The first half of the equivalent command succeeds. We are on $branch. The second half finds there is nothing to be done.

I think the current code (and probably with your patch) does this in such a case:

has_action "$todo" ||
	die_abort "Nothing to do"

and the die happens on $branch before we detach (iow, between the two checkouts), and the user should end up on $branch. Unlike the case where the user told us to abort the entire rebase, which we agreed that we should come back to the original branch above, I think in this case we should not nullify the first "checkout $branch".

Johannes Sixt· Apr 20, 2012, 16:45 UTC · re: Thomas Rast · lore

Re: [PATCH] rebase -i: avoid checking out $branch when possible

Am 20.04.2012 17:05, schrieb Thomas Rast:
Show 14 quoted lines
> The command
> 
>   git rebase [-i] [--onto $onto] $base $branch
> 
> is defined to be equivalent to
> 
>   git checkout $branch && git rebase [-i] [--onto $onto] $base
> 
> However, we do not have to actually perform the checkout.  The rebase
> starts building on top of $base (or $onto, if given).  The tree
> _state_ (not diff) of $branch is irrelevant.  Actually performing the
> checkout has some downsides: $branch may potentially be way out of
> touch with HEAD, and thus e.g. trigger a full rebuild in a timestamp-
> based build system, even if $base..$branch only edits the README.

Thanks, this is very much appreciated. I like it (because it would prevent lots of unneeded rebuilds in my use-cases).

>  cp "$todo" "$todo".backup
>  git_sequence_editor "$todo" ||
> -	die_abort "Could not execute editor"
> +	die_abort_with_checkout "Could not execute editor"
I don't think that we want to checkout here, even if the docs say we do.
-- Hannes

← back to recent threads