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

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

Commit Message

Ben Wijen Aug. 30, 2019, 3:16 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

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

Comments

Junio C Hamano Aug. 30, 2019, 8:15 p.m. UTC | #1
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);
Ben Wijen Aug. 31, 2019, 7:17 a.m. UTC | #2
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?
Junio C Hamano Sept. 1, 2019, 4:01 p.m. UTC | #3
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.
Ben Wijen Sept. 1, 2019, 4:27 p.m. UTC | #4
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.
>
Junio C Hamano Sept. 2, 2019, 5:33 p.m. UTC | #5
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
diff mbox series

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