| Message ID | 20190820201237.10205-2-ben@wijen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git rebase: Make sure upstream branch is left alone. | expand |
On Tue, Aug 20, 2019 at 4:12 PM Ben Wijen <ben@wijen.net> wrote: > diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh > @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' ' > +test_expect_success 'never change upstream branch' ' > + test_when_finished "git reset --hard && git branch -D upstream" && > + git checkout -b upstream unrelated-onto-branch && > + echo changed >file0 && > + git add file0 && > + git rebase --autostash upstream feature-branch && > + test_cmp_rev upstream unrelated-onto-branch > +' To be friendly to tests added after this one in the future, follow the example of other tests in this script by ensuring that the test switches back to the branch which was active prior to starting this test. For instance: test_expect_success 'never change upstream branch' ' git checkout -b upstream unrelated-onto-branch && test_when_finished "git reset --hard && git checkout - && git branch -D upstream" && echo changed >file0 && git add file0 && git rebase --autostash upstream feature-branch && test_cmp_rev upstream unrelated-onto-branch ' (Don't actually copy/paste the code from the other tests, since most of them switch back to the previous branch incorrectly by using "git checkout <whatever>" as the last line of the test. That won't restore the branch correctly if the test fails before it reaches that line; instead, test_when_finished() should be used to switch back to the original branch.)
Ben Wijen <ben@wijen.net> writes: > Consider the following scenario: > git checkout not-the-master > work work work > git rebase --autostash upstream master > > Here 'rebase --autostash <upstream> <branch>' incorrectly moves the > upstream branch to master. > > The expected behavior: (58794775:/git-rebase.sh:526) > AUTOSTASH=$(git stash create autostash) > git reset --hard > git checkout master > git rebase upstream > git stash apply $AUTOSTASH > > The actual behavior: (6defce2b:/builtin/rebase.c:1062) > AUTOSTASH=$(git stash create autostash) > git reset --hard master > git checkout master > git rebase upstream > git stash apply $AUTOSTASH In the scenario at the top, the branch that is checked out while you are working is "not-the-master" branch, and you run the rebase command. If we follow the "actual behaviour" in our head, after stashing away the local change, the tip of the current branch (i.e. not-the-master) is reset to the same commit as the tip of 'master'. But earlier, you said, "incorrectlly moves the upstream branch". It looks like either one of the use of branches in the "scenario", or the problem statement, is incorrect. The reason why "HEAD is..." comments are all gone (as shown in the test) is not explained well in the proposed commit log message, either. I think the change is correct (i.e. we were moving HEAD incorrectly, and the messages were given incorrectly, and we are fixing this behaviour hence there is no longer any need to say we are moving the HEAD anymore), but there should be some mention of the change, I would think. Thanks. > create_expected_failure_am () { > cat >expected <<-EOF > $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) > - HEAD is now at $(git rev-parse --short feature-branch) third commit > First, rewinding head to replay your work on top of it... > Applying: second commit > Applying: third commit > @@ -70,7 +67,6 @@ create_expected_failure_am () { > create_expected_failure_interactive () { > cat >expected <<-EOF > $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) > - HEAD is now at $(git rev-parse --short feature-branch) third commit > Applying autostash resulted in conflicts. > Your changes are safe in the stash. > You can run "git stash pop" or "git stash drop" at any time. > @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' ' > test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" > ' > > +test_expect_success 'never change upstream branch' ' > + test_when_finished "git reset --hard && git branch -D upstream" && > + git checkout -b upstream unrelated-onto-branch && > + echo changed >file0 && > + git add file0 && > + git rebase --autostash upstream feature-branch && > + test_cmp_rev upstream unrelated-onto-branch > +' > + > test_done
diff --git a/builtin/rebase.c b/builtin/rebase.c index 670096c065..a928f44941 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) state_dir_path("autostash", &options); struct child_process stash = CHILD_PROCESS_INIT; struct object_id oid; - struct commit *head = - lookup_commit_reference(the_repository, - &options.orig_head); argv_array_pushl(&stash.args, "stash", "create", "autostash", NULL); @@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.state_dir); write_file(autostash, "%s", oid_to_hex(&oid)); printf(_("Created autostash: %s\n"), buf.buf); - if (reset_head(&head->object.oid, "reset --hard", + + /* + * We might not be on orig_head yet: + * Make sure to reset w/o switching branches... + */ + if (reset_head(NULL, "reset --hard", NULL, RESET_HEAD_HARD, NULL, NULL) < 0) die(_("could not reset --hard")); - printf(_("HEAD is now at %s"), - find_unique_abbrev(&head->object.oid, - DEFAULT_ABBREV)); - strbuf_reset(&buf); - pp_commit_easy(CMIT_FMT_ONELINE, head, &buf); - if (buf.len > 0) - printf(" %s", buf.buf); - putchar('\n'); if (discard_index(the_repository->index) < 0 || repo_read_index(the_repository) < 0) diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index b8f4d03467..c26b4b0885 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -37,7 +37,6 @@ test_expect_success setup ' create_expected_success_am () { cat >expected <<-EOF $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) - HEAD is now at $(git rev-parse --short feature-branch) third commit First, rewinding head to replay your work on top of it... Applying: second commit Applying: third commit @@ -48,7 +47,6 @@ create_expected_success_am () { create_expected_success_interactive () { q_to_cr >expected <<-EOF $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) - HEAD is now at $(git rev-parse --short feature-branch) third commit Applied autostash. Successfully rebased and updated refs/heads/rebased-feature-branch. EOF @@ -57,7 +55,6 @@ create_expected_success_interactive () { create_expected_failure_am () { cat >expected <<-EOF $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) - HEAD is now at $(git rev-parse --short feature-branch) third commit First, rewinding head to replay your work on top of it... Applying: second commit Applying: third commit @@ -70,7 +67,6 @@ create_expected_failure_am () { create_expected_failure_interactive () { cat >expected <<-EOF $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual) - HEAD is now at $(git rev-parse --short feature-branch) third commit Applying autostash resulted in conflicts. Your changes are safe in the stash. You can run "git stash pop" or "git stash drop" at any time. @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' ' test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" ' +test_expect_success 'never change upstream branch' ' + test_when_finished "git reset --hard && git branch -D upstream" && + git checkout -b upstream unrelated-onto-branch && + echo changed >file0 && + git add file0 && + git rebase --autostash upstream feature-branch && + test_cmp_rev upstream unrelated-onto-branch +' + test_done
Consider the following scenario: git checkout not-the-master work work work git rebase --autostash upstream master Here 'rebase --autostash <upstream> <branch>' incorrectly moves the upstream branch to master. The expected behavior: (58794775:/git-rebase.sh:526) AUTOSTASH=$(git stash create autostash) git reset --hard git checkout master git rebase upstream git stash apply $AUTOSTASH The actual behavior: (6defce2b:/builtin/rebase.c:1062) AUTOSTASH=$(git stash create autostash) git reset --hard master git checkout master git rebase upstream git stash apply $AUTOSTASH This commit reinstates the 'legacy script' behavior as introduced with 58794775: rebase: implement --[no-]autostash and rebase.autostash Signed-off-by: Ben Wijen <ben@wijen.net> --- builtin/rebase.c | 18 ++++++------------ t/t3420-rebase-autostash.sh | 13 +++++++++---- 2 files changed, 15 insertions(+), 16 deletions(-)