threads / patch / 51663
Subject: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.
Ben WijenI 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
Ben Wijen[PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing
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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
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.
> BenRe: [PATCH 2/2] rebase.c: make sure current branch isn't moved when autostashing
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.
>>
> Ben Wijen[PATCH v2 0/1] git rebase: Make sure upstream branch is left alone.
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
Ben Wijen[PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing
Consider the following scenario:
git checkout not-the-master
work work work
git rebase --autostash upstream masterHere '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 $AUTOSTASHThe 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 $AUTOSTASHThis 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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Eric SunshineRe: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing
On Tue, Aug 20, 2019 at 4:12 PM Ben Wijen <ben@wijen.net> wrote:
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh > @@ -306,4 +302,13 @@ test_expect_success 'branch is left alone when possible' ' > +test_expect_success 'never change upstream branch' ' > + test_when_finished "git reset --hard && git branch -D upstream" && > + git checkout -b upstream unrelated-onto-branch && > + echo changed >file0 && > + git add file0 && > + git rebase --autostash upstream feature-branch && > + test_cmp_rev upstream unrelated-onto-branch > +'
To be friendly to tests added after this one in the future, follow the example of other tests in this script by ensuring that the test switches back to the branch which was active prior to starting this test. For instance:
test_expect_success 'never change upstream branch' '
git checkout -b upstream unrelated-onto-branch &&
test_when_finished "git reset --hard &&
git checkout - &&
git branch -D upstream" &&
echo changed >file0 &&
git add file0 &&
git rebase --autostash upstream feature-branch &&
test_cmp_rev upstream unrelated-onto-branch
'(Don't actually copy/paste the code from the other tests, since most of them switch back to the previous branch incorrectly by using "git checkout <whatever>" as the last line of the test. That won't restore the branch correctly if the test fails before it reaches that line; instead, test_when_finished() should be used to switch back to the original branch.)
Junio C HamanoRe: [PATCH v2 1/1] rebase.c: make sure current branch isn't moved when autostashing
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_doneBen Wijen[PATCH v3 0/1] rebase.c: make sure the active branch isn't moved when autostashing
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
Ben Wijen[PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing
Consider the following scenario:
git checkout not-the-master
work work work
git rebase --autostash upstream masterHere '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 $AUTOSTASHThe 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 $AUTOSTASHThis 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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Johannes SchindelinRe: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing
Hi Ben,
On Wed, 21 Aug 2019, Ben Wijen wrote:
> Consider the following scenario: > git checkout not-the-master > work work work > git rebase --autostash upstream master > > Here 'rebase --autostash <upstream> <branch>' incorrectly moves the > active branch (not-the-master) to master (before the rebase). > > The expected behavior: (58794775:/git-rebase.sh:526) > AUTOSTASH=$(git stash create autostash) > git reset --hard > git checkout master > git rebase upstream > git stash apply $AUTOSTASH > > The actual behavior: (6defce2b:/builtin/rebase.c:1062) > AUTOSTASH=$(git stash create autostash) > git reset --hard master > git checkout master > git rebase upstream > git stash apply $AUTOSTASH
Interesting. So the only difference is that the original rebase called `git reset --hard` on the current HEAD, while the new behavior tries to reset to the branch to which the user wants to switch, right away.
I can see that this would lead to possible problems e.g. if a file had been added between master and the current branch.
> This commit reinstates the 'legacy script' behavior as introduced with > 58794775: rebase: implement --[no-]autostash and rebase.autostash > > As with this commit the reset must never change the active branch, > the 'HEAD is now at ...' message has now been removed.
Actually, I am not so sure that I like this change.
Previously, users had a chance to figure out to which revision the worktree was reset, before switching the branch (because switching the branch we _do_, via `git checkout master`).
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 670096c065..a928f44941 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1968,9 +1968,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> state_dir_path("autostash", &options);
> struct child_process stash = CHILD_PROCESS_INIT;
> struct object_id oid;
> - struct commit *head =
> - lookup_commit_reference(the_repository,
> - &options.orig_head);This should probably be changed to look up `HEAD` instead, then, so that we can keep the message.
I.e. you probably want to use `get_oid("HEAD", &head_oid)` instead.> argv_array_pushl(&stash.args,
> "stash", "create", "autostash", NULL);
> @@ -1991,17 +1988,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.state_dir);
> write_file(autostash, "%s", oid_to_hex(&oid));
> printf(_("Created autostash: %s\n"), buf.buf);
> - if (reset_head(&head->object.oid, "reset --hard",
> +
> + /*
> + * We might not be on orig_head yet:
> + * Make sure to reset w/o switching branches...
> + */
> + if (reset_head(NULL, "reset --hard",
> NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
> die(_("could not reset --hard"));
> - printf(_("HEAD is now at %s"),
> - find_unique_abbrev(&head->object.oid,
> - DEFAULT_ABBREV));
> - strbuf_reset(&buf);
> - pp_commit_easy(CMIT_FMT_ONELINE, head, &buf);
> - if (buf.len > 0)
> - printf(" %s", buf.buf);
> - putchar('\n');
>
> if (discard_index(the_repository->index) < 0 ||
> repo_read_index(the_repository) < 0)
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index b8f4d03467..d1352096f2 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -37,7 +37,6 @@ test_expect_success setup '
> create_expected_success_am () {
> cat >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - HEAD is now at $(git rev-parse --short feature-branch) third commit
> First, rewinding head to replay your work on top of it...
> Applying: second commit
> Applying: third commit
> @@ -48,7 +47,6 @@ create_expected_success_am () {
> create_expected_success_interactive () {
> q_to_cr >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - HEAD is now at $(git rev-parse --short feature-branch) third commit
> Applied autostash.
> Successfully rebased and updated refs/heads/rebased-feature-branch.
> EOF
> @@ -57,7 +55,6 @@ create_expected_success_interactive () {
> create_expected_failure_am () {
> cat >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - HEAD is now at $(git rev-parse --short feature-branch) third commit
> First, rewinding head to replay your work on top of it...
> Applying: second commit
> Applying: third commit
> @@ -70,7 +67,6 @@ create_expected_failure_am () {
> create_expected_failure_interactive () {
> cat >expected <<-EOF
> $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
> - HEAD is now at $(git rev-parse --short feature-branch) third commit
> Applying autostash resulted in conflicts.
> Your changes are safe in the stash.
> You can run "git stash pop" or "git stash drop" at any time.
> @@ -306,4 +302,16 @@ test_expect_success 'branch is left alone when possible' '
> test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)"
> '
>
> +test_expect_success 'never change active branch' '
> + git checkout -b upstream unrelated-onto-branch &&
> + test_when_finished "
> + git reset --hard &&
> + git checkout - &&
> + git branch -D upstream" &&I feel like we want to have a more meaningful branch name than "upstream", and then we can get away with leaving it in place, i.e. just
test_when_finished "git reset --hard && git checkout -" &&
> + echo changed >file0 && > + git add file0 &&
Since `file0` is already tracked, I think that this `git add` invocation only distracts from the essence of this test case.
> + git rebase --autostash upstream feature-branch && > + test_cmp_rev upstream unrelated-onto-branch
Otherwise: well done! And thank you so much for fixing this.
Ciao, Dscho
> +' > + > test_done > -- > 2.22.0 > >
Junio C HamanoRe: [PATCH v3 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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.
Ben Wijen[PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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
Ben Wijen[PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
Consider the following scenario:
git checkout not-the-master
work work work
git rebase --autostash upstream masterHere '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 $AUTOSTASHThe 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 $AUTOSTASHThis 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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
SZEDER GáborRe: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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 >
Johannes SchindelinRe: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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
Junio C HamanoRe: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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.
Junio C HamanoRe: [PATCH v4 1/1] rebase.c: make sure the active branch isn't moved when autostashing
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 ;-)
Ben Wijen[PATCH v5 0/2] rebase.c: make sure current branch isn't moved when autostashing
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 messagebuiltin/rebase.c | 13 +------------ t/t3420-rebase-autostash.sh | 12 ++++++++---- 2 files changed, 9 insertions(+), 16 deletions(-)
-- 2.22.0
Ben Wijen[PATCH v5 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
Consider the following scenario:
git checkout not-the-master
work work work
git rebase --autostash upstream masterHere '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 $AUTOSTASHThe 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 $AUTOSTASHThis 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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Ben Wijen[PATCH v5 2/2] builtin/rebase.c: Remove pointless message
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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Ben Wijen[PATCH v6 0/2] rebase.c: make sure current branch isn't moved when autostashing
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 messagebuiltin/rebase.c | 13 +------------ t/t3420-rebase-autostash.sh | 12 ++++++++---- 2 files changed, 9 insertions(+), 16 deletions(-)
-- 2.22.0
Ben Wijen[PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
Consider the following scenario:
git checkout not-the-master
work work work
git rebase --autostash upstream masterHere '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 $AUTOSTASHThe 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 $AUTOSTASHThis 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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Junio C HamanoRe: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
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);
BenRe: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
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 HamanoRe: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
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.
BenRe: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
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 HamanoRe: [PATCH v6 1/2] builtin/rebase.c: make sure the active branch isn't moved when autostashing
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.
Ben Wijen[PATCH 2/2] builtin/rebase.c: Remove pointless message
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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Ben Wijen[PATCH v6 2/2] builtin/rebase.c: Remove pointless message
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(-)
builtin/rebase.c, t/t3420-rebase-autostash.sh
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
Junio C HamanoRe: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message
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)
BenRe: [PATCH v6 2/2] builtin/rebase.c: Remove pointless message
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. >
Ben Wijen[PATCH 1/2] t3420: never change upstream branch
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
Junio C HamanoRe: [PATCH 1/2] t3420: never change upstream branch
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
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 >
Re: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.
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(-) >
BenRe: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.
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(-) >> >
Junio C HamanoRe: [PATCH 0/2] git rebase: Make sure upstream branch is left alone.
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 masterThat 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.