Re: [PATCH v3 11/14] commit: integrate commit graph with commit parsing

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

> +struct object_id *get_nth_commit_oid(struct commit_graph *g,
> +				     uint32_t n,
> +				     struct object_id *oid)
> +{
> +	hashcpy(oid->hash, g->chunk_oid_lookup + g->hash_len * n);
> +	return oid;
> +}

This looks like a rather klunky API to me.  

It seems that many current callers in this series (not limited to
this step but in later patches in the series) discard the returned
value.

I would understand the API a lot better if the function returned
"const struct object_id *" that points into the copy of the oid the
graph structure keeps (and the caller can do hashcpy() if it wants
to).

That would allow the API to later check for errors when the caller
gives 'n' that is too large by returning a NULL, for example.

> +static struct commit_list **insert_parent_or_die(struct commit_graph *g,
> +					   int pos,
> +					   struct commit_list **pptr)
> +{
> +	struct commit *c;
> +	struct object_id oid;
> +	get_nth_commit_oid(g, pos, &oid);
> +	c = lookup_commit(&oid);