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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 25, 2018, 22:09 UTC
Message-ID: <20180625220915.198932-1-jonathantanmy@google.com>
In-Reply-To: <xmqqwoummvmw.fsf@gitster-ct.c.googlers.com>

Show 33 quoted lines

> 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);
> }

Thanks for letting me know - when I reroll, I'll ensure that I reroll on top of this change.

As for whether both these functions are necessary in the first place, I think they are: we could have a load_commit_graph_info() that ignores the parsedness of the commit and just copies everything over, but that means that we can't have the optimization of returning immediately without consulting the graph when attempting to reparse a parsed object.

So we can't delete parse_commit_in_graph(). And we can't delete load_commit_graph_info() either, because after reading the documentation in commit-graph.h, looking at the places where it is used, and observing some test failures when I make parse_object_buffer() not use the graph, I see that there are code paths in Git in which we use both parsed-from-buffer and parsed-without-buffer commits at the same time.

So, I now look at the code duplication, and I see that it is mostly the check-prepare-check step; my patch set reduces it a little, but I'll try to reduce it nearly completely when I reroll too.

Maybe when the object store becomes more pervasive, we can more clearly separate out the buffers that come from a repository (and maybe, when obtaining those buffers, we should obtain graph information at the same time) and buffers that we obtain from some arbitrary non-repository source, but that will take some time. In the meantime, I'll do what I suggested above when I reroll, maintaining these 2 functions.