Re: [PATCH v2] branch: gracefully handle '-d' on orphan HEAD

["Rubén Justo" <rjusto@gmail.com>]

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/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..2c1c16cc17 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -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.
Rubén.