Re: [PATCH v3] revision: use commit graph in get_reference()

From: Jeff King <peff@peff.net>
Date: Dec 14, 2018, 08:45 UTC
Message-ID: <20181214084528.GC11777@sigill.intra.peff.net>
In-Reply-To: <20181213185450.230953-1-jonathantanmy@google.com>

On Thu, Dec 13, 2018 at 10:54:50AM -0800, Jonathan Tan wrote:

> -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)
> +static struct commit *parse_commit_in_graph_one(struct repository *r,
> +						struct commit_graph *g,
> +						const struct object_id *oid)

Making sure I understand the new logic...

Show 11 quoted lines

>  {
> +	struct object *obj;
> +	struct commit *commit;
>  	uint32_t pos;
>  
> -	if (item->object.parsed)
> -		return 1;
> +	obj = lookup_object(r, oid->hash);
> +	commit = obj && obj->type == OBJ_COMMIT ? (struct commit *) obj : NULL;
> +	if (commit && obj->parsed)
> +		return commit;

OK, so if it's a commit and we have it parsed, we return that. By using lookup_object(), if it's a non-commit, we haven't changed anything. Good.

Show 8 quoted lines

> -	if (find_commit_in_graph(item, g, &pos))
> -		return fill_commit_in_graph(item, g, pos);
> +	if (commit && commit->graph_pos != COMMIT_NOT_FROM_GRAPH)
> +		pos = commit->graph_pos;
> +	else if (bsearch_graph(g, oid, &pos))
> +		; /* bsearch_graph sets pos */
> +	else
> +		return NULL;

And then we try to find it in the commit graph. If we didn't, then we'll end up returning NULL. Good.

Show 6 quoted lines

> -	return 0;
> +	if (!commit) {
> +		commit = lookup_commit(r, oid);
> +		if (!commit)
> +			return NULL;
> +	}

And at this point we found it in the commit graph, so we know it's a commit. lookup_commit() should succeed, but in the off chance that it's in the commit graph _and_ we previously found it as a non-commit (yikes!), we'll return NULL. That's equivalent to just pretending we didn't find it in the commit graph, and the caller can sort it out (when they read the object, either it will match the previous type, or it really will be a commit and they'll follow the normal complaining path). Good.

So this all makes sense. The one thing we don't do here is actually parse an unparsed commit that isn't in the graph, and instead leave that to the caller. E.g. get_reference() now does:

> -	object = parse_object(revs->repo, oid);
> +	object = (struct object *) parse_commit_in_graph(revs->repo, oid);
> +	if (!object)
> +		object = parse_object(revs->repo, oid);

In theory we could save another lookup_object() in parse_object() by combining these steps, but I don't think it's really worth worrying too much about.

So overall this looks good to me.

-Peff