Re: [PATCH] revision: allow missing promisor objects on CLI

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2019, 03:50 UTC
Message-ID: <xmqqlfqxhzvu.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191228003430.241283-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 7 quoted lines

>  	object = get_reference(revs, arg, &oid, flags ^ local_flags);
>  	if (!object)
> -		return revs->ignore_missing ? 0 : -1;
> +		/*
> +		 * Either this object is missing and ignore_missing is true, or
> +		 * this object is a (missing) promisor object and
> +		 * exclude_promisor_objects is true.

I had to guess and dig where these assertions are coming from; we should not force future readers of the code to.

At least this comment must say why these assertions hold. Say something like "get_reference() yields NULL on only such and such cases" before concluding with "and in any of these cases, we can safely ignore it because ...".

I think the two cases the comment covers are safe for this caller to silently return 0. Another case get_reference() yields NULL is when oid_object_info() says it is a commit but it turns out that the object is found by repo_parse_commit() to be a non-commit, isn't it? I am not sure if it is safe for this caller to just return 0. There may be some other "unusual-but-not-fatal" cases where get_reference() does not hit a die() but returns NULL.

Thanks.