From: Jeff King <peff@peff.net>
Date: Thu, 14 May 2020 18:09:47 GMT
Subject: Re: [PATCH v3 8/8] commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag
Message-ID: <20200514180947.GC2430834@coredump.intra.peff.net>
In-Reply-To: <3cb0bd306cbb971715f84db07a8d1acac658f2d0.1589407015.git.me@ttaylorr.com>

On Wed, May 13, 2020 at 03:59:55PM -0600, Taylor Blau wrote:

> -	result = lookup_commit_reference_gently(the_repository, &oid, 1);
> -	if (result)
> -		oidset_insert(commits, &result->object.oid);
> -	else
> -		return error(_("invalid commit object id: %s"), hash);
> +	result = deref_tag(the_repository, parse_object(the_repository, &oid),
> +			   NULL, 0);
> +	if (!result)
> +		return error(_("invalid object: %s"), hash);

OK. As you noted earlier, this is really "we don't have that object"
_or_ "we don't have an object it points to". But since either is a
corruption, and I'd expect parse_object() to produce a more detailed
message anyway, I don't think it's worth trying to get more specific
here.

> +	else if (object_as_type(the_repository, result, OBJ_COMMIT, 1))
> +		oidset_insert(commits, &result->oid);

I suspect this could just be "if (result->type == OBJ_COMMIT)", as we'd
never see OBJ_NONE from a tag deref (which would have required the
casting behavior), but I don't think it hurts to use this function to be
on the safe side.

-Peff