Re: [PATCH 2/3] commit-graph: introduce `repo_find_commit_pos_in_graph()`

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 15, 2022, 03:17 UTC
Message-ID: <3036d949-63b2-9d6c-b3f6-0075fe9e1ca5@github.com>
In-Reply-To: <e988c974119edbabc06dae6d24810fc0bafbdc94.1657667404.git.me@ttaylorr.com>

On 7/12/2022 7:10 PM, Taylor Blau wrote:

Show 8 quoted lines

> +int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
> +				  uint32_t *pos)
> +{
> +	if (!prepare_commit_graph(r))
> +		return 0;
> +	return find_commit_pos_in_graph(c, r->objects->commit_graph, pos);
> +}
> +

This is absolutely correct for the given prototype.

Show 8 quoted lines

>  void load_commit_graph_info(struct repository *r, struct commit *item)
>  {
>  	uint32_t pos;
> -	if (!prepare_commit_graph(r))
> -		return;
> -	if (find_commit_pos_in_graph(item, r->objects->commit_graph, &pos))
> +	if (repo_find_commit_pos_in_graph(r, item, &pos))
>  		fill_commit_graph_info(item, r->objects->commit_graph, pos);

Normally I would complain about referring directly to r->objects->commit_graph without an explicit prepare_commit_graph() in the method. My initial thought was that we would need to know that the new method prepares the graph, but obviously the commit-graph needs to exist and be prepared if we are loading a position from it.

All good here.

Thanks, -Stolee