* [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 related [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 related [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, other threads:[~2018-11-07 20:53 UTC | newest] 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
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).