{"thread":{"id":"49778","subject":"[PATCH 0/2] built-in rebase --autostash: fix regression","startedAt":"2018-11-07T14:00:50Z","lastAt":"2018-11-07T20:53:32Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"362660","messageId":"pull.70.git.gitgitgadget@gmail.com","threadId":"49778","inReplyTo":null,"subject":"[PATCH 0/2] built-in rebase --autostash: fix regression","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-07T14:00:47Z","receivedAt":"2018-11-07T14:00:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was reported that the new, shiny built-in git rebase has a bug where it\nwould detach the HEAD when it was not even necessary to detach it.\n\nKeeping with my fine tradition to first demonstrate what is the actual\nproblem (and making it easy to verify my claim), this patch series first\nintroduces the regression test, and then the (quite simple) fix.\n\nAEvar, sorry for the ASCII-fication of your name, I still did not find the\ntime to look at the GitGitGadget bug closely where it does the wrong thing\nwhen Cc:ing with non-ASCII names.\n\nJohannes Schindelin (2):\n built-in rebase: demonstrate regression with --autostash\n built-in rebase --autostash: leave the current branch alone if\n possible\n\n builtin/rebase.c | 3 ++-\n t/t3420-rebase-autostash.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-70%2Fdscho%2Ffix-built-in-rebase-autostash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-70/dscho/fix-built-in-rebase-autostash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/70\n-- \ngitgitgadget\n"},{"id":"362661","messageId":"9a5237b13bb22e192cbe5777e6338d78e0773bad.1541599246.git.gitgitgadget@gmail.com","threadId":"49778","inReplyTo":"pull.70.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] built-in rebase: demonstrate regression with --autostash","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-07T14:00:48Z","receivedAt":"2018-11-07T14:00:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAn unnamed colleague of Ævar Arnfjörð Bjarmason reported a breakage\nwhere a `pull --rebase` (which did not really need to do anything but\nstash, see that nothing was changed, and apply the stash again) also\ndetached the HEAD.\n\nThis patch adds a minimal reproducer for this regression.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3420-rebase-autostash.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex f355c6825a..d4e2520bcb 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -361,4 +361,12 @@ test_expect_success 'autostash with dirty submodules' '\n \tgit rebase -i --autostash HEAD\n '\n \n+test_expect_failure 'branch is left alone when possible' '\n+\tgit checkout -b unchanged-branch &&\n+\techo changed >file0 &&\n+\tgit rebase --autostash unchanged-branch &&\n+\ttest changed = \"$(cat file0)\" &&\n+\ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"362662","messageId":"a2f0b53db269e9a6d7c29b665c9be53bd2f30e42.1541599246.git.gitgitgadget@gmail.com","threadId":"49778","inReplyTo":"pull.70.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-07T14:00:50Z","receivedAt":"2018-11-07T14:00:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen we converted a `git reset --hard` call in the original Unix shell\nscript to built-in code, we asked to reset the worktree and the index\nand explicitly *not* to detach the HEAD. By mistake, though, we still\ndid. Let's fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 3 ++-\n t/t3420-rebase-autostash.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ee06aa363..4a608d0a78 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -613,7 +613,8 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\treflog_head = msg.buf;\n \t}\n \tif (!switch_to_branch)\n-\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig, REF_NO_DEREF,\n+\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n+\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \telse {\n \t\tret = create_symref(\"HEAD\", switch_to_branch, msg.buf);\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex d4e2520bcb..4c7494cc8f 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -361,7 +361,7 @@ test_expect_success 'autostash with dirty submodules' '\n \tgit rebase -i --autostash HEAD\n '\n \n-test_expect_failure 'branch is left alone when possible' '\n+test_expect_success 'branch is left alone when possible' '\n \tgit checkout -b unchanged-branch &&\n \techo changed >file0 &&\n \tgit rebase --autostash unchanged-branch &&\n-- \ngitgitgadget\n"},{"id":"362671","messageId":"20181107205329.GB30078@sigill.intra.peff.net","threadId":"49778","inReplyTo":"a2f0b53db269e9a6d7c29b665c9be53bd2f30e42.1541599246.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-07T20:53:29Z","receivedAt":"2018-11-07T20:53:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 07, 2018 at 06:00:50AM -0800, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> When we converted a `git reset --hard` call in the original Unix shell\n> script to built-in code, we asked to reset the worktree and the index\n> and explicitly *not* to detach the HEAD. By mistake, though, we still\n> did. Let's fix this.\n> [...]\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0ee06aa363..4a608d0a78 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -613,7 +613,8 @@ static int reset_head(struct object_id *oid, const char *action,\n> \t\treflog_head = msg.buf;\n> \t}\n> \tif (!switch_to_branch)\n> -\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig, REF_NO_DEREF,\n> +\t\tret = update_ref(reflog_head, \"HEAD\", oid, orig,\n> +\t\t\t\t detach_head ? REF_NO_DEREF : 0,\n> \t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n\nThis makes sense. There are actually a bunch of calls that pass\ndetach_head==0, besides the one related to autostash. I suspect for most\nof them it does not matter, because either:\n\n 1. We are already on a detached HEAD, since we detach as the first\n step of the rebase. So for the call in ACTION_SKIP, for example, we\n probably cannot trigger the problem.\n\n 2. They pass a switch_to_branch arg, so we do not hit this code path\n anyway (the call to fast-forward is like this, for example).\n\nSo there may be other ways to trigger the problem, but I didn't dig.\nEither way, your fix is clearly the right thing to do.\n\n-Peff\n"}]}