Bug with rebase and commit hashes

Bug with rebase and commit hashes

[Michael McClimon]

I have run into a bug with rebase when operating with commit hashes directly
(rather than branch names).

Say that I have two branches, main and topic. Branch topic consists of a
single commit whose parent is main. If I'm on main, and I run
'git rebase main topic', I end up on branch topic, as expected (my prompt here
displays the current branch):

[~/scratch on main] $ git rebase main topic
Successfully rebased and updated refs/heads/topic.
[~/scratch on topic] $


If I do exactly the same thing, but substitute the commit shas for those
branches, git _doesn't_ leave me on branch topic, but instead fast-forwards
main to topic. This is very surprising to me!

[~/scratch on main] $ git rev-parse main
464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
[~/scratch on main] $ git rev-parse topic
c3c862105dfbb2f30137a0875e8e5d9dfec334f8
[~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
Current branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.
[~/scratch on main] $ git rev-parse main
c3c862105dfbb2f30137a0875e8e5d9dfec334f8


Part of the reason this is surprising is that in the case when topic is not a
fast-forward from main (i.e., does need to be rebased), git does what I'd
expect, and leaves me detached on the newly rebased head.

[~/scratch on main] $ git rev-parse main
464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
[~/scratch on main] $ git rev-parse topic
8d7d712bad0c32cd87aa814730317178b2e46b93
[~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
Successfully rebased and updated detached HEAD.
[~/scratch at 1477bc43] $ git rev-parse HEAD
1477bc43a3bc7868ba1da8a919a60432bedbd34a


I ran into this because I was writing some software to enforce semilinear
history (all commits on main are merge commits, and the topic branches are all
rebased on main before merge). That workflow is: for every branch,
rebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.
Because of this bug, when we got to the merge --no-ff, git didn't do anything
at all, because it had already fast-forwarded main! I worked around this in
my program by just passing --force-rebase to my rebase invocation, which fixes
this particular problem by leaving me in a detached head (as in the last case
above).

I hit this in production on git 2.30.2 (debian bullseye), but reproduced
locally using the latest git main, which is git version 2.35.1.415.gc2162907.
In both cases I wiped my user gitconfig, so I'm using only the defaults. (If
it helps: with my rebase.autosquash = true, the bad case above does not behave
badly and leaves me in detached head as I'd expect.) It's totally possible
this isn't _meant_ to work, in which case I think the docs could use an
update.

Thanks!

-- 
Michael McClimon
michael@mcclimon.org

Re: Bug with rebase and commit hashes

[John Cai]

Hi Michael,

I looked into this a little bit. I may not have the full answer, so others may want to chime in.

On 10 Mar 2022, at 11:16, Michael McClimon wrote:

> I have run into a bug with rebase when operating with commit hashes directly
> (rather than branch names).
>
> Say that I have two branches, main and topic. Branch topic consists of a
> single commit whose parent is main. If I'm on main, and I run
> 'git rebase main topic', I end up on branch topic, as expected (my prompt here
> displays the current branch):
>
> [~/scratch on main] $ git rebase main topic
> Successfully rebased and updated refs/heads/topic.
> [~/scratch on topic] $
>
>
> If I do exactly the same thing, but substitute the commit shas for those
> branches, git _doesn't_ leave me on branch topic, but instead fast-forwards
> main to topic. This is very surprising to me!
>
> [~/scratch on main] $ git rev-parse main
> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
> [~/scratch on main] $ git rev-parse topic
> c3c862105dfbb2f30137a0875e8e5d9dfec334f8
> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
> Current branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.
> [~/scratch on main] $ git rev-parse main
> c3c862105dfbb2f30137a0875e8e5d9dfec334f8

Taking a look at the code in bulitin/rebase.c, it will check whether or not
<branch> is resolveable as a valid ref. If not, then this code [1] sets the head
name that will get switched to, to NULL.

Then, when checkout_up_to_date() is called, it calls reset_head() which does not
switch to the branch since opts->branch is NULL. But (and I haven't looked into
detail how reset_head() works) it seems like it will still set the current HEAD
(main) to $(git rev-parse topic).

This diff seems to fix this behavior, but it's untested.

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e72..bcbac75c705e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1634,7 +1634,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                                die(_("no such branch/commit '%s'"),
                                    branch_name);
                        oidcpy(&options.orig_head, &commit->object.oid);
-                       options.head_name = NULL;
+                       options.head_name = xstrdup(buf.buf);
                }
        } else if (argc == 0) {
                /* Do not need to switch branches, we are already on it. */


1. https://github.com/git/git/blob/master/builtin/rebase.c#L1637

>
>
> Part of the reason this is surprising is that in the case when topic is not a
> fast-forward from main (i.e., does need to be rebased), git does what I'd
> expect, and leaves me detached on the newly rebased head.
>
> [~/scratch on main] $ git rev-parse main
> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
> [~/scratch on main] $ git rev-parse topic
> 8d7d712bad0c32cd87aa814730317178b2e46b93
> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
> Successfully rebased and updated detached HEAD.
> [~/scratch at 1477bc43] $ git rev-parse HEAD
> 1477bc43a3bc7868ba1da8a919a60432bedbd34a
>
>
> I ran into this because I was writing some software to enforce semilinear
> history (all commits on main are merge commits, and the topic branches are all
> rebased on main before merge). That workflow is: for every branch,
> rebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.
> Because of this bug, when we got to the merge --no-ff, git didn't do anything
> at all, because it had already fast-forwarded main! I worked around this in
> my program by just passing --force-rebase to my rebase invocation, which fixes
> this particular problem by leaving me in a detached head (as in the last case
> above).
>
> I hit this in production on git 2.30.2 (debian bullseye), but reproduced
> locally using the latest git main, which is git version 2.35.1.415.gc2162907.
> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If
> it helps: with my rebase.autosquash = true, the bad case above does not behave
> badly and leaves me in detached head as I'd expect.) It's totally possible
> this isn't _meant_ to work, in which case I think the docs could use an
> update.
>
> Thanks!
>
> -- 
> Michael McClimon
> michael@mcclimon.org

Re: Bug with rebase and commit hashes

[John Cai]

On 10 Mar 2022, at 17:25, John Cai wrote:

> Hi Michael,
>
> I looked into this a little bit. I may not have the full answer, so others may want to chime in.
>
> On 10 Mar 2022, at 11:16, Michael McClimon wrote:
>
>> I have run into a bug with rebase when operating with commit hashes directly
>> (rather than branch names).
>>
>> Say that I have two branches, main and topic. Branch topic consists of a
>> single commit whose parent is main. If I'm on main, and I run
>> 'git rebase main topic', I end up on branch topic, as expected (my prompt here
>> displays the current branch):
>>
>> [~/scratch on main] $ git rebase main topic
>> Successfully rebased and updated refs/heads/topic.
>> [~/scratch on topic] $
>>
>>
>> If I do exactly the same thing, but substitute the commit shas for those
>> branches, git _doesn't_ leave me on branch topic, but instead fast-forwards
>> main to topic. This is very surprising to me!
>>
>> [~/scratch on main] $ git rev-parse main
>> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
>> [~/scratch on main] $ git rev-parse topic
>> c3c862105dfbb2f30137a0875e8e5d9dfec334f8
>> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
>> Current branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.
>> [~/scratch on main] $ git rev-parse main
>> c3c862105dfbb2f30137a0875e8e5d9dfec334f8
>
> Taking a look at the code in bulitin/rebase.c, it will check whether or not
> <branch> is resolveable as a valid ref. If not, then this code [1] sets the head
> name that will get switched to, to NULL.
>
> Then, when checkout_up_to_date() is called, it calls reset_head() which does not
> switch to the branch since opts->branch is NULL. But (and I haven't looked into
> detail how reset_head() works) it seems like it will still set the current HEAD
> (main) to $(git rev-parse topic).
>
> This diff seems to fix this behavior, but it's untested.
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e72..bcbac75c705e 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1634,7 +1634,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                                 die(_("no such branch/commit '%s'"),
>                                     branch_name);
>                         oidcpy(&options.orig_head, &commit->object.oid);
> -                       options.head_name = NULL;
> +                       options.head_name = xstrdup(buf.buf);
>                 }
>         } else if (argc == 0) {
>                 /* Do not need to switch branches, we are already on it. */

Well, upon further examination this totally does the wrong thing :) Will look into the
root cause further.

>
>
> 1. https://github.com/git/git/blob/master/builtin/rebase.c#L1637
>
>>
>>
>> Part of the reason this is surprising is that in the case when topic is not a
>> fast-forward from main (i.e., does need to be rebased), git does what I'd
>> expect, and leaves me detached on the newly rebased head.
>>
>> [~/scratch on main] $ git rev-parse main
>> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6
>> [~/scratch on main] $ git rev-parse topic
>> 8d7d712bad0c32cd87aa814730317178b2e46b93
>> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)
>> Successfully rebased and updated detached HEAD.
>> [~/scratch at 1477bc43] $ git rev-parse HEAD
>> 1477bc43a3bc7868ba1da8a919a60432bedbd34a
>>
>>
>> I ran into this because I was writing some software to enforce semilinear
>> history (all commits on main are merge commits, and the topic branches are all
>> rebased on main before merge). That workflow is: for every branch,
>> rebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.
>> Because of this bug, when we got to the merge --no-ff, git didn't do anything
>> at all, because it had already fast-forwarded main! I worked around this in
>> my program by just passing --force-rebase to my rebase invocation, which fixes
>> this particular problem by leaving me in a detached head (as in the last case
>> above).
>>
>> I hit this in production on git 2.30.2 (debian bullseye), but reproduced
>> locally using the latest git main, which is git version 2.35.1.415.gc2162907.
>> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If
>> it helps: with my rebase.autosquash = true, the bad case above does not behave
>> badly and leaves me in detached head as I'd expect.) It's totally possible
>> this isn't _meant_ to work, in which case I think the docs could use an
>> update.
>>
>> Thanks!
>>
>> -- 
>> Michael McClimon
>> michael@mcclimon.org

Re: Bug with rebase and commit hashes

[Junio C Hamano]

John Cai <johncai86@gmail.com> writes:

>> I hit this in production on git 2.30.2 (debian bullseye), but reproduced
>> locally using the latest git main, which is git version 2.35.1.415.gc2162907.
>> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If
>> it helps: with my rebase.autosquash = true, the bad case above does not behave
>> badly and leaves me in detached head as I'd expect.) It's totally possible
>> this isn't _meant_ to work, in which case I think the docs could use an
>> update.

A quick bisect session leads us to 176f5d96 (built-in rebase
--autostash: leave the current branch alone if possible,
2018-11-07), but I do not know how much commonality exists in the
current code (read: we may well have inherited the bug from there,
or we rewrote it completely away but reinvented the same bug in the
current code).

with this test script dropped as t/x9999-c.sh
--- >8 ---
#!/bin/sh

test_description='rebase???'

. ./test-lib.sh

test_expect_success setup '
	git init &&
	>file &&
	git add file &&
	git commit -m initial &&
	git checkout -b side &&
	echo >>file &&
	git commit -a -m side &&
	git checkout master &&
	git tag hold
'

test_expect_success rebase '
	git checkout -B master hold &&
	git rev-parse master >pre &&
	git rebase $(git rev-parse master) $(git rev-parse side) &&
	git rev-parse master >post &&
	test_cmp pre post
'

test_done
--- 8< ---

and this as "git bisect run ./runme.sh" script:

--- >8 ---
#!/bin/sh

make -j16 NO_OPENSSL=Yes CFLAGS="-g -O" || exit 125
cd t && sh -v ./x9999-c.sh -i -v
--- 8< ---

The NO_OPENSSL thing is there only because I started bisecting from
an ancient version that no longer compiles out of the box in my
environment.

Re: Bug with rebase and commit hashes

[Michael McClimon]

Thanks both! I had a look at this on the couch this evening, and with the
caveat that I am not at all a C programmer, I think have a patch that fixes
it:

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b6..82fb5e2c 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);


I haven't yet run the entire test suite, but I did run all the t*-rebase*
tests, which passed, including Junio's up-thread here. If this seems not
totally off-base, then I'll actually read the "my first contribution" docs and
send in a proper patch and whatnot.

-- 
Michael McClimon
michael@mcclimon.org

Re: Bug with rebase and commit hashes

[John Cai]

Hey Michael!

On 11 Mar 2022, at 22:10, Michael McClimon wrote:

> Thanks both! I had a look at this on the couch this evening, and with the
> caveat that I am not at all a C programmer, I think have a patch that fixes
> it:
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b6..82fb5e2c 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);
>

Thanks for looking into this yourself! I actually have a patch out already [1],
which is pretty similar to what you suggested.

1. https://lore.kernel.org/git/pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com/
>
> I haven't yet run the entire test suite, but I did run all the t*-rebase*
> tests, which passed, including Junio's up-thread here. If this seems not
> totally off-base, then I'll actually read the "my first contribution" docs and
> send in a proper patch and whatnot.
>
> -- 
> Michael McClimon
> michael@mcclimon.org

Re: Bug with rebase and commit hashes

[Michael McClimon]

> Thanks for looking into this yourself! I actually have a patch out already [1],
> which is pretty similar to what you suggested.
>
> 1. https://lore.kernel.org/git/pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com/

Oh hey, I missed this when I was skimming the list archives.  Thanks for the
fix, and I'm pleased that I was mostly on the right track!

-- 
Michael McClimon
michael@mcclimon.org