{"thread":{"id":"57530","subject":"Bug with rebase and commit hashes","startedAt":"2022-03-10T16:16:33Z","lastAt":"2022-03-12T14:22:02Z","messageCount":7,"participants":["Michael McClimon","John Cai","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"451036","messageId":"YiokTm3GxIZQQUow@newk","threadId":"57530","inReplyTo":null,"subject":"Bug with rebase and commit hashes","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-03-10T16:16:14Z","receivedAt":"2022-03-10T16:16:33Z","isPatch":false,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"I have run into a bug with rebase when operating with commit hashes directly\n(rather than branch names).\n\nSay that I have two branches, main and topic. Branch topic consists of a\nsingle commit whose parent is main. If I'm on main, and I run\n'git rebase main topic', I end up on branch topic, as expected (my prompt here\ndisplays the current branch):\n\n[~/scratch on main] $ git rebase main topic\nSuccessfully rebased and updated refs/heads/topic.\n[~/scratch on topic] $\n\n\nIf I do exactly the same thing, but substitute the commit shas for those\nbranches, git _doesn't_ leave me on branch topic, but instead fast-forwards\nmain to topic. This is very surprising to me!\n\n[~/scratch on main] $ git rev-parse main\n464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n[~/scratch on main] $ git rev-parse topic\nc3c862105dfbb2f30137a0875e8e5d9dfec334f8\n[~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\nCurrent branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.\n[~/scratch on main] $ git rev-parse main\nc3c862105dfbb2f30137a0875e8e5d9dfec334f8\n\n\nPart of the reason this is surprising is that in the case when topic is not a\nfast-forward from main (i.e., does need to be rebased), git does what I'd\nexpect, and leaves me detached on the newly rebased head.\n\n[~/scratch on main] $ git rev-parse main\n464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n[~/scratch on main] $ git rev-parse topic\n8d7d712bad0c32cd87aa814730317178b2e46b93\n[~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\nSuccessfully rebased and updated detached HEAD.\n[~/scratch at 1477bc43] $ git rev-parse HEAD\n1477bc43a3bc7868ba1da8a919a60432bedbd34a\n\n\nI ran into this because I was writing some software to enforce semilinear\nhistory (all commits on main are merge commits, and the topic branches are all\nrebased on main before merge). That workflow is: for every branch,\nrebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.\nBecause of this bug, when we got to the merge --no-ff, git didn't do anything\nat all, because it had already fast-forwarded main! I worked around this in\nmy program by just passing --force-rebase to my rebase invocation, which fixes\nthis particular problem by leaving me in a detached head (as in the last case\nabove).\n\nI hit this in production on git 2.30.2 (debian bullseye), but reproduced\nlocally using the latest git main, which is git version 2.35.1.415.gc2162907.\nIn both cases I wiped my user gitconfig, so I'm using only the defaults. (If\nit helps: with my rebase.autosquash = true, the bad case above does not behave\nbadly and leaves me in detached head as I'd expect.) It's totally possible\nthis isn't _meant_ to work, in which case I think the docs could use an\nupdate.\n\nThanks!\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n\n"},{"id":"451086","messageId":"C92B297B-0634-40CC-B1C6-B2A7325BF7DA@gmail.com","threadId":"57530","inReplyTo":"YiokTm3GxIZQQUow@newk","subject":"Re: Bug with rebase and commit hashes","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-10T22:25:16Z","receivedAt":"2022-03-10T22:25:21Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Michael,\n\nI looked into this a little bit. I may not have the full answer, so others may want to chime in.\n\nOn 10 Mar 2022, at 11:16, Michael McClimon wrote:\n\n> I have run into a bug with rebase when operating with commit hashes directly\n> (rather than branch names).\n>\n> Say that I have two branches, main and topic. Branch topic consists of a\n> single commit whose parent is main. If I'm on main, and I run\n> 'git rebase main topic', I end up on branch topic, as expected (my prompt here\n> displays the current branch):\n>\n> [~/scratch on main] $ git rebase main topic\n> Successfully rebased and updated refs/heads/topic.\n> [~/scratch on topic] $\n>\n>\n> If I do exactly the same thing, but substitute the commit shas for those\n> branches, git _doesn't_ leave me on branch topic, but instead fast-forwards\n> main to topic. This is very surprising to me!\n>\n> [~/scratch on main] $ git rev-parse main\n> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n> [~/scratch on main] $ git rev-parse topic\n> c3c862105dfbb2f30137a0875e8e5d9dfec334f8\n> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\n> Current branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.\n> [~/scratch on main] $ git rev-parse main\n> c3c862105dfbb2f30137a0875e8e5d9dfec334f8\n\nTaking a look at the code in bulitin/rebase.c, it will check whether or not\n<branch> is resolveable as a valid ref. If not, then this code [1] sets the head\nname that will get switched to, to NULL.\n\nThen, when checkout_up_to_date() is called, it calls reset_head() which does not\nswitch to the branch since opts->branch is NULL. But (and I haven't looked into\ndetail how reset_head() works) it seems like it will still set the current HEAD\n(main) to $(git rev-parse topic).\n\nThis diff seems to fix this behavior, but it's untested.\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e72..bcbac75c705e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1634,7 +1634,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n                                die(_(\"no such branch/commit '%s'\"),\n                                    branch_name);\n                        oidcpy(&options.orig_head, &commit->object.oid);\n-                       options.head_name = NULL;\n+                       options.head_name = xstrdup(buf.buf);\n                }\n        } else if (argc == 0) {\n                /* Do not need to switch branches, we are already on it. */\n\n\n1. https://github.com/git/git/blob/master/builtin/rebase.c#L1637\n\n>\n>\n> Part of the reason this is surprising is that in the case when topic is not a\n> fast-forward from main (i.e., does need to be rebased), git does what I'd\n> expect, and leaves me detached on the newly rebased head.\n>\n> [~/scratch on main] $ git rev-parse main\n> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n> [~/scratch on main] $ git rev-parse topic\n> 8d7d712bad0c32cd87aa814730317178b2e46b93\n> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\n> Successfully rebased and updated detached HEAD.\n> [~/scratch at 1477bc43] $ git rev-parse HEAD\n> 1477bc43a3bc7868ba1da8a919a60432bedbd34a\n>\n>\n> I ran into this because I was writing some software to enforce semilinear\n> history (all commits on main are merge commits, and the topic branches are all\n> rebased on main before merge). That workflow is: for every branch,\n> rebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.\n> Because of this bug, when we got to the merge --no-ff, git didn't do anything\n> at all, because it had already fast-forwarded main! I worked around this in\n> my program by just passing --force-rebase to my rebase invocation, which fixes\n> this particular problem by leaving me in a detached head (as in the last case\n> above).\n>\n> I hit this in production on git 2.30.2 (debian bullseye), but reproduced\n> locally using the latest git main, which is git version 2.35.1.415.gc2162907.\n> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If\n> it helps: with my rebase.autosquash = true, the bad case above does not behave\n> badly and leaves me in detached head as I'd expect.) It's totally possible\n> this isn't _meant_ to work, in which case I think the docs could use an\n> update.\n>\n> Thanks!\n>\n> -- \n> Michael McClimon\n> michael@mcclimon.org\n"},{"id":"451095","messageId":"036EAEE4-D957-41F2-8B17-3AD7B2E49611@gmail.com","threadId":"57530","inReplyTo":"C92B297B-0634-40CC-B1C6-B2A7325BF7DA@gmail.com","subject":"Re: Bug with rebase and commit hashes","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-10T22:46:02Z","receivedAt":"2022-03-10T22:46:06Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\nOn 10 Mar 2022, at 17:25, John Cai wrote:\n\n> Hi Michael,\n>\n> I looked into this a little bit. I may not have the full answer, so others may want to chime in.\n>\n> On 10 Mar 2022, at 11:16, Michael McClimon wrote:\n>\n>> I have run into a bug with rebase when operating with commit hashes directly\n>> (rather than branch names).\n>>\n>> Say that I have two branches, main and topic. Branch topic consists of a\n>> single commit whose parent is main. If I'm on main, and I run\n>> 'git rebase main topic', I end up on branch topic, as expected (my prompt here\n>> displays the current branch):\n>>\n>> [~/scratch on main] $ git rebase main topic\n>> Successfully rebased and updated refs/heads/topic.\n>> [~/scratch on topic] $\n>>\n>>\n>> If I do exactly the same thing, but substitute the commit shas for those\n>> branches, git _doesn't_ leave me on branch topic, but instead fast-forwards\n>> main to topic. This is very surprising to me!\n>>\n>> [~/scratch on main] $ git rev-parse main\n>> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n>> [~/scratch on main] $ git rev-parse topic\n>> c3c862105dfbb2f30137a0875e8e5d9dfec334f8\n>> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\n>> Current branch c3c862105dfbb2f30137a0875e8e5d9dfec334f8 is up to date.\n>> [~/scratch on main] $ git rev-parse main\n>> c3c862105dfbb2f30137a0875e8e5d9dfec334f8\n>\n> Taking a look at the code in bulitin/rebase.c, it will check whether or not\n> <branch> is resolveable as a valid ref. If not, then this code [1] sets the head\n> name that will get switched to, to NULL.\n>\n> Then, when checkout_up_to_date() is called, it calls reset_head() which does not\n> switch to the branch since opts->branch is NULL. But (and I haven't looked into\n> detail how reset_head() works) it seems like it will still set the current HEAD\n> (main) to $(git rev-parse topic).\n>\n> This diff seems to fix this behavior, but it's untested.\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b29ad2b65e72..bcbac75c705e 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1634,7 +1634,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>                                 die(_(\"no such branch/commit '%s'\"),\n>                                     branch_name);\n>                         oidcpy(&options.orig_head, &commit->object.oid);\n> -                       options.head_name = NULL;\n> +                       options.head_name = xstrdup(buf.buf);\n>                 }\n>         } else if (argc == 0) {\n>                 /* Do not need to switch branches, we are already on it. */\n\nWell, upon further examination this totally does the wrong thing :) Will look into the\nroot cause further.\n\n>\n>\n> 1. https://github.com/git/git/blob/master/builtin/rebase.c#L1637\n>\n>>\n>>\n>> Part of the reason this is surprising is that in the case when topic is not a\n>> fast-forward from main (i.e., does need to be rebased), git does what I'd\n>> expect, and leaves me detached on the newly rebased head.\n>>\n>> [~/scratch on main] $ git rev-parse main\n>> 464adc6a6f8aa0a943dbf886df1eb6497f70f6e6\n>> [~/scratch on main] $ git rev-parse topic\n>> 8d7d712bad0c32cd87aa814730317178b2e46b93\n>> [~/scratch on main] $ git rebase $(git rev-parse main) $(git rev-parse topic)\n>> Successfully rebased and updated detached HEAD.\n>> [~/scratch at 1477bc43] $ git rev-parse HEAD\n>> 1477bc43a3bc7868ba1da8a919a60432bedbd34a\n>>\n>>\n>> I ran into this because I was writing some software to enforce semilinear\n>> history (all commits on main are merge commits, and the topic branches are all\n>> rebased on main before merge). That workflow is: for every branch,\n>> rebase $main_sha $topic_sha, then checkout main and merge --no-ff $topic_sha.\n>> Because of this bug, when we got to the merge --no-ff, git didn't do anything\n>> at all, because it had already fast-forwarded main! I worked around this in\n>> my program by just passing --force-rebase to my rebase invocation, which fixes\n>> this particular problem by leaving me in a detached head (as in the last case\n>> above).\n>>\n>> I hit this in production on git 2.30.2 (debian bullseye), but reproduced\n>> locally using the latest git main, which is git version 2.35.1.415.gc2162907.\n>> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If\n>> it helps: with my rebase.autosquash = true, the bad case above does not behave\n>> badly and leaves me in detached head as I'd expect.) It's totally possible\n>> this isn't _meant_ to work, in which case I think the docs could use an\n>> update.\n>>\n>> Thanks!\n>>\n>> -- \n>> Michael McClimon\n>> michael@mcclimon.org\n"},{"id":"451097","messageId":"xmqqsfrpbepd.fsf@gitster.g","threadId":"57530","inReplyTo":"C92B297B-0634-40CC-B1C6-B2A7325BF7DA@gmail.com","subject":"Re: Bug with rebase and commit hashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-10T22:55:10Z","receivedAt":"2022-03-10T22:55:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai <johncai86@gmail.com> writes:\n\n>> I hit this in production on git 2.30.2 (debian bullseye), but reproduced\n>> locally using the latest git main, which is git version 2.35.1.415.gc2162907.\n>> In both cases I wiped my user gitconfig, so I'm using only the defaults. (If\n>> it helps: with my rebase.autosquash = true, the bad case above does not behave\n>> badly and leaves me in detached head as I'd expect.) It's totally possible\n>> this isn't _meant_ to work, in which case I think the docs could use an\n>> update.\n\nA quick bisect session leads us to 176f5d96 (built-in rebase\n--autostash: leave the current branch alone if possible,\n2018-11-07), but I do not know how much commonality exists in the\ncurrent code (read: we may well have inherited the bug from there,\nor we rewrote it completely away but reinvented the same bug in the\ncurrent code).\n\nwith this test script dropped as t/x9999-c.sh\n--- >8 ---\n#!/bin/sh\n\ntest_description='rebase???'\n\n. ./test-lib.sh\n\ntest_expect_success setup '\n\tgit init &&\n\t>file &&\n\tgit add file &&\n\tgit commit -m initial &&\n\tgit checkout -b side &&\n\techo >>file &&\n\tgit commit -a -m side &&\n\tgit checkout master &&\n\tgit tag hold\n'\n\ntest_expect_success rebase '\n\tgit checkout -B master hold &&\n\tgit rev-parse master >pre &&\n\tgit rebase $(git rev-parse master) $(git rev-parse side) &&\n\tgit rev-parse master >post &&\n\ttest_cmp pre post\n'\n\ntest_done\n--- 8< ---\n\nand this as \"git bisect run ./runme.sh\" script:\n\n--- >8 ---\n#!/bin/sh\n\nmake -j16 NO_OPENSSL=Yes CFLAGS=\"-g -O\" || exit 125\ncd t && sh -v ./x9999-c.sh -i -v\n--- 8< ---\n\nThe NO_OPENSSL thing is there only because I started bisecting from\nan ancient version that no longer compiles out of the box in my\nenvironment.\n"},{"id":"451207","messageId":"YiwPJD39+/hnu36j@newk","threadId":"57530","inReplyTo":"036EAEE4-D957-41F2-8B17-3AD7B2E49611@gmail.com","subject":"Re: Bug with rebase and commit hashes","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-03-12T03:10:28Z","receivedAt":"2022-03-12T03:15:53Z","isPatch":false,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"Thanks both! I had a look at this on the couch this evening, and with the\ncaveat that I am not at all a C programmer, I think have a patch that fixes\nit:\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b6..82fb5e2c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n        ropts.oid = &options->orig_head;\n        ropts.branch = options->head_name;\n        ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+       if (!ropts.branch)\n+               ropts.flags |= RESET_HEAD_DETACH;\n        ropts.head_msg = buf.buf;\n        if (reset_head(the_repository, &ropts) < 0)\n                ret = error(_(\"could not switch to %s\"), options->switch_to);\n\n\nI haven't yet run the entire test suite, but I did run all the t*-rebase*\ntests, which passed, including Junio's up-thread here. If this seems not\ntotally off-base, then I'll actually read the \"my first contribution\" docs and\nsend in a proper patch and whatnot.\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n"},{"id":"451219","messageId":"B91AE306-6836-47C1-AC99-B06A50BAF61A@gmail.com","threadId":"57530","inReplyTo":"YiwPJD39+/hnu36j@newk","subject":"Re: Bug with rebase and commit hashes","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-12T13:32:49Z","receivedAt":"2022-03-12T13:32:56Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hey Michael!\n\nOn 11 Mar 2022, at 22:10, Michael McClimon wrote:\n\n> Thanks both! I had a look at this on the couch this evening, and with the\n> caveat that I am not at all a C programmer, I think have a patch that fixes\n> it:\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b29ad2b6..82fb5e2c 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n>         ropts.oid = &options->orig_head;\n>         ropts.branch = options->head_name;\n>         ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> +       if (!ropts.branch)\n> +               ropts.flags |= RESET_HEAD_DETACH;\n>         ropts.head_msg = buf.buf;\n>         if (reset_head(the_repository, &ropts) < 0)\n>                 ret = error(_(\"could not switch to %s\"), options->switch_to);\n>\n\nThanks for looking into this yourself! I actually have a patch out already [1],\nwhich is pretty similar to what you suggested.\n\n1. https://lore.kernel.org/git/pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com/\n>\n> I haven't yet run the entire test suite, but I did run all the t*-rebase*\n> tests, which passed, including Junio's up-thread here. If this seems not\n> totally off-base, then I'll actually read the \"my first contribution\" docs and\n> send in a proper patch and whatnot.\n>\n> -- \n> Michael McClimon\n> michael@mcclimon.org\n"},{"id":"451220","messageId":"YiysZtW/vRqIYHXp@newk","threadId":"57530","inReplyTo":"B91AE306-6836-47C1-AC99-B06A50BAF61A@gmail.com","subject":"Re: Bug with rebase and commit hashes","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-03-12T14:21:26Z","receivedAt":"2022-03-12T14:22:02Z","isPatch":false,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"> Thanks for looking into this yourself! I actually have a patch out already [1],\n> which is pretty similar to what you suggested.\n>\n> 1. https://lore.kernel.org/git/pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com/\n\nOh hey, I missed this when I was skimming the list archives.  Thanks for the\nfix, and I'm pleased that I was mostly on the right track!\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n"}]}