rebase: update HEAD when <branch> is an oid #1226
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
john-cai
changed the title
john-cai
changed the title
john-cai
changed the title
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
661af70 to
77b6545
Compare
|
/preview |
|
Preview email sent as pull.1226.git.git.1646974533682.gitgitgadget@gmail.com |
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
77b6545 to
1c6e8cf
Compare
|
/preview |
|
Preview email sent as pull.1226.git.git.1646974941470.gitgitgadget@gmail.com |
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
1c6e8cf to
2538fd9
Compare
|
/submit |
|
Submitted as pull.1226.git.git.1646975144178.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, John Cai wrote (reply to this): |
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
2538fd9 to
06bd225
Compare
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
User |
|
On the Git mailing list, John Cai wrote (reply to this): |
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
2 times, most recently
from
5a2ef9a to
6ea8b24
Compare
|
/preview |
|
Preview email sent as pull.1226.v2.git.git.1647017180.gitgitgadget@gmail.com |
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
6ea8b24 to
0e3c733
Compare
|
/submit |
|
Submitted as pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, John Cai wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address | |||
| export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> To prepare for the next commit that will test rebase with oids instead
> of branch names, update the rebase setup test to add a couple of tags we
> can use. This uses the test_commit helper so we can replace some lines
> that add a commit manually.
OK.
> test_expect_success 'prepare repository with topic branches' '
> - git config core.logAllRefUpdates true &&
This lossage is not explained. I do not know if we actually make
use of the reflog in the tests, though.
> - echo First >A &&
> - git update-index --add A &&
> - git commit -m "Add A." &&
> + test_commit "Add A." A First First &&
> git checkout -b force-3way &&
> echo Dummy >Y &&
> git update-index --add Y &&
> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
> git mv A D/A &&
> git commit -m "Move A." &&
> git checkout -b my-topic-branch main &&
> - echo Second >B &&
> - git update-index --add B &&
> - git commit -m "Add B." &&
> + test_commit "Add B." B Second Second &&
> git checkout -f main &&
> echo Third >>A &&
> git update-index A &&
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 13/03/2022 07:50, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: John Cai <johncai86@gmail.com>
>>
>> To prepare for the next commit that will test rebase with oids instead
>> of branch names, update the rebase setup test to add a couple of tags we
>> can use. This uses the test_commit helper so we can replace some lines
>> that add a commit manually.
> > OK.
> >> test_expect_success 'prepare repository with topic branches' '
>> - git config core.logAllRefUpdates true &&
> > This lossage is not explained. I do not know if we actually make
> use of the reflog in the tests, though.
It is the default these days so we don't need to waste a process setting it here.
Best Wishes
Phillip
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 13/03/2022 07:50, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: John Cai <johncai86@gmail.com>
>>>
>>> To prepare for the next commit that will test rebase with oids instead
>>> of branch names, update the rebase setup test to add a couple of tags we
>>> can use. This uses the test_commit helper so we can replace some lines
>>> that add a commit manually.
>> OK.
>>
>>> test_expect_success 'prepare repository with topic branches' '
>>> - git config core.logAllRefUpdates true &&
>> This lossage is not explained. I do not know if we actually make
>> use of the reflog in the tests, though.
>
> It is the default these days so we don't need to waste a process
> setting it here.
I said "not explained" in the log message. I didn't ask anybody to
explain it to me or to readers on the list.
Thanks.
builtin/rebase.c
Outdated
| @@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options) | |||
| getenv(GIT_REFLOG_ACTION_ENVIRONMENT), | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/reset.c b/reset.c
> index e3383a93343..f8e32fcc240 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
> if (opts->branch_msg && !opts->branch)
> BUG("branch reflog message given without a branch");
>
> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
It's just style thing but it probably is easier to read to have
an extra () around the bitwise-&.
> + BUG("attempting to detach HEAD when branch is given");
I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
when switch_to_branch == NULL. If there isn't, it could be that
we can get rid of RESET_HEAD_DETACH bit and base this decision
solely on switch_to_branch'es NULLness.
But that is totally outside the scope of this fix. I'd prefer to
see a narrowly and sharply focused fix, and to be quite honest, I
would be happier if this assertion weren't in this topic.
> if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
> ret = -1;
> goto leave_reset_head;
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 0643d015255..d5a8ee39fc4 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
> git rebase main other
> '
>
> +test_expect_success 'switch to non-branch detaches HEAD' '
> + git checkout main &&
> + old_main=$(git rev-parse HEAD) &&
> + git rebase First Second^0 &&
Good. For reproduction, the fork-point "First" does not have to be
a raw object name. "Second^0" not being a branch name does matter.
> + test_cmp_rev HEAD Second &&
> + test_cmp_rev main $old_main &&
> + test_must_fail git symbolic-ref HEAD
All correct and to the point. Good.
Will queue. Thanks.
> +'
> +
> test_expect_success 'refuse to switch to branch checked out elsewhere' '
> git checkout main &&
> git worktree add wt &&
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 13/03/2022 07:58, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> diff --git a/reset.c b/reset.c
>> index e3383a93343..f8e32fcc240 100644
>> --- a/reset.c
>> +++ b/reset.c
>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
>> if (opts->branch_msg && !opts->branch)
>> BUG("branch reflog message given without a branch");
>> >> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
> > It's just style thing but it probably is easier to read to have
> an extra () around the bitwise-&.
> >> + BUG("attempting to detach HEAD when branch is given");
> > I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
> when switch_to_branch == NULL. If there isn't, it could be that
> we can get rid of RESET_HEAD_DETACH bit and base this decision
> solely on switch_to_branch'es NULLness.
"rebase --skip" and "rebase --autostash" are two such uses I think
Best Wishes
Phillip
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 14/03/2022 10:54, Phillip Wood wrote:
> On 13/03/2022 07:58, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> diff --git a/reset.c b/reset.c
>>> index e3383a93343..f8e32fcc240 100644
>>> --- a/reset.c
>>> +++ b/reset.c
>>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct >>> reset_head_opts *opts)
>>> if (opts->branch_msg && !opts->branch)
>>> BUG("branch reflog message given without a branch");
>>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
>>
>> It's just style thing but it probably is easier to read to have
>> an extra () around the bitwise-&.
>>
>>> + BUG("attempting to detach HEAD when branch is given");
>>
>> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
>> when switch_to_branch == NULL. If there isn't, it could be that
>> we can get rid of RESET_HEAD_DETACH bit and base this decision
>> solely on switch_to_branch'es NULLness.
> > "rebase --skip" and "rebase --autostash" are two such uses I think
Those don't update any refs though so are not helpful examples. Possibly when we fast-forward because HEAD is an ancestor of 'onto' is a potential case but at the moment I think we detach HEAD when checking out 'onto' and then update and checkout the branch if there is one (I've been thinking about fixing that so we only detach HEAD if we're going to rebase).
Best Wishes
Phillip
> Best Wishes
> > Phillip
There was a problem hiding this comment.
On the Git mailing list, John Cai wrote (reply to this):
Hi Junio,
On 13 Mar 2022, at 3:58, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/reset.c b/reset.c
>> index e3383a93343..f8e32fcc240 100644
>> --- a/reset.c
>> +++ b/reset.c
>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
>> if (opts->branch_msg && !opts->branch)
>> BUG("branch reflog message given without a branch");
>>
>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
>
> It's just style thing but it probably is easier to read to have
> an extra () around the bitwise-&.
>
>> + BUG("attempting to detach HEAD when branch is given");
>
> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
> when switch_to_branch == NULL. If there isn't, it could be that
> we can get rid of RESET_HEAD_DETACH bit and base this decision
> solely on switch_to_branch'es NULLness.
>
> But that is totally outside the scope of this fix. I'd prefer to
> see a narrowly and sharply focused fix, and to be quite honest, I
> would be happier if this assertion weren't in this topic.
I'm okay with taking this out, but would be good to get an ack from Phillip.
>
>> if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
>> ret = -1;
>> goto leave_reset_head;
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 0643d015255..d5a8ee39fc4 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
>> git rebase main other
>> '
>>
>> +test_expect_success 'switch to non-branch detaches HEAD' '
>> + git checkout main &&
>> + old_main=$(git rev-parse HEAD) &&
>> + git rebase First Second^0 &&
>
> Good. For reproduction, the fork-point "First" does not have to be
> a raw object name. "Second^0" not being a branch name does matter.
>
>> + test_cmp_rev HEAD Second &&
>> + test_cmp_rev main $old_main &&
>> + test_must_fail git symbolic-ref HEAD
>
> All correct and to the point. Good.
Thanks to Phillip for his help on this!
>
> Will queue. Thanks.
More of a question about protocol. If there are some minor adjustments that are
not really worth more disussion on the ML (like removing the BUG assertion),
is it still protocol to send another series or just re-roll on the branch without
sending out another patch series?
Thanks!
John
>
>> +'
>> +
>> test_expect_success 'refuse to switch to branch checked out elsewhere' '
>> git checkout main &&
>> git worktree add wt &&
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi John
On 14/03/2022 14:11, John Cai wrote:
> Hi Junio,
> > On 13 Mar 2022, at 3:58, Junio C Hamano wrote:
> >> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> diff --git a/reset.c b/reset.c
>>> index e3383a93343..f8e32fcc240 100644
>>> --- a/reset.c
>>> +++ b/reset.c
>>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
>>> if (opts->branch_msg && !opts->branch)
>>> BUG("branch reflog message given without a branch");
>>>
>>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
>>
>> It's just style thing but it probably is easier to read to have
>> an extra () around the bitwise-&.
>>
>>> + BUG("attempting to detach HEAD when branch is given");
>>
>> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
>> when switch_to_branch == NULL. If there isn't, it could be that
>> we can get rid of RESET_HEAD_DETACH bit and base this decision
>> solely on switch_to_branch'es NULLness.
>>
>> But that is totally outside the scope of this fix. I'd prefer to
>> see a narrowly and sharply focused fix, and to be quite honest, I
>> would be happier if this assertion weren't in this topic.
> > I'm okay with taking this out, but would be good to get an ack from Phillip.
That's fine with me. The rest of the patch looks good as far as I'm concerned.
Best Wishes
Phillip
| @@ -399,6 +399,15 @@ test_expect_success 'switch to branch not checked out' ' | |||
| git rebase main other | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Currently when rebase is used with a non branch, and <oid> is up to
> date with base:
>
> git rebase base <oid>
>
> It will update the ref that HEAD is pointing at to <oid>, and leave HEAD
> unmodified.
>
> This is a bug. The expected behavior is that the branch HEAD points at
> remains unmodified while HEAD is updated to point to <oid> in detached
> HEAD mode.
Never do tests this way.
The primary reason why we do not want to write our tests the way
this patch does is because we do not _care_ how it is broken in the
behaviour of the original code. 'main' moving out of $old_main is
the bug we care about. It is still buggy if it did not move to
Second, but some other commit. Yet this patch insists that 'main'
to move to Second and nothing else. What we want is 'main' to stay
at $old_main in the end anyway, and we should directly test that
condition.
If you insist to have two commits (which I strongly recommend
against in this case), you write a test that makes sure that 'main'
stays at $old_main, but mark the test with test_expect_failure. And
then later in the step that fixes the code, flip "expect_failure" to
"expect_success".
But it is not ideal, either. Imagine what you see in "git show"
output of the commit that fixed the problem. Most of the test that
shows the behaviour that the commit _cares_ about will be outside
post-context of the hunk that flips test_expect_failure to
test_expect_success.
The best and the simplest way, for a simple case like this, to write
test is to add the test to expect what we want to see in the end,
and do so in the same commit as the one that corrects the behaviour
of the code. If somebody wants to see what the breakage looks like,
it is easy to
(1) checkout the commit that fixes the code and adds such a test,
(2) tentatively revert everything outside t/, and
(3) run the test with "-i -v" options.
Then test_expect_success that wants to see 'main' to stay at
$old_main will show that 'main' moved by a test failure. Working
from a patch is the same way, i.e. you can apply only the parts
inside t/ and run the current code to see the breakage, and then
apply the rest to see the fix.
> +test_expect_success 'switch to non-branch changes branch HEAD points to' '
> + git checkout main &&
> + old_main=$(git rev-parse HEAD) &&
> + git rebase First Second^0 &&
> + test_cmp_rev HEAD main &&
> + test_cmp_rev main $(git rev-parse Second) &&
> + git symbolic-ref HEAD
I already said that the second one should expect main to be at
$old_main, but the "HEAD and main are the same" and "HEAD is a
symolic-ref" test can be replaced with a single test that is "HEAD
is a symbolic-ref to 'main'", which would be more strict. I.e.
test "$(git symbolic-ref HEAD)" = refs/heads/main &&
test_cmp_rev main "$old_main"
And such a test that expects the correct behaviour we want to have
in the end should be added in [PATCH 3/3] when the code is fixed,
not here in a separate commit.
> +'
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
>> + test_cmp_rev HEAD main &&
>> + test_cmp_rev main $(git rev-parse Second) &&
>> + git symbolic-ref HEAD
>
> I already said that the second one should expect main to be at
> $old_main, but the "HEAD and main are the same" and "HEAD is a
> symolic-ref" test can be replaced with a single test that is "HEAD
> is a symbolic-ref to 'main'", which would be more strict. I.e.
>
> test "$(git symbolic-ref HEAD)" = refs/heads/main &&
> test_cmp_rev main "$old_main"
Scratch that. No, we do not want HEAD to be pointing at 'main'
after rebase, so the above is totally wrong. What you have at the
end of the series is the right version, as I said in my review of
the [PATCH 3/3].
Thanks for working on this topic.
There was a problem hiding this comment.
On the Git mailing list, John Cai wrote (reply to this):
Hi Junio,
On 17 Mar 2022, at 17:10, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> Currently when rebase is used with a non branch, and <oid> is up to
>> date with base:
>>
>> git rebase base <oid>
>>
>> It will update the ref that HEAD is pointing at to <oid>, and leave HEAD
>> unmodified.
>>
>> This is a bug. The expected behavior is that the branch HEAD points at
>> remains unmodified while HEAD is updated to point to <oid> in detached
>> HEAD mode.
>
> Never do tests this way.
>
> The primary reason why we do not want to write our tests the way
> this patch does is because we do not _care_ how it is broken in the
> behaviour of the original code. 'main' moving out of $old_main is
> the bug we care about. It is still buggy if it did not move to
> Second, but some other commit. Yet this patch insists that 'main'
> to move to Second and nothing else. What we want is 'main' to stay
> at $old_main in the end anyway, and we should directly test that
> condition.
I was attemping to follow the advice to "show" vs "tell" in [1]. All this
make sense to me however.
>
> If you insist to have two commits (which I strongly recommend
> against in this case), you write a test that makes sure that 'main'
> stays at $old_main, but mark the test with test_expect_failure. And
> then later in the step that fixes the code, flip "expect_failure" to
> "expect_success".
>
> But it is not ideal, either. Imagine what you see in "git show"
> output of the commit that fixed the problem. Most of the test that
> shows the behaviour that the commit _cares_ about will be outside
> post-context of the hunk that flips test_expect_failure to
> test_expect_success.
>
> The best and the simplest way, for a simple case like this, to write
> test is to add the test to expect what we want to see in the end,
> and do so in the same commit as the one that corrects the behaviour
> of the code. If somebody wants to see what the breakage looks like,
> it is easy to
>
> (1) checkout the commit that fixes the code and adds such a test,
>
> (2) tentatively revert everything outside t/, and
>
> (3) run the test with "-i -v" options.
>
> Then test_expect_success that wants to see 'main' to stay at
> $old_main will show that 'main' moved by a test failure. Working
> from a patch is the same way, i.e. you can apply only the parts
> inside t/ and run the current code to see the breakage, and then
> apply the rest to see the fix.
>
>> +test_expect_success 'switch to non-branch changes branch HEAD points to' '
>> + git checkout main &&
>> + old_main=$(git rev-parse HEAD) &&
>> + git rebase First Second^0 &&
>
>> + test_cmp_rev HEAD main &&
>> + test_cmp_rev main $(git rev-parse Second) &&
>> + git symbolic-ref HEAD
>
> I already said that the second one should expect main to be at
> $old_main, but the "HEAD and main are the same" and "HEAD is a
> symolic-ref" test can be replaced with a single test that is "HEAD
> is a symbolic-ref to 'main'", which would be more strict. I.e.
>
> test "$(git symbolic-ref HEAD)" = refs/heads/main &&
> test_cmp_rev main "$old_main"
>
> And such a test that expects the correct behaviour we want to have
> in the end should be added in [PATCH 3/3] when the code is fixed,
> not here in a separate commit.
1. https://lore.kernel.org/git/220317.86r170d6zs.gmgdl@evledraar.gmail.com/
>
>> +'
>
> Thanks.
| @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options) | |||
| ropts.oid = &options->orig_head; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Fixes a bug whereby rebase updates the deferenced reference HEAD points
> to instead of HEAD directly.
Perhaps
"git rebase A B", where B is not a commit, should behave as if
the HEAD got detached at B and then the detached HEAD got
rebased on top of A. A bug however overwrites the current
branch to point at B, when B is a descendant of A (i.e. the
rebase ends up being a fast-forward).
> ... See [1] for
> the original bug report.
OK (URL is wrong; see below).
The explanation of how the bug occurs (elided) in the patch looked
all reasonable. It read well.
> ...
> Also add a test to ensure the correct behavior.
Yup. _Add_ a test to ensure that. Not replace a misleading test
that expected to see a wrong behaviour.
> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/
This is not the original bug report. It was an early hint for
diagnosis.
[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/
would be a more appropriate pointer.
> ropts.oid = &options->orig_head;
> ropts.branch = options->head_name;
> ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> + if (!ropts.branch)
> + ropts.flags |= RESET_HEAD_DETACH;
> ropts.head_msg = buf.buf;
OK. If head_name is not set, we do not want to touch the branch
the HEAD happens to be pointing at, so we want to detach.
> +test_expect_success 'switch to non-branch detaches HEAD' '
> git checkout main &&
> old_main=$(git rev-parse HEAD) &&
> git rebase First Second^0 &&
> - test_cmp_rev HEAD main &&
> - test_cmp_rev main $(git rev-parse Second) &&
> - git symbolic-ref HEAD
> + test_cmp_rev HEAD Second &&
> + test_cmp_rev main $old_main &&
> + test_must_fail git symbolic-ref HEAD
As we want (1) HEAD (detached) is pointing at Second, (2) 'main'
stayed at $old_main, and (3) HEAD is detched, these three conditions
look sane.
Thanks.
For reference, I discarded [1/3], queued [2/3] and replaced this
[3/3] with the following for now.
---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ----
From: John Cai <johncai86@gmail.com>
Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()
"git rebase A B" where B is not a commit should behave as if the
HEAD got detached at B and then the detached HEAD got rebased on top
of A. A bug however overwrites the current branch to point at B,
when B is a descendant of A (i.e. the rebase ends up being a
fast-forward). See [1] for the original bug report.
The callstack from checkout_up_to_date() is the following:
cmd_rebase()
-> checkout_up_to_date()
-> reset_head()
-> update_refs()
-> update_ref()
When B is not a valid branch but an oid, rebase sets the head_name
of rebase_options to NULL. This value gets passed down this call
chain through the branch member of reset_head_opts also getting set
to NULL all the way to update_refs().
Then update_refs() checks ropts.branch to decide whether or not to switch
branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.
At this point however, from rebase's point of view, we want a detached
HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH
flag, the update_ref() call will deference HEAD and update the branch its
pointing to. We want the HEAD detached at B instead.
Fix this bug by adding the RESET_HEAD_DETACH flag in
checkout_up_to_date if B is not a valid branch, so that once
reset_head() calls update_refs(), it calls update_ref() with
REF_NO_DEREF which updates HEAD directly intead of deferencing it
and updating the branch that HEAD points to.
Also add a test to ensure the correct behavior.
[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/
Reported-by: Michael McClimon <michael@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/rebase.c | 2 ++
t/t3400-rebase.sh | 9 +++++++++
2 files changed, 11 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e..27fde7bf28 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)
ropts.oid = &options->orig_head;
ropts.branch = options->head_name;
ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ if (!ropts.branch)
+ ropts.flags |= RESET_HEAD_DETACH;
ropts.head_msg = buf.buf;
if (reset_head(the_repository, &ropts) < 0)
ret = error(_("could not switch to %s"), options->switch_to);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6dc8df8be7..cf55b017ff 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -389,6 +389,15 @@ test_expect_success 'switch to branch not checked out' '
git rebase main other
'
+test_expect_success 'switch to non-branch detaches HEAD' '
+ git checkout main &&
+ old_main=$(git rev-parse HEAD) &&
+ git rebase First Second^0 &&
+ test_cmp_rev HEAD Second &&
+ test_cmp_rev main $old_main &&
+ test_must_fail git symbolic-ref HEAD
+'
+
test_expect_success 'refuse to switch to branch checked out elsewhere' '
git checkout main &&
git worktree add wt &&
--
2.35.1-757-g4266a5c05c
There was a problem hiding this comment.
On the Git mailing list, John Cai wrote (reply to this):
Hi Junio,
On 17 Mar 2022, at 17:36, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> Fixes a bug whereby rebase updates the deferenced reference HEAD points
>> to instead of HEAD directly.
>
> Perhaps
>
> "git rebase A B", where B is not a commit, should behave as if
> the HEAD got detached at B and then the detached HEAD got
> rebased on top of A. A bug however overwrites the current
> branch to point at B, when B is a descendant of A (i.e. the
> rebase ends up being a fast-forward).
>
>> ... See [1] for
>> the original bug report.
>
> OK (URL is wrong; see below).
>
> The explanation of how the bug occurs (elided) in the patch looked
> all reasonable. It read well.
>
>> ...
>> Also add a test to ensure the correct behavior.
>
> Yup. _Add_ a test to ensure that. Not replace a misleading test
> that expected to see a wrong behaviour.
>
>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/
>
> This is not the original bug report. It was an early hint for
> diagnosis.
>
> [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/
>
> would be a more appropriate pointer.
>
>> ropts.oid = &options->orig_head;
>> ropts.branch = options->head_name;
>> ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
>> + if (!ropts.branch)
>> + ropts.flags |= RESET_HEAD_DETACH;
>> ropts.head_msg = buf.buf;
>
> OK. If head_name is not set, we do not want to touch the branch
> the HEAD happens to be pointing at, so we want to detach.
>
>> +test_expect_success 'switch to non-branch detaches HEAD' '
>> git checkout main &&
>> old_main=$(git rev-parse HEAD) &&
>> git rebase First Second^0 &&
>> - test_cmp_rev HEAD main &&
>> - test_cmp_rev main $(git rev-parse Second) &&
>> - git symbolic-ref HEAD
>> + test_cmp_rev HEAD Second &&
>> + test_cmp_rev main $old_main &&
>> + test_must_fail git symbolic-ref HEAD
>
> As we want (1) HEAD (detached) is pointing at Second, (2) 'main'
> stayed at $old_main, and (3) HEAD is detched, these three conditions
> look sane.
>
> Thanks.
>
> For reference, I discarded [1/3], queued [2/3] and replaced this
> [3/3] with the following for now.
Sounds good--this is what I was about to do anyways!
>
> ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ----
> From: John Cai <johncai86@gmail.com>
> Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()
>
> "git rebase A B" where B is not a commit should behave as if the
> HEAD got detached at B and then the detached HEAD got rebased on top
> of A. A bug however overwrites the current branch to point at B,
> when B is a descendant of A (i.e. the rebase ends up being a
> fast-forward). See [1] for the original bug report.
>
> The callstack from checkout_up_to_date() is the following:
>
> cmd_rebase()
> -> checkout_up_to_date()
> -> reset_head()
> -> update_refs()
> -> update_ref()
>
> When B is not a valid branch but an oid, rebase sets the head_name
> of rebase_options to NULL. This value gets passed down this call
> chain through the branch member of reset_head_opts also getting set
> to NULL all the way to update_refs().
>
> Then update_refs() checks ropts.branch to decide whether or not to switch
> branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.
> At this point however, from rebase's point of view, we want a detached
> HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH
> flag, the update_ref() call will deference HEAD and update the branch its
> pointing to. We want the HEAD detached at B instead.
>
> Fix this bug by adding the RESET_HEAD_DETACH flag in
> checkout_up_to_date if B is not a valid branch, so that once
> reset_head() calls update_refs(), it calls update_ref() with
> REF_NO_DEREF which updates HEAD directly intead of deferencing it
> and updating the branch that HEAD points to.
>
> Also add a test to ensure the correct behavior.
>
> [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/
>
> Reported-by: Michael McClimon <michael@mcclimon.org>
> Signed-off-by: John Cai <johncai86@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/rebase.c | 2 ++
> t/t3400-rebase.sh | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e..27fde7bf28 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)
> ropts.oid = &options->orig_head;
> ropts.branch = options->head_name;
> ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> + if (!ropts.branch)
> + ropts.flags |= RESET_HEAD_DETACH;
> ropts.head_msg = buf.buf;
> if (reset_head(the_repository, &ropts) < 0)
> ret = error(_("could not switch to %s"), options->switch_to);
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 6dc8df8be7..cf55b017ff 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -389,6 +389,15 @@ test_expect_success 'switch to branch not checked out' '
> git rebase main other
> '
>
> +test_expect_success 'switch to non-branch detaches HEAD' '
> + git checkout main &&
> + old_main=$(git rev-parse HEAD) &&
> + git rebase First Second^0 &&
> + test_cmp_rev HEAD Second &&
> + test_cmp_rev main $old_main &&
> + test_must_fail git symbolic-ref HEAD
> +'
> +
> test_expect_success 'refuse to switch to branch checked out elsewhere' '
> git checkout main &&
> git worktree add wt &&
> --
> 2.35.1-757-g4266a5c05c
Thanks
John
|
This patch series was integrated into seen via cd11b7a. |
|
This patch series was integrated into seen via fb5afc1. |
| @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address | |||
| export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi John
On 17/03/2022 19:53, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
> > To prepare for the next commit that will test rebase with oids instead
> of branch names, update the rebase setup test to add a couple of tags we
> can use. This uses the test_commit helper so we can replace some lines
> that add a commit manually.
> > Setting logAllRefUpdates is not necessary because it's on by default for
> repositories with a working tree.
> > Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t3400-rebase.sh | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 5c4073f06d6..2fb3fabe60e 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
> > test_expect_success 'prepare repository with topic branches' '
> - git config core.logAllRefUpdates true &&
> - echo First >A &&
> - git update-index --add A &&
> - git commit -m "Add A." &&
> + test_commit "Add A." A First First &&
> git checkout -b force-3way &&
> echo Dummy >Y &&
> git update-index --add Y &&
> @@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '
> git mv A D/A &&
> git commit -m "Move A." &&
> git checkout -b my-topic-branch main &&
> - echo Second >B &&
> - git update-index --add B &&
> - git commit -m "Add B." &&
> + test_commit "Add B." B Second Second &&
> git checkout -f main &&
> echo Third >>A &&
> git update-index A &&
> git commit -m "Modify A." &&
> git checkout -b side my-topic-branch &&
This version has added some more conversions that are not mentioned it the commit message. If you want to covert the whole setup to use test_commit then that's great but I think you need to do the whole thing and say in the commit message that you're modernizing everything. As it stands a reader is left wondering why some of the setup that is not used in the next commit has been converted but other bits such as the "Modify A." above are left as is. I think the two possibilities that make sense are (a) convert only what we need for the next commit, or (b) modernize the test and convert everything.
Best Wishes
Phillip
> - echo Side >>C &&
> - git add C &&
> - git commit -m "Add C" &&
> + test_commit --no-tag "Add C" C Side &&
> git checkout -f my-topic-branch &&
> git tag topic
> '
> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' '
> test_expect_success 'rebase a single mode change' '
> git checkout main &&
> git branch -D topic &&
> - echo 1 >X &&
> - git add X &&
> - test_tick &&
> - git commit -m prepare &&
> + test_commit prepare X 1 &&
> git checkout -b modechange HEAD^ &&
> echo 1 >X &&
> git add X &&
There was a problem hiding this comment.
On the Git mailing list, John Cai wrote (reply to this):
Hi Phillip,
On 18 Mar 2022, at 7:14, Phillip Wood wrote:
> Hi John
>
> On 17/03/2022 19:53, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> To prepare for the next commit that will test rebase with oids instead
>> of branch names, update the rebase setup test to add a couple of tags we
>> can use. This uses the test_commit helper so we can replace some lines
>> that add a commit manually.
>>
>> Setting logAllRefUpdates is not necessary because it's on by default for
>> repositories with a working tree.
>>
>> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>> t/t3400-rebase.sh | 18 ++++--------------
>> 1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 5c4073f06d6..2fb3fabe60e 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
>> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
>> test_expect_success 'prepare repository with topic branches' '
>> - git config core.logAllRefUpdates true &&
>> - echo First >A &&
>> - git update-index --add A &&
>> - git commit -m "Add A." &&
>> + test_commit "Add A." A First First &&
>> git checkout -b force-3way &&
>> echo Dummy >Y &&
>> git update-index --add Y &&
>> @@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '
>> git mv A D/A &&
>> git commit -m "Move A." &&
>> git checkout -b my-topic-branch main &&
>> - echo Second >B &&
>> - git update-index --add B &&
>> - git commit -m "Add B." &&
>> + test_commit "Add B." B Second Second &&
>> git checkout -f main &&
>> echo Third >>A &&
>> git update-index A &&
>> git commit -m "Modify A." &&
>> git checkout -b side my-topic-branch &&
>
> This version has added some more conversions that are not mentioned it the commit message. If you want to covert the whole setup to use test_commit then that's great but I think you need to do the whole thing and say in the commit message that you're modernizing everything. As it stands a reader is left wondering why some of the setup that is not used in the next commit has been converted but other bits such as the "Modify A." above are left as is. I think the two possibilities that make sense are (a) convert only what we need for the next commit, or (b) modernize the test and convert everything.
I see your point. Then for the sake of a smaller series for the patch, I'll opt
to only update the parts we need. Then maybe we can have a separate series to
modernize this suite.
I'll re-roll this series with the clarifications to the commit message that
Junio made.
>
> Best Wishes
>
> Phillip
Thanks!
John
>
>> - echo Side >>C &&
>> - git add C &&
>> - git commit -m "Add C" &&
>> + test_commit --no-tag "Add C" C Side &&
>> git checkout -f my-topic-branch &&
>> git tag topic
>> '
>> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' '
>> test_expect_success 'rebase a single mode change' '
>> git checkout main &&
>> git branch -D topic &&
>> - echo 1 >X &&
>> - git add X &&
>> - test_tick &&
>> - git commit -m prepare &&
>> + test_commit prepare X 1 &&
>> git checkout -b modechange HEAD^ &&
>> echo 1 >X &&
>> git add X &&
To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually. Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com>
"git rebase A B" where B is not a commit should behave as if the
HEAD got detached at B and then the detached HEAD got rebased on top
of A. A bug however overwrites the current branch to point at B,
when B is a descendant of A (i.e. the rebase ends up being a
fast-forward). See [1] for the original bug report.
The callstack from checkout_up_to_date() is the following:
cmd_rebase()
-> checkout_up_to_date()
-> reset_head()
-> update_refs()
-> update_ref()
When B is not a valid branch but an oid, rebase sets the head_name
of rebase_options to NULL. This value gets passed down this call
chain through the branch member of reset_head_opts also getting set
to NULL all the way to update_refs().
Then update_refs() checks ropts.branch to decide whether or not to switch
branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.
At this point however, from rebase's point of view, we want a detached
HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH
flag, the update_ref() call will deference HEAD and update the branch its
pointing to. We want the HEAD detached at B instead.
Fix this bug by adding the RESET_HEAD_DETACH flag in
checkout_up_to_date if B is not a valid branch, so that once
reset_head() calls update_refs(), it calls update_ref() with
REF_NO_DEREF which updates HEAD directly intead of deferencing it
and updating the branch that HEAD points to.
Also add a test to ensure the correct behavior.
[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/
Reported-by: Michael McClimon <michael@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
john-cai
force-pushed
the
jc/fix-rebase-oids
branch
from
13c5955 to
1dd5bb2
Compare
|
/submit |
|
Submitted as pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address | |||
| export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> To prepare for the next commit that will test rebase with oids instead
> of branch names, update the rebase setup test to add a couple of tags we
> can use. This uses the test_commit helper so we can replace some lines
> that add a commit manually.
>
> Setting logAllRefUpdates is not necessary because it's on by default for
> repositories with a working tree.
>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t3400-rebase.sh | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
Looks much simpler and obviously correct ;-)
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1dd..0643d015255 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
>
> test_expect_success 'prepare repository with topic branches' '
> - git config core.logAllRefUpdates true &&
> - echo First >A &&
> - git update-index --add A &&
> - git commit -m "Add A." &&
> + test_commit "Add A." A First First &&
> git checkout -b force-3way &&
> echo Dummy >Y &&
> git update-index --add Y &&
> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
> git mv A D/A &&
> git commit -m "Move A." &&
> git checkout -b my-topic-branch main &&
> - echo Second >B &&
> - git update-index --add B &&
> - git commit -m "Add B." &&
> + test_commit "Add B." B Second Second &&
> git checkout -f main &&
> echo Third >>A &&
> git update-index A &&
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via 9887c06. |
|
This patch series was integrated into seen via b9ef9c8. |
|
This patch series was integrated into seen via e7ff84f. |
|
This patch series was integrated into next via 1eb51c6. |
|
This patch series was integrated into seen via 552aa10. |
|
There was a status update in the "Cooking" section about the branch "git rebase $base $non_branch_commit", when $base is an ancestor or the $non_branch_commit, modified the current branch, which has been corrected. Will merge to 'master'. source: <pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 458e7b1. |
|
This patch series was integrated into seen via 681ddbc. |
|
This patch series was integrated into seen via 49ee13c. |
|
This patch series was integrated into seen via 47db42f. |
|
This patch series was integrated into seen via 25c11e8. |
|
There was a status update in the "Cooking" section about the branch "git rebase $base $non_branch_commit", when $base is an ancestor or the $non_branch_commit, modified the current branch, which has been corrected. Will merge to 'master'. source: <pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via f818536. |
|
This patch series was integrated into master via f818536. |
|
Closed via f818536. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug [1] reported by Michael McClimon where rebase with oids will erroneously update the branch HEAD points to.
This patch has two parts:
changes since v4:
changes since v3:
changes since v2:
changes since v1:
cc: Phillip Wood phillip.wood123@gmail.com
cc: Junio C Hamano gitster@pobox.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Michael McClimon michael@mcclimon.org