* Bug with rebase and commit hashes @ 2022-03-10 16:16 Michael McClimon 2022-03-10 22:25 ` John Cai 0 siblings, 1 reply; 7+ messages in thread From: Michael McClimon @ 2022-03-10 16:16 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-10 16:16 Bug with rebase and commit hashes Michael McClimon @ 2022-03-10 22:25 ` John Cai 2022-03-10 22:46 ` John Cai 2022-03-10 22:55 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: John Cai @ 2022-03-10 22:25 UTC (permalink / raw) To: Michael McClimon; +Cc: git 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-10 22:25 ` John Cai @ 2022-03-10 22:46 ` John Cai 2022-03-12 3:10 ` Michael McClimon 2022-03-10 22:55 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: John Cai @ 2022-03-10 22:46 UTC (permalink / raw) To: Michael McClimon; +Cc: git 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-10 22:46 ` John Cai @ 2022-03-12 3:10 ` Michael McClimon 2022-03-12 13:32 ` John Cai 0 siblings, 1 reply; 7+ messages in thread From: Michael McClimon @ 2022-03-12 3:10 UTC (permalink / raw) To: John Cai; +Cc: git 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-12 3:10 ` Michael McClimon @ 2022-03-12 13:32 ` John Cai 2022-03-12 14:21 ` Michael McClimon 0 siblings, 1 reply; 7+ messages in thread From: John Cai @ 2022-03-12 13:32 UTC (permalink / raw) To: Michael McClimon; +Cc: git 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-12 13:32 ` John Cai @ 2022-03-12 14:21 ` Michael McClimon 0 siblings, 0 replies; 7+ messages in thread From: Michael McClimon @ 2022-03-12 14:21 UTC (permalink / raw) To: John Cai; +Cc: git > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Bug with rebase and commit hashes 2022-03-10 22:25 ` John Cai 2022-03-10 22:46 ` John Cai @ 2022-03-10 22:55 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2022-03-10 22:55 UTC (permalink / raw) To: John Cai; +Cc: Michael McClimon, git 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-12 14:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-10 16:16 Bug with rebase and commit hashes Michael McClimon 2022-03-10 22:25 ` John Cai 2022-03-10 22:46 ` John Cai 2022-03-12 3:10 ` Michael McClimon 2022-03-12 13:32 ` John Cai 2022-03-12 14:21 ` Michael McClimon 2022-03-10 22:55 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).