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