diff mbox series

[2/2] built-in rebase --autostash: leave the current branch alone if possible

Message ID a2f0b53db269e9a6d7c29b665c9be53bd2f30e42.1541599246.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series built-in rebase --autostash: fix regression | expand

Commit Message

Bruce Perry via GitGitGadget Nov. 7, 2018, 2 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When we converted a `git reset --hard` call in the original Unix shell
script to built-in code, we asked to reset the worktree and the index
and explicitly *not* to detach the HEAD. By mistake, though, we still
did. Let's fix this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c            | 3 ++-
 t/t3420-rebase-autostash.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Jeff King Nov. 7, 2018, 8:53 p.m. UTC | #1 
On Wed, Nov 07, 2018 at 06:00:50AM -0800, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When we converted a `git reset --hard` call in the original Unix shell
> script to built-in code, we asked to reset the worktree and the index
> and explicitly *not* to detach the HEAD. By mistake, though, we still
> did. Let's fix this.
> [...]
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..4a608d0a78 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -613,7 +613,8 @@ static int reset_head(struct object_id *oid, const char *action,
>  		reflog_head = msg.buf;
>  	}
>  	if (!switch_to_branch)
> -		ret = update_ref(reflog_head, "HEAD", oid, orig, REF_NO_DEREF,
> +		ret = update_ref(reflog_head, "HEAD", oid, orig,
> +				 detach_head ? REF_NO_DEREF : 0,
>  				 UPDATE_REFS_MSG_ON_ERR);

This makes sense. There are actually a bunch of calls that pass
detach_head==0, besides the one related to autostash. I suspect for most
of them it does not matter, because either:

  1. We are already on a detached HEAD, since we detach as the first
     step of the rebase. So for the call in ACTION_SKIP, for example, we
     probably cannot trigger the problem.

  2. They pass a switch_to_branch arg, so we do not hit this code path
     anyway (the call to fast-forward is like this, for example).

So there may be other ways to trigger the problem, but I didn't dig.
Either way, your fix is clearly the right thing to do.

-Peff
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..4a608d0a78 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -613,7 +613,8 @@  static int reset_head(struct object_id *oid, const char *action,
 		reflog_head = msg.buf;
 	}
 	if (!switch_to_branch)
-		ret = update_ref(reflog_head, "HEAD", oid, orig, REF_NO_DEREF,
+		ret = update_ref(reflog_head, "HEAD", oid, orig,
+				 detach_head ? REF_NO_DEREF : 0,
 				 UPDATE_REFS_MSG_ON_ERR);
 	else {
 		ret = create_symref("HEAD", switch_to_branch, msg.buf);
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index d4e2520bcb..4c7494cc8f 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -361,7 +361,7 @@  test_expect_success 'autostash with dirty submodules' '
 	git rebase -i --autostash HEAD
 '
 
-test_expect_failure 'branch is left alone when possible' '
+test_expect_success 'branch is left alone when possible' '
 	git checkout -b unchanged-branch &&
 	echo changed >file0 &&
 	git rebase --autostash unchanged-branch &&