| Message ID | 20190830151607.4208-2-ben@wijen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase.c: make sure current branch isn't moved when autostashing | expand |
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);
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?
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.
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. >
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.
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
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(-)