Re: [PATCH 4/5] commit-graph: store graph in struct object_store

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2018, 20:57 UTC
Message-ID: <xmqqwoummvmw.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <3410494e3505a7b1b895869c3702dbba0f0cf5a6.1529616356.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 32 quoted lines

> Instead of storing commit graphs in static variables, store them in
> struct object_store. There are no changes to the signatures of existing
> functions - they all still only support the_repository, and support for
> other instances of struct repository will be added in a subsequent
> commit.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  commit-graph.c | 34 ++++++++++++++++++----------------
>  object-store.h |  3 +++
>  object.c       |  5 +++++
>  3 files changed, 26 insertions(+), 16 deletions(-)
> ...
> @@ -293,7 +290,9 @@ int parse_commit_in_graph(struct commit *item)
>  		return 1;
>  
>  	prepare_commit_graph();
> -	if (commit_graph) {
> +	if (the_repository->objects->commit_graph) {
> +		struct commit_graph *commit_graph =
> +			the_repository->objects->commit_graph;
>  		uint32_t pos;
>  		int found;
>  		if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
> @@ -329,7 +328,8 @@ struct tree *get_commit_tree_in_graph(const struct commit *c)
>  	if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
>  		BUG("get_commit_tree_in_graph called from non-commit-graph commit");
>  
> -	return load_tree_for_commit(commit_graph, (struct commit *)c);
> +	return load_tree_for_commit(the_repository->objects->commit_graph,
> +				    (struct commit *)c);
>  }

I was looking at semantic merge conflicts between this and your e2838d85 ("commit-graph: always load commit-graph information", 2018-05-01), the latter of which I planned to merge to 'master' as a part of the first batch in this cycle, and noticed that there are two very similar functions, without enough document the callers would not know which one is the correct one to call. Needless to say, such a code duplication would mean the work required for resolving semantic conflict doubles needlessly X-<.

int parse_commit_in_graph(struct commit *item)
{
	uint32_t pos;

	if (!core_commit_graph)
		return 0;
	if (item->object.parsed)
		return 1;
	prepare_commit_graph();
	if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
		return fill_commit_in_graph(item, commit_graph, pos);
	return 0;
}

void load_commit_graph_info(struct commit *item)
{
	uint32_t pos;
	if (!core_commit_graph)
		return;
	prepare_commit_graph();
	if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
		fill_commit_graph_info(item, commit_graph, pos);
}