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

On 4/11/22 6:36, Jeff King wrote:

> I didn't find any instances, either, but I also didn't look. My
> reasoning was mostly that by making the change to this code in
> isolation, we could be sure not to have accidental effects in other
> code. Now it _could_ be useful to handle NULL in those other call-sites,
> but I didn't want making a judgement on that to hold up this fix.
>> 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
>> like:
> Yes, the segfault possibility was introduced there. But that doesn't
> mean the code intended to handle a NULL commit in that case. I think it
> ends up doing the right thing, but the behavior is a little
> questionable. It actually sees an error from repo_parse_commit(), and
> then aborts the whole in_merge_bases_many() operation (not even looking
> at the other entries in the "reference" array, although in this caller
> it will always be the only element of the array).
> So I find it too hard to blame 6cc017431 here; I don't think
> is_descendant_of() ever intended to handle NULL, and it was just luck
> that it did before then.
> So a fix there might be OK, but...
>> 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 don't think that does the right thing. We are asking if "commit" is a
> descendant of any element in "with_commit". If "with_commit" is empty,
> we say "yes" by returning 1.  But if there is no "commit", is the answer
> also "yes"? It seems like it should be "no", returning 0.

Correct.  I'm sorry :-/, I meant 0.  My reasoning was to maintain, for NULL
commit, the same result with or without generation_numbers_enabled.  Nothing to
blame on 6cc017431, as nothing states that repo_is_descendant_of needs to
support a NULL commit; but as generation_numbers is not enabled by default, it
is easy to leave that aside if only checked with defaults.  But of course, your
change and 0cc017431 are correct, and your tests cover both execution paths, so
nothing needs to be changed.

>> 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 ...
> OK, good. That surprised me at first, because the check in
> branch_checked_out() doesn't use the same head_rev variable. But it is
> just the case that the die() I removed was aborting much earlier, and
> now we get far enough to do the right message. The distinction is
> relevant because it means that I didn't miss a spot where I should have
> checked the behavior of NULL head_rev; the head_rev value is not used
> directly here.

My comment was just to suggest that maybe it is worth adding a test for this :-)
If you think it is, maybe this can be useful:

----- 8< -----

diff --git a/t/ b/t/
index 5a169b68d6..2c1c16cc17 100755
--- a/t/
+++ b/t/
@@ -315,6 +315,13 @@ test_expect_success 'git branch -d on orphan HEAD (unmerged, graph)' '
 	grep "not fully merged" err
+test_expect_success 'git branch -d orphan error message' '
+	test_when_finished git checkout main &&
+	git checkout --orphan orphan &&
+	test_must_fail git branch -d orphan 2>err &&
+	grep "checked out at" err
 test_expect_success 'git branch -v -d t should work' '
 	git branch t &&
 	git rev-parse --verify refs/heads/t &&
----- >8 -----

> -Peff

Sorry for the noise.

Un saludo.

      reply	other threads:[~2022-11-06 22:22 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
2022-11-04  5:36           ` Jeff King
2022-11-06 22:22             ` Rubén Justo [this message]

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).