Re: Bug with rebase and commit hashes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux