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

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

[Thomas Rast]

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(-)

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

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

[Junio C Hamano]

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?

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

[Thomas Rast]

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.

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

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

[Junio C Hamano]

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.

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

[Shezan Baig]

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:
> 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.
>

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

[Junio C Hamano]

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?

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

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

>  - 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.

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

>    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

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

>>  - 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".

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

[Johannes Sixt]

Am 20.04.2012 17:05, schrieb Thomas Rast:
> 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