[PATCH 0/2] git rebase: Make sure upstream branch is left alone.

[PATCH 0/2] git rebase: Make sure upstream branch is left alone.

[Ben Wijen]

I found an issue with git rebase --autostash <upstream> <branch> with an dirty worktree/index.
It seems the currently active branch is moved, which is not correct.
The following patches contain both a test and a fix.

Ben Wijen (2):
  t3420: never change upstream branch
  rebase.c: make sure current branch isn't moved when autostashing

 builtin/rebase.c            | 18 ++++++------------
 t/t3420-rebase-autostash.sh | 13 +++++++++----
 2 files changed, 15 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH 1/2] t3420: never change upstream branch

[Ben Wijen]

When using `git rebase --autostash <upstream> <branch>` and
the workarea is dirty, the active branch is incorrectly reset
to the rebase <upstream> branch.

This test will check for such behavior.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 t/t3420-rebase-autostash.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index b8f4d03467..867e4e0b17 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -306,4 +306,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 $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)
+'
+
 test_done
-- 
2.22.0

Re: [PATCH 1/2] t3420: never change upstream branch

[Junio C Hamano]

Ben Wijen <ben@wijen.net> writes:

> When using `git rebase --autostash <upstream> <branch>` and
> the workarea is dirty, the active branch is incorrectly reset
> to the rebase <upstream> branch.
>
> This test will check for such behavior.
>
> Signed-off-by: Ben Wijen <ben@wijen.net>
> ---
>  t/t3420-rebase-autostash.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index b8f4d03467..867e4e0b17 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -306,4 +306,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 $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)
> +'
> +
>  test_done

If you are going to make these into two separate commits (which I do
not necessarily recommend), introduce it as "test_expect_failure" in
step 1/2 and flip it to "test_expect_success" in step 2/2, when the
breakage is corrected.

This breakage may have happened somewhere between v2.19 and v2.20,
if my hunch is correct.  If it is easy to identify the exact point
of breakage, it may make sense to note it in the log message of 2/2
as well.  My guess is 176f5d96 ("built-in rebase --autostash: leave
the current branch alone if possible", 2018-11-07) is the plausible
candidate (iow, I suspect that the "do not detach" optimization was
made a bit too aggressively by that commit), but don't quote me on
it as this was purely done by "git log --grep -p" and not compiling
or running any tests ;-)

Thanks.

Re: [PATCH 1/2] t3420: never change upstream branch

[Phillip Wood]

Hi Ben

On 18/08/2019 10:53, Ben Wijen wrote:
> When using `git rebase --autostash <upstream> <branch>` and
> the workarea is dirty, the active branch is incorrectly reset
> to the rebase <upstream> branch.
> 
> This test will check for such behavior.
> 
> Signed-off-by: Ben Wijen <ben@wijen.net>
> ---
>   t/t3420-rebase-autostash.sh | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index b8f4d03467..867e4e0b17 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -306,4 +306,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 $(git rev-parse upstream) = $(git rev-parse unrelated-onto-branch)

In addition to Junio's suggestions I'd add using
	test_cmp_rev upstream unrelated-onto-branch
for the last line.

Best Wishes

Phillip

> +'
> +
>   test_done
> 

[PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing

[Ben Wijen]

The rebase --autostash incorrectly moved the current branch to orig_head, where
orig_head -- commit object name of tip of the branch before rebasing

It seems this was incorrectly taken over from git-legacy-rebase.sh

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 18 ++++++------------
 t/t3420-rebase-autostash.sh |  4 ----
 2 files changed, 6 insertions(+), 16 deletions(-)

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 867e4e0b17..2ea1909881 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.
-- 
2.22.0

Re: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing

[Phillip Wood]

Hi Ben

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.

Best Wishes

Phillip

On 18/08/2019 10:53, Ben Wijen wrote:
> The rebase --autostash incorrectly moved the current branch to orig_head, where
> orig_head -- commit object name of tip of the branch before rebasing
> 
> It seems this was incorrectly taken over from git-legacy-rebase.sh
> 
> Signed-off-by: Ben Wijen <ben@wijen.net>
> ---
>   builtin/rebase.c            | 18 ++++++------------
>   t/t3420-rebase-autostash.sh |  4 ----
>   2 files changed, 6 insertions(+), 16 deletions(-)
> 
> 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 867e4e0b17..2ea1909881 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.
> 

Re: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing

[Ben]

Hi Phillip,

'git stash create autostash' does not clear the workarea (like 'git stash' does)
That's the reason for the 'git reset --hard'

The difference you see in the test between the legacy rebase and the builtin rebase is because
the legacy 'git reset --hard' emits the 'HEAD is now at ...' which was also included in the builtin rebase
I saw no reason to keep that message as - with my patch - we have concluded the HEAD must not change.


Ben...

On 20-08-2019 11:00, Phillip Wood wrote:
> Hi Ben
> 
> 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.
> 
> Best Wishes
> 
> Phillip
> 
> On 18/08/2019 10:53, Ben Wijen wrote:
>> The rebase --autostash incorrectly moved the current branch to orig_head, where
>> orig_head -- commit object name of tip of the branch before rebasing
>>
>> It seems this was incorrectly taken over from git-legacy-rebase.sh
>>
>> Signed-off-by: Ben Wijen <ben@wijen.net>
>> ---
>>   builtin/rebase.c            | 18 ++++++------------
>>   t/t3420-rebase-autostash.sh |  4 ----
>>   2 files changed, 6 insertions(+), 16 deletions(-)
>>
>> 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 867e4e0b17..2ea1909881 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.
>>
> 

[PATCH v2 0/1] git rebase: Make sure upstream branch is left alone.

[Ben Wijen]

Hi Phillip, Junio,

Thank you for taking the time to look into this.
With this new patch I think I've addressed all your concerns.


Ben Wijen (1):
  rebase.c: make sure current branch isn't moved when autostashing

 builtin/rebase.c            | 18 ++++++------------
 t/t3420-rebase-autostash.sh | 13 +++++++++----
 2 files changed, 15 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing

[Ben Wijen]

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(-)

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

Re: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing

[Eric Sunshine]

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

Re: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing

[Junio C Hamano]

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 v3 0/1] rebase.c: make sure the active branch isn't moved when autostashing

[Ben Wijen]

Hi,

I have done some more tests on what's actually going on.
The active branch is currently reset to master (before the rebase)
The confusion was because of me naming the active branch 'upstream'

I hope this clears things up...


Ben Wijen (1):
  rebase.c: make sure the active branch isn't moved when autostashing

 builtin/rebase.c            | 18 ++++++------------
 t/t3420-rebase-autostash.sh | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.22.0

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

[Ben Wijen]

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(-)

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

Re: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Johannes Schindelin]

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

Re: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

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 v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Ben Wijen]

Dscho's review got me thinking about the rationale behind the 'HEAD is now at...'
message.

A 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is
not, we must do it ourselves. I guess the legacy implementation could have been
'reset --hard -q' which would have also prevented the 'HEAD is now at...' message.

Ofcourse I'm happy to reinstate the message, but I'm convinced it doesn't add
information, as with this commit the original branch is no longer moved and 
- as before - the autostash is re-applied after the rebase, leaving nothing
to be guessed about.


Thank you,

Ben Wijen (1):
  rebase.c: make sure the active branch isn't moved when autostashing

 builtin/rebase.c            | 18 ++++++------------
 t/t3420-rebase-autostash.sh | 12 ++++++++----
 2 files changed, 14 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Ben Wijen]

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 | 12 ++++++++----
 2 files changed, 14 insertions(+), 16 deletions(-)

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..2421bc39f5 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,12 @@ 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 not-the-feature-branch unrelated-onto-branch &&
+	test_when_finished "git reset --hard && git checkout -" &&
+	echo changed >file0 &&
+	git rebase --autostash not-the-feature-branch feature-branch &&
+	test_cmp_rev not-the-feature-branch unrelated-onto-branch
+'
+
 test_done
-- 
2.22.0

Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[SZEDER Gábor]

On Mon, Aug 26, 2019 at 06:45:13PM +0200, Ben Wijen wrote:
> +test_expect_success 'never change active branch' '
> +	git checkout -b not-the-feature-branch unrelated-onto-branch &&
> +	test_when_finished "git reset --hard && git checkout -" &&

I think it would be safer to explicitly spell out the branch that
should be checked out at the end than to rely on 'git checkout -'
always being able to figure that out, even in case of a breakage.

> +	echo changed >file0 &&
> +	git rebase --autostash not-the-feature-branch feature-branch &&
> +	test_cmp_rev not-the-feature-branch unrelated-onto-branch
> +'
> +
>  test_done
> -- 
> 2.22.0
> 

Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Johannes Schindelin]

Hi Ben,

On Mon, 26 Aug 2019, Ben Wijen wrote:

> Dscho's review got me thinking about the rationale behind the 'HEAD is now at...'
> message.
>
> A 'stash push' is followed by a 'reset -q' but since 'stash create autostash' is
> not, we must do it ourselves. I guess the legacy implementation could have been
> 'reset --hard -q' which would have also prevented the 'HEAD is now at...' message.
>
> Ofcourse I'm happy to reinstate the message, but I'm convinced it doesn't add
> information, as with this commit the original branch is no longer moved and
> - as before - the autostash is re-applied after the rebase, leaving nothing
> to be guessed about.

FWIW I disagree with the decision to mingle a bug fix with a change of
behavior. Resetting to the correct OID is of course the bug fix.
Dropping the message is a change of behavior.

I would be a lot more comfortable with a bug fix that did *not* change
the behavior, fast-tracking that to even maintenance branches.

And leaving the behavior change to cook in `next` for a while.

Of course, I am not Git's maintainer, if I were, I would insist on this
more careful approach.

Ciao,
Johannes

Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> FWIW I disagree with the decision to mingle a bug fix with a change of
> behavior. Resetting to the correct OID is of course the bug fix.
> Dropping the message is a change of behavior.

In general I strongly advocate that a patch should fix one thing and
one thing well without breaking other things, so we are on the same
page.

As I said in <xmqqftltqjy1.fsf@gitster-ct.c.googlers.com>, I think
the message that is leaked from "reset --hard" was reporting an
incorrect thing, iow, showing the message itself is another bug.

IIUC, the bug is twofold:

 - When --autostash creats a stash entry, the command attempts to
   reset the working tree and the tip of the current branch to where
   it should be (i.e. HEAD).  As we know, this attempt is faulty and
   resets to a wrong commit, not to HEAD.  This is the primary bug
   the patch under discussion fixes.

 - A message is given only when the above happens.  When rebasing
   from a clean working tree, we do not report "HEAD is now at..."
   at all.  And when autostash happens, the message is still not
   correct even after fixing to which commit we reset to.  "HEAD is
   now at ..." is misleading in that it implies that we changed to
   something else, but in reality, we have been on the same commit
   all the time since the command started, created a stash and wiped
   the working tree clean after doing so, when the message is given.
   That "reset --hard" is done only to clean the index and the
   working tree and talking about "HEAD is now..." is a bug in its
   context.

So, from the purist point of view, I see it may make sense to update
this patch to add logic to give a pointless and misleading "HEAD is
now at..." message so that we will report the location of HEAD where
the "rebase --autostash" command started at, to fix only the first
bug.  We still need a follow up patch that removes the message to
fix the other bug, perhaps with a follow-up to update the "first
rewinding..."  message, which is shown whether autostash kicks in or
not, so that it reports which commit we are rebuilding the history.

Thans.

Re: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> IIUC, the bug is twofold:
> ...
>  - A message is given only when the above happens.  When rebasing
> ...
>    That "reset --hard" is done only to clean the index and the
>    working tree and talking about "HEAD is now..." is a bug in its
>    context.

Actually, this "latter" bug can further be split into two

 * The "HEAD is now" is given only when autostash feature needs to
   clean the working tree, and we have never moved HEAD anyway.

 * The message does not indicate what we are rebuilding on top of.

and dealt with separately, so with that in mind the step that would
follow the first patch, i.e.

> ... update
> this patch to add logic to give a pointless and misleading "HEAD is
> now at..." message so that we will report the location of HEAD where
> the "rebase --autostash" command started at, to fix only the first
> bug.

may become different.  The fact that the "HEAD is now..." is given
only when autostash actually happens _might_ be taken as a feature
by some users---the location of HEAD reported by the message is
irrelevant to them (we know that as a fact---we have been reporting
a wrong commit all along anyway), but the single-bit "we got a
message" is a signal that "--autostash" had something valuable to
save.

So the second step may be to replace the "HEAD is now..." message we
add back (relative to Ben's patch under discussion) to the first
patch with a more direct "stashed away your local changes" message
(perhaps with diffstat???  I do not care about the details, as we
are talking about resurrecting one single useful bit of information
and extending it futher is beyond the scope of this analysis).

And the last point, i.e. "First, rewinding head to replay your
work..." does not give enough information to be truly useful, is a
totally separate bug (that Ben's patch does not even mention or
attempt to address), so we can leave it out of this analysis, too.

So, yeah, if we are to spend extra effort to polish Ben's patch
further while keeping the "fix things without making unnecessary
changes", I think the approach that takes least amount of effort may
not to make the code manually say "Head is at ...", but to add a new
message to report that autostash happened.  That fixes two bugs
(i.e. the original bug, and the "we autostashed" bit is reported in
a roundabout and misleading way via "HEAD is now at ...") in a
single patch ;-)

[PATCH v5 0/2] rebase.c: make sure current branch isn't moved when autostashing

[Ben Wijen]

Here are my "fix things without making unnecessary changes"


Ben Wijen (2):
  builtin/rebase.c: make sure the active branch isn't moved when
    autostashing
  builtin/rebase.c: Remove obsolete message

 builtin/rebase.c            | 13 +------------
 t/t3420-rebase-autostash.sh | 12 ++++++++----
 2 files changed, 9 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH v5 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Ben Wijen]

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

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 8 ++++++--
 t/t3420-rebase-autostash.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 670096c065..b3b17669e3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,9 +1968,13 @@ 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 object_id head_oid;
+			if (get_oid("HEAD", &head_oid)) {
+				ret = error(_("could not determine HEAD revision"));
+			}
+
 			struct commit *head =
-				lookup_commit_reference(the_repository,
-							&options.orig_head);
+				lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index b8f4d03467..43685a5c8e 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -306,4 +306,12 @@ 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 not-the-feature-branch unrelated-onto-branch &&
+	test_when_finished "git reset --hard && git checkout master" &&
+	echo changed >file0 &&
+	git rebase --autostash not-the-feature-branch feature-branch &&
+	test_cmp_rev not-the-feature-branch unrelated-onto-branch
+'
+
 test_done
-- 
2.22.0

[PATCH v5 2/2] builtin/rebase.c: Remove pointless message

[Ben Wijen]

When doing 'git rebase --autostash <upstream> <master>' with a dirty worktree
a 'HEAD is now at ...' message is emitted, which is pointless as it refers to
the old active branch which isn't actually moved.

This commit removes the 'HEAD is now at...' message.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 17 +----------------
 t/t3420-rebase-autostash.sh |  4 ----
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b3b17669e3..118205e481 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,13 +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 object_id head_oid;
-			if (get_oid("HEAD", &head_oid)) {
-				ret = error(_("could not determine HEAD revision"));
-			}
-
-			struct commit *head =
-				lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
@@ -1995,17 +1988,9 @@ 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",
+			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 43685a5c8e..2421bc39f5 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.
-- 
2.22.0

[PATCH v6 0/2] rebase.c: make sure current branch isn't moved when autostashing

[Ben Wijen]

Here are my "fix things without making unnecessary changes"
Fixing a copy-paste fault which I missed in v5...


Ben Wijen (2):
  builtin/rebase.c: make sure the active branch isn't moved when
    autostashing
  builtin/rebase.c: Remove obsolete message

 builtin/rebase.c            | 13 +------------
 t/t3420-rebase-autostash.sh | 12 ++++++++----
 2 files changed, 9 insertions(+), 16 deletions(-)

-- 
2.22.0

[PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Ben Wijen]

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

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 8 ++++++--
 t/t3420-rebase-autostash.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 670096c065..abcbfb8f01 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,9 +1968,13 @@ 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 object_id head_oid;
+			if (get_oid("HEAD", &head_oid)) {
+				die(_("could not determine HEAD revision"));
+			}
+
 			struct commit *head =
-				lookup_commit_reference(the_repository,
-							&options.orig_head);
+				lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index b8f4d03467..1131e0016a 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -306,4 +306,12 @@ 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 not-the-feature-branch unrelated-onto-branch &&
+	test_when_finished "git reset --hard && git checkout master" &&
+	echo changed >file0 &&
+	git rebase --autostash not-the-feature-branch feature-branch &&
+	test_cmp_rev not-the-feature-branch unrelated-onto-branch
+'
+
 test_done
-- 
2.22.0

Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

Ben Wijen <ben@wijen.net> writes:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 670096c065..abcbfb8f01 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1968,9 +1968,13 @@ 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 object_id head_oid;
> +			if (get_oid("HEAD", &head_oid)) {
> +				die(_("could not determine HEAD revision"));
> +			}

Pointless {} pair around a single statement.

> +
>  			struct commit *head =
> -				lookup_commit_reference(the_repository,
> -							&options.orig_head);
> +				lookup_commit_reference(the_repository, &head_oid);

This introduces decl-after-statement error, doesn't it?

Perhaps like so...

diff --git a/builtin/rebase.c b/builtin/rebase.c
index abcbfb8f01..0a2f9273ee 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1969,12 +1969,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			struct child_process stash = CHILD_PROCESS_INIT;
 			struct object_id oid;
 			struct object_id head_oid;
-			if (get_oid("HEAD", &head_oid)) {
-				die(_("could not determine HEAD revision"));
-			}
+			struct commit *head;
 
-			struct commit *head =
-				lookup_commit_reference(the_repository, &head_oid);
+			if (get_oid("HEAD", &head_oid))
+				die(_("could not determine HEAD revision"));
+			head = lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);

Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Ben]

On 30-08-2019 22:15, Junio C Hamano wrote:
> Ben Wijen <ben@wijen.net> writes:
> 
>> +
>>  			struct commit *head =
>> -				lookup_commit_reference(the_repository,
>> -							&options.orig_head);
>> +				lookup_commit_reference(the_repository, &head_oid);
> 
> This introduces decl-after-statement error, doesn't it?
> 
> Perhaps like so...

Would you like me to send in another patch or leave it like this?

Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

Ben <ben@wijen.net> writes:

> On 30-08-2019 22:15, Junio C Hamano wrote:
>> Ben Wijen <ben@wijen.net> writes:
>> 
>>> +
>>>  			struct commit *head =
>>> -				lookup_commit_reference(the_repository,
>>> -							&options.orig_head);
>>> +				lookup_commit_reference(the_repository, &head_oid);
>> 
>> This introduces decl-after-statement error, doesn't it?
>> 
>> Perhaps like so...
>
> Would you like me to send in another patch or leave it like this?

As long as you make it clear that you are 100% happy with the
fixed-up result that appeared in 'pu', there is no need to resend
(if you want to make any other changes, I do want to avoid me
screwing up by listening to you and hand applying those changes; I'd
rather want updated patch(es) be sent in such a case).

Thanks.

Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Ben]

On 01-09-2019 18:01, Junio C Hamano wrote:
> Ben <ben@wijen.net> writes:
> 
>>
>> Would you like me to send in another patch or leave it like this?
> 
> As long as you make it clear that you are 100% happy with the
> fixed-up result that appeared in 'pu', there is no need to resend
> (if you want to make any other changes, I do want to avoid me
> screwing up by listening to you and hand applying those changes; I'd
> rather want updated patch(es) be sent in such a case).
> 

Hi Junio,

I am 100% happy with with your fixed-up result.
I have no (planned) updates ATM.


Thank you all for the thorough reviews.

Ben...

> Thanks.
> 

Re: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing

[Junio C Hamano]

Ben <ben@wijen.net> writes:

>> As long as you make it clear that you are 100% happy with the
>> fixed-up result that appeared in 'pu', there is no need to resend
>> (if you want to make any other changes, I do want to avoid me
>> screwing up by listening to you and hand applying those changes; I'd
>> rather want updated patch(es) be sent in such a case).
>> 
>
> Hi Junio,
>
> I am 100% happy with with your fixed-up result.
> I have no (planned) updates ATM.
>
>
> Thank you all for the thorough reviews.

Thanks. 

I meant to say "(and it is pretty clear already in this case)" after
the "... appeared in 'pu'," but forgot to do so; sorry for making
you send an extra round of response.

[PATCH v6 2/2] builtin/rebase.c: Remove pointless message

[Ben Wijen]

When doing 'git rebase --autostash <upstream> <master>' with a dirty worktree
a 'HEAD is now at ...' message is emitted, which is pointless as it refers to
the old active branch which isn't actually moved.

This commit removes the 'HEAD is now at...' message.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 17 +----------------
 t/t3420-rebase-autostash.sh |  4 ----
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b3b17669e3..118205e481 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,13 +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 object_id head_oid;
-			if (get_oid("HEAD", &head_oid)) {
-				ret = error(_("could not determine HEAD revision"));
-			}
-
-			struct commit *head =
-				lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
@@ -1995,17 +1988,9 @@ 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",
+			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 43685a5c8e..2421bc39f5 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.
-- 
2.22.0

Re: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message

[Junio C Hamano]

Ben Wijen <ben@wijen.net> writes:

> @@ -1968,13 +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 object_id head_oid;
> -			if (get_oid("HEAD", &head_oid)) {
> -				ret = error(_("could not determine HEAD revision"));

I think we saw die() in the previous one.  This patch would not
apply on top of the result of applying 1/2.

I'll tentatively queue this instead on top of the corrected 1/2.

Thanks.

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0a2f9273ee..118205e481 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,12 +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 object_id head_oid;
-			struct commit *head;
-
-			if (get_oid("HEAD", &head_oid))
-				die(_("could not determine HEAD revision"));
-			head = lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
@@ -1994,17 +1988,9 @@ 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",
+			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)

Re: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message

[Ben]

Hi Junio,

On 30-08-2019 22:16, Junio C Hamano wrote:
> Ben Wijen <ben@wijen.net> writes:
> 
>> -			struct object_id head_oid;
>> -			if (get_oid("HEAD", &head_oid)) {
>> -				ret = error(_("could not determine HEAD revision"));
> 
> I think we saw die() in the previous one.  This patch would not
> apply on top of the result of applying 1/2.

Yes, my fault, sorry about that...

> 
> I'll tentatively queue this instead on top of the corrected 1/2.

Your patch is indeed correct.

Thank you!

> 
> Thanks.
> 

[PATCH 2/2] builtin/rebase.c: Remove pointless message

[Ben Wijen]

When doing 'git rebase --autostash <upstream> <master>' with a dirty worktree
a 'HEAD is now at ...' message is emitted, which is pointless as it refers to
the old active branch which isn't actually moved.

This commit removes the 'HEAD is now at...' message.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/rebase.c            | 17 +----------------
 t/t3420-rebase-autostash.sh |  4 ----
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index abcbfb8f01..118205e481 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1968,13 +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 object_id head_oid;
-			if (get_oid("HEAD", &head_oid)) {
-				die(_("could not determine HEAD revision"));
-			}
-
-			struct commit *head =
-				lookup_commit_reference(the_repository, &head_oid);
 
 			argv_array_pushl(&stash.args,
 					 "stash", "create", "autostash", NULL);
@@ -1995,17 +1988,9 @@ 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",
+			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 1131e0016a..5f7e73cf83 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.
-- 
2.22.0

Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.

[Phillip Wood]

Hi Ben

On 18/08/2019 10:53, Ben Wijen wrote:
> I found an issue with git rebase --autostash <upstream> <branch> with an dirty worktree/index.
> It seems the currently active branch is moved, which is not correct.

I'm not sure I understand what you mean by "the currently active branch 
is moved". 'git rebase --autostash <upstream> <branch>' should checkout 
<branch> (presumably stashing any unstaged and uncommitted changes 
first) and then do rebase <upstream> - what's it doing instead?

Best Wishes

Phillip

> The following patches contain both a test and a fix.
> 
> Ben Wijen (2):
>    t3420: never change upstream branch
>    rebase.c: make sure current branch isn't moved when autostashing
> 
>   builtin/rebase.c            | 18 ++++++------------
>   t/t3420-rebase-autostash.sh | 13 +++++++++----
>   2 files changed, 15 insertions(+), 16 deletions(-)
> 

Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.

[Ben]

Hi Phillip,

The expected behavior: (as per v2.21.0:/git-legacy-rebase.sh:679)
    AUTOSTASH=$(git stash create autostash)
    git reset --hard
    git checkout master
    git rebase upstream
    git stash apply $AUTOSTASH 


The actual behavior:
    AUTOSTASH=$(git stash create autostash)
    git reset --hard master
    git checkout master
    git rebase upstream
    git stash apply $AUTOSTASH 

So, the problem with the actual behavior is the move of the currently active branch with 'git reset --hard master'


Best regards,

Ben...

On 19-08-2019 11:26, Phillip Wood wrote:
> Hi Ben
> 
> On 18/08/2019 10:53, Ben Wijen wrote:
>> I found an issue with git rebase --autostash <upstream> <branch> with an dirty worktree/index.
>> It seems the currently active branch is moved, which is not correct.
> 
> I'm not sure I understand what you mean by "the currently active branch is moved". 'git rebase --autostash <upstream> <branch>' should checkout <branch> (presumably stashing any unstaged and uncommitted changes first) and then do rebase <upstream> - what's it doing instead?
> 
> Best Wishes
> 
> Phillip
> 
>> The following patches contain both a test and a fix.
>>
>> Ben Wijen (2):
>>    t3420: never change upstream branch
>>    rebase.c: make sure current branch isn't moved when autostashing
>>
>>   builtin/rebase.c            | 18 ++++++------------
>>   t/t3420-rebase-autostash.sh | 13 +++++++++----
>>   2 files changed, 15 insertions(+), 16 deletions(-)
>>
> 

Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.

[Junio C Hamano]

Ben <ben@wijen.net> writes:

> Hi Phillip,
>
> The expected behavior: (as per v2.21.0:/git-legacy-rebase.sh:679)
>     AUTOSTASH=$(git stash create autostash)
>     git reset --hard
>     git checkout master
>     git rebase upstream
>     git stash apply $AUTOSTASH 
>
>
> The actual behavior:
>     AUTOSTASH=$(git stash create autostash)
>     git reset --hard master
>     git checkout master
>     git rebase upstream
>     git stash apply $AUTOSTASH 
>
> So, the problem with the actual behavior is the move of the currently active branch with 'git reset --hard master'

Your expected and actual behaviour are described above, but it is
not mentioned out of what command (and in what settings) you expect
the above behaviour.

I am guessing that the setup and the command is something along this
line?

    git checkout not-the-master
    work work work
    git rebase --autostash upstream master

That is, you have checked out a branch that is not the 'master'
branch, you have accumulated some work in the working tree, and then
you ask "please stash away the work in progress and then rebase the
'master' branch on top of the upstream branch"?

If so, my expectation for the third step (i.e. the actual "rebase")
aligns with yours.  After stashing away the local change, we want to
get a clean working tree, checkout the master branch and rebase it
on top of the upstream.  With the command sequence you wrote in the
"actual" section, after stashing away the local change, the current
branch that is *not* the master is reset to the tip of 'master',
which would not be what we want.