| Message ID | 20190821182941.12674-2-ben@wijen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase.c: make sure the active branch isn't moved when autostashing | expand |
Hi Ben, On Wed, 21 Aug 2019, Ben Wijen wrote: > 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 > active branch (not-the-master) to master (before the rebase). > > 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 Interesting. So the only difference is that the original rebase called `git reset --hard` on the current HEAD, while the new behavior tries to reset to the branch to which the user wants to switch, right away. I can see that this would lead to possible problems e.g. if a file had been added between master and the current branch. > This commit reinstates the 'legacy script' behavior as introduced with > 58794775: rebase: implement --[no-]autostash and rebase.autostash > > As with this commit the reset must never change the active branch, > the 'HEAD is now at ...' message has now been removed. Actually, I am not so sure that I like this change. Previously, users had a chance to figure out to which revision the worktree was reset, before switching the branch (because switching the branch we _do_, via `git checkout master`). > 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); This should probably be changed to look up `HEAD` instead, then, so that we can keep the message. I.e. you probably want to use `get_oid("HEAD", &head_oid)` instead. > 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..d1352096f2 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,16 @@ test_expect_success 'branch is left alone when possible' ' > test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" > ' > > +test_expect_success 'never change active branch' ' > + git checkout -b upstream unrelated-onto-branch && > + test_when_finished " > + git reset --hard && > + git checkout - && > + git branch -D upstream" && I feel like we want to have a more meaningful branch name than "upstream", and then we can get away with leaving it in place, i.e. just test_when_finished "git reset --hard && git checkout -" && > + echo changed >file0 && > + git add file0 && Since `file0` is already tracked, I think that this `git add` invocation only distracts from the essence of this test case. > + git rebase --autostash upstream feature-branch && > + test_cmp_rev upstream unrelated-onto-branch Otherwise: well done! And thank you so much for fixing this. Ciao, Dscho > +' > + > test_done > -- > 2.22.0 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> As with this commit the reset must never change the active branch, >> the 'HEAD is now at ...' message has now been removed. > > Actually, I am not so sure that I like this change. > > Previously, users had a chance to figure out to which revision the > worktree was reset, before switching the branch (because switching the > branch we _do_, via `git checkout master`). Hmph, that only happens when --autostash is in effect and actually had created a stash, no? If your working tree is clean, or if you did not pass --autostash, "HEAD is now at ..." is not reported. I am not sure why that particular piece of information is only useful in the case we actually created a stash and unnecessary if we did not create a stash. When we do not create a stash, the output starts from "First, rewinding head to replay your work on top of it...", which sort of gives a warm and fuzzy impression that it is reporting what it is doing, but without giving the most useful information (i.e. what "it" refers to). Because I am all for preserving the existing behaviour as much as possible when fixing real bugs, I would not strongly object to your idea of resurrecting the message. But I am not sure if the existing message was all that useful in the first place. I'd rather see these messages that were only emitted when --autostash was given removed first (like this patch does), and then the "First rewinding..." message reworded to show where we rebuilt the history on top of. Other than that, thanks for a good review, and thanks Ben for working on this.
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..d1352096f2 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,16 @@ test_expect_success 'branch is left alone when possible' ' test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" ' +test_expect_success 'never change active 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 +' + 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 active branch (not-the-master) to master (before the rebase). 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 As with this commit the reset must never change the active branch, the 'HEAD is now at ...' message has now been removed. Signed-off-by: Ben Wijen <ben@wijen.net> --- builtin/rebase.c | 18 ++++++------------ t/t3420-rebase-autostash.sh | 16 ++++++++++++---- 2 files changed, 18 insertions(+), 16 deletions(-)