Derrick Stolee <dstolee@xxxxxxxxxxxxx> writes:
> Before verifying a commit-graph file against the object database, we
> need to parse all commits from the given commit-graph file. Create
> parse_commit_in_graph_one() to target a given struct commit_graph.
If I understand it properly the problem is that when verifying against
the object database we want to check one single commit-graph file, not
concatenation of data from commit-graph file for the repository and
commit-graph files from its alternates -- like prepare_commit_graph()
does; which is called by parse_commit_in_graph().
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
O.K., so you introduce here a layer of indirection; parse_commit_in_graph()
now just uses parse_commit_in_graph_one(), passing core_commit_graph
(or the_commit_graph) to it, after checking that core_commit_graph is set
(which handles the case when feature is not turned off) and loading
commit-graph file.
Nice and simple 'split function' refactoring, with new function taking
over when there is commit graph file prepared.
So, after the changes:
* parse_commit_in_graph() is responsible for checking whether to use
commit-graph feature and ensuring that data from commit-graph is
loaded, where it passes the control to parse_commit_in_graph_one()
* parse_commit_in_graph_one() checks whether commit-graph feature is
turned on, whether commit we are interested in was already parsed,
and then uses fill_commit_in_graph() to actually get the data
* fill_commit_in_graph() gets data out of commit-graph file, extracting
it from commit data chunk (and if needed large edges chunk).
All those functions return 1 if they got data from commit-graph, and 0
if they didn't.
One minor nitpick / complaint / question is about naming of global
variables used here, namely:
* static struct commit_graph *commit_graph
from commit-graph.c for global storage of commit-graph[s] data
* int core_commit_graph
from environment.c for storing core.commitGraph config
But I see that at least the latter is common convention in Git source
code; I guess that the former maybe follows convention as used for "the
index" and "the repository" - additionally it is static / file-local.
> ---
> commit-graph.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 82295f0975..78ba0edc80 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -310,7 +310,7 @@ static int find_commit_in_graph(struct commit *item, struct commit_graph *g, uin
> }
> }
>
> -int parse_commit_in_graph(struct commit *item)
> +static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)
> {
> uint32_t pos;
>
> @@ -318,9 +318,21 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)
> if (!core_commit_graph)
> return 0;
All right, we check that commit-graph feature is enabled because
parse_commit_in_graph_one() will be used standalone, not by being
invoked from parse_commit_in_graph().
This check is fast.
> if (item->object.parsed)
> return 1;
Sidenote: I just wonder why object.parsed and not for example
object.graph_pos is used to checck whether the object was filled if
possible with commit-graph data...
> +
> + if (find_commit_in_graph(item, g, &pos))
> + return fill_commit_in_graph(item, g, pos);
> +
> + return 0;
> +}
> +
> +int parse_commit_in_graph(struct commit *item)
> +{
> + if (!core_commit_graph)
> + return 0;
All right, this check is here to short-circuit and make it so git does
not even try to lod commit-graph file[s] if the feature is disabled.
> +
> prepare_commit_graph();
> - if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
> - return fill_commit_in_graph(item, commit_graph, pos);
> + if (commit_graph)
> + return parse_commit_in_graph_one(commit_graph, item);
> return 0;
> }
