Re: [PATCH v3 03/20] commit-graph: parse commit from chosen graph

From: Jakub Narebski <jnareb@gmail.com>
Date: May 27, 2018, 10:23 UTC
Message-ID: <86y3g5z9ah.fsf@gmail.com>
In-Reply-To: <20180524162504.158394-4-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> 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@microsoft.com>

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.

Show 20 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

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