Re: [PATCH] Segmentation Fault on non-commit --branch clone

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2019, 03:55 UTC
Message-ID: <xmqq36f41ejm.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191103181657.GA89185@carpenter.lan>

Davide Berardi <berardi.dav@gmail.com> writes:

Show 13 quoted lines

> The problem with the proposed approach was that the code was
> incompatible with some tests (specifically the tests which specifies an
> empty .git directory would fail and fallback to the unborn master
> branch).
>
> The lookup commit have two error-paths:
>
> 1. the commit cannot be found;
> 2. the commit is found and cannot be casted to a commit (whoops!).
>
> so, I've returned the second condition using an auxiliary variable and
> declaring a new lookup_commit function keeping compatibility with the
> old one.

It's more like three, I think.

  1a. The given object name is a sentinel, "no such object", value.

  1b. The given object name is meant to name a real object, but
      there is no object with such a name in the repository.

  2.  The given object name names an existing object, but it does
      not peel to a commit.

Traditionally, we had only lookup_commit() and died on any of the above. Later, we added the _gently() variant for callers that want to use a commit and also want to handle an error case where the object name they are handed by their callers does not peel to a commit.

In the "unborn repository, empty .git" case, are you getting a random object name, or null_oid (aka case 1a. above)? If that is the case, then your solution to introduce another variant of lookup_commit() that takes *err parameter is a wrong approach would not differenciate between 1a. and 1b., which would not help, as I suspect that we still do want to treat 1b. as an error.

Wouldn't it be cleaner to catch 1a. upfront, e.g.

	if (our)
		tip = &our->old_oid;
	else if (remote)
		tip = &remote->old_oid;
	else
		return NULL;

	if (is_null_oid(tip))
        	return NULL;

	tip_commit = lookup_commit_reference_gently(the_repository, tip, 1);
	if (!tip_commit) {
		warning(...);
		create_symref(...);
		return NULL;
	}

I am not offhand sure if the places we return NULL upfront want to also create HEAD symref, or that is something automatically happens for us in the downstream of this function, though.

Thanks.