mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Rubén Justo" <>
To: Jeff King <>, Taylor Blau <>
	Martin von Zweigbergk <>,
	Derrick Stolee <>
Subject: Re: [PATCH v2] branch: gracefully handle '-d' on orphan HEAD
Date: Fri, 4 Nov 2022 02:26:00 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <Y2H/1S3G+KeeEN/>

On 2/11/22 6:27, Jeff King wrote:

> We can fix both by removing the die() in delete_branches() completely,
> leaving head_rev NULL in this case. It's tempting to stop there, as it
> appears at first glance that the rest of the code does the right thing
> with a NULL. But sadly, it's not quite true.
> We end up feeding the NULL to repo_is_descendant_of(). In the
> traditional code path there, we call repo_in_merge_bases_many(). It
> feeds the NULL to repo_parse_commit(), which is smart enough to return
> an error, and we immediately return "no, it's not a descendant".
> But there's an alternate code path: if we have a commit graph with
> generation numbers, we end up in can_all_from_reach(), which does
> eventually try to set a flag on the NULL commit and segfaults.
> So instead, we'll teach the local branch_merged() helper to treat a NULL
> as "not merged". This would be a little more elegant in in_merge_bases()
> itself, but that function is called in a lot of places, and it's not
> clear that quietly returning "not merged" is the right thing everywhere
> (I'd expect in many cases, feeding a NULL is a sign of a bug).
> There are four tests here:
> ...
I've reviewed the change and looks fine to me.  Fixes the issue with the
deletion and avoids the segfault you discovered.

But the last paragraph in your message, before describing the tests, makes me
scratch my head.

Certainly there are a few dozen places where we have direct calls to
in_merge_bases.  I haven't found any (beyond the modified in this patch) where
a NULL commit can be used in the call.  All I have reviewed have a
check_for_null protection before calling, mainly to show an error message.  And
this makes me think about, what almost happened here, leaving that uncovered
left us open to a change where the error condition (NULL commit) doesn't matter
(just the not_merged), and/or does not have a proper test with generation

The segfault possibility was introduced in 6cc017431 (commit-reach: use
can_all_from_reach, 2018-07-20).  Before that, NULL was tolerated by
is_descendant_of (and indirectly by in_merge_bases) and returned, still today
(as you described in your message) as 1.  So IMHO we can safely put a check for
NULL there and return 1, as a fix (or protection) for this segfault.  Something

diff --git a/commit-reach.c b/commit-reach.c
index c226ee3da4..246eaf093d 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -445,7 +445,7 @@ int repo_is_descendant_of(struct repository *r,
                          struct commit *commit,
                          struct commit_list *with_commit)
-       if (!with_commit)
+       if (!with_commit || !commit)
                return 1;
        if (generation_numbers_enabled(the_repository)) {

and leave the checks for NULL in branch.c, as optimizations.

I've cc Derrick, maybe he can give us an opinion on this.

This patch also /fixes/ the error message when:

	$ git init -b initial
	$ git branch -d initial
	fatal: Couldn't look up commit object for HEAD

Now we get the much clear:

	error: Cannot delete branch 'initial' checked out at ...

A nice patch.
Thank you.

  reply	other threads:[~2022-11-04  1:26 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-29  5:46 Bug in `git branch --delete main` when on other orphan branch Martin von Zweigbergk
2022-11-01 10:15 ` Jeff King
2022-11-01 15:31   ` Martin von Zweigbergk
2022-11-01 20:12   ` Taylor Blau
2022-11-01 20:14     ` Bug in `git branch --delete main` when on other orphan brancht Taylor Blau
2022-11-01 21:45       ` Jeff King
2022-11-02  0:59         ` Taylor Blau
2022-11-01 20:32     ` [PATCH] branch: gracefully handle '-d' on detached HEAD Taylor Blau
2022-11-02  5:27       ` [PATCH v2] branch: gracefully handle '-d' on orphan HEAD Jeff King
2022-11-04  1:26         ` Rubén Justo [this message]
2022-11-04  5:36           ` Jeff King
2022-11-06 22:22             ` Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).