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

Message ID 20190821182941.12674-2-ben@wijen.net
State New
Headers show
Series
  • rebase.c: make sure the active branch isn't moved when autostashing
Related show

Commit Message

Ben Wijen Aug. 21, 2019, 6:29 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
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(-)

Comments

Johannes Schindelin Aug. 22, 2019, 12:27 p.m. UTC | #1
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
>
>
Junio C Hamano Aug. 22, 2019, 3:49 p.m. UTC | #2
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.

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..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