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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2019, 20:58 UTC
Message-ID: <xmqqlfvntuz4.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190820201237.10205-2-ben@wijen.net>

Ben Wijen <ben@wijen.net> writes:

Show 21 quoted lines

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

Show 29 quoted lines

>  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