{"thread":{"id":"51663","subject":"[PATCH 0/2] git rebase: Make sure upstream branch is left alone.","startedAt":"2019-08-18T10:02:44Z","lastAt":"2019-09-02T17:33:58Z","messageCount":38,"participants":["Ben Wijen","Phillip Wood","Ben","Junio C Hamano","Eric Sunshine","Johannes Schindelin","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"380618","messageId":"20190818095349.3218-1-ben@wijen.net","threadId":"51663","inReplyTo":null,"subject":"[PATCH 0/2] git rebase: Make sure upstream branch is left alone.","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-18T09:53:47Z","receivedAt":"2019-08-18T10:02:44Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"I found an issue with git rebase --autostash with an dirty worktree/index.\nIt seems the currently active branch is moved, which is not correct.\nThe following patches contain both a test and a fix.\n\nBen Wijen (2):\n t3420: never change upstream branch\n rebase.c: make sure current branch isn't moved when autostashing\n\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 13 +++++++++----\n 2 files changed, 15 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"380619","messageId":"20190818095349.3218-3-ben@wijen.net","threadId":"51663","inReplyTo":"20190818095349.3218-1-ben@wijen.net","subject":"[PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-18T09:53:49Z","receivedAt":"2019-08-18T10:02:55Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"The rebase --autostash incorrectly moved the current branch to orig_head, where\norig_head -- commit object name of tip of the branch before rebasing\n\nIt seems this was incorrectly taken over from git-legacy-rebase.sh\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 4 ----\n 2 files changed, 6 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..a928f44941 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\n+\t\t\t/*\n+\t\t\t * We might not be on orig_head yet:\n+\t\t\t * Make sure to reset w/o switching branches...\n+\t\t\t */\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 867e4e0b17..2ea1909881 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-- \n2.22.0\n\n"},{"id":"380620","messageId":"20190818095349.3218-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190818095349.3218-1-ben@wijen.net","subject":"[PATCH 1/2] t3420: never change upstream branch","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-18T09:53:48Z","receivedAt":"2019-08-18T10:03:03Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When using `git rebase --autostash ` and\nthe workarea is dirty, the active branch is incorrectly reset\nto the rebase branch.\n\nThis test will check for such behavior.\n\nSigned-off-by: Ben Wijen \n---\n t/t3420-rebase-autostash.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..867e4e0b17 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -306,4 +306,13 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change upstream branch' '\n+\ttest_when_finished \"git reset --hard && git branch -D upstream\" &&\n+\tgit checkout -b upstream unrelated-onto-branch &&\n+\techo changed >file0 &&\n+\tgit add file0 &&\n+\tgit rebase --autostash upstream feature-branch &&\n+\ttest $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"380695","messageId":"0bef598c-6c89-c699-5290-ee2003db5979@gmail.com","threadId":"51663","inReplyTo":"20190818095349.3218-1-ben@wijen.net","subject":"Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-19T09:26:48Z","receivedAt":"2019-08-19T09:26:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 18/08/2019 10:53, Ben Wijen wrote:\n> I found an issue with git rebase --autostash with an dirty worktree/index.\n> It seems the currently active branch is moved, which is not correct.\n\nI'm not sure I understand what you mean by \"the currently active branch \nis moved\". 'git rebase --autostash ' should checkout \n (presumably stashing any unstaged and uncommitted changes \nfirst) and then do rebase - what's it doing instead?\n\nBest Wishes\n\nPhillip\n\n> The following patches contain both a test and a fix.\n> \n> Ben Wijen (2):\n> t3420: never change upstream branch\n> rebase.c: make sure current branch isn't moved when autostashing\n> \n> builtin/rebase.c | 18 ++++++------------\n> t/t3420-rebase-autostash.sh | 13 +++++++++----\n> 2 files changed, 15 insertions(+), 16 deletions(-)\n> \n"},{"id":"380709","messageId":"81b244b9-8b26-e638-56ae-827bbc522dc2@wijen.net","threadId":"51663","inReplyTo":"0bef598c-6c89-c699-5290-ee2003db5979@gmail.com","subject":"Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2019-08-19T15:33:15Z","receivedAt":"2019-08-19T15:33:27Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi Phillip,\n\nThe expected behavior: (as per v2.21.0:/git-legacy-rebase.sh:679)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH \n\n\nThe actual behavior:\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH \n\nSo, the problem with the actual behavior is the move of the currently active branch with 'git reset --hard master'\n\n\nBest regards,\n\nBen...\n\nOn 19-08-2019 11:26, Phillip Wood wrote:\n> Hi Ben\n> \n> On 18/08/2019 10:53, Ben Wijen wrote:\n>> I found an issue with git rebase --autostash with an dirty worktree/index.\n>> It seems the currently active branch is moved, which is not correct.\n> \n> I'm not sure I understand what you mean by \"the currently active branch is moved\". 'git rebase --autostash ' should checkout (presumably stashing any unstaged and uncommitted changes first) and then do rebase - what's it doing instead?\n> \n> Best Wishes\n> \n> Phillip\n> \n>> The following patches contain both a test and a fix.\n>>\n>> Ben Wijen (2):\n>>    t3420: never change upstream branch\n>>    rebase.c: make sure current branch isn't moved when autostashing\n>>\n>>   builtin/rebase.c            | 18 ++++++------------\n>>   t/t3420-rebase-autostash.sh | 13 +++++++++----\n>>   2 files changed, 15 insertions(+), 16 deletions(-)\n>>\n> \n"},{"id":"380722","messageId":"xmqqef1hxbhg.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"81b244b9-8b26-e638-56ae-827bbc522dc2@wijen.net","subject":"Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-19T18:21:15Z","receivedAt":"2019-08-19T18:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben writes:\n\n> Hi Phillip,\n>\n> The expected behavior: (as per v2.21.0:/git-legacy-rebase.sh:679)\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH \n>\n>\n> The actual behavior:\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard master\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH \n>\n> So, the problem with the actual behavior is the move of the currently active branch with 'git reset --hard master'\n\nYour expected and actual behaviour are described above, but it is\nnot mentioned out of what command (and in what settings) you expect\nthe above behaviour.\n\nI am guessing that the setup and the command is something along this\nline?\n\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nThat is, you have checked out a branch that is not the 'master'\nbranch, you have accumulated some work in the working tree, and then\nyou ask \"please stash away the work in progress and then rebase the\n'master' branch on top of the upstream branch\"?\n\nIf so, my expectation for the third step (i.e. the actual \"rebase\")\naligns with yours. After stashing away the local change, we want to\nget a clean working tree, checkout the master branch and rebase it\non top of the upstream. With the command sequence you wrote in the\n\"actual\" section, after stashing away the local change, the current\nbranch that is *not* the master is reset to the tip of 'master',\nwhich would not be what we want.\n\n"},{"id":"380743","messageId":"xmqqr25gx1k1.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"20190818095349.3218-2-ben@wijen.net","subject":"Re: [PATCH 1/2] t3420: never change upstream branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-19T21:55:42Z","receivedAt":"2019-08-19T21:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Wijen writes:\n\n> When using `git rebase --autostash ` and\n> the workarea is dirty, the active branch is incorrectly reset\n> to the rebase branch.\n>\n> This test will check for such behavior.\n>\n> Signed-off-by: Ben Wijen \n> ---\n> t/t3420-rebase-autostash.sh | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index b8f4d03467..867e4e0b17 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -306,4 +306,13 @@ test_expect_success 'branch is left alone when possible' '\n> \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n> '\n> \n> +test_expect_success 'never change upstream branch' '\n> +\ttest_when_finished \"git reset --hard && git branch -D upstream\" &&\n> +\tgit checkout -b upstream unrelated-onto-branch &&\n> +\techo changed >file0 &&\n> +\tgit add file0 &&\n> +\tgit rebase --autostash upstream feature-branch &&\n> +\ttest $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)\n> +'\n> +\n> test_done\n\nIf you are going to make these into two separate commits (which I do\nnot necessarily recommend), introduce it as \"test_expect_failure\" in\nstep 1/2 and flip it to \"test_expect_success\" in step 2/2, when the\nbreakage is corrected.\n\nThis breakage may have happened somewhere between v2.19 and v2.20,\nif my hunch is correct. If it is easy to identify the exact point\nof breakage, it may make sense to note it in the log message of 2/2\nas well. My guess is 176f5d96 (\"built-in rebase --autostash: leave\nthe current branch alone if possible\", 2018-11-07) is the plausible\ncandidate (iow, I suspect that the \"do not detach\" optimization was\nmade a bit too aggressively by that commit), but don't quote me on\nit as this was purely done by \"git log --grep -p\" and not compiling\nor running any tests ;-)\n\nThanks.\n\n\n"},{"id":"380793","messageId":"a8645289-ff60-7739-b010-039583beef1c@gmail.com","threadId":"51663","inReplyTo":"20190818095349.3218-2-ben@wijen.net","subject":"Re: [PATCH 1/2] t3420: never change upstream branch","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-20T08:58:28Z","receivedAt":"2019-08-20T08:58:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 18/08/2019 10:53, Ben Wijen wrote:\n> When using `git rebase --autostash ` and\n> the workarea is dirty, the active branch is incorrectly reset\n> to the rebase branch.\n> \n> This test will check for such behavior.\n> \n> Signed-off-by: Ben Wijen \n> ---\n> t/t3420-rebase-autostash.sh | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index b8f4d03467..867e4e0b17 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -306,4 +306,13 @@ test_expect_success 'branch is left alone when possible' '\n> \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n> '\n> \n> +test_expect_success 'never change upstream branch' '\n> +\ttest_when_finished \"git reset --hard && git branch -D upstream\" &&\n> +\tgit checkout -b upstream unrelated-onto-branch &&\n> +\techo changed >file0 &&\n> +\tgit add file0 &&\n> +\tgit rebase --autostash upstream feature-branch &&\n> +\ttest $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)\n\nIn addition to Junio's suggestions I'd add using\n\ttest_cmp_rev upstream unrelated-onto-branch\nfor the last line.\n\nBest Wishes\n\nPhillip\n\n> +'\n> +\n> test_done\n> \n"},{"id":"380794","messageId":"ff239cf8-f46e-82c5-788a-3484deff51cd@gmail.com","threadId":"51663","inReplyTo":"20190818095349.3218-3-ben@wijen.net","subject":"Re: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-20T09:00:46Z","receivedAt":"2019-08-20T09:00:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nI need to have a longer look at this (I don't understand why we're \ncalling reset --hard after we've stashed the changes) but I notice that \nthe test lines you're changing predate the switch to the builtin rebase \nso those changes are not related to the branch switching problem.\n\nBest Wishes\n\nPhillip\n\nOn 18/08/2019 10:53, Ben Wijen wrote:\n> The rebase --autostash incorrectly moved the current branch to orig_head, where\n> orig_head -- commit object name of tip of the branch before rebasing\n> \n> It seems this was incorrectly taken over from git-legacy-rebase.sh\n> \n> Signed-off-by: Ben Wijen \n> ---\n> builtin/rebase.c | 18 ++++++------------\n> t/t3420-rebase-autostash.sh | 4 ----\n> 2 files changed, 6 insertions(+), 16 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 670096c065..a928f44941 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstate_dir_path(\"autostash\", &options);\n> \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n> \t\t\tstruct object_id oid;\n> -\t\t\tstruct commit *head =\n> -\t\t\t\tlookup_commit_reference(the_repository,\n> -\t\t\t\t\t\t\t&options.orig_head);\n> \n> \t\t\targv_array_pushl(&stash.args,\n> \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n> @@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\t options.state_dir);\n> \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n> \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n> -\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n> +\n> +\t\t\t/*\n> +\t\t\t * We might not be on orig_head yet:\n> +\t\t\t * Make sure to reset w/o switching branches...\n> +\t\t\t */\n> +\t\t\tif (reset_head(NULL, \"reset --hard\",\n> \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n> \t\t\t\tdie(_(\"could not reset --hard\"));\n> -\t\t\tprintf(_(\"HEAD is now at %s\"),\n> -\t\t\t find_unique_abbrev(&head->object.oid,\n> -\t\t\t\t\t\t DEFAULT_ABBREV));\n> -\t\t\tstrbuf_reset(&buf);\n> -\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n> -\t\t\tif (buf.len > 0)\n> -\t\t\t\tprintf(\" %s\", buf.buf);\n> -\t\t\tputchar('\\n');\n> \n> \t\t\tif (discard_index(the_repository->index) < 0 ||\n> \t\t\t\trepo_read_index(the_repository) < 0)\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index 867e4e0b17..2ea1909881 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -37,7 +37,6 @@ test_expect_success setup '\n> create_expected_success_am () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tFirst, rewinding head to replay your work on top of it...\n> \tApplying: second commit\n> \tApplying: third commit\n> @@ -48,7 +47,6 @@ create_expected_success_am () {\n> create_expected_success_interactive () {\n> \tq_to_cr >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tApplied autostash.\n> \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n> \tEOF\n> @@ -57,7 +55,6 @@ create_expected_success_interactive () {\n> create_expected_failure_am () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tFirst, rewinding head to replay your work on top of it...\n> \tApplying: second commit\n> \tApplying: third commit\n> @@ -70,7 +67,6 @@ create_expected_failure_am () {\n> create_expected_failure_interactive () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tApplying autostash resulted in conflicts.\n> \tYour changes are safe in the stash.\n> \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n> \n"},{"id":"380850","messageId":"dd1008fc-fdc6-8783-b9db-30a5c7e0a373@wijen.net","threadId":"51663","inReplyTo":"ff239cf8-f46e-82c5-788a-3484deff51cd@gmail.com","subject":"Re: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2019-08-20T19:53:59Z","receivedAt":"2019-08-20T19:54:13Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi Phillip,\n\n'git stash create autostash' does not clear the workarea (like 'git stash' does)\nThat's the reason for the 'git reset --hard'\n\nThe difference you see in the test between the legacy rebase and the builtin rebase is because\nthe legacy 'git reset --hard' emits the 'HEAD is now at ...' which was also included in the builtin rebase\nI saw no reason to keep that message as - with my patch - we have concluded the HEAD must not change.\n\n\nBen...\n\nOn 20-08-2019 11:00, Phillip Wood wrote:\n> Hi Ben\n> \n> I need to have a longer look at this (I don't understand why we're calling reset --hard after we've stashed the changes) but I notice that the test lines you're changing predate the switch to the builtin rebase so those changes are not related to the branch switching problem.\n> \n> Best Wishes\n> \n> Phillip\n> \n> On 18/08/2019 10:53, Ben Wijen wrote:\n>> The rebase --autostash incorrectly moved the current branch to orig_head, where\n>> orig_head -- commit object name of tip of the branch before rebasing\n>>\n>> It seems this was incorrectly taken over from git-legacy-rebase.sh\n>>\n>> Signed-off-by: Ben Wijen \n>> ---\n>>   builtin/rebase.c            | 18 ++++++------------\n>>   t/t3420-rebase-autostash.sh |  4 ----\n>>   2 files changed, 6 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 670096c065..a928f44941 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>>                   state_dir_path(\"autostash\", &options);\n>>               struct child_process stash = CHILD_PROCESS_INIT;\n>>               struct object_id oid;\n>> -            struct commit *head =\n>> -                lookup_commit_reference(the_repository,\n>> -                            &options.orig_head);\n>>                 argv_array_pushl(&stash.args,\n>>                        \"stash\", \"create\", \"autostash\", NULL);\n>> @@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>>                       options.state_dir);\n>>               write_file(autostash, \"%s\", oid_to_hex(&oid));\n>>               printf(_(\"Created autostash: %s\\n\"), buf.buf);\n>> -            if (reset_head(&head->object.oid, \"reset --hard\",\n>> +\n>> +            /*\n>> +             * We might not be on orig_head yet:\n>> +             * Make sure to reset w/o switching branches...\n>> +             */\n>> +            if (reset_head(NULL, \"reset --hard\",\n>>                          NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n>>                   die(_(\"could not reset --hard\"));\n>> -            printf(_(\"HEAD is now at %s\"),\n>> -                   find_unique_abbrev(&head->object.oid,\n>> -                          DEFAULT_ABBREV));\n>> -            strbuf_reset(&buf);\n>> -            pp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n>> -            if (buf.len > 0)\n>> -                printf(\" %s\", buf.buf);\n>> -            putchar('\\n');\n>>                 if (discard_index(the_repository->index) < 0 ||\n>>                   repo_read_index(the_repository) < 0)\n>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n>> index 867e4e0b17..2ea1909881 100755\n>> --- a/t/t3420-rebase-autostash.sh\n>> +++ b/t/t3420-rebase-autostash.sh\n>> @@ -37,7 +37,6 @@ test_expect_success setup '\n>>   create_expected_success_am () {\n>>       cat >expected <<-EOF\n>>       $(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n>> -    HEAD is now at $(git rev-parse --short feature-branch) third commit\n>>       First, rewinding head to replay your work on top of it...\n>>       Applying: second commit\n>>       Applying: third commit\n>> @@ -48,7 +47,6 @@ create_expected_success_am () {\n>>   create_expected_success_interactive () {\n>>       q_to_cr >expected <<-EOF\n>>       $(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n>> -    HEAD is now at $(git rev-parse --short feature-branch) third commit\n>>       Applied autostash.\n>>       Successfully rebased and updated refs/heads/rebased-feature-branch.\n>>       EOF\n>> @@ -57,7 +55,6 @@ create_expected_success_interactive () {\n>>   create_expected_failure_am () {\n>>       cat >expected <<-EOF\n>>       $(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n>> -    HEAD is now at $(git rev-parse --short feature-branch) third commit\n>>       First, rewinding head to replay your work on top of it...\n>>       Applying: second commit\n>>       Applying: third commit\n>> @@ -70,7 +67,6 @@ create_expected_failure_am () {\n>>   create_expected_failure_interactive () {\n>>       cat >expected <<-EOF\n>>       $(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n>> -    HEAD is now at $(git rev-parse --short feature-branch) third commit\n>>       Applying autostash resulted in conflicts.\n>>       Your changes are safe in the stash.\n>>       You can run \"git stash pop\" or \"git stash drop\" at any time.\n>>\n> \n"},{"id":"380852","messageId":"20190820201237.10205-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190820201237.10205-1-ben@wijen.net","subject":"[PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-20T20:12:37Z","receivedAt":"2019-08-20T20:13:58Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Consider the following scenario:\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nHere 'rebase --autostash ' incorrectly moves the\nupstream branch to master.\n\nThe expected behavior: (58794775:/git-rebase.sh:526)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThe actual behavior: (6defce2b:/builtin/rebase.c:1062)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\n\nThis commit reinstates the 'legacy script' behavior as introduced with\n58794775: rebase: implement --[no-]autostash and rebase.autostash\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 13 +++++++++----\n 2 files changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..a928f44941 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\n+\t\t\t/*\n+\t\t\t * We might not be on orig_head yet:\n+\t\t\t * Make sure to reset w/o switching branches...\n+\t\t\t */\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..c26b4b0885 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n@@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change upstream branch' '\n+\ttest_when_finished \"git reset --hard && git branch -D upstream\" &&\n+\tgit checkout -b upstream unrelated-onto-branch &&\n+\techo changed >file0 &&\n+\tgit add file0 &&\n+\tgit rebase --autostash upstream feature-branch &&\n+\ttest_cmp_rev upstream unrelated-onto-branch\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"380853","messageId":"20190820201237.10205-1-ben@wijen.net","threadId":"51663","inReplyTo":"20190818095349.3218-3-ben@wijen.net","subject":"[PATCH v2 0/1] git rebase: Make sure upstream branch is left alone.","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-20T20:12:36Z","receivedAt":"2019-08-20T20:13:58Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi Phillip, Junio,\n\nThank you for taking the time to look into this.\nWith this new patch I think I've addressed all your concerns.\n\n\nBen Wijen (1):\n rebase.c: make sure current branch isn't moved when autostashing\n\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 13 +++++++++----\n 2 files changed, 15 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"380854","messageId":"CAPig+cQG0HDDBz-o=-7STos6LwYX377oFiBGBiDY+P27Q7ua-A@mail.gmail.com","threadId":"51663","inReplyTo":"20190820201237.10205-2-ben@wijen.net","subject":"Re: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-20T20:24:25Z","receivedAt":"2019-08-20T20:25:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 20, 2019 at 4:12 PM Ben Wijen wrote:\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' '\n> +test_expect_success 'never change upstream branch' '\n> + test_when_finished \"git reset --hard && git branch -D upstream\" &&\n> + git checkout -b upstream unrelated-onto-branch &&\n> + echo changed >file0 &&\n> + git add file0 &&\n> + git rebase --autostash upstream feature-branch &&\n> + test_cmp_rev upstream unrelated-onto-branch\n> +'\n\nTo be friendly to tests added after this one in the future, follow the\nexample of other tests in this script by ensuring that the test\nswitches back to the branch which was active prior to starting this\ntest. For instance:\n\n test_expect_success 'never change upstream branch' '\n git checkout -b upstream unrelated-onto-branch &&\n test_when_finished \"git reset --hard &&\n git checkout - &&\n git branch -D upstream\" &&\n echo changed >file0 &&\n git add file0 &&\n git rebase --autostash upstream feature-branch &&\n test_cmp_rev upstream unrelated-onto-branch\n '\n\n(Don't actually copy/paste the code from the other tests, since most\nof them switch back to the previous branch incorrectly by using \"git\ncheckout \" as the last line of the test. That won't restore\nthe branch correctly if the test fails before it reaches that line;\ninstead, test_when_finished() should be used to switch back to the\noriginal branch.)\n"},{"id":"380858","messageId":"xmqqlfvntuz4.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"20190820201237.10205-2-ben@wijen.net","subject":"Re: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T20:58:23Z","receivedAt":"2019-08-20T20:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Wijen writes:\n\n> Consider the following scenario:\n> git checkout not-the-master\n> work work work\n> git rebase --autostash upstream master\n>\n> Here 'rebase --autostash ' incorrectly moves the\n> upstream branch to master.\n>\n> The expected behavior: (58794775:/git-rebase.sh:526)\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH\n>\n> The actual behavior: (6defce2b:/builtin/rebase.c:1062)\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard master\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH\n\nIn the scenario at the top, the branch that is checked out while you\nare working is \"not-the-master\" branch, and you run the rebase\ncommand. If we follow the \"actual behaviour\" in our head, after\nstashing away the local change, the tip of the current branch\n(i.e. not-the-master) is reset to the same commit as the tip of\n'master'.\n\nBut earlier, you said, \"incorrectlly moves the upstream branch\".\n\nIt looks like either one of the use of branches in the \"scenario\",\nor the problem statement, is incorrect.\n\nThe reason why \"HEAD is...\" comments are all gone (as shown in the\ntest) is not explained well in the proposed commit log message,\neither. I think the change is correct (i.e. we were moving HEAD\nincorrectly, and the messages were given incorrectly, and we are\nfixing this behaviour hence there is no longer any need to say we\nare moving the HEAD anymore), but there should be some mention of\nthe change, I would think.\n\nThanks.\n\n> create_expected_failure_am () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tFirst, rewinding head to replay your work on top of it...\n> \tApplying: second commit\n> \tApplying: third commit\n> @@ -70,7 +67,6 @@ create_expected_failure_am () {\n> create_expected_failure_interactive () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tApplying autostash resulted in conflicts.\n> \tYour changes are safe in the stash.\n> \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n> @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' '\n> \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n> '\n> \n> +test_expect_success 'never change upstream branch' '\n> +\ttest_when_finished \"git reset --hard && git branch -D upstream\" &&\n> +\tgit checkout -b upstream unrelated-onto-branch &&\n> +\techo changed >file0 &&\n> +\tgit add file0 &&\n> +\tgit rebase --autostash upstream feature-branch &&\n> +\ttest_cmp_rev upstream unrelated-onto-branch\n> +'\n> +\n> test_done\n"},{"id":"380900","messageId":"20190821182941.12674-1-ben@wijen.net","threadId":"51663","inReplyTo":"20190820201237.10205-1-ben@wijen.net","subject":"[PATCH v3 0/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-21T18:29:40Z","receivedAt":"2019-08-21T18:29:54Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi,\n\nI have done some more tests on what's actually going on.\nThe active branch is currently reset to master (before the rebase)\nThe confusion was because of me naming the active branch 'upstream'\n\nI hope this clears things up...\n\n\nBen Wijen (1):\n rebase.c: make sure the active branch isn't moved when autostashing\n\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 16 ++++++++++++----\n 2 files changed, 18 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"380901","messageId":"20190821182941.12674-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190821182941.12674-1-ben@wijen.net","subject":"[PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-21T18:29:41Z","receivedAt":"2019-08-21T18:30:07Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Consider the following scenario:\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nHere 'rebase --autostash ' incorrectly moves the\nactive branch (not-the-master) to master (before the rebase).\n\nThe expected behavior: (58794775:/git-rebase.sh:526)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThe actual behavior: (6defce2b:/builtin/rebase.c:1062)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\n\nThis commit reinstates the 'legacy script' behavior as introduced with\n58794775: rebase: implement --[no-]autostash and rebase.autostash\n\nAs with this commit the reset must never change the active branch,\nthe 'HEAD is now at ...' message has now been removed.\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 16 ++++++++++++----\n 2 files changed, 18 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..a928f44941 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\n+\t\t\t/*\n+\t\t\t * We might not be on orig_head yet:\n+\t\t\t * Make sure to reset w/o switching branches...\n+\t\t\t */\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..d1352096f2 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n@@ -306,4 +302,16 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change active branch' '\n+\tgit checkout -b upstream unrelated-onto-branch &&\n+\ttest_when_finished \"\n+\t\tgit reset --hard &&\n+\t\tgit checkout - &&\n+\t\tgit branch -D upstream\" &&\n+\techo changed >file0 &&\n+\tgit add file0 &&\n+\tgit rebase --autostash upstream feature-branch &&\n+\ttest_cmp_rev upstream unrelated-onto-branch\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"380925","messageId":"nycvar.QRO.7.76.6.1908221418540.46@tvgsbejvaqbjf.bet","threadId":"51663","inReplyTo":"20190821182941.12674-2-ben@wijen.net","subject":"Re: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-22T12:27:29Z","receivedAt":"2019-08-22T12:28:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ben,\n\nOn Wed, 21 Aug 2019, Ben Wijen wrote:\n\n> Consider the following scenario:\n> git checkout not-the-master\n> work work work\n> git rebase --autostash upstream master\n>\n> Here 'rebase --autostash ' incorrectly moves the\n> active branch (not-the-master) to master (before the rebase).\n>\n> The expected behavior: (58794775:/git-rebase.sh:526)\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH\n>\n> The actual behavior: (6defce2b:/builtin/rebase.c:1062)\n> AUTOSTASH=$(git stash create autostash)\n> git reset --hard master\n> git checkout master\n> git rebase upstream\n> git stash apply $AUTOSTASH\n\nInteresting. So the only difference is that the original rebase called\n`git reset --hard` on the current HEAD, while the new behavior tries to\nreset to the branch to which the user wants to switch, right away.\n\nI can see that this would lead to possible problems e.g. if a file had\nbeen added between master and the current branch.\n\n> This commit reinstates the 'legacy script' behavior as introduced with\n> 58794775: rebase: implement --[no-]autostash and rebase.autostash\n>\n> As with this commit the reset must never change the active branch,\n> the 'HEAD is now at ...' message has now been removed.\n\nActually, I am not so sure that I like this change.\n\nPreviously, users had a chance to figure out to which revision the\nworktree was reset, before switching the branch (because switching the\nbranch we _do_, via `git checkout master`).\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 670096c065..a928f44941 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstate_dir_path(\"autostash\", &options);\n> \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n> \t\t\tstruct object_id oid;\n> -\t\t\tstruct commit *head =\n> -\t\t\t\tlookup_commit_reference(the_repository,\n> -\t\t\t\t\t\t\t&options.orig_head);\n\nThis should probably be changed to look up `HEAD` instead, then, so that\nwe can keep the message.\n\nI.e. you probably want to use `get_oid(\"HEAD\", &head_oid)` instead.\n\n> \t\t\targv_array_pushl(&stash.args,\n> \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n> @@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\t options.state_dir);\n> \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n> \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n> -\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n> +\n> +\t\t\t/*\n> +\t\t\t * We might not be on orig_head yet:\n> +\t\t\t * Make sure to reset w/o switching branches...\n> +\t\t\t */\n> +\t\t\tif (reset_head(NULL, \"reset --hard\",\n> \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n> \t\t\t\tdie(_(\"could not reset --hard\"));\n> -\t\t\tprintf(_(\"HEAD is now at %s\"),\n> -\t\t\t find_unique_abbrev(&head->object.oid,\n> -\t\t\t\t\t\t DEFAULT_ABBREV));\n> -\t\t\tstrbuf_reset(&buf);\n> -\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n> -\t\t\tif (buf.len > 0)\n> -\t\t\t\tprintf(\" %s\", buf.buf);\n> -\t\t\tputchar('\\n');\n>\n> \t\t\tif (discard_index(the_repository->index) < 0 ||\n> \t\t\t\trepo_read_index(the_repository) < 0)\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index b8f4d03467..d1352096f2 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -37,7 +37,6 @@ test_expect_success setup '\n> create_expected_success_am () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tFirst, rewinding head to replay your work on top of it...\n> \tApplying: second commit\n> \tApplying: third commit\n> @@ -48,7 +47,6 @@ create_expected_success_am () {\n> create_expected_success_interactive () {\n> \tq_to_cr >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tApplied autostash.\n> \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n> \tEOF\n> @@ -57,7 +55,6 @@ create_expected_success_interactive () {\n> create_expected_failure_am () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tFirst, rewinding head to replay your work on top of it...\n> \tApplying: second commit\n> \tApplying: third commit\n> @@ -70,7 +67,6 @@ create_expected_failure_am () {\n> create_expected_failure_interactive () {\n> \tcat >expected <<-EOF\n> \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n> -\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n> \tApplying autostash resulted in conflicts.\n> \tYour changes are safe in the stash.\n> \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n> @@ -306,4 +302,16 @@ test_expect_success 'branch is left alone when possible' '\n> \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n> '\n>\n> +test_expect_success 'never change active branch' '\n> +\tgit checkout -b upstream unrelated-onto-branch &&\n> +\ttest_when_finished \"\n> +\t\tgit reset --hard &&\n> +\t\tgit checkout - &&\n> +\t\tgit branch -D upstream\" &&\n\nI feel like we want to have a more meaningful branch name than\n\"upstream\", and then we can get away with leaving it in place, i.e. just\n\n\ttest_when_finished \"git reset --hard && git checkout -\" &&\n\n> +\techo changed >file0 &&\n> +\tgit add file0 &&\n\nSince `file0` is already tracked, I think that this `git add` invocation\nonly distracts from the essence of this test case.\n\n> +\tgit rebase --autostash upstream feature-branch &&\n> +\ttest_cmp_rev upstream unrelated-onto-branch\n\nOtherwise: well done! And thank you so much for fixing this.\n\nCiao,\nDscho\n\n> +'\n> +\n> test_done\n> --\n> 2.22.0\n>\n>\n"},{"id":"380939","messageId":"xmqqftltqjy1.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"nycvar.QRO.7.76.6.1908221418540.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-22T15:49:26Z","receivedAt":"2019-08-22T15:49:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> As with this commit the reset must never change the active branch,\n>> the 'HEAD is now at ...' message has now been removed.\n>\n> Actually, I am not so sure that I like this change.\n>\n> Previously, users had a chance to figure out to which revision the\n> worktree was reset, before switching the branch (because switching the\n> branch we _do_, via `git checkout master`).\n\nHmph, that only happens when --autostash is in effect and actually\nhad created a stash, no? If your working tree is clean, or if you\ndid not pass --autostash, \"HEAD is now at ...\" is not reported.\nI am not sure why that particular piece of information is only\nuseful in the case we actually created a stash and unnecessary if we\ndid not create a stash.\n\nWhen we do not create a stash, the output starts from \"First,\nrewinding head to replay your work on top of it...\", which sort of\ngives a warm and fuzzy impression that it is reporting what it is\ndoing, but without giving the most useful information (i.e. what\n\"it\" refers to).\n\nBecause I am all for preserving the existing behaviour as much as\npossible when fixing real bugs, I would not strongly object to your\nidea of resurrecting the message. But I am not sure if the existing\nmessage was all that useful in the first place. I'd rather see\nthese messages that were only emitted when --autostash was given\nremoved first (like this patch does), and then the \"First rewinding...\"\nmessage reworded to show where we rebuilt the history on top of.\n\nOther than that, thanks for a good review, and thanks Ben for\nworking on this.\n"},{"id":"381210","messageId":"20190826164513.9102-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190826164513.9102-1-ben@wijen.net","subject":"[PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-26T16:45:13Z","receivedAt":"2019-08-26T16:45:26Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Consider the following scenario:\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nHere 'rebase --autostash ' incorrectly moves the\nactive branch (not-the-master) to master (before the rebase).\n\nThe expected behavior: (58794775:/git-rebase.sh:526)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThe actual behavior: (6defce2b:/builtin/rebase.c:1062)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\n\nThis commit reinstates the 'legacy script' behavior as introduced with\n58794775: rebase: implement --[no-]autostash and rebase.autostash\n\nAs with this commit the reset must never change the active branch,\nthe 'HEAD is now at ...' message has now been removed.\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 12 ++++++++----\n 2 files changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..a928f44941 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\n+\t\t\t/*\n+\t\t\t * We might not be on orig_head yet:\n+\t\t\t * Make sure to reset w/o switching branches...\n+\t\t\t */\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..2421bc39f5 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n@@ -306,4 +302,12 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change active branch' '\n+\tgit checkout -b not-the-feature-branch unrelated-onto-branch &&\n+\ttest_when_finished \"git reset --hard && git checkout -\" &&\n+\techo changed >file0 &&\n+\tgit rebase --autostash not-the-feature-branch feature-branch &&\n+\ttest_cmp_rev not-the-feature-branch unrelated-onto-branch\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"381211","messageId":"20190826164513.9102-1-ben@wijen.net","threadId":"51663","inReplyTo":"20190821182941.12674-1-ben@wijen.net","subject":"[PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-26T16:45:12Z","receivedAt":"2019-08-26T16:45:41Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Dscho's review got me thinking about the rationale behind the 'HEAD is now at...'\nmessage.\n\nA 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is\nnot, we must do it ourselves. I guess the legacy implementation could have been\n'reset --hard -q' which would have also prevented the 'HEAD is now at...' message.\n\nOfcourse I'm happy to reinstate the message, but I'm convinced it doesn't add\ninformation, as with this commit the original branch is no longer moved and \n- as before - the autostash is re-applied after the rebase, leaving nothing\nto be guessed about.\n\n\nThank you,\n\nBen Wijen (1):\n rebase.c: make sure the active branch isn't moved when autostashing\n\n builtin/rebase.c | 18 ++++++------------\n t/t3420-rebase-autostash.sh | 12 ++++++++----\n 2 files changed, 14 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"381218","messageId":"20190826171024.GR20404@szeder.dev","threadId":"51663","inReplyTo":"20190826164513.9102-2-ben@wijen.net","subject":"Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-26T17:10:24Z","receivedAt":"2019-08-26T17:10:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 26, 2019 at 06:45:13PM +0200, Ben Wijen wrote:\n> +test_expect_success 'never change active branch' '\n> +\tgit checkout -b not-the-feature-branch unrelated-onto-branch &&\n> +\ttest_when_finished \"git reset --hard && git checkout -\" &&\n\nI think it would be safer to explicitly spell out the branch that\nshould be checked out at the end than to rely on 'git checkout -'\nalways being able to figure that out, even in case of a breakage.\n\n> +\techo changed >file0 &&\n> +\tgit rebase --autostash not-the-feature-branch feature-branch &&\n> +\ttest_cmp_rev not-the-feature-branch unrelated-onto-branch\n> +'\n> +\n> test_done\n> -- \n> 2.22.0\n> \n"},{"id":"381438","messageId":"nycvar.QRO.7.76.6.1908281454070.46@tvgsbejvaqbjf.bet","threadId":"51663","inReplyTo":"20190826164513.9102-1-ben@wijen.net","subject":"Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-28T12:56:03Z","receivedAt":"2019-08-28T12:56:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ben,\n\nOn Mon, 26 Aug 2019, Ben Wijen wrote:\n\n> Dscho's review got me thinking about the rationale behind the 'HEAD is now at...'\n> message.\n>\n> A 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is\n> not, we must do it ourselves. I guess the legacy implementation could have been\n> 'reset --hard -q' which would have also prevented the 'HEAD is now at...' message.\n>\n> Ofcourse I'm happy to reinstate the message, but I'm convinced it doesn't add\n> information, as with this commit the original branch is no longer moved and\n> - as before - the autostash is re-applied after the rebase, leaving nothing\n> to be guessed about.\n\nFWIW I disagree with the decision to mingle a bug fix with a change of\nbehavior. Resetting to the correct OID is of course the bug fix.\nDropping the message is a change of behavior.\n\nI would be a lot more comfortable with a bug fix that did *not* change\nthe behavior, fast-tracking that to even maintenance branches.\n\nAnd leaving the behavior change to cook in `next` for a while.\n\nOf course, I am not Git's maintainer, if I were, I would insist on this\nmore careful approach.\n\nCiao,\nJohannes\n"},{"id":"381450","messageId":"xmqqh8611eza.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"nycvar.QRO.7.76.6.1908281454070.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-28T15:34:01Z","receivedAt":"2019-08-28T15:34:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> FWIW I disagree with the decision to mingle a bug fix with a change of\n> behavior. Resetting to the correct OID is of course the bug fix.\n> Dropping the message is a change of behavior.\n\nIn general I strongly advocate that a patch should fix one thing and\none thing well without breaking other things, so we are on the same\npage.\n\nAs I said in , I think\nthe message that is leaked from \"reset --hard\" was reporting an\nincorrect thing, iow, showing the message itself is another bug.\n\nIIUC, the bug is twofold:\n\n - When --autostash creats a stash entry, the command attempts to\n reset the working tree and the tip of the current branch to where\n it should be (i.e. HEAD). As we know, this attempt is faulty and\n resets to a wrong commit, not to HEAD. This is the primary bug\n the patch under discussion fixes.\n\n - A message is given only when the above happens. When rebasing\n from a clean working tree, we do not report \"HEAD is now at...\"\n at all. And when autostash happens, the message is still not\n correct even after fixing to which commit we reset to. \"HEAD is\n now at ...\" is misleading in that it implies that we changed to\n something else, but in reality, we have been on the same commit\n all the time since the command started, created a stash and wiped\n the working tree clean after doing so, when the message is given.\n That \"reset --hard\" is done only to clean the index and the\n working tree and talking about \"HEAD is now...\" is a bug in its\n context.\n\nSo, from the purist point of view, I see it may make sense to update\nthis patch to add logic to give a pointless and misleading \"HEAD is\nnow at...\" message so that we will report the location of HEAD where\nthe \"rebase --autostash\" command started at, to fix only the first\nbug. We still need a follow up patch that removes the message to\nfix the other bug, perhaps with a follow-up to update the \"first\nrewinding...\" message, which is shown whether autostash kicks in or\nnot, so that it reports which commit we are rebuilding the history.\n\nThans.\n"},{"id":"381456","messageId":"xmqq8srd1dld.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"xmqqh8611eza.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-28T16:03:58Z","receivedAt":"2019-08-28T16:04:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> IIUC, the bug is twofold:\n> ...\n> - A message is given only when the above happens. When rebasing\n> ...\n> That \"reset --hard\" is done only to clean the index and the\n> working tree and talking about \"HEAD is now...\" is a bug in its\n> context.\n\nActually, this \"latter\" bug can further be split into two\n\n * The \"HEAD is now\" is given only when autostash feature needs to\n clean the working tree, and we have never moved HEAD anyway.\n\n * The message does not indicate what we are rebuilding on top of.\n\nand dealt with separately, so with that in mind the step that would\nfollow the first patch, i.e.\n\n> ... update\n> this patch to add logic to give a pointless and misleading \"HEAD is\n> now at...\" message so that we will report the location of HEAD where\n> the \"rebase --autostash\" command started at, to fix only the first\n> bug.\n\nmay become different. The fact that the \"HEAD is now...\" is given\nonly when autostash actually happens _might_ be taken as a feature\nby some users---the location of HEAD reported by the message is\nirrelevant to them (we know that as a fact---we have been reporting\na wrong commit all along anyway), but the single-bit \"we got a\nmessage\" is a signal that \"--autostash\" had something valuable to\nsave.\n\nSo the second step may be to replace the \"HEAD is now...\" message we\nadd back (relative to Ben's patch under discussion) to the first\npatch with a more direct \"stashed away your local changes\" message\n(perhaps with diffstat??? I do not care about the details, as we\nare talking about resurrecting one single useful bit of information\nand extending it futher is beyond the scope of this analysis).\n\nAnd the last point, i.e. \"First, rewinding head to replay your\nwork...\" does not give enough information to be truly useful, is a\ntotally separate bug (that Ben's patch does not even mention or\nattempt to address), so we can leave it out of this analysis, too.\n\nSo, yeah, if we are to spend extra effort to polish Ben's patch\nfurther while keeping the \"fix things without making unnecessary\nchanges\", I think the approach that takes least amount of effort may\nnot to make the code manually say \"Head is at ...\", but to add a new\nmessage to report that autostash happened. That fixes two bugs\n(i.e. the original bug, and the \"we autostashed\" bit is reported in\na roundabout and misleading way via \"HEAD is now at ...\") in a\nsingle patch ;-)\n\n"},{"id":"381518","messageId":"20190829164757.7301-1-ben@wijen.net","threadId":"51663","inReplyTo":"20190826164513.9102-1-ben@wijen.net","subject":"[PATCH v5 0/2] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-29T16:47:55Z","receivedAt":"2019-08-29T16:48:09Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Here are my \"fix things without making unnecessary changes\"\n\n\nBen Wijen (2):\n builtin/rebase.c: make sure the active branch isn't moved when\n autostashing\n builtin/rebase.c: Remove obsolete message\n\n builtin/rebase.c | 13 +------------\n t/t3420-rebase-autostash.sh | 12 ++++++++----\n 2 files changed, 9 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"381519","messageId":"20190829164757.7301-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190829164757.7301-1-ben@wijen.net","subject":"[PATCH v5 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-29T16:47:56Z","receivedAt":"2019-08-29T16:48:21Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Consider the following scenario:\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nHere 'rebase --autostash ' incorrectly moves the\nactive branch (not-the-master) to master (before the rebase).\n\nThe expected behavior: (58794775:/git-rebase.sh:526)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThe actual behavior: (6defce2b:/builtin/rebase.c:1062)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThis commit reinstates the 'legacy script' behavior as introduced with\n58794775: rebase: implement --[no-]autostash and rebase.autostash\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 8 ++++++--\n t/t3420-rebase-autostash.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..b3b17669e3 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n+\t\t\tstruct object_id head_oid;\n+\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n+\t\t\t\tret = error(_(\"could not determine HEAD revision\"));\n+\t\t\t}\n+\n \t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n+\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..43685a5c8e 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -306,4 +306,12 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change active branch' '\n+\tgit checkout -b not-the-feature-branch unrelated-onto-branch &&\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\techo changed >file0 &&\n+\tgit rebase --autostash not-the-feature-branch feature-branch &&\n+\ttest_cmp_rev not-the-feature-branch unrelated-onto-branch\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"381520","messageId":"20190829164757.7301-3-ben@wijen.net","threadId":"51663","inReplyTo":"20190829164757.7301-1-ben@wijen.net","subject":"[PATCH v5 2/2] builtin/rebase.c: Remove pointless message","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-29T16:47:57Z","receivedAt":"2019-08-29T16:48:34Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When doing 'git rebase --autostash ' with a dirty worktree\na 'HEAD is now at ...' message is emitted, which is pointless as it refers to\nthe old active branch which isn't actually moved.\n\nThis commit removes the 'HEAD is now at...' message.\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 17 +----------------\n t/t3420-rebase-autostash.sh | 4 ----\n 2 files changed, 1 insertion(+), 20 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b3b17669e3..118205e481 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,13 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct object_id head_oid;\n-\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\t\tret = error(_(\"could not determine HEAD revision\"));\n-\t\t\t}\n-\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1995,17 +1988,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 43685a5c8e..2421bc39f5 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-- \n2.22.0\n\n"},{"id":"381582","messageId":"20190830151607.4208-1-ben@wijen.net","threadId":"51663","inReplyTo":"20190829164757.7301-1-ben@wijen.net","subject":"[PATCH v6 0/2] rebase.c: make sure current branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-30T15:16:04Z","receivedAt":"2019-08-30T15:16:20Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Here are my \"fix things without making unnecessary changes\"\nFixing a copy-paste fault which I missed in v5...\n\n\nBen Wijen (2):\n builtin/rebase.c: make sure the active branch isn't moved when\n autostashing\n builtin/rebase.c: Remove obsolete message\n\n builtin/rebase.c | 13 +------------\n t/t3420-rebase-autostash.sh | 12 ++++++++----\n 2 files changed, 9 insertions(+), 16 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"381583","messageId":"20190830151607.4208-2-ben@wijen.net","threadId":"51663","inReplyTo":"20190830151607.4208-1-ben@wijen.net","subject":"[PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-30T15:16:05Z","receivedAt":"2019-08-30T15:16:33Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Consider the following scenario:\n git checkout not-the-master\n work work work\n git rebase --autostash upstream master\n\nHere 'rebase --autostash ' incorrectly moves the\nactive branch (not-the-master) to master (before the rebase).\n\nThe expected behavior: (58794775:/git-rebase.sh:526)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThe actual behavior: (6defce2b:/builtin/rebase.c:1062)\n AUTOSTASH=$(git stash create autostash)\n git reset --hard master\n git checkout master\n git rebase upstream\n git stash apply $AUTOSTASH\n\nThis commit reinstates the 'legacy script' behavior as introduced with\n58794775: rebase: implement --[no-]autostash and rebase.autostash\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 8 ++++++--\n t/t3420-rebase-autostash.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 670096c065..abcbfb8f01 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,9 +1968,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n+\t\t\tstruct object_id head_oid;\n+\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n+\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n+\t\t\t}\n+\n \t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t&options.orig_head);\n+\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b8f4d03467..1131e0016a 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -306,4 +306,12 @@ test_expect_success 'branch is left alone when possible' '\n \ttest unchanged-branch = \"$(git rev-parse --abbrev-ref HEAD)\"\n '\n \n+test_expect_success 'never change active branch' '\n+\tgit checkout -b not-the-feature-branch unrelated-onto-branch &&\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\techo changed >file0 &&\n+\tgit rebase --autostash not-the-feature-branch feature-branch &&\n+\ttest_cmp_rev not-the-feature-branch unrelated-onto-branch\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"381584","messageId":"20190830151607.4208-4-ben@wijen.net","threadId":"51663","inReplyTo":"20190830151607.4208-1-ben@wijen.net","subject":"[PATCH 2/2] builtin/rebase.c: Remove pointless message","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-30T15:16:07Z","receivedAt":"2019-08-30T15:16:47Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When doing 'git rebase --autostash ' with a dirty worktree\na 'HEAD is now at ...' message is emitted, which is pointless as it refers to\nthe old active branch which isn't actually moved.\n\nThis commit removes the 'HEAD is now at...' message.\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 17 +----------------\n t/t3420-rebase-autostash.sh | 4 ----\n 2 files changed, 1 insertion(+), 20 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex abcbfb8f01..118205e481 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,13 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct object_id head_oid;\n-\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n-\t\t\t}\n-\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1995,17 +1988,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 1131e0016a..5f7e73cf83 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-- \n2.22.0\n\n"},{"id":"381585","messageId":"20190830151607.4208-3-ben@wijen.net","threadId":"51663","inReplyTo":"20190830151607.4208-1-ben@wijen.net","subject":"[PATCH v6 2/2] builtin/rebase.c: Remove pointless message","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2019-08-30T15:16:06Z","receivedAt":"2019-08-30T15:17:01Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When doing 'git rebase --autostash ' with a dirty worktree\na 'HEAD is now at ...' message is emitted, which is pointless as it refers to\nthe old active branch which isn't actually moved.\n\nThis commit removes the 'HEAD is now at...' message.\n\nSigned-off-by: Ben Wijen \n---\n builtin/rebase.c | 17 +----------------\n t/t3420-rebase-autostash.sh | 4 ----\n 2 files changed, 1 insertion(+), 20 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b3b17669e3..118205e481 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,13 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct object_id head_oid;\n-\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\t\tret = error(_(\"could not determine HEAD revision\"));\n-\t\t\t}\n-\n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1995,17 +1988,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 43685a5c8e..2421bc39f5 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -37,7 +37,6 @@ test_expect_success setup '\n create_expected_success_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -48,7 +47,6 @@ create_expected_success_am () {\n create_expected_success_interactive () {\n \tq_to_cr >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplied autostash.\n \tSuccessfully rebased and updated refs/heads/rebased-feature-branch.\n \tEOF\n@@ -57,7 +55,6 @@ create_expected_success_interactive () {\n create_expected_failure_am () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tFirst, rewinding head to replay your work on top of it...\n \tApplying: second commit\n \tApplying: third commit\n@@ -70,7 +67,6 @@ create_expected_failure_am () {\n create_expected_failure_interactive () {\n \tcat >expected <<-EOF\n \t$(grep \"^Created autostash: [0-9a-f][0-9a-f]*\\$\" actual)\n-\tHEAD is now at $(git rev-parse --short feature-branch) third commit\n \tApplying autostash resulted in conflicts.\n \tYour changes are safe in the stash.\n \tYou can run \"git stash pop\" or \"git stash drop\" at any time.\n-- \n2.22.0\n\n"},{"id":"381612","messageId":"xmqqsgpiwgts.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"20190830151607.4208-2-ben@wijen.net","subject":"Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T20:15:11Z","receivedAt":"2019-08-30T20:15:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Wijen writes:\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 670096c065..abcbfb8f01 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1968,9 +1968,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstate_dir_path(\"autostash\", &options);\n> \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n> \t\t\tstruct object_id oid;\n> +\t\t\tstruct object_id head_oid;\n> +\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n> +\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n> +\t\t\t}\n\nPointless {} pair around a single statement.\n\n> +\n> \t\t\tstruct commit *head =\n> -\t\t\t\tlookup_commit_reference(the_repository,\n> -\t\t\t\t\t\t\t&options.orig_head);\n> +\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n\nThis introduces decl-after-statement error, doesn't it?\n\nPerhaps like so...\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex abcbfb8f01..0a2f9273ee 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1969,12 +1969,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n \t\t\tstruct object_id head_oid;\n-\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n-\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n-\t\t\t}\n+\t\t\tstruct commit *head;\n \n-\t\t\tstruct commit *head =\n-\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n+\t\t\tif (get_oid(\"HEAD\", &head_oid))\n+\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n+\t\t\thead = lookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n"},{"id":"381614","messageId":"xmqqimqewgrd.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"20190830151607.4208-3-ben@wijen.net","subject":"Re: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T20:16:38Z","receivedAt":"2019-08-30T20:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Wijen writes:\n\n> @@ -1968,13 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstate_dir_path(\"autostash\", &options);\n> \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n> \t\t\tstruct object_id oid;\n> -\t\t\tstruct object_id head_oid;\n> -\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n> -\t\t\t\tret = error(_(\"could not determine HEAD revision\"));\n\nI think we saw die() in the previous one. This patch would not\napply on top of the result of applying 1/2.\n\nI'll tentatively queue this instead on top of the corrected 1/2.\n\nThanks.\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0a2f9273ee..118205e481 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1968,12 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\tstate_dir_path(\"autostash\", &options);\n \t\t\tstruct child_process stash = CHILD_PROCESS_INIT;\n \t\t\tstruct object_id oid;\n-\t\t\tstruct object_id head_oid;\n-\t\t\tstruct commit *head;\n-\n-\t\t\tif (get_oid(\"HEAD\", &head_oid))\n-\t\t\t\tdie(_(\"could not determine HEAD revision\"));\n-\t\t\thead = lookup_commit_reference(the_repository, &head_oid);\n \n \t\t\targv_array_pushl(&stash.args,\n \t\t\t\t\t \"stash\", \"create\", \"autostash\", NULL);\n@@ -1994,17 +1988,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t options.state_dir);\n \t\t\twrite_file(autostash, \"%s\", oid_to_hex(&oid));\n \t\t\tprintf(_(\"Created autostash: %s\\n\"), buf.buf);\n-\t\t\tif (reset_head(&head->object.oid, \"reset --hard\",\n+\t\t\tif (reset_head(NULL, \"reset --hard\",\n \t\t\t\t NULL, RESET_HEAD_HARD, NULL, NULL) < 0)\n \t\t\t\tdie(_(\"could not reset --hard\"));\n-\t\t\tprintf(_(\"HEAD is now at %s\"),\n-\t\t\t find_unique_abbrev(&head->object.oid,\n-\t\t\t\t\t\t DEFAULT_ABBREV));\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tpp_commit_easy(CMIT_FMT_ONELINE, head, &buf);\n-\t\t\tif (buf.len > 0)\n-\t\t\t\tprintf(\" %s\", buf.buf);\n-\t\t\tputchar('\\n');\n \n \t\t\tif (discard_index(the_repository->index) < 0 ||\n \t\t\t\trepo_read_index(the_repository) < 0)\n"},{"id":"381619","messageId":"bf659b1d-b9a4-cb9e-e660-fa743b4db84c@wijen.net","threadId":"51663","inReplyTo":"xmqqsgpiwgts.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2019-08-31T07:17:09Z","receivedAt":"2019-08-31T07:17:20Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"\n\nOn 30-08-2019 22:15, Junio C Hamano wrote:\n> Ben Wijen writes:\n> \n>> +\n>> \t\t\tstruct commit *head =\n>> -\t\t\t\tlookup_commit_reference(the_repository,\n>> -\t\t\t\t\t\t\t&options.orig_head);\n>> +\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n> \n> This introduces decl-after-statement error, doesn't it?\n> \n> Perhaps like so...\n\nWould you like me to send in another patch or leave it like this?\n\n"},{"id":"381620","messageId":"d5a70a17-9b4c-f060-5b4c-1e0bdb594390@wijen.net","threadId":"51663","inReplyTo":"xmqqimqewgrd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2019-08-31T07:17:18Z","receivedAt":"2019-08-31T07:17:29Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi Junio,\n\nOn 30-08-2019 22:16, Junio C Hamano wrote:\n> Ben Wijen writes:\n> \n>> -\t\t\tstruct object_id head_oid;\n>> -\t\t\tif (get_oid(\"HEAD\", &head_oid)) {\n>> -\t\t\t\tret = error(_(\"could not determine HEAD revision\"));\n> \n> I think we saw die() in the previous one. This patch would not\n> apply on top of the result of applying 1/2.\n\nYes, my fault, sorry about that...\n\n> \n> I'll tentatively queue this instead on top of the corrected 1/2.\n\nYour patch is indeed correct.\n\nThank you!\n\n> \n> Thanks.\n> \n"},{"id":"381631","messageId":"xmqqr250uhtr.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"bf659b1d-b9a4-cb9e-e660-fa743b4db84c@wijen.net","subject":"Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-01T16:01:04Z","receivedAt":"2019-09-01T16:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben writes:\n\n> On 30-08-2019 22:15, Junio C Hamano wrote:\n>> Ben Wijen writes:\n>> \n>>> +\n>>> \t\t\tstruct commit *head =\n>>> -\t\t\t\tlookup_commit_reference(the_repository,\n>>> -\t\t\t\t\t\t\t&options.orig_head);\n>>> +\t\t\t\tlookup_commit_reference(the_repository, &head_oid);\n>> \n>> This introduces decl-after-statement error, doesn't it?\n>> \n>> Perhaps like so...\n>\n> Would you like me to send in another patch or leave it like this?\n\nAs long as you make it clear that you are 100% happy with the\nfixed-up result that appeared in 'pu', there is no need to resend\n(if you want to make any other changes, I do want to avoid me\nscrewing up by listening to you and hand applying those changes; I'd\nrather want updated patch(es) be sent in such a case).\n\nThanks.\n"},{"id":"381633","messageId":"57de794a-52a7-2d00-8267-127317ac9bee@wijen.net","threadId":"51663","inReplyTo":"xmqqr250uhtr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2019-09-01T16:27:29Z","receivedAt":"2019-09-01T16:27:37Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"\n\nOn 01-09-2019 18:01, Junio C Hamano wrote:\n> Ben writes:\n> \n>>\n>> Would you like me to send in another patch or leave it like this?\n> \n> As long as you make it clear that you are 100% happy with the\n> fixed-up result that appeared in 'pu', there is no need to resend\n> (if you want to make any other changes, I do want to avoid me\n> screwing up by listening to you and hand applying those changes; I'd\n> rather want updated patch(es) be sent in such a case).\n> \n\nHi Junio,\n\nI am 100% happy with with your fixed-up result.\nI have no (planned) updates ATM.\n\n\nThank you all for the thorough reviews.\n\nBen...\n\n> Thanks.\n> \n"},{"id":"381661","messageId":"xmqqef0yvc02.fsf@gitster-ct.c.googlers.com","threadId":"51663","inReplyTo":"57de794a-52a7-2d00-8267-127317ac9bee@wijen.net","subject":"Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-02T17:33:49Z","receivedAt":"2019-09-02T17:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben writes:\n\n>> As long as you make it clear that you are 100% happy with the\n>> fixed-up result that appeared in 'pu', there is no need to resend\n>> (if you want to make any other changes, I do want to avoid me\n>> screwing up by listening to you and hand applying those changes; I'd\n>> rather want updated patch(es) be sent in such a case).\n>> \n>\n> Hi Junio,\n>\n> I am 100% happy with with your fixed-up result.\n> I have no (planned) updates ATM.\n>\n>\n> Thank you all for the thorough reviews.\n\nThanks. \n\nI meant to say \"(and it is pretty clear already in this case)\" after\nthe \"... appeared in 'pu',\" but forgot to do so; sorry for making\nyou send an extra round of response.\n"}]}