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

[Jonathan Tan <jonathantanmy@google.com>]

> 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.