[v2,1/1] rebase.c: make sure current branch isn't moved when autostashing
diff mbox series

Message ID 20190820201237.10205-2-ben@wijen.net
State New
Headers show
Series
  • git rebase: Make sure upstream branch is left alone.
Related show

Commit Message

Ben Wijen Aug. 20, 2019, 8:12 p.m. UTC
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(-)

Comments

Eric Sunshine Aug. 20, 2019, 8:24 p.m. UTC | #1
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.)
Junio C Hamano Aug. 20, 2019, 8:58 p.m. UTC | #2
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

Patch
diff mbox series

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