[PATCH 0/2] built-in rebase --autostash: fix regression

[PATCH 0/2] built-in rebase --autostash: fix regression

[Johannes Schindelin via GitGitGadget]

It was reported that the new, shiny built-in git rebase has a bug where it
would detach the HEAD when it was not even necessary to detach it.

Keeping with my fine tradition to first demonstrate what is the actual
problem (and making it easy to verify my claim), this patch series first
introduces the regression test, and then the (quite simple) fix.

AEvar, sorry for the ASCII-fication of your name, I still did not find the
time to look at the GitGitGadget bug closely where it does the wrong thing
when Cc:ing with non-ASCII names.

Johannes Schindelin (2):
  built-in rebase: demonstrate regression with --autostash
  built-in rebase --autostash: leave the current branch alone if
    possible

 builtin/rebase.c            | 3 ++-
 t/t3420-rebase-autostash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-70%2Fdscho%2Ffix-built-in-rebase-autostash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-70/dscho/fix-built-in-rebase-autostash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/70
-- 
gitgitgadget

[PATCH 1/2] built-in rebase: demonstrate regression with --autostash

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

An unnamed colleague of Ævar Arnfjörð Bjarmason reported a breakage
where a `pull --rebase` (which did not really need to do anything but
stash, see that nothing was changed, and apply the stash again) also
detached the HEAD.

This patch adds a minimal reproducer for this regression.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3420-rebase-autostash.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index f355c6825a..d4e2520bcb 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -361,4 +361,12 @@ test_expect_success 'autostash with dirty submodules' '
 	git rebase -i --autostash HEAD
 '
 
+test_expect_failure 'branch is left alone when possible' '
+	git checkout -b unchanged-branch &&
+	echo changed >file0 &&
+	git rebase --autostash unchanged-branch &&
+	test changed = "$(cat file0)" &&
+	test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)"
+'
+
 test_done
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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

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 &&
-- 
gitgitgadget

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

[Jeff King]

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