{"thread":{"id":"50615","subject":"[PATCH 3/4] built-in rebase: demonstrate that ORIG_HEAD is not set correctly","startedAt":"2019-02-28T15:27:52Z","lastAt":"2019-03-03T19:11:53Z","messageCount":16,"participants":["Johannes Schindelin via GitGitGadget","Phillip Wood","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"370364","messageId":"1bc3cbad265926d963655a0112db74e2c655adb4.1551367664.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.git.gitgitgadget@gmail.com","subject":"[PATCH 3/4] built-in rebase: demonstrate that ORIG_HEAD is not set correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T15:27:47Z","receivedAt":"2019-02-28T15:27:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe ORIG_HEAD pseudo ref is supposed to refer to the original,\npre-rebase state after a successful rebase. Let's add a regression test\nto prove that this regressed: With GIT_TEST_REBASE_USE_BUILTIN=false,\nthis test case passes, with GIT_TEST_REBASE_USE_BUILTIN=true (or unset),\nit fails.\n\nReported by Nazri Ramliy.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3400-rebase.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 3e73f7584c..7e8d5bb200 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -59,6 +59,14 @@ test_expect_success 'rebase against master' '\n \tgit rebase master\n '\n \n+test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' '\n+\tgit checkout -b orig-head topic &&\n+\tpre=\"$(git rev-parse --verify HEAD)\" &&\n+\tgit rebase master &&\n+\ttest_cmp_rev \"$pre\" ORIG_HEAD &&\n+\t! test_cmp_rev \"$pre\" HEAD\n+'\n+\n test_expect_success 'rebase, with and specified as :/quuxery' '\n \ttest_when_finished \"git branch -D torebase\" &&\n \tgit checkout -b torebase my-topic-branch^ &&\n-- \ngitgitgadget\n\n"},{"id":"370365","messageId":"1786c172effb38fa44cea0c1d98de604602a5634.1551367664.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.git.gitgitgadget@gmail.com","subject":"[PATCH 4/4] built-in rebase: set ORIG_HEAD just once, before the rebase","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T15:27:47Z","receivedAt":"2019-02-28T15:27:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTechnically, the scripted version set ORIG_HEAD only in two spots (which\nreally could have been one, because it called `git checkout $onto^0` to\nstart the rebase and also if it could take a shortcut, and in both cases\nit called `git update-ref $orig_head`).\n\nPractically, it *implicitly* reset ORIG_HEAD whenever `git reset --hard`\nwas called.\n\nHowever, what we really want is that it is set exactly once, at the\nbeginning of the rebase.\n\nSo let's do that.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 31 ++++++++++++++++++-------------\n t/t3400-rebase.sh | 2 +-\n 2 files changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex aa469ec964..0f4e1ead49 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -369,6 +369,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n #define RESET_HEAD_REFS_ONLY (1<<2)\n+#define RESET_ORIG_HEAD (1<<3)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t const char *switch_to_branch, unsigned flags,\n@@ -377,6 +378,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n \tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n+\tunsigned update_orig_head = flags & RESET_ORIG_HEAD;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -453,18 +455,21 @@ static int reset_head(struct object_id *oid, const char *action,\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n \tprefix_len = msg.len;\n \n-\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n-\t\told_orig = &oid_old_orig;\n-\tif (!get_oid(\"HEAD\", &oid_orig)) {\n-\t\torig = &oid_orig;\n-\t\tif (!reflog_orig_head) {\n-\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n-\t\t\treflog_orig_head = msg.buf;\n-\t\t}\n-\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n-\t\t\t UPDATE_REFS_MSG_ON_ERR);\n-\t} else if (old_orig)\n-\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\tif (update_orig_head) {\n+\t\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n+\t\t\told_orig = &oid_old_orig;\n+\t\tif (!get_oid(\"HEAD\", &oid_orig)) {\n+\t\t\torig = &oid_orig;\n+\t\t\tif (!reflog_orig_head) {\n+\t\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n+\t\t\t\treflog_orig_head = msg.buf;\n+\t\t\t}\n+\t\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig,\n+\t\t\t\t old_orig, 0, UPDATE_REFS_MSG_ON_ERR);\n+\t\t} else if (old_orig)\n+\t\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\t}\n+\n \tif (!reflog_head) {\n \t\tstrbuf_setlen(&msg, prefix_len);\n \t\tstrbuf_addstr(&msg, \"updating HEAD\");\n@@ -1725,7 +1730,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&msg, \"%s: checkout %s\",\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n-\t\t RESET_HEAD_DETACH, NULL, msg.buf))\n+\t\t RESET_HEAD_DETACH | RESET_ORIG_HEAD, NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n \ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 7e8d5bb200..460d0523be 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -59,7 +59,7 @@ test_expect_success 'rebase against master' '\n \tgit rebase master\n '\n \n-test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' '\n+test_expect_success 'rebase sets ORIG_HEAD to pre-rebase state' '\n \tgit checkout -b orig-head topic &&\n \tpre=\"$(git rev-parse --verify HEAD)\" &&\n \tgit rebase master &&\n-- \ngitgitgadget\n"},{"id":"370366","messageId":"b92d76065d92cb7003dd9d69525a1ed790a93921.1551367664.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.git.gitgitgadget@gmail.com","subject":"[PATCH 2/4] built-in rebase: use the correct reflog when switching branches","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T15:27:46Z","receivedAt":"2019-02-28T15:27:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBy mistake, we used the reflog intended for ORIG_HEAD.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 813ec284ca..aa469ec964 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -475,7 +475,7 @@ static int reset_head(struct object_id *oid, const char *action,\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 = update_ref(reflog_orig_head, switch_to_branch, oid,\n+\t\tret = update_ref(reflog_head, switch_to_branch, oid,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n \t\tif (!ret)\n \t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n-- \ngitgitgadget\n\n"},{"id":"370367","messageId":"pull.153.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":null,"subject":"[PATCH 0/4] Fix ORIG_HEAD behavior of the built-in rebase","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T15:27:44Z","receivedAt":"2019-02-28T15:27:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was reported by Nazri Ramliy that ORIG_HEAD was set incorrectly again,\nthis time caused by the shortcut to call git am directly, without detouring\nto a Unix shell script.\n\nPatch 2/4 might look like something completely unrelated, but without it,\nthe update to HEAD might use an incorrect reflog message.\n\nPatch 1/4 is more a \"while at it\" patch; while looking which code paths\nmight need to update ORIG_HEAD, I noticed that this reset_head() call did\nunnecessary work.\n\nJohannes Schindelin (4):\n built-in rebase: no need to check out `onto` twice\n built-in rebase: use the correct reflog when switching branches\n built-in rebase: demonstrate that ORIG_HEAD is not set correctly\n built-in rebase: set ORIG_HEAD just once, before the rebase\n\n builtin/rebase.c | 37 +++++++++++++++++++++----------------\n t/t3400-rebase.sh | 8 ++++++++\n 2 files changed, 29 insertions(+), 16 deletions(-)\n\n\nbase-commit: 21853626eac565dd42572d90724b29863f61eb3b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-153%2Fdscho%2Frebase-am-and-orig-head-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-153/dscho/rebase-am-and-orig-head-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/153\n-- \ngitgitgadget\n"},{"id":"370368","messageId":"2d99429387ba63526efe233bbefe851c5d556fdc.1551367664.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.git.gitgitgadget@gmail.com","subject":"[PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-02-28T15:27:45Z","receivedAt":"2019-02-28T15:27:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the case that the rebase boils down to a fast-forward, the built-in\nrebase reset the working tree twice: once to start the rebase at `onto`,\nthen realizing that the original HEAD was an ancestor, `reset_head()`\nwas called to update the original ref and to point HEAD back to it.\n\nThat second `reset_head()` call does not need to touch the working tree,\nthough, as it does not change the actual tip commit. So let's avoid that\nunnecessary work.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 08ec4d52c7..813ec284ca 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n \t\t\toid_to_hex(&options.onto->object.oid));\n-\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n-\t\t\t \"HEAD\", msg.buf);\n+\t\treset_head(NULL, \"Fast-forwarded\", options.head_name,\n+\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf);\n \t\tstrbuf_release(&msg);\n \t\tret = !!finish_rebase(&options);\n \t\tgoto cleanup;\n-- \ngitgitgadget\n\n"},{"id":"370375","messageId":"7b1282bf-4b94-5725-00df-2dc63eaa93f0@gmail.com","threadId":"50615","inReplyTo":"2d99429387ba63526efe233bbefe851c5d556fdc.1551367664.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-02-28T20:55:41Z","receivedAt":"2019-02-28T20:55:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In the case that the rebase boils down to a fast-forward, the built-in\n> rebase reset the working tree twice: once to start the rebase at `onto`,\n> then realizing that the original HEAD was an ancestor, `reset_head()`\n> was called to update the original ref and to point HEAD back to it.\n> \n> That second `reset_head()` call does not need to touch the working tree,\n> though, as it does not change the actual tip commit. So let's avoid that\n> unnecessary work.\n\nI'm confused by this I think I must be missing something. If we've \nchecked out onto then why does the working copy not need updating when \nwe fast forward. (also why to we checkout onto before seeing if we can \nfast-forward but that's not related to this patch series)\n\nBest Wishes\n\nPhillip\n\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/rebase.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 08ec4d52c7..813ec284ca 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n> \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n> \t\t\toid_to_hex(&options.onto->object.oid));\n> -\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n> -\t\t\t \"HEAD\", msg.buf);\n> +\t\treset_head(NULL, \"Fast-forwarded\", options.head_name,\n> +\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf);\n> \t\tstrbuf_release(&msg);\n> \t\tret = !!finish_rebase(&options);\n> \t\tgoto cleanup;\n> \n"},{"id":"370414","messageId":"nycvar.QRO.7.76.6.1903011415220.41@tvgsbejvaqbjf.bet","threadId":"50615","inReplyTo":"7b1282bf-4b94-5725-00df-2dc63eaa93f0@gmail.com","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-01T13:19:37Z","receivedAt":"2019-03-01T13:20:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 28 Feb 2019, Phillip Wood wrote:\n\n> On 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> > \n> > In the case that the rebase boils down to a fast-forward, the built-in\n> > rebase reset the working tree twice: once to start the rebase at `onto`,\n> > then realizing that the original HEAD was an ancestor, `reset_head()`\n> > was called to update the original ref and to point HEAD back to it.\n> > \n> > That second `reset_head()` call does not need to touch the working tree,\n> > though, as it does not change the actual tip commit. So let's avoid that\n> > unnecessary work.\n> \n> I'm confused by this I think I must be missing something. If we've checked out\n> onto then why does the working copy not need updating when we fast forward.\n> (also why to we checkout onto before seeing if we can fast-forward but that's\n> not related to this patch series)\n\nSorry, I really try to learn how to express things clearly. Still learning\na lot.\n\nSo what happens is this: we detect the situation where the pre-rebase\n`HEAD` is an ancestor of `onto`. We do this *after* checking out `onto`.\nSo we now know that we essentially fast-forwarded to the post-rebase\nstate.\n\nWhat we still need to do is to update the original ref (unless we were on\na detached HEAD when the rebase was started).\n\nThe original shell code updates the original ref to the current HEAD, and\nthe updates HEAD to point to that symbolic ref instead of being detached.\n\nIn the C code, we abused `reset_head()` to do the same. But `reset_head()`\ndoes more: it does a two-way merge (in this instance, because\n`RESET_HEAD_HARD` was not part of the flags). Which is unnecessary.\n\nThat's all this commit is about.\n\nCiao,\nDscho\n\n> \n> Best Wishes\n> \n> Phillip\n> \n> > \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/rebase.c | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 08ec4d52c7..813ec284ca 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char\n> > *prefix)\n> > strbuf_addf(&msg, \"rebase finished: %s onto %s\",\n> > options.head_name ? options.head_name : \"detached HEAD\",\n> > oid_to_hex(&options.onto->object.oid));\n> > -\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n> > -\t\t\t \"HEAD\", msg.buf);\n> > +\t\treset_head(NULL, \"Fast-forwarded\", options.head_name,\n> > +\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf);\n> > strbuf_release(&msg);\n> > ret = !!finish_rebase(&options);\n> > goto cleanup;\n> > \n> \n"},{"id":"370419","messageId":"22878dbe-d9f0-4737-3a49-ece695c2d0c2@gmail.com","threadId":"50615","inReplyTo":"nycvar.QRO.7.76.6.1903011415220.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-01T15:00:17Z","receivedAt":"2019-03-01T15:00:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 01/03/2019 13:19, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 28 Feb 2019, Phillip Wood wrote:\n> \n>> On 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>>\n>>> In the case that the rebase boils down to a fast-forward, the built-in\n>>> rebase reset the working tree twice: once to start the rebase at `onto`,\n>>> then realizing that the original HEAD was an ancestor, `reset_head()`\n>>> was called to update the original ref and to point HEAD back to it.\n>>>\n>>> That second `reset_head()` call does not need to touch the working tree,\n>>> though, as it does not change the actual tip commit. So let's avoid that\n>>> unnecessary work.\n>>\n>> I'm confused by this I think I must be missing something. If we've checked out\n>> onto then why does the working copy not need updating when we fast forward.\n>> (also why to we checkout onto before seeing if we can fast-forward but that's\n>> not related to this patch series)\n> \n> Sorry, I really try to learn how to express things clearly. Still learning\n> a lot.\n> \n> So what happens is this: we detect the situation where the pre-rebase\n> `HEAD` is an ancestor of `onto`. We do this *after* checking out `onto`.\n> So we now know that we essentially fast-forwarded to the post-rebase\n> state.\n\nAh that makes sense, for some reason I misread the commit message and \nthought that we were dealing with the case where onto was an ancestor of \nthe original HEAD and we were fast-forwarding from onto back to the \noriginal HEAD.\n\n> What we still need to do is to update the original ref (unless we were on\n> a detached HEAD when the rebase was started).\n> \n> The original shell code updates the original ref to the current HEAD, and\n> the updates HEAD to point to that symbolic ref instead of being detached.\n> \n> In the C code, we abused `reset_head()` to do the same. But `reset_head()`\n> does more: it does a two-way merge (in this instance, because\n> `RESET_HEAD_HARD` was not part of the flags). Which is unnecessary.\n> \n> That's all this commit is about.\n\nThanks for explaining, it all makes sense to me now\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>>\n>>> Signed-off-by: Johannes Schindelin \n>>> ---\n>>> builtin/rebase.c | 4 ++--\n>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>>> index 08ec4d52c7..813ec284ca 100644\n>>> --- a/builtin/rebase.c\n>>> +++ b/builtin/rebase.c\n>>> @@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char\n>>> *prefix)\n>>> strbuf_addf(&msg, \"rebase finished: %s onto %s\",\n>>> options.head_name ? options.head_name : \"detached HEAD\",\n>>> oid_to_hex(&options.onto->object.oid));\n>>> -\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n>>> -\t\t\t \"HEAD\", msg.buf);\n>>> +\t\treset_head(NULL, \"Fast-forwarded\", options.head_name,\n>>> +\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf);\n>>> strbuf_release(&msg);\n>>> ret = !!finish_rebase(&options);\n>>> goto cleanup;\n>>>\n>>\n"},{"id":"370497","messageId":"xmqqef7o3fl8.fsf@gitster-ct.c.googlers.com","threadId":"50615","inReplyTo":"22878dbe-d9f0-4737-3a49-ece695c2d0c2@gmail.com","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T01:35:15Z","receivedAt":"2019-03-03T01:35:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for explaining, it all makes sense to me now\n\nIt would be necessary to make sure that it all makes sense to all\nfuture readers. Are they patches good enough as-is for that, or do\nthey need some updates before I take a look at them to pick up?\n"},{"id":"370540","messageId":"nycvar.QRO.7.76.6.1903031808380.45@tvgsbejvaqbjf.bet","threadId":"50615","inReplyTo":"xmqqef7o3fl8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-03T17:09:26Z","receivedAt":"2019-03-03T17:09:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 3 Mar 2019, Junio C Hamano wrote:\n\n> Phillip Wood writes:\n> \n> > Thanks for explaining, it all makes sense to me now\n> \n> It would be necessary to make sure that it all makes sense to all\n> future readers. Are they patches good enough as-is for that, or do\n> they need some updates before I take a look at them to pick up?\n\nAs you frequently say: if a reviewer gets puzzled, it is often a good\nindicator that the commit message needs to be improved. I did exactly that\nand will send out the next iteration in a minute.\n\nCiao,\nDscho\n"},{"id":"370542","messageId":"pull.153.v2.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] Fix ORIG_HEAD behavior of the built-in rebase","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-03T17:11:52Z","receivedAt":"2019-03-03T17:11:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It was reported by Nazri Ramliy that ORIG_HEAD was set incorrectly again,\nthis time caused by the shortcut to call git am directly, without detouring\nto a Unix shell script.\n\nPatch 2/4 might look like something completely unrelated, but without it,\nthe update to HEAD might use an incorrect reflog message.\n\nPatch 1/4 is more a \"while at it\" patch; while looking which code paths\nmight need to update ORIG_HEAD, I noticed that this reset_head() call did\nunnecessary work.\n\nChanges since v1:\n\n * Reworded the unclear commit message of patch 1/4.\n\nJohannes Schindelin (4):\n built-in rebase: no need to check out `onto` twice\n built-in rebase: use the correct reflog when switching branches\n built-in rebase: demonstrate that ORIG_HEAD is not set correctly\n built-in rebase: set ORIG_HEAD just once, before the rebase\n\n builtin/rebase.c | 37 +++++++++++++++++++++----------------\n t/t3400-rebase.sh | 8 ++++++++\n 2 files changed, 29 insertions(+), 16 deletions(-)\n\n\nbase-commit: 21853626eac565dd42572d90724b29863f61eb3b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-153%2Fdscho%2Frebase-am-and-orig-head-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-153/dscho/rebase-am-and-orig-head-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/153\n\nRange-diff vs v1:\n\n 1: 2d99429387 ! 1: 59c3266ed5 built-in rebase: no need to check out `onto` twice\n @@ -4,12 +4,21 @@\n \n In the case that the rebase boils down to a fast-forward, the built-in\n rebase reset the working tree twice: once to start the rebase at `onto`,\n - then realizing that the original HEAD was an ancestor, `reset_head()`\n - was called to update the original ref and to point HEAD back to it.\n + then realizing that the original (pre-rebase) HEAD was an ancestor and\n + we basically already fast-forwarded to the post-rebase HEAD,\n + `reset_head()` was called to update the original ref and to point HEAD\n + back to it.\n \n That second `reset_head()` call does not need to touch the working tree,\n - though, as it does not change the actual tip commit. So let's avoid that\n - unnecessary work.\n + though, as it does not change the actual tip commit (and therefore the\n + working tree should stay unchanged anyway): only the ref needs to be\n + updated (because the rebase detached the HEAD, and we want to go back to\n + the branch on which the rebase was started).\n +\n + But that second `reset_head()` was called without the flag to leave the\n + working tree alone (the reason: when that call was introduced, that flag\n + was not yet even thought of). Let's avoid that unnecessary work by\n + passing that flag.\n \n Signed-off-by: Johannes Schindelin \n \n 2: b92d76065d = 2: 67e41c032a built-in rebase: use the correct reflog when switching branches\n 3: 1bc3cbad26 = 3: 17737998f4 built-in rebase: demonstrate that ORIG_HEAD is not set correctly\n 4: 1786c172ef = 4: 7891c05f51 built-in rebase: set ORIG_HEAD just once, before the rebase\n\n-- \ngitgitgadget\n"},{"id":"370543","messageId":"59c3266ed523be6f92e7cb991436be3dac96a81f.1551633112.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-03T17:11:53Z","receivedAt":"2019-03-03T17:11:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the case that the rebase boils down to a fast-forward, the built-in\nrebase reset the working tree twice: once to start the rebase at `onto`,\nthen realizing that the original (pre-rebase) HEAD was an ancestor and\nwe basically already fast-forwarded to the post-rebase HEAD,\n`reset_head()` was called to update the original ref and to point HEAD\nback to it.\n\nThat second `reset_head()` call does not need to touch the working tree,\nthough, as it does not change the actual tip commit (and therefore the\nworking tree should stay unchanged anyway): only the ref needs to be\nupdated (because the rebase detached the HEAD, and we want to go back to\nthe branch on which the rebase was started).\n\nBut that second `reset_head()` was called without the flag to leave the\nworking tree alone (the reason: when that call was introduced, that flag\nwas not yet even thought of). Let's avoid that unnecessary work by\npassing that flag.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 08ec4d52c7..813ec284ca 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n \t\t\toid_to_hex(&options.onto->object.oid));\n-\t\treset_head(NULL, \"Fast-forwarded\", options.head_name, 0,\n-\t\t\t \"HEAD\", msg.buf);\n+\t\treset_head(NULL, \"Fast-forwarded\", options.head_name,\n+\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf);\n \t\tstrbuf_release(&msg);\n \t\tret = !!finish_rebase(&options);\n \t\tgoto cleanup;\n-- \ngitgitgadget\n\n"},{"id":"370544","messageId":"67e41c032a744096bf7e63c30a570bac01e2f656.1551633112.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] built-in rebase: use the correct reflog when switching branches","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-03T17:11:54Z","receivedAt":"2019-03-03T17:11:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBy mistake, we used the reflog intended for ORIG_HEAD.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 813ec284ca..aa469ec964 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -475,7 +475,7 @@ static int reset_head(struct object_id *oid, const char *action,\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 = update_ref(reflog_orig_head, switch_to_branch, oid,\n+\t\tret = update_ref(reflog_head, switch_to_branch, oid,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n \t\tif (!ret)\n \t\t\tret = create_symref(\"HEAD\", switch_to_branch,\n-- \ngitgitgadget\n\n"},{"id":"370545","messageId":"17737998f45ed8b4e38fa58ff82309231230fc81.1551633112.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] built-in rebase: demonstrate that ORIG_HEAD is not set correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-03T17:11:54Z","receivedAt":"2019-03-03T17:11:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe ORIG_HEAD pseudo ref is supposed to refer to the original,\npre-rebase state after a successful rebase. Let's add a regression test\nto prove that this regressed: With GIT_TEST_REBASE_USE_BUILTIN=false,\nthis test case passes, with GIT_TEST_REBASE_USE_BUILTIN=true (or unset),\nit fails.\n\nReported by Nazri Ramliy.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3400-rebase.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 3e73f7584c..7e8d5bb200 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -59,6 +59,14 @@ test_expect_success 'rebase against master' '\n \tgit rebase master\n '\n \n+test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' '\n+\tgit checkout -b orig-head topic &&\n+\tpre=\"$(git rev-parse --verify HEAD)\" &&\n+\tgit rebase master &&\n+\ttest_cmp_rev \"$pre\" ORIG_HEAD &&\n+\t! test_cmp_rev \"$pre\" HEAD\n+'\n+\n test_expect_success 'rebase, with and specified as :/quuxery' '\n \ttest_when_finished \"git branch -D torebase\" &&\n \tgit checkout -b torebase my-topic-branch^ &&\n-- \ngitgitgadget\n\n"},{"id":"370546","messageId":"7891c05f51583e2391c74753c0377ed416314b8c.1551633112.git.gitgitgadget@gmail.com","threadId":"50615","inReplyTo":"pull.153.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] built-in rebase: set ORIG_HEAD just once, before the rebase","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-03T17:11:55Z","receivedAt":"2019-03-03T17:12:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTechnically, the scripted version set ORIG_HEAD only in two spots (which\nreally could have been one, because it called `git checkout $onto^0` to\nstart the rebase and also if it could take a shortcut, and in both cases\nit called `git update-ref $orig_head`).\n\nPractically, it *implicitly* reset ORIG_HEAD whenever `git reset --hard`\nwas called.\n\nHowever, what we really want is that it is set exactly once, at the\nbeginning of the rebase.\n\nSo let's do that.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 31 ++++++++++++++++++-------------\n t/t3400-rebase.sh | 2 +-\n 2 files changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex aa469ec964..0f4e1ead49 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -369,6 +369,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)\n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n #define RESET_HEAD_REFS_ONLY (1<<2)\n+#define RESET_ORIG_HEAD (1<<3)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t const char *switch_to_branch, unsigned flags,\n@@ -377,6 +378,7 @@ static int reset_head(struct object_id *oid, const char *action,\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n \tunsigned refs_only = flags & RESET_HEAD_REFS_ONLY;\n+\tunsigned update_orig_head = flags & RESET_ORIG_HEAD;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -453,18 +455,21 @@ static int reset_head(struct object_id *oid, const char *action,\n \tstrbuf_addf(&msg, \"%s: \", reflog_action ? reflog_action : \"rebase\");\n \tprefix_len = msg.len;\n \n-\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n-\t\told_orig = &oid_old_orig;\n-\tif (!get_oid(\"HEAD\", &oid_orig)) {\n-\t\torig = &oid_orig;\n-\t\tif (!reflog_orig_head) {\n-\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n-\t\t\treflog_orig_head = msg.buf;\n-\t\t}\n-\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig, old_orig, 0,\n-\t\t\t UPDATE_REFS_MSG_ON_ERR);\n-\t} else if (old_orig)\n-\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\tif (update_orig_head) {\n+\t\tif (!get_oid(\"ORIG_HEAD\", &oid_old_orig))\n+\t\t\told_orig = &oid_old_orig;\n+\t\tif (!get_oid(\"HEAD\", &oid_orig)) {\n+\t\t\torig = &oid_orig;\n+\t\t\tif (!reflog_orig_head) {\n+\t\t\t\tstrbuf_addstr(&msg, \"updating ORIG_HEAD\");\n+\t\t\t\treflog_orig_head = msg.buf;\n+\t\t\t}\n+\t\t\tupdate_ref(reflog_orig_head, \"ORIG_HEAD\", orig,\n+\t\t\t\t old_orig, 0, UPDATE_REFS_MSG_ON_ERR);\n+\t\t} else if (old_orig)\n+\t\t\tdelete_ref(NULL, \"ORIG_HEAD\", old_orig, 0);\n+\t}\n+\n \tif (!reflog_head) {\n \t\tstrbuf_setlen(&msg, prefix_len);\n \t\tstrbuf_addstr(&msg, \"updating HEAD\");\n@@ -1725,7 +1730,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&msg, \"%s: checkout %s\",\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n-\t\t RESET_HEAD_DETACH, NULL, msg.buf))\n+\t\t RESET_HEAD_DETACH | RESET_ORIG_HEAD, NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n \ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 7e8d5bb200..460d0523be 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -59,7 +59,7 @@ test_expect_success 'rebase against master' '\n \tgit rebase master\n '\n \n-test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' '\n+test_expect_success 'rebase sets ORIG_HEAD to pre-rebase state' '\n \tgit checkout -b orig-head topic &&\n \tpre=\"$(git rev-parse --verify HEAD)\" &&\n \tgit rebase master &&\n-- \ngitgitgadget\n"},{"id":"370554","messageId":"f38ebc4c-f0a6-c57b-1d0e-8208ff638a73@gmail.com","threadId":"50615","inReplyTo":"xmqqef7o3fl8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-03T19:11:48Z","receivedAt":"2019-03-03T19:11:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 03/03/2019 01:35, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Thanks for explaining, it all makes sense to me now\n> \n> It would be necessary to make sure that it all makes sense to all\n> future readers. Are they patches good enough as-is for that, or do\n> they need some updates before I take a look at them to pick up?\n> \n\nI've just re-read them and I think they're fine as is, though the first\n paragraph of the first commit message might be clearer if \"HEAD was an\nancestor and\" was changed to \"HEAD was an ancestor of onto and\".\n\nBest Wishes\n\nPhillip\n\n"}]}