{"thread":{"id":"57277","subject":"[BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'","startedAt":"2022-01-20T16:51:42Z","lastAt":"2022-01-23T16:38:51Z","messageCount":16,"participants":["Todd Zullinger","Ævar Arnfjörð Bjarmason","Junio C Hamano","Petr Šplíchal","Johannes Sixt","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"446579","messageId":"YemTGQZ97vAPUPY0@pobox.com","threadId":"57277","inReplyTo":null,"subject":"[BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-01-20T16:51:37Z","receivedAt":"2022-01-20T16:51:42Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nA bug was filed in the Fedora/Red Hat bugzilla today for the\ngit 2.35.0 rc (rc1, but it's the same in rc2).  Petr (Cc'd),\nran the following\n\n    git clone https://github.com/psss/fmf /tmp/fmf\n    cd /tmp/fmf\n    cp .git/refs/remotes/origin/HEAD .git/refs/heads/__DEFAULT__\n    git checkout -f __DEFAULT__\n    git checkout -f __DEFAULT__\n\nThe second git checkout call runs into the BUG() call added\nin 9081a421a6 (checkout: fix \"branch info\" memory leaks,\n2021-11-16):\n\n    BUG: builtin/checkout.c:1098: should be able to skip\n    past 'refs/heads/' in 'refs/remotes/origin/master'!\n    Aborted (core dumped)\n\nThis worked in 2.34.1, so it's new to 2.35.0.  Should this\nwork or does the manual copy to setup a branch fall into a\nnew category of \"don't do that\"?\n\n(It's novel to get a bug report from rc testing of a distro\nbuild -- that doesn't happen often.)\n\n-- \nTodd\n"},{"id":"446580","messageId":"YemWGf1Rc+uOF3UJ@pobox.com","threadId":"57277","inReplyTo":"YemTGQZ97vAPUPY0@pobox.com","subject":"Re: [BUG] builtin/checkout.c:1098: should be able to skip past 'refs/heads/'","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-01-20T17:04:25Z","receivedAt":"2022-01-20T17:04:47Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"I wrote:\n> A bug was filed in the Fedora/Red Hat bugzilla today for the\n> git 2.35.0 rc (rc1, but it's the same in rc2).\n\nFor completeness, https://bugzilla.redhat.com/2042920 is the\nticket.\n\n(It's apparently too early for me, despite the clock's claim\nof noon).\n\n-- \nTodd\n"},{"id":"446598","messageId":"patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com","threadId":"57277","inReplyTo":"YemTGQZ97vAPUPY0@pobox.com","subject":"[PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-20T21:26:57Z","receivedAt":"2022-01-20T21:27:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in my 9081a421a6d (checkout: fix \"branch info\" memory\nleaks, 2021-11-16) where I'd assumed that the old_branch_info.path\nwould have to start with refs/heads/*, but as has been reported[1]\nthat's not the case.\n\nAs a test case[2] to reproduce this shows the second \"git checkout\"\nhere runs into the BUG() in the pre-image. The test being added is\namended from[2] and will pass both with this change, and before\n9081a421a6. I.e. our behavior now is again the same as before that\ncommit.\n\n1. https://bugzilla.redhat.com/show_bug.cgi?id=2042920\n2. https://lore.kernel.org/git/YemTGQZ97vAPUPY0@pobox.com/\n\nReported-by: Petr Šplíchal <psplicha@redhat.com>\nReported-by: Todd Zullinger <tmz@pobox.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Thu, Jan 20 2022, Todd Zullinger wrote:\n\n> Hi,\n>\n> A bug was filed in the Fedora/Red Hat bugzilla today for the\n> git 2.35.0 rc (rc1, but it's the same in rc2).  Petr (Cc'd),\n> ran the following\n>\n>     git clone https://github.com/psss/fmf /tmp/fmf\n>     cd /tmp/fmf\n>     cp .git/refs/remotes/origin/HEAD .git/refs/heads/__DEFAULT__\n>     git checkout -f __DEFAULT__\n>     git checkout -f __DEFAULT__\n>\n> The second git checkout call runs into the BUG() call added\n> in 9081a421a6 (checkout: fix \"branch info\" memory leaks,\n> 2021-11-16):\n>\n>     BUG: builtin/checkout.c:1098: should be able to skip\n>     past 'refs/heads/' in 'refs/remotes/origin/master'!\n>     Aborted (core dumped)\n>\n> This worked in 2.34.1, so it's new to 2.35.0.  Should this\n> work or does the manual copy to setup a branch fall into a\n> new category of \"don't do that\"?\n>\n> (It's novel to get a bug report from rc testing of a distro\n> build -- that doesn't happen often.)\n\nThanks to both you and Petr for the report and easy to reproduce case,\nand sorry about causing it.\n\nIn retrospec it's a rather obvious thinko. Here's a minimal fix for\nit, along with a derived test case that I made more exhaustive to\ncheck the state of the repo before, after, and in-between the two \"git\ncheckout\" commands. As noted it'll also pass with 9081a421a6d\nreverted, showing that our behavior is the same as before that commit.\n\n builtin/checkout.c         | 11 ++++-------\n t/t2018-checkout-branch.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6a5dd2a2a22..52a47ef40e1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1090,13 +1090,10 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tFREE_AND_NULL(old_branch_info.path);\n \n \tif (old_branch_info.path) {\n-\t\tconst char *const prefix = \"refs/heads/\";\n-\t\tconst char *p;\n-\t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n-\t\t\told_branch_info.name = xstrdup(p);\n-\t\telse\n-\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n-\t\t\t    prefix, old_branch_info.path);\n+\t\tconst char *p = old_branch_info.path;\n+\n+\t\tskip_prefix(old_branch_info.path, \"refs/heads/\", &p);\n+\t\told_branch_info.name = xstrdup(p);\n \t}\n \n \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 3e93506c045..82df9b8bf64 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -85,6 +85,39 @@ test_expect_success 'setup' '\n \tgit branch -m branch1\n '\n \n+test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '\n+\tgit clone --no-tags . repo-odd-prefix &&\n+\t(\n+\t\tcd repo-odd-prefix &&\n+\n+\t\tcp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&\n+\n+\t\techo branch1 >expect.ref &&\n+\t\tgit rev-parse --abbrev-ref HEAD >actual.ref &&\n+\t\ttest_cmp expect.ref actual.ref &&\n+\n+\t\tgit checkout -f a-branch &&\n+\n+\t\techo origin/branch1 >expect.ref &&\n+\t\tgit rev-parse --abbrev-ref HEAD >actual.ref &&\n+\t\ttest_cmp expect.ref actual.ref &&\n+\n+\t\tgit checkout -f a-branch &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\t$(git rev-parse HEAD) commit\trefs/heads/a-branch\n+\t\t$(git rev-parse HEAD) commit\trefs/heads/branch1\n+\t\t$(git rev-parse HEAD) commit\trefs/remotes/origin/HEAD\n+\t\t$(git rev-parse HEAD) commit\trefs/remotes/origin/branch1\n+\t\tEOF\n+\t\tgit for-each-ref >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit rev-parse --abbrev-ref HEAD >actual &&\n+\t\ttest_cmp expect.ref actual.ref\n+\t)\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n-- \n2.35.0.rc1.864.g57621b115b6\n\n"},{"id":"446605","messageId":"xmqqee52ghwg.fsf@gitster.g","threadId":"57277","inReplyTo":"patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T22:29:51Z","receivedAt":"2022-01-20T22:29:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a regression in my 9081a421a6d (checkout: fix \"branch info\" memory\n> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path\n> would have to start with refs/heads/*, but as has been reported[1]\n> that's not the case.\n>\n> As a test case[2] to reproduce this shows the second \"git checkout\"\n> here runs into the BUG() in the pre-image. The test being added is\n> amended from[2] and will pass both with this change, and before\n> 9081a421a6. I.e. our behavior now is again the same as before that\n> commit.\n\n> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '\n> +\tgit clone --no-tags . repo-odd-prefix &&\n> +\t(\n> +\t\tcd repo-odd-prefix &&\n> +\n> +\t\tcp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&\n\nI am not sure if this is a sensible test case to begin with.\n\nIt sets up recursive symbolic ref in this way:\n\n\tHEAD points at refs/heads/a-branch\n\trefs/heads/a-branch points at refs/remotes/origin/HEAD\n\trefs/remotes/origin/HEAD points at refs/remotes/origin/branch1\n\nThe checked out branch (i.e. what HEAD points at) is nominally a\nlocal branch, but it totally violates the spirit of the safety valve\nthat says \"HEAD\" MUST point at a local branch or otherwise it is\ndetached.  Creating a commit while \"a-branch\" is checked out would\nnot affect *ANY* local branch state and instead makes an update to\nthe remote tracking branch that does not reflect *any* past states\nat the remote repository.  Even worse, a \"git fetch\" that updates\nthe remote tracking branches will make the HEAD, the index and the\nworking tree into an inconsistent state.\n\nSimply put, I think the BUG() is catching a case where we should\nhave been diagnosing as a broken repository.\n\nSo from my point of view, BUG() is indeed inappropriate because what\nthe conditional statement noticed was a broken repository, and not a\nprogramming bug.\n\nWhat we should never do is to promise this \"only kosher in letter\nbut not in spirit\" violation of \"HEAD must point at a local branch\"\nrule will be supported.\n\nSo, unless I hear more convincing arguments (and Todd's example or\nanything similar that makes \"git commit\" from that state update a\nref outside local branches is *not*), I am hesitant to call the new\nbehaviour and 9081a421a6d a regression.\n\nWhat did the code before that BUG() do when faced with this nonsense\nconfiguration?  If forbidding outright broke a sensible workflow\nthat happened to have been \"working\", I am OK to demote it to\nwarning() and restore the previous behaviour temporarily, whatever\nit was (I think it was just old_branch_info.name was left unset\nbecause we were not on local branch, but I don't know if the missing\n.name was making any irrecoverable damage).  But the longer term\ndirection should be that we treat the \"update HEAD ends up updating\nsome ref outside refs/heads/\" a longstanding bug that needs to be\nfixed.\n\nThanks.\n\n"},{"id":"446606","messageId":"YenjLhy3/q5Jnd+J@pobox.com","threadId":"57277","inReplyTo":"patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-01-20T22:33:18Z","receivedAt":"2022-01-20T22:33:26Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n> Thanks to both you and Petr for the report and easy to reproduce case,\n> and sorry about causing it.\n> \n> In retrospec it's a rather obvious thinko. Here's a minimal fix for\n> it, along with a derived test case that I made more exhaustive to\n> check the state of the repo before, after, and in-between the two \"git\n> checkout\" commands. As noted it'll also pass with 9081a421a6d\n> reverted, showing that our behavior is the same as before that commit.\n\nI can confirm the new test succeeds and the case which Petr\nreported works as it did in previous releases.\n\nMany thanks for the quick fix and more extensive tests Ævar!\n\n-- \nTodd\n"},{"id":"446607","messageId":"xmqqa6fqghq4.fsf@gitster.g","threadId":"57277","inReplyTo":"xmqqee52ghwg.fsf@gitster.g","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T22:33:39Z","receivedAt":"2022-01-20T22:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So from my point of view, BUG() is indeed inappropriate because what\n> the conditional statement noticed was a broken repository, and not a\n> programming bug.\n\nForgot to say: \"; it should have been at least a warning(), if not a die()\".\n"},{"id":"446619","messageId":"220121.86iludl4d9.gmgdl@evledraar.gmail.com","threadId":"57277","inReplyTo":"xmqqee52ghwg.fsf@gitster.g","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-21T11:14:58Z","receivedAt":"2022-01-21T11:23:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 20 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Fix a regression in my 9081a421a6d (checkout: fix \"branch info\" memory\n>> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path\n>> would have to start with refs/heads/*, but as has been reported[1]\n>> that's not the case.\n>>\n>> As a test case[2] to reproduce this shows the second \"git checkout\"\n>> here runs into the BUG() in the pre-image. The test being added is\n>> amended from[2] and will pass both with this change, and before\n>> 9081a421a6. I.e. our behavior now is again the same as before that\n>> commit.\n>\n>> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '\n>> +\tgit clone --no-tags . repo-odd-prefix &&\n>> +\t(\n>> +\t\tcd repo-odd-prefix &&\n>> +\n>> +\t\tcp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&\n>\n> I am not sure if this is a sensible test case to begin with.\n>\n> It sets up recursive symbolic ref in this way:\n>\n> \tHEAD points at refs/heads/a-branch\n> \trefs/heads/a-branch points at refs/remotes/origin/HEAD\n> \trefs/remotes/origin/HEAD points at refs/remotes/origin/branch1\n>\n> The checked out branch (i.e. what HEAD points at) is nominally a\n> local branch, but it totally violates the spirit of the safety valve\n> that says \"HEAD\" MUST point at a local branch or otherwise it is\n> detached.  Creating a commit while \"a-branch\" is checked out would\n> not affect *ANY* local branch state and instead makes an update to\n> the remote tracking branch that does not reflect *any* past states\n> at the remote repository.  Even worse, a \"git fetch\" that updates\n> the remote tracking branches will make the HEAD, the index and the\n> working tree into an inconsistent state.\n>\n> Simply put, I think the BUG() is catching a case where we should\n> have been diagnosing as a broken repository.\n\nYes, I agree that we should be spotting this . Todd/Petr: If you're able\nto describe it I'm curious about the original case you encountered that\nthe test case is derived from.\n\n> So from my point of view, BUG() is indeed inappropriate because what\n> the conditional statement noticed was a broken repository, and not a\n> programming bug.\n>\n> What we should never do is to promise this \"only kosher in letter\n> but not in spirit\" violation of \"HEAD must point at a local branch\"\n> rule will be supported.\n>\n> So, unless I hear more convincing arguments (and Todd's example or\n> anything similar that makes \"git commit\" from that state update a\n> ref outside local branches is *not*), I am hesitant to call the new\n> behaviour and 9081a421a6d a regression.\n\nWell, the user is doing odd things with git, but we should reserve BUG()\nfor things that aren't rechable. Any time a user is able to arrange our\ntooling in such a way as to call BUG() is a ... bug.\n\n> What did the code before that BUG() do when faced with this nonsense\n> configuration?  If forbidding outright broke a sensible workflow\n> that happened to have been \"working\", I am OK to demote it to\n> warning() and restore the previous behaviour temporarily, whatever\n> it was (I think it was just old_branch_info.name was left unset\n> because we were not on local branch, but I don't know if the missing\n> .name was making any irrecoverable damage).  But the longer term\n> direction should be that we treat the \"update HEAD ends up updating\n> some ref outside refs/heads/\" a longstanding bug that needs to be\n> fixed.\n\nThe behavior with my patch here is exactly the same as before. I.e. it\nwas rather straightforward, the xstrdup() is new, but before we'd just\ntake the un-skipped string that didn't start with refs/heads/ as-is.\n\nI agree that it's better to look at this more deeply, but given the rc2\nbeing out, and this surely being something we want in the final I'd\nthink we'd want to keep this patch as-is.\n\nI.e. this is all a bit odd, but it was odd in exactly this way in v2.34.0 too.\n\nWe can add a warning() or die(), but a change that does that & convinces\nyou it's the right thing thing will require more careful consideration,\ntesting etc.\n\nSo I really think we should narrow our focus and keep this as-is.\n"},{"id":"446631","messageId":"CABXw6YMztos7UYJn0LxKiWvZ_oy55mu+k__jABdXwAW0H3J0cA@mail.gmail.com","threadId":"57277","inReplyTo":"220121.86iludl4d9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Petr Šplíchal","fromEmail":"psplicha@redhat.com","sentAt":"2022-01-21T14:29:40Z","receivedAt":"2022-01-21T14:30:16Z","isPatch":true,"sender":{"key":"psplicha@redhat.com","avatar":null},"body":"On Fri, 21 Jan 2022 at 12:23, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Thu, Jan 20 2022, Junio C Hamano wrote:\n>\n> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> >\n> >> Fix a regression in my 9081a421a6d (checkout: fix \"branch info\" memory\n> >> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path\n> >> would have to start with refs/heads/*, but as has been reported[1]\n> >> that's not the case.\n> >>\n> >> As a test case[2] to reproduce this shows the second \"git checkout\"\n> >> here runs into the BUG() in the pre-image. The test being added is\n> >> amended from[2] and will pass both with this change, and before\n> >> 9081a421a6. I.e. our behavior now is again the same as before that\n> >> commit.\n> >\n> >> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '\n> >> +    git clone --no-tags . repo-odd-prefix &&\n> >> +    (\n> >> +            cd repo-odd-prefix &&\n> >> +\n> >> +            cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&\n> >\n> > I am not sure if this is a sensible test case to begin with.\n> >\n> > It sets up recursive symbolic ref in this way:\n> >\n> >       HEAD points at refs/heads/a-branch\n> >       refs/heads/a-branch points at refs/remotes/origin/HEAD\n> >       refs/remotes/origin/HEAD points at refs/remotes/origin/branch1\n> >\n> > The checked out branch (i.e. what HEAD points at) is nominally a\n> > local branch, but it totally violates the spirit of the safety valve\n> > that says \"HEAD\" MUST point at a local branch or otherwise it is\n> > detached.  Creating a commit while \"a-branch\" is checked out would\n> > not affect *ANY* local branch state and instead makes an update to\n> > the remote tracking branch that does not reflect *any* past states\n> > at the remote repository.  Even worse, a \"git fetch\" that updates\n> > the remote tracking branches will make the HEAD, the index and the\n> > working tree into an inconsistent state.\n> >\n> > Simply put, I think the BUG() is catching a case where we should\n> > have been diagnosing as a broken repository.\n>\n> Yes, I agree that we should be spotting this . Todd/Petr: If you're able\n> to describe it I'm curious about the original case you encountered that\n> the test case is derived from.\n\nThanks for explaining in detail what's wrong about the approach.\nWe didn't know about the \"HEAD must point at a local branch\" rule\nand copying the ref seemed to be the easiest way to create a local\nbranch pointing always to the latest content of the remote default\nbranch. I described the use case here:\n\n    https://bugzilla.redhat.com/show_bug.cgi?id=2042920#c2\n\nBasically we just need to checkout and reset --hard to the default\nremote branch after entering a git repository while HEAD can be\npointing anywhere. Could you suggest some more straightforward way\nto achieve this from a script? Thanks.\n\npsss...\n\n> > So from my point of view, BUG() is indeed inappropriate because what\n> > the conditional statement noticed was a broken repository, and not a\n> > programming bug.\n> >\n> > What we should never do is to promise this \"only kosher in letter\n> > but not in spirit\" violation of \"HEAD must point at a local branch\"\n> > rule will be supported.\n> >\n> > So, unless I hear more convincing arguments (and Todd's example or\n> > anything similar that makes \"git commit\" from that state update a\n> > ref outside local branches is *not*), I am hesitant to call the new\n> > behaviour and 9081a421a6d a regression.\n>\n> Well, the user is doing odd things with git, but we should reserve BUG()\n> for things that aren't rechable. Any time a user is able to arrange our\n> tooling in such a way as to call BUG() is a ... bug.\n>\n> > What did the code before that BUG() do when faced with this nonsense\n> > configuration?  If forbidding outright broke a sensible workflow\n> > that happened to have been \"working\", I am OK to demote it to\n> > warning() and restore the previous behaviour temporarily, whatever\n> > it was (I think it was just old_branch_info.name was left unset\n> > because we were not on local branch, but I don't know if the missing\n> > .name was making any irrecoverable damage).  But the longer term\n> > direction should be that we treat the \"update HEAD ends up updating\n> > some ref outside refs/heads/\" a longstanding bug that needs to be\n> > fixed.\n>\n> The behavior with my patch here is exactly the same as before. I.e. it\n> was rather straightforward, the xstrdup() is new, but before we'd just\n> take the un-skipped string that didn't start with refs/heads/ as-is.\n>\n> I agree that it's better to look at this more deeply, but given the rc2\n> being out, and this surely being something we want in the final I'd\n> think we'd want to keep this patch as-is.\n>\n> I.e. this is all a bit odd, but it was odd in exactly this way in v2.34.0 too.\n>\n> We can add a warning() or die(), but a change that does that & convinces\n> you it's the right thing thing will require more careful consideration,\n> testing etc.\n>\n> So I really think we should narrow our focus and keep this as-is.\n\n"},{"id":"446655","messageId":"xmqqczkkg51h.fsf@gitster.g","threadId":"57277","inReplyTo":"220121.86iludl4d9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-21T21:19:54Z","receivedAt":"2022-01-21T21:19:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> So, unless I hear more convincing arguments (and Todd's example or\n>> anything similar that makes \"git commit\" from that state update a\n>> ref outside local branches is *not*), I am hesitant to call the new\n>> behaviour and 9081a421a6d a regression.\n>\n> Well, the user is doing odd things with git, but we should reserve BUG()\n> for things that aren't rechable. Any time a user is able to arrange our\n> tooling in such a way as to call BUG() is a ... bug.\n\nYes, I concur.\n\n>> What did the code before that BUG() do when faced with this nonsense\n>> configuration?  If forbidding outright broke a sensible workflow\n>> that happened to have been \"working\", I am OK to demote it to\n>> warning() and restore the previous behaviour temporarily, whatever\n>> it was (I think it was just old_branch_info.name was left unset\n>> because we were not on local branch, but I don't know if the missing\n>> .name was making any irrecoverable damage).  But the longer term\n>> direction should be that we treat the \"update HEAD ends up updating\n>> some ref outside refs/heads/\" a longstanding bug that needs to be\n>> fixed.\n>\n> The behavior with my patch here is exactly the same as before. I.e. it\n> was rather straightforward, the xstrdup() is new, but before we'd just\n> take the un-skipped string that didn't start with refs/heads/ as-is.\n\nOK, that might have done a wrong thing (instead of dying) for a\nstrange settings like that, but the change was never about\ntightening and detecting such a strangeness but only about plugging\nleaks, so reverting that narrow part of the patch is the right thing\nto do.\n\n> I agree that it's better to look at this more deeply, but given the rc2\n> being out, and this surely being something we want in the final I'd\n> think we'd want to keep this patch as-is.\n\nYes, except for the update in the test.  I do not think we want to\npromise what should happen to the _values_ of these refs after the\noperation at all.  If it only says \"checkout should not exit with\nnon-zero status\", I would be OK.  Promising anything more than that,\nI do not think it is a good idea.\n\nFor now, I plan to do the \"revert the check-and-BUG and nothing\nelse\" change.\n\nThanks.\n\n"},{"id":"446656","messageId":"YessbkNZlxLOTzBE@pobox.com","threadId":"57277","inReplyTo":"CABXw6YMztos7UYJn0LxKiWvZ_oy55mu+k__jABdXwAW0H3J0cA@mail.gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2022-01-21T21:58:06Z","receivedAt":"2022-01-21T21:58:15Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Petr Šplíchal wrote:\n> Thanks for explaining in detail what's wrong about the approach.\n\nYes, thank you Ævar and Junio for providing context and the\nthoughtful planning on how to fix this in both the short and\nlong term.\n\n> We didn't know about the \"HEAD must point at a local branch\" rule\n> and copying the ref seemed to be the easiest way to create a local\n> branch pointing always to the latest content of the remote default\n> branch. I described the use case here:\n> \n>     https://bugzilla.redhat.com/show_bug.cgi?id=2042920#c2\n> \n> Basically we just need to checkout and reset --hard to the default\n> remote branch after entering a git repository while HEAD can be\n> pointing anywhere. Could you suggest some more straightforward way\n> to achieve this from a script? Thanks.\n\nI'm nearly positive that I don't know the best way, but\nhere's _a_ way to do it.  It assumes the default remote name\nis origin, which seems less than ideal.\n\n(Hopefully this isn't egregiously wrong.  But if it is, I'm\nhappy to serve as an example of how _not_ to do it for\nothers.)\n\n    git clone https://github.com/psss/fmf /tmp/fmf\n    cd /tmp/fmf\n    git switch -c custom-branch\n    : work work work\n    defbranch=$(git symbolic-ref --short refs/remotes/origin/HEAD)\n    git switch -f ${defbranch#*/}\n    git reset --hard @{u}\n\nOdds are there's a better way to do this or to arrange\nthings in a way that lets you solve an easier problem.\n\n-- \nTodd\n"},{"id":"446664","messageId":"xmqqr190d2xg.fsf@gitster.g","threadId":"57277","inReplyTo":"patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-22T00:33:47Z","receivedAt":"2022-01-22T00:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 6a5dd2a2a22..52a47ef40e1 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1090,13 +1090,10 @@ static int switch_branches(const struct checkout_opts *opts,\n>  \t\tFREE_AND_NULL(old_branch_info.path);\n>  \n>  \tif (old_branch_info.path) {\n> -\t\tconst char *const prefix = \"refs/heads/\";\n> -\t\tconst char *p;\n> -\t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n> -\t\t\told_branch_info.name = xstrdup(p);\n> -\t\telse\n> -\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n> -\t\t\t    prefix, old_branch_info.path);\n> +\t\tconst char *p = old_branch_info.path;\n> +\n> +\t\tskip_prefix(old_branch_info.path, \"refs/heads/\", &p);\n> +\t\told_branch_info.name = xstrdup(p);\n>  \t}\n\nWhat we want is a faithful reversion wrt the behaviour, while\nkeeping the leakfix.\n\nThe old code did\n\n\tskip_prefix(old_branch_info.path, \"refs/heads\", &old_branch_info.name);\n\nmeaning that old_branch_info.name is left unchanged if .path did not\nbegin with \"refs/heads\".\n\nThe function that uses old_branch_info.name prepared at this point\nin the flow is merge_working_tree(), where it uses .name for the\ntextual label for the \"ancestor\" tree.  It is preferrable to have a\nbranch name, but the code is prepared to see NULL in there, where it\ncomes up with an abbreviated commit object name.\n\n                o.ancestor = old_branch_info->name;\n                if (old_branch_info->name == NULL) {\n                        strbuf_add_unique_abbrev(&old_commit_shortname,\n                                                 &old_branch_info->commit->object.oid,\n                                                 DEFAULT_ABBREV);\n                        o.ancestor = old_commit_shortname.buf;\n                }\n\nAs 9081a421a6 properly 0-initializes old_branch_info, I think it is\nsufficient to just drop the \"else BUG\" clause; we shouldn't have to\ncopy the .path that does not begin with \"refs/heads/\" there; .name\nmember would be NULL in such a case.\n\nI do not care too much about reverting the constant that is only\nused once into a literal.  So, how about doing it this way instead?\n\n builtin/checkout.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git i/builtin/checkout.c w/builtin/checkout.c\nindex 43d0275187..1fb34d537d 100644\n--- i/builtin/checkout.c\n+++ w/builtin/checkout.c\n@@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tconst char *p;\n \t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n \t\t\told_branch_info.name = xstrdup(p);\n-\t\telse\n-\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n-\t\t\t    prefix, old_branch_info.path);\n \t}\n \n \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\n"},{"id":"446665","messageId":"xmqqlez8d2e6.fsf@gitster.g","threadId":"57277","inReplyTo":"patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com","subject":"Re: [PATCH] checkout: fix BUG() case in 9081a421a6","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-22T00:45:21Z","receivedAt":"2022-01-22T00:45:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '\n> +\tgit clone --no-tags . repo-odd-prefix &&\n> +\t(\n> +\t\tcd repo-odd-prefix &&\n> +\n> +\t\tcp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&\n\nI already said that I do not think we want the exact behaviour (like\n\"remote-tracking branches are moved when we change the local branch\")\nto be cast in stone like this, but we should be able to lose the\nREFFILES prerequisite by using proper plumbing, e.g. these two lines\n\n\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n\ninstead of the direct manipulation of filesystem with \"cp\".\n\nI am tempted to say that checking the command does not trigger\nBUG(), like the attached, may be sufficient.  I do not care too\ndeeply either way, though.\n\ndiff --git i/t/t2018-checkout-branch.sh w/t/t2018-checkout-branch.sh\nindex 93be1c0eae..5dda5ad4cb 100755\n--- i/t/t2018-checkout-branch.sh\n+++ w/t/t2018-checkout-branch.sh\n@@ -85,6 +85,19 @@ test_expect_success 'setup' '\n \tgit branch -m branch1\n '\n \n+test_expect_success 'checkout a branch without refs/heads/* prefix' '\n+\tgit clone --no-tags . repo-odd-prefix &&\n+\t(\n+\t\tcd repo-odd-prefix &&\n+\n+\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n+\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n+\n+\t\tgit checkout -f a-branch &&\n+\t\tgit checkout -f a-branch\n+\t)\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n"},{"id":"446667","messageId":"xmqqbl04d1s9.fsf_-_@gitster.g","threadId":"57277","inReplyTo":"xmqqlez8d2e6.fsf@gitster.g","subject":"[PATCH] checkout: avoid BUG() when hitting a broken repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-22T00:58:30Z","receivedAt":"2022-01-22T00:58:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So, taking the two earlier comments from me together...\n\nI _think_ I was the one who spotted the funny skip_prefix() whose\nresult was not used, and suggested this unrelated check, during the\nreview.  Sorry about that.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n\nSubject: [PATCH] checkout: avoid BUG() when hitting a broken repository\n\nWhen 9081a421 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\ncleaned up existing memory leaks, we added an unrelated sanity check\nto ensure that a local branch is truly local and not a symref to\nelsewhere that dies with BUG() otherwise.  This was misguided in two\nways.  First of all, such a tightening did not belong to a leak-fix\npatch.  And the condition it detected was *not* a bug in our program\nbut a problem in user data, where warning() or die() would have been\nmore appropriate.\n\nAs the condition is not fatal (the result of computing the local\nbranch name in the code that is involved in the faulty check is only\nused as a textual label for the commit), let's revert the code to\nthe original state, i.e. strip \"refs/heads/\" to compute the local\nbranch name if possible, and otherwise leave it NULL.  The consumer\nof the information in merge_working_tree() is prepared to see NULL\nin there and act accordingly.\n\ncf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920\n\nReported-by: Petr Šplíchal <psplicha@redhat.com>\nReported-by: Todd Zullinger <tmz@pobox.com>\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/checkout.c         |  3 ---\n t/t2018-checkout-branch.sh | 13 +++++++++++++\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 43d0275187..1fb34d537d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tconst char *p;\n \t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n \t\t\told_branch_info.name = xstrdup(p);\n-\t\telse\n-\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n-\t\t\t    prefix, old_branch_info.path);\n \t}\n \n \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 93be1c0eae..5dda5ad4cb 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -85,6 +85,19 @@ test_expect_success 'setup' '\n \tgit branch -m branch1\n '\n \n+test_expect_success 'checkout a branch without refs/heads/* prefix' '\n+\tgit clone --no-tags . repo-odd-prefix &&\n+\t(\n+\t\tcd repo-odd-prefix &&\n+\n+\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n+\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n+\n+\t\tgit checkout -f a-branch &&\n+\t\tgit checkout -f a-branch\n+\t)\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n-- \n2.35.0-rc2-150-gc312dde8e9\n\n"},{"id":"446681","messageId":"3063ea48-077c-df12-de91-1e8f2f62d3e9@kdbg.org","threadId":"57277","inReplyTo":"xmqqbl04d1s9.fsf_-_@gitster.g","subject":"Re: [PATCH] checkout: avoid BUG() when hitting a broken repository","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-01-22T08:10:00Z","receivedAt":"2022-01-22T08:10:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.01.22 um 01:58 schrieb Junio C Hamano:\n> So, taking the two earlier comments from me together...\n> \n> I _think_ I was the one who spotted the funny skip_prefix() whose\n> result was not used, and suggested this unrelated check, during the\n> review.  Sorry about that.\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> \n> Subject: [PATCH] checkout: avoid BUG() when hitting a broken repository\n> \n> When 9081a421 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\n> cleaned up existing memory leaks, we added an unrelated sanity check\n> to ensure that a local branch is truly local and not a symref to\n> elsewhere that dies with BUG() otherwise.  This was misguided in two\n> ways.  First of all, such a tightening did not belong to a leak-fix\n> patch.  And the condition it detected was *not* a bug in our program\n> but a problem in user data, where warning() or die() would have been\n> more appropriate.\n> \n> As the condition is not fatal (the result of computing the local\n> branch name in the code that is involved in the faulty check is only\n> used as a textual label for the commit), let's revert the code to\n> the original state, i.e. strip \"refs/heads/\" to compute the local\n> branch name if possible, and otherwise leave it NULL.  The consumer\n> of the information in merge_working_tree() is prepared to see NULL\n> in there and act accordingly.\n> \n> cf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920\n> \n> Reported-by: Petr Šplíchal <psplicha@redhat.com>\n> Reported-by: Todd Zullinger <tmz@pobox.com>\n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/checkout.c         |  3 ---\n>  t/t2018-checkout-branch.sh | 13 +++++++++++++\n>  2 files changed, 13 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 43d0275187..1fb34d537d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,\n>  \t\tconst char *p;\n>  \t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n>  \t\t\told_branch_info.name = xstrdup(p);\n> -\t\telse\n> -\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n> -\t\t\t    prefix, old_branch_info.path);\n>  \t}\n>  \n>  \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 93be1c0eae..5dda5ad4cb 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -85,6 +85,19 @@ test_expect_success 'setup' '\n>  \tgit branch -m branch1\n>  '\n>  \n> +test_expect_success 'checkout a branch without refs/heads/* prefix' '\n> +\tgit clone --no-tags . repo-odd-prefix &&\n> +\t(\n> +\t\tcd repo-odd-prefix &&\n> +\n> +\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n> +\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n> +\n> +\t\tgit checkout -f a-branch &&\n> +\t\tgit checkout -f a-branch\n\nI haven't grasped the hairy details of the circumstances regarding this\nissue, and are observing this thread only from the sideline. I wonder\nwhether there is a significance that there are two identical checkout\ncommands in a row. In particular, could the second checkout not just\nswitch back to main? A comment in the test case would help me and future\nreaders.\n\n> +\t)\n> +'\n> +\n>  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n>  \ttest_when_finished \"\n>  \t\tgit checkout branch1 &&\n\n-- Hannes\n"},{"id":"446690","messageId":"220122.86y238ht7r.gmgdl@evledraar.gmail.com","threadId":"57277","inReplyTo":"xmqqbl04d1s9.fsf_-_@gitster.g","subject":"Re: [PATCH] checkout: avoid BUG() when hitting a broken repository","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-22T11:55:02Z","receivedAt":"2022-01-22T12:04:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 21 2022, Junio C Hamano wrote:\n\n> So, taking the two earlier comments from me together...\n>\n> I _think_ I was the one who spotted the funny skip_prefix() whose\n> result was not used, and suggested this unrelated check, during the\n> review.  Sorry about that.\n\nWhere was that? I don't see a comment like that in the original\nthread[1], or do you mean the recent one in [2]?\n\nDoesn't matter much now, but whatever it was I can't find it nor recall\nit, or I've misread this.\n\n> [...]\n> +test_expect_success 'checkout a branch without refs/heads/* prefix' '\n> +\tgit clone --no-tags . repo-odd-prefix &&\n> +\t(\n> +\t\tcd repo-odd-prefix &&\n> +\n> +\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n> +\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n> +\n> +\t\tgit checkout -f a-branch &&\n> +\t\tgit checkout -f a-branch\n> +\t)\n> +'\n> +\n>  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n>  \ttest_when_finished \"\n>  \t\tgit checkout branch1 &&\n\nIt's shortly before the release, so whatever fixes the bug is good with\nme, and this patch works.\n\nI don't think dropping the parts of the tests that actually check the\nresulting repository state is a good change in this re-imagining of my\ninitial fix[3].\n\nI see that per your [4] you disagree with the current behavior being\n\"cast in stone\". I also think we should change it, I just think testing\nexactly what state we're in before and after will make that easier.\n\nHere we're just testing that we don't die, and not even that it's not a\nnoop. If and when we change the behavior it'll be extra work to check\nthat we didn't change something we didn't expect (and basically\nrequiring digging up [3] again).\n\nIn this case we didn't have any test coverage (hence missing the\nregression), and with this test we still don't have meaningful coverage.\n\nIf you're looking to clearly mark things that are desired v.s. expected\nbehavior wouldn't that be better done in general via something like a\nnew \"test_expect_oddity\"?\n\nAgain, for the upcoming release I think this is fine. I'd just like to\nclarify the above, since this isn't the first time we've had a back and\nforth where you wanted a less specific test that (in the \"make coverage\"\netc. sense) would lose coverage v.s. a more specific check.\n\n1. https://lore.kernel.org/git/patch-1.1-9b17170b794-20211014T000949Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/xmqqr190d2xg.fsf@gitster.g/\n3. https://lore.kernel.org/git/patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com/\n4. https://lore.kernel.org/git/xmqqlez8d2e6.fsf@gitster.g/\n"},{"id":"446719","messageId":"nycvar.QRO.7.76.6.2201231738050.2121@tvgsbejvaqbjf.bet","threadId":"57277","inReplyTo":"xmqqbl04d1s9.fsf_-_@gitster.g","subject":"Re: [PATCH] checkout: avoid BUG() when hitting a broken repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-23T16:38:34Z","receivedAt":"2022-01-23T16:38:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 21 Jan 2022, Junio C Hamano wrote:\n\n> So, taking the two earlier comments from me together...\n>\n> I _think_ I was the one who spotted the funny skip_prefix() whose\n> result was not used, and suggested this unrelated check, during the\n> review.  Sorry about that.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n>\n> Subject: [PATCH] checkout: avoid BUG() when hitting a broken repository\n>\n> When 9081a421 (checkout: fix \"branch info\" memory leaks, 2021-11-16)\n> cleaned up existing memory leaks, we added an unrelated sanity check\n> to ensure that a local branch is truly local and not a symref to\n> elsewhere that dies with BUG() otherwise.  This was misguided in two\n> ways.  First of all, such a tightening did not belong to a leak-fix\n> patch.  And the condition it detected was *not* a bug in our program\n> but a problem in user data, where warning() or die() would have been\n> more appropriate.\n>\n> As the condition is not fatal (the result of computing the local\n> branch name in the code that is involved in the faulty check is only\n> used as a textual label for the commit), let's revert the code to\n> the original state, i.e. strip \"refs/heads/\" to compute the local\n> branch name if possible, and otherwise leave it NULL.  The consumer\n> of the information in merge_working_tree() is prepared to see NULL\n> in there and act accordingly.\n>\n> cf. https://bugzilla.redhat.com/show_bug.cgi?id=2042920\n\nThe commit message, as well as the patch, look good to me.\n\nThank you,\nDscho\n\n> Reported-by: Petr Šplíchal <psplicha@redhat.com>\n> Reported-by: Todd Zullinger <tmz@pobox.com>\n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/checkout.c         |  3 ---\n>  t/t2018-checkout-branch.sh | 13 +++++++++++++\n>  2 files changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 43d0275187..1fb34d537d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1094,9 +1094,6 @@ static int switch_branches(const struct checkout_opts *opts,\n>  \t\tconst char *p;\n>  \t\tif (skip_prefix(old_branch_info.path, prefix, &p))\n>  \t\t\told_branch_info.name = xstrdup(p);\n> -\t\telse\n> -\t\t\tBUG(\"should be able to skip past '%s' in '%s'!\",\n> -\t\t\t    prefix, old_branch_info.path);\n>  \t}\n>\n>  \tif (opts->new_orphan_branch && opts->orphan_from_empty_tree) {\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 93be1c0eae..5dda5ad4cb 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -85,6 +85,19 @@ test_expect_success 'setup' '\n>  \tgit branch -m branch1\n>  '\n>\n> +test_expect_success 'checkout a branch without refs/heads/* prefix' '\n> +\tgit clone --no-tags . repo-odd-prefix &&\n> +\t(\n> +\t\tcd repo-odd-prefix &&\n> +\n> +\t\torigin=$(git symbolic-ref refs/remotes/origin/HEAD) &&\n> +\t\tgit symbolic-ref refs/heads/a-branch \"$origin\" &&\n> +\n> +\t\tgit checkout -f a-branch &&\n> +\t\tgit checkout -f a-branch\n> +\t)\n> +'\n> +\n>  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n>  \ttest_when_finished \"\n>  \t\tgit checkout branch1 &&\n> --\n> 2.35.0-rc2-150-gc312dde8e9\n>\n>\n"}]}