{"thread":{"id":"58714","subject":"Bug in `git branch --delete main` when on other orphan branch","startedAt":"2022-10-29T05:46:55Z","lastAt":"2022-11-06T22:22:27Z","messageCount":12,"participants":["Martin von Zweigbergk","Jeff King","Taylor Blau","Rubén Justo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"466005","messageId":"CAESOdVBpsbJ0obD=qDjHBJg-wwWUL5sQ-7X_h13Vw39Q9QUzHA@mail.gmail.com","threadId":"58714","inReplyTo":null,"subject":"Bug in `git branch --delete main` when on other orphan branch","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@google.com","sentAt":"2022-10-29T05:46:37Z","receivedAt":"2022-10-29T05:46:55Z","isPatch":false,"sender":{"key":"martinvonz@google.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Hi,\n\nI did this:\ngit init test\ncd test\necho a > file\ngit add file\ngit commit -m a\ngit checkout --orphan other\ngit branch --delete main\n\nThe last command fails with:\nfatal: Couldn't look up commit object for HEAD\n\nThat's a bug, right? I can of course work around it with `rm\n.git/refs/heads/main`.\n"},{"id":"466194","messageId":"Y2DxxZAFbN8juHY6@coredump.intra.peff.net","threadId":"58714","inReplyTo":"CAESOdVBpsbJ0obD=qDjHBJg-wwWUL5sQ-7X_h13Vw39Q9QUzHA@mail.gmail.com","subject":"Re: Bug in `git branch --delete main` when on other orphan branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T10:15:33Z","receivedAt":"2022-11-01T10:15:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 28, 2022 at 10:46:37PM -0700, Martin von Zweigbergk wrote:\n\n> I did this:\n> git init test\n> cd test\n> echo a > file\n> git add file\n> git commit -m a\n> git checkout --orphan other\n> git branch --delete main\n> \n> The last command fails with:\n> fatal: Couldn't look up commit object for HEAD\n> \n> That's a bug, right? I can of course work around it with `rm\n> .git/refs/heads/main`.\n\nSort of. This is part of the \"is the thing we are deleting merged into\nHEAD\" check. It tries to look up the HEAD and calls die() when it can't.\nThe more correct thing, I think, would be for it to just return \"nope,\nthere is no HEAD so nothing is merged into it\".\n\nBut that probably won't make your command succeed; you'll just get:\n\n  error: The branch 'main' is not fully merged.\n\nAt which point you'd retry with \"-f\" (or \"-D\"). And then it succeeds,\nbecause the force path is smart enough to skip loading HEAD, from\n67affd5173 (git-branch -D: make it work even when on a yet-to-be-born\nbranch, 2006-11-24).\n\nAt the time, I suspect that logic was \"good enough\". You'd need \"-f\"\neither way, so it is really just a question of producing a lousy error\nmessage.\n\nBut since then, I think there are more cases. For example, 99c419c915\n(branch -d: base the \"already-merged\" safety on the branch it merges\nwith, 2009-12-29) makes it OK to delete the branch if it's merged to\nHEAD _or_ to its upstream. You don't have an upstream in your example,\nbut it's not hard to imagine one (just start the repo via \"clone\" rather\nthan from scratch).\n\nAnd in that case I think the HEAD check calling die() is actively doing\nthe wrong thing, and would prevent an otherwise successful deletion.\n\nThe fix might be as simple as:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 15be0c03ef..f6ff9084c8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -235,11 +235,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t}\n \tbranch_name_pos = strcspn(fmt, \"%\");\n \n-\tif (!force) {\n+\tif (!force)\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n-\t\tif (!head_rev)\n-\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n-\t}\n \n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n\nas the later code seems to do the right thing with the NULL head_rev. It\nwould definitely need more careful investigation (and tests!) to confirm\nthat, though.\n\nAnd in the meantime, hopefully you noticed that \"-f\" is a better\nworkaround than manually deleting the refs file. :)\n\n-Peff\n\n"},{"id":"466203","messageId":"CAESOdVDmLxj2chGZzJYPjD6bw4XqWjjrPesc2ZCiE8JLPenADw@mail.gmail.com","threadId":"58714","inReplyTo":"Y2DxxZAFbN8juHY6@coredump.intra.peff.net","subject":"Re: Bug in `git branch --delete main` when on other orphan branch","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@google.com","sentAt":"2022-11-01T15:31:39Z","receivedAt":"2022-11-01T15:32:00Z","isPatch":false,"sender":{"key":"martinvonz@google.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Tue, Nov 1, 2022 at 3:15 AM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Oct 28, 2022 at 10:46:37PM -0700, Martin von Zweigbergk wrote:\n>\n> > I did this:\n> > git init test\n> > cd test\n> > echo a > file\n> > git add file\n> > git commit -m a\n> > git checkout --orphan other\n> > git branch --delete main\n> >\n> > The last command fails with:\n> > fatal: Couldn't look up commit object for HEAD\n> >\n> > That's a bug, right? I can of course work around it with `rm\n> > .git/refs/heads/main`.\n>\n> Sort of. This is part of the \"is the thing we are deleting merged into\n> HEAD\" check. It tries to look up the HEAD and calls die() when it can't.\n> The more correct thing, I think, would be for it to just return \"nope,\n> there is no HEAD so nothing is merged into it\".\n\nAh, so that's what it was about. Thanks for looking into it!\n\n> And in the meantime, hopefully you noticed that \"-f\" is a better\n> workaround than manually deleting the refs file. :)\n\nNope, because I had no idea it was something that could be first.\nAlso, this was just in a script to reproduce an unrelated (non-Git)\nbug, so my hacky workaround was okay :)\n\nThanks!\n"},{"id":"466211","messageId":"Y2F9lkCWf/2rjT2E@nand.local","threadId":"58714","inReplyTo":"Y2DxxZAFbN8juHY6@coredump.intra.peff.net","subject":"Re: Bug in `git branch --delete main` when on other orphan branch","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:12:06Z","receivedAt":"2022-11-01T20:12:15Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 06:15:33AM -0400, Jeff King wrote:\n> On Fri, Oct 28, 2022 at 10:46:37PM -0700, Martin von Zweigbergk wrote:\n>\n> > I did this:\n> > git init test\n> > cd test\n> > echo a > file\n> > git add file\n> > git commit -m a\n> > git checkout --orphan other\n> > git branch --delete main\n> >\n> > The last command fails with:\n> > fatal: Couldn't look up commit object for HEAD\n> >\n> > That's a bug, right? I can of course work around it with `rm\n> > .git/refs/heads/main`.\n>\n> Sort of. This is part of the \"is the thing we are deleting merged into\n> HEAD\" check. It tries to look up the HEAD and calls die() when it can't.\n> The more correct thing, I think, would be for it to just return \"nope,\n> there is no HEAD so nothing is merged into it\".\n\nYeah, I think that it's fair to call being unable to find HEAD in 'git\nbranch -d' when we are detached a bug. Indeed, if we can't find a HEAD,\nthen that's fine (there is just nothing merged into it, as you note).\n\n> And in that case I think the HEAD check calling die() is actively doing\n> the wrong thing, and would prevent an otherwise successful deletion.\n>\n> The fix might be as simple as:\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 15be0c03ef..f6ff9084c8 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -235,11 +235,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t}\n>  \tbranch_name_pos = strcspn(fmt, \"%\");\n>\n> -\tif (!force) {\n> +\tif (!force)\n>  \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n> -\t\tif (!head_rev)\n> -\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n> -\t}\n>\n>  \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n>  \t\tchar *target = NULL;\n>\n> as the later code seems to do the right thing with the NULL head_rev. It\n> would definitely need more careful investigation (and tests!) to confirm\n> that, though.\n\nYeah, that looks reasonable to me. Presumably we want a small test, as\nwell, but I doubt that is any more complicated than:\n\n--- 8< ---\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b..6ace22f7ce 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,14 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n\n+test_expect_success 'git branch -d on detached HEAD' '\n+\ttest_when_finished \"git checkout main && git branch -D other\" &&\n+\tgit branch other &&\n+\tgit checkout --orphan orphan &&\n+\ttest_must_fail git branch -d other 2>err &&\n+\tgrep \"not fully merged\" err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n--- >8 ---\n\nI'm happy to wrap all of that up into a patch, and equally happy for you\nto do so (feel free to forge my S-o-b here if you do).\n\nThanks,\nTaylor\n"},{"id":"466212","messageId":"Y2F+MA+cAsEB0Glb@nand.local","threadId":"58714","inReplyTo":"Y2F9lkCWf/2rjT2E@nand.local","subject":"Re: Bug in `git branch --delete main` when on other orphan brancht","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:14:40Z","receivedAt":"2022-11-01T20:14:46Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 04:12:06PM -0400, Taylor Blau wrote:\n> Yeah, that looks reasonable to me. Presumably we want a small test, as\n> well, but I doubt that is any more complicated than:\n>\n> --- 8< ---\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 7f605f865b..6ace22f7ce 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -279,6 +279,14 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n>  \ttest_cmp expect err\n>  '\n>\n> +test_expect_success 'git branch -d on detached HEAD' '\n> +\ttest_when_finished \"git checkout main && git branch -D other\" &&\n> +\tgit branch other &&\n> +\tgit checkout --orphan orphan &&\n> +\ttest_must_fail git branch -d other 2>err &&\n> +\tgrep \"not fully merged\" err\n> +'\n> +\n>  test_expect_success 'git branch -v -d t should work' '\n>  \tgit branch t &&\n>  \tgit rev-parse --verify refs/heads/t &&\n> --- >8 ---\n\nActually, the test doesn't need \"other\" here, since we aren't actually\ngoing to delete the target branch (for the exact same reason that you\npointed out to Martin earlier in the thread).\n\nSo it could actually be as small as something like this:\n\n--- >8 ---\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b..464d3f610b 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,13 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n\n+test_expect_success 'git branch -d on detached HEAD' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_must_fail git branch -d main 2>err &&\n+\tgrep \"not fully merged\" err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n--- 8< ---\n\nThanks,\nTaylor\n"},{"id":"466213","messageId":"c68f4b140f2495a35c5f30bec4e2e56c246160f4.1667334672.git.me@ttaylorr.com","threadId":"58714","inReplyTo":"Y2F9lkCWf/2rjT2E@nand.local","subject":"[PATCH] branch: gracefully handle '-d' on detached HEAD","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:32:18Z","receivedAt":"2022-11-01T20:32:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nSince 67affd5173 (git-branch -D: make it work even when on a\nyet-to-be-born branch, 2006-11-24), 'git branch -d' refuses to work when\norphaned, since there is no HEAD to resolve.\n\nBut since 67affd5173, there have been other checks, like 99c419c915\n(branch -d: base the \"already-merged\" safety on the branch it merges\nwith, 2009-12-29), which makes it OK to delete a branch if it is merged\nto HEAD or its upstream.\n\n99c419c915 makes the check in 67affd5173 wrong, since it's OK to delete\na branch if it is merged to its upstream.\n\nSince the code in delete_branches() tolerates a NULL head_rev perfectly\nfine, make it non-fatal to fail to resolve a commit object at HEAD.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nOK, here's that patch. Since Peff wrote the main portion of it, he gets\nauthorship credit. My version comes with a test, which we should\nconsider picking up.\n\nEven though it doesn't resolve Martin's original scenario (i.e., he'd\nstill have to use -D or -f to actually delete 'main' there), I still\nthink the bugfix is worth pursuing in its own right.\n\n builtin/branch.c  | 5 +----\n t/t3200-branch.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 15be0c03ef..f6ff9084c8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -235,11 +235,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t}\n \tbranch_name_pos = strcspn(fmt, \"%\");\n\n-\tif (!force) {\n+\tif (!force)\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n-\t\tif (!head_rev)\n-\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n-\t}\n\n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b..464d3f610b 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,13 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n\n+test_expect_success 'git branch -d on detached HEAD' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_must_fail git branch -d main 2>err &&\n+\tgrep \"not fully merged\" err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n--\n2.38.0.16.g393fd4c6db\n"},{"id":"466221","messageId":"Y2GTgB0oTW3rR1HZ@coredump.intra.peff.net","threadId":"58714","inReplyTo":"Y2F+MA+cAsEB0Glb@nand.local","subject":"Re: Bug in `git branch --delete main` when on other orphan brancht","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T21:45:36Z","receivedAt":"2022-11-01T21:45:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 04:14:40PM -0400, Taylor Blau wrote:\n\n> Actually, the test doesn't need \"other\" here, since we aren't actually\n> going to delete the target branch (for the exact same reason that you\n> pointed out to Martin earlier in the thread).\n> \n> So it could actually be as small as something like this:\n> \n> --- >8 ---\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 7f605f865b..464d3f610b 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -279,6 +279,13 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n>  \ttest_cmp expect err\n>  '\n> \n> +test_expect_success 'git branch -d on detached HEAD' '\n> +\ttest_when_finished git checkout main &&\n> +\tgit checkout --orphan orphan &&\n> +\ttest_must_fail git branch -d main 2>err &&\n> +\tgrep \"not fully merged\" err\n> +'\n\nI think there's a more interesting case, which is when \"main\" has a\nconfigured upstream to which it's fully merged. And then the deletion\nactually succeeds (and the bug is not just giving us a crappy message,\nbut actually doing causing the wrong outcome).\n\nAnd for that we'd probably not want to use \"main\", since a successful\ntest will actually delete it. ;)\n\nI'll try later tonight to integrate that test into the patch you've\nwritten.\n\n-Peff\n"},{"id":"466276","messageId":"Y2HA/8OuAmynVhQp@nand.local","threadId":"58714","inReplyTo":"Y2GTgB0oTW3rR1HZ@coredump.intra.peff.net","subject":"Re: Bug in `git branch --delete main` when on other orphan brancht","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-02T00:59:43Z","receivedAt":"2022-11-02T00:59:53Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 05:45:36PM -0400, Jeff King wrote:\n> I'll try later tonight to integrate that test into the patch you've\n> written.\n\nVery much appreciated, thanks.\n\nThanks,\nTaylor\n"},{"id":"466278","messageId":"Y2H/1S3G+KeeEN/l@coredump.intra.peff.net","threadId":"58714","inReplyTo":"c68f4b140f2495a35c5f30bec4e2e56c246160f4.1667334672.git.me@ttaylorr.com","subject":"[PATCH v2] branch: gracefully handle '-d' on orphan HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T05:27:49Z","receivedAt":"2022-11-02T05:27:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 04:32:18PM -0400, Taylor Blau wrote:\n\n> Subject: Re: [PATCH] branch: gracefully handle '-d' on detached HEAD\n\nOK, here's my version of the patch (which I called v2 for the sake of\nconfusion). I ended up rewriting the tests and commit message, as there\nwere a few inaccuracies, and some subtle things our earlier conversation\ndidn't turn up.\n\nThis should really be s/detached/orphan/ here and elsewhere (including\nthe branch name you used). All of this works fine on a detached HEAD.\nIt's only a problem when it can't be resolved to a commit at all (one\ncould perhaps argue that a branch being merged to a detached HEAD isn't\nany real kind of safety valve, but that's how it has always worked and\nis outside the scope of this fix).\n\n> Since 67affd5173 (git-branch -D: make it work even when on a\n> yet-to-be-born branch, 2006-11-24), 'git branch -d' refuses to work when\n> orphaned, since there is no HEAD to resolve.\n\nIt was true even before that commit that \"git branch -d\" would refuse to\nwork. That commit just made it so that \"branch -D\" worked as a safety\nhatch (rather than also dying!).\n\n> But since 67affd5173, there have been other checks, like 99c419c915\n> (branch -d: base the \"already-merged\" safety on the branch it merges\n> with, 2009-12-29), which makes it OK to delete a branch if it is merged\n> to HEAD or its upstream.\n\nI always thought of this as \"either-or\", but it is a little different.\nIf we are merged to HEAD and there is an upstream but we are not merged\nto it, we'll still refuse the deletion. Not super relevant for our\npurposes here, except that there's extra code to produce a custom\nmessage for this case, which we need to fix to handle NULL. ;)\n\n> Since the code in delete_branches() tolerates a NULL head_rev perfectly\n> fine, make it non-fatal to fail to resolve a commit object at HEAD.\n\nAnd alas, this \"perfectly fine\" turns out to not be true. :) See below\nfor the gory details.\n\n-- >8 --\nSubject: [PATCH] branch: gracefully handle '-d' on orphan HEAD\n\nWhen deleting a branch, \"git branch -d\" has a safety check that ensures\nthe branch is merged to its upstream (if any), or to HEAD. To do that,\nnaturally we try to resolve HEAD to a commit object. If we're on an\norphan branch (i.e., HEAD points to a branch that does not yet exist),\nthat will fail, and we'll bail with an error:\n\n  $ git branch -d to-delete\n  fatal: Couldn't look up commit object for HEAD\n\nThis usually isn't that big of a deal. The deletion would fail anyway,\nsince the branch isn't merged to HEAD, and you'd need to use \"-D\" (or\n\"-f\"). And doing so skips the HEAD resolution, courtesy of 67affd5173\n(git-branch -D: make it work even when on a yet-to-be-born branch,\n2006-11-24).\n\nBut there are still two problems:\n\n  1. The error message isn't very helpful. We should give the usual \"not\n     fully merged\" message, which points the user at \"branch -D\". That\n     was a problem even back in 67affd5173.\n\n  2. Even without a HEAD, these days it's still possible for the\n     deletion to succeed. After 67affd5173, commit 99c419c915 (branch\n     -d: base the \"already-merged\" safety on the branch it merges with,\n     2009-12-29) made it OK to delete a branch if it is merged to its\n     upstream.\n\nWe can fix both by removing the die() in delete_branches() completely,\nleaving head_rev NULL in this case. It's tempting to stop there, as it\nappears at first glance that the rest of the code does the right thing\nwith a NULL. But sadly, it's not quite true.\n\nWe end up feeding the NULL to repo_is_descendant_of(). In the\ntraditional code path there, we call repo_in_merge_bases_many(). It\nfeeds the NULL to repo_parse_commit(), which is smart enough to return\nan error, and we immediately return \"no, it's not a descendant\".\n\nBut there's an alternate code path: if we have a commit graph with\ngeneration numbers, we end up in can_all_from_reach(), which does\neventually try to set a flag on the NULL commit and segfaults.\n\nSo instead, we'll teach the local branch_merged() helper to treat a NULL\nas \"not merged\". This would be a little more elegant in in_merge_bases()\nitself, but that function is called in a lot of places, and it's not\nclear that quietly returning \"not merged\" is the right thing everywhere\n(I'd expect in many cases, feeding a NULL is a sign of a bug).\n\nThere are four tests here:\n\n  a. The first one confirms that deletion succeeds with an orphaned HEAD\n     when the branch is merged to its upstream. This is case (2) above.\n\n  b. Same, but with commit graphs enabled. Even if it is merged to\n     upstream, we still check head_rev so that we can say \"deleting\n     because it's merged to upstream, even though it's not merged to\n     HEAD\". Without the second hunk in branch_merged(), this test would\n     segfault in can_all_from_reach().\n\n  c. The third one confirms that we correctly say \"not merged to HEAD\"\n     when we can't resolve HEAD, and reject the deletion.\n\n  d. Same, but with commit graphs enabled. Without the first hunk in\n     branch_merged(), this one would segfault.\n\nReported-by: Martin von Zweigbergk <martinvonz@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c  |  9 +++------\n t/t3200-branch.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 15be0c03ef..9470c980c1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -150,7 +150,7 @@ static int branch_merged(int kind, const char *name,\n \tif (!reference_rev)\n \t\treference_rev = head_rev;\n \n-\tmerged = in_merge_bases(rev, reference_rev);\n+\tmerged = reference_rev ? in_merge_bases(rev, reference_rev) : 0;\n \n \t/*\n \t * After the safety valve is fully redefined to \"check with\n@@ -160,7 +160,7 @@ static int branch_merged(int kind, const char *name,\n \t * a gentle reminder is in order.\n \t */\n \tif ((head_rev != reference_rev) &&\n-\t    in_merge_bases(rev, head_rev) != merged) {\n+\t    (head_rev ? in_merge_bases(rev, head_rev) : 0) != merged) {\n \t\tif (merged)\n \t\t\twarning(_(\"deleting branch '%s' that has been merged to\\n\"\n \t\t\t\t\"         '%s', but not yet merged to HEAD.\"),\n@@ -235,11 +235,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t}\n \tbranch_name_pos = strcspn(fmt, \"%\");\n \n-\tif (!force) {\n+\tif (!force)\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n-\t\tif (!head_rev)\n-\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n-\t}\n \n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b..5a169b68d6 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,42 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -d on orphan HEAD (merged)' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_when_finished \"rm -rf .git/objects/commit-graph*\" &&\n+\tgit commit-graph write --reachable &&\n+\tgit branch --track to-delete main &&\n+\tgit branch -d to-delete\n+'\n+\n+test_expect_success 'git branch -d on orphan HEAD (merged, graph)' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\tgit branch --track to-delete main &&\n+\tgit branch -d to-delete\n+'\n+\n+test_expect_success 'git branch -d on orphan HEAD (unmerged)' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_when_finished \"git branch -D to-delete\" &&\n+\tgit branch to-delete main &&\n+\ttest_must_fail git branch -d to-delete 2>err &&\n+\tgrep \"not fully merged\" err\n+'\n+\n+test_expect_success 'git branch -d on orphan HEAD (unmerged, graph)' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_when_finished \"git branch -D to-delete\" &&\n+\tgit branch to-delete main &&\n+\ttest_when_finished \"rm -rf .git/objects/commit-graph*\" &&\n+\tgit commit-graph write --reachable &&\n+\ttest_must_fail git branch -d to-delete 2>err &&\n+\tgrep \"not fully merged\" err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n-- \n2.38.1.669.g2ee9a5b0e3\n\n"},{"id":"466469","messageId":"f21bf37f-4efe-326c-0090-d13ed54696b9@gmail.com","threadId":"58714","inReplyTo":"Y2H/1S3G+KeeEN/l@coredump.intra.peff.net","subject":"Re: [PATCH v2] branch: gracefully handle '-d' on orphan HEAD","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-11-04T01:26:00Z","receivedAt":"2022-11-04T01:26:13Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 2/11/22 6:27, Jeff King wrote:\n\n> We can fix both by removing the die() in delete_branches() completely,\n> leaving head_rev NULL in this case. It's tempting to stop there, as it\n> appears at first glance that the rest of the code does the right thing\n> with a NULL. But sadly, it's not quite true.\n> \n> We end up feeding the NULL to repo_is_descendant_of(). In the\n> traditional code path there, we call repo_in_merge_bases_many(). It\n> feeds the NULL to repo_parse_commit(), which is smart enough to return\n> an error, and we immediately return \"no, it's not a descendant\".\n> \n> But there's an alternate code path: if we have a commit graph with\n> generation numbers, we end up in can_all_from_reach(), which does\n> eventually try to set a flag on the NULL commit and segfaults.\n> \n> So instead, we'll teach the local branch_merged() helper to treat a NULL\n> as \"not merged\". This would be a little more elegant in in_merge_bases()\n> itself, but that function is called in a lot of places, and it's not\n> clear that quietly returning \"not merged\" is the right thing everywhere\n> (I'd expect in many cases, feeding a NULL is a sign of a bug).\n> \n> There are four tests here:\n> ...\nI've reviewed the change and looks fine to me.  Fixes the issue with the\ndeletion and avoids the segfault you discovered.\n\nBut the last paragraph in your message, before describing the tests, makes me\nscratch my head.\n\nCertainly there are a few dozen places where we have direct calls to\nin_merge_bases.  I haven't found any (beyond the modified in this patch) where\na NULL commit can be used in the call.  All I have reviewed have a\ncheck_for_null protection before calling, mainly to show an error message.  And\nthis makes me think about, what almost happened here, leaving that uncovered\nleft us open to a change where the error condition (NULL commit) doesn't matter\n(just the not_merged), and/or does not have a proper test with generation\nnumbers.\n\nThe segfault possibility was introduced in 6cc017431 (commit-reach: use\ncan_all_from_reach, 2018-07-20).  Before that, NULL was tolerated by\nis_descendant_of (and indirectly by in_merge_bases) and returned, still today\n(as you described in your message) as 1.  So IMHO we can safely put a check for\nNULL there and return 1, as a fix (or protection) for this segfault.  Something\nlike:\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex c226ee3da4..246eaf093d 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -445,7 +445,7 @@ int repo_is_descendant_of(struct repository *r,\n                          struct commit *commit,\n                          struct commit_list *with_commit)\n {\n-       if (!with_commit)\n+       if (!with_commit || !commit)\n                return 1;\n \n        if (generation_numbers_enabled(the_repository)) {\n\nand leave the checks for NULL in branch.c, as optimizations.\n\nI've cc Derrick, maybe he can give us an opinion on this.\n\n\nThis patch also /fixes/ the error message when:\n\n\t$ git init -b initial\n\t$ git branch -d initial\n\tfatal: Couldn't look up commit object for HEAD\n\nNow we get the much clear:\n\n\terror: Cannot delete branch 'initial' checked out at ...\n\nA nice patch.\nThank you.\n"},{"id":"466491","messageId":"Y2SkxJAnjOtwKX6o@coredump.intra.peff.net","threadId":"58714","inReplyTo":"f21bf37f-4efe-326c-0090-d13ed54696b9@gmail.com","subject":"Re: [PATCH v2] branch: gracefully handle '-d' on orphan HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-04T05:36:04Z","receivedAt":"2022-11-04T05:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2022 at 02:26:00AM +0100, Rubén Justo wrote:\n\n> > So instead, we'll teach the local branch_merged() helper to treat a NULL\n> > as \"not merged\". This would be a little more elegant in in_merge_bases()\n> > itself, but that function is called in a lot of places, and it's not\n> > clear that quietly returning \"not merged\" is the right thing everywhere\n> > (I'd expect in many cases, feeding a NULL is a sign of a bug).\n> > \n> > There are four tests here:\n> > ...\n> I've reviewed the change and looks fine to me.  Fixes the issue with the\n> deletion and avoids the segfault you discovered.\n\nJust to make things clear, the segfault doesn't exist before my patch.\nIt's only once we remove the die() call that we need to make sure the\ndownstream code does the right thing with the resulting NULL, and does\nnot segfault.\n\n> But the last paragraph in your message, before describing the tests, makes me\n> scratch my head.\n> \n> Certainly there are a few dozen places where we have direct calls to\n> in_merge_bases.  I haven't found any (beyond the modified in this patch) where\n> a NULL commit can be used in the call.  All I have reviewed have a\n> check_for_null protection before calling, mainly to show an error message.  And\n> this makes me think about, what almost happened here, leaving that uncovered\n> left us open to a change where the error condition (NULL commit) doesn't matter\n> (just the not_merged), and/or does not have a proper test with generation\n> numbers.\n\nI didn't find any instances, either, but I also didn't look. My\nreasoning was mostly that by making the change to this code in\nisolation, we could be sure not to have accidental effects in other\ncode. Now it _could_ be useful to handle NULL in those other call-sites,\nbut I didn't want making a judgement on that to hold up this fix.\n\n> The segfault possibility was introduced in 6cc017431 (commit-reach: use\n> can_all_from_reach, 2018-07-20).  Before that, NULL was tolerated by\n> is_descendant_of (and indirectly by in_merge_bases) and returned, still today\n> (as you described in your message) as 1.  So IMHO we can safely put a check for\n> NULL there and return 1, as a fix (or protection) for this segfault.  Something\n> like:\n\nYes, the segfault possibility was introduced there. But that doesn't\nmean the code intended to handle a NULL commit in that case. I think it\nends up doing the right thing, but the behavior is a little\nquestionable. It actually sees an error from repo_parse_commit(), and\nthen aborts the whole in_merge_bases_many() operation (not even looking\nat the other entries in the \"reference\" array, although in this caller\nit will always be the only element of the array).\n\nSo I find it too hard to blame 6cc017431 here; I don't think\nis_descendant_of() ever intended to handle NULL, and it was just luck\nthat it did before then.\n\nSo a fix there might be OK, but...\n\n> diff --git a/commit-reach.c b/commit-reach.c\n> index c226ee3da4..246eaf093d 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -445,7 +445,7 @@ int repo_is_descendant_of(struct repository *r,\n>                           struct commit *commit,\n>                           struct commit_list *with_commit)\n>  {\n> -       if (!with_commit)\n> +       if (!with_commit || !commit)\n>                 return 1;\n>  \n>         if (generation_numbers_enabled(the_repository)) {\n> \n> and leave the checks for NULL in branch.c, as optimizations.\n\nI don't think that does the right thing. We are asking if \"commit\" is a\ndescendant of any element in \"with_commit\". If \"with_commit\" is empty,\nwe say \"yes\" by returning 1.  But if there is no \"commit\", is the answer\nalso \"yes\"? It seems like it should be \"no\", returning 0.\n\nTBH, I find the existing \"return 1\" questionable. It comes originally\nfrom 694a577519 (git-branch --contains=commit, 2007-11-07). Back then\nthe function was used only for checking --contains, where a NULL list\nmeant \"the user did not ask to constrain the list at all\".\n\nI think it may be luck that no other caller has relied on that in the\nintervening years.\n\n> This patch also /fixes/ the error message when:\n> \n> \t$ git init -b initial\n> \t$ git branch -d initial\n> \tfatal: Couldn't look up commit object for HEAD\n> \n> Now we get the much clear:\n> \n> \terror: Cannot delete branch 'initial' checked out at ...\n\nOK, good. That surprised me at first, because the check in\nbranch_checked_out() doesn't use the same head_rev variable. But it is\njust the case that the die() I removed was aborting much earlier, and\nnow we get far enough to do the right message. The distinction is\nrelevant because it means that I didn't miss a spot where I should have\nchecked the behavior of NULL head_rev; the head_rev value is not used\ndirectly here.\n\n-Peff\n"},{"id":"466637","messageId":"a42f2d94-d727-fd99-1116-593dd3814a88@gmail.com","threadId":"58714","inReplyTo":"Y2SkxJAnjOtwKX6o@coredump.intra.peff.net","subject":"Re: [PATCH v2] branch: gracefully handle '-d' on orphan HEAD","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-11-06T22:22:19Z","receivedAt":"2022-11-06T22:22:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 4/11/22 6:36, Jeff King wrote:\n\n> I didn't find any instances, either, but I also didn't look. My\n> reasoning was mostly that by making the change to this code in\n> isolation, we could be sure not to have accidental effects in other\n> code. Now it _could_ be useful to handle NULL in those other call-sites,\n> but I didn't want making a judgement on that to hold up this fix.\n> \n>> The segfault possibility was introduced in 6cc017431 (commit-reach: use\n>> can_all_from_reach, 2018-07-20).  Before that, NULL was tolerated by\n>> is_descendant_of (and indirectly by in_merge_bases) and returned, still today\n>> (as you described in your message) as 1.  So IMHO we can safely put a check for\n>> NULL there and return 1, as a fix (or protection) for this segfault.  Something\n>> like:\n> \n> Yes, the segfault possibility was introduced there. But that doesn't\n> mean the code intended to handle a NULL commit in that case. I think it\n> ends up doing the right thing, but the behavior is a little\n> questionable. It actually sees an error from repo_parse_commit(), and\n> then aborts the whole in_merge_bases_many() operation (not even looking\n> at the other entries in the \"reference\" array, although in this caller\n> it will always be the only element of the array).\n> \n> So I find it too hard to blame 6cc017431 here; I don't think\n> is_descendant_of() ever intended to handle NULL, and it was just luck\n> that it did before then.\n> \n> So a fix there might be OK, but...\n> \n>> diff --git a/commit-reach.c b/commit-reach.c\n>> index c226ee3da4..246eaf093d 100644\n>> --- a/commit-reach.c\n>> +++ b/commit-reach.c\n>> @@ -445,7 +445,7 @@ int repo_is_descendant_of(struct repository *r,\n>>                           struct commit *commit,\n>>                           struct commit_list *with_commit)\n>>  {\n>> -       if (!with_commit)\n>> +       if (!with_commit || !commit)\n>>                 return 1;\n>>  \n>>         if (generation_numbers_enabled(the_repository)) {\n>>\n>> and leave the checks for NULL in branch.c, as optimizations.\n> \n> I don't think that does the right thing. We are asking if \"commit\" is a\n> descendant of any element in \"with_commit\". If \"with_commit\" is empty,\n> we say \"yes\" by returning 1.  But if there is no \"commit\", is the answer\n> also \"yes\"? It seems like it should be \"no\", returning 0.\n\nCorrect.  I'm sorry :-/, I meant 0.  My reasoning was to maintain, for NULL\ncommit, the same result with or without generation_numbers_enabled.  Nothing to\nblame on 6cc017431, as nothing states that repo_is_descendant_of needs to\nsupport a NULL commit; but as generation_numbers is not enabled by default, it\nis easy to leave that aside if only checked with defaults.  But of course, your\nchange and 0cc017431 are correct, and your tests cover both execution paths, so\nnothing needs to be changed.\n\n>> This patch also /fixes/ the error message when:\n>>\n>> \t$ git init -b initial\n>> \t$ git branch -d initial\n>> \tfatal: Couldn't look up commit object for HEAD\n>>\n>> Now we get the much clear:\n>>\n>> \terror: Cannot delete branch 'initial' checked out at ...\n> \n> OK, good. That surprised me at first, because the check in\n> branch_checked_out() doesn't use the same head_rev variable. But it is\n> just the case that the die() I removed was aborting much earlier, and\n> now we get far enough to do the right message. The distinction is\n> relevant because it means that I didn't miss a spot where I should have\n> checked the behavior of NULL head_rev; the head_rev value is not used\n> directly here.\n\nMy comment was just to suggest that maybe it is worth adding a test for this :-)\nIf you think it is, maybe this can be useful:\n\n----- 8< -----\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..2c1c16cc17 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -315,6 +315,13 @@ test_expect_success 'git branch -d on orphan HEAD (unmerged, graph)' '\n \tgrep \"not fully merged\" err\n '\n \n+test_expect_success 'git branch -d orphan error message' '\n+\ttest_when_finished git checkout main &&\n+\tgit checkout --orphan orphan &&\n+\ttest_must_fail git branch -d orphan 2>err &&\n+\tgrep \"checked out at\" err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n----- >8 -----\n\n> \n> -Peff\n> \n\nSorry for the noise.\n\nUn saludo.\nRubén.\n"}]}