git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/2] built-in rebase --autostash: fix regression
@ 2018-11-07 14:00 Johannes Schindelin via GitGitGadget
  2018-11-07 14:00 ` [PATCH 1/2] built-in rebase: demonstrate regression with --autostash Johannes Schindelin via GitGitGadget
  2018-11-07 14:00 ` [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible Johannes Schindelin via GitGitGadget
  0 siblings, 2 replies; 4+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-11-07 14:00 UTC (permalink / raw)
  To: git; +Cc: AEvar, Pratik Karki, Jeff King, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] built-in rebase: demonstrate regression with --autostash
  2018-11-07 14:00 [PATCH 0/2] built-in rebase --autostash: fix regression Johannes Schindelin via GitGitGadget
@ 2018-11-07 14:00 ` Johannes Schindelin via GitGitGadget
  2018-11-07 14:00 ` [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible Johannes Schindelin via GitGitGadget
  1 sibling, 0 replies; 4+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-11-07 14:00 UTC (permalink / raw)
  To: git; +Cc: AEvar, Pratik Karki, Jeff King, Junio C Hamano, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible
  2018-11-07 14:00 [PATCH 0/2] built-in rebase --autostash: fix regression Johannes Schindelin via GitGitGadget
  2018-11-07 14:00 ` [PATCH 1/2] built-in rebase: demonstrate regression with --autostash Johannes Schindelin via GitGitGadget
@ 2018-11-07 14:00 ` Johannes Schindelin via GitGitGadget
  2018-11-07 20:53   ` Jeff King
  1 sibling, 1 reply; 4+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-11-07 14:00 UTC (permalink / raw)
  To: git; +Cc: AEvar, Pratik Karki, Jeff King, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible
  2018-11-07 14:00 ` [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible Johannes Schindelin via GitGitGadget
@ 2018-11-07 20:53   ` Jeff King
  0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2018-11-07 20:53 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, AEvar, Pratik Karki, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-07 14:00 [PATCH 0/2] built-in rebase --autostash: fix regression Johannes Schindelin via GitGitGadget
2018-11-07 14:00 ` [PATCH 1/2] built-in rebase: demonstrate regression with --autostash Johannes Schindelin via GitGitGadget
2018-11-07 14:00 ` [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible Johannes Schindelin via GitGitGadget
2018-11-07 20:53   ` Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox