Re: [PATCH v3 05/20] commit-graph: load a root tree from specific graph

From: Jakub Narebski <jnareb@gmail.com>
Date: May 27, 2018, 19:12 UTC
Message-ID: <86h8mszzdy.fsf@gmail.com>
In-Reply-To: <20180524162504.158394-6-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

> When lazy-loading a tree for a commit, it will be important to select
> the tree from a specific struct commit_graph. Create a new method that
> specifies the commit-graph file and use that in
> get_commit_tree_in_graph().

Is this for the same reason why parse_commit_in_graph_one() was created in ptch 03/20? Why it would be important?

Show 5 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  commit-graph.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Simple and straightforward refactoring in the same vein as parse_commit_in_graph_one() one.

Show 18 quoted lines

>
> diff --git a/commit-graph.c b/commit-graph.c
> index 78ba0edc80..25893ec096 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -358,14 +358,20 @@ static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit *
>  	return c->maybe_tree;
>  }
>  
> -struct tree *get_commit_tree_in_graph(const struct commit *c)
> +static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
> +						 const struct commit *c)
>  {
>  	if (c->maybe_tree)
>  		return c->maybe_tree;
>  	if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
> -		BUG("get_commit_tree_in_graph called from non-commit-graph commit");
> +		BUG("get_commit_tree_in_graph_one called from non-commit-graph commit");

Sidenote: I wonder if it would be better or worse to use __func__ magic
costant variable here (part of C99 and C++11 standards).

Show 11 quoted lines

> +
> +	return load_tree_for_commit(g, (struct commit *)c);
> +}
>  
> -	return load_tree_for_commit(commit_graph, (struct commit *)c);
> +struct tree *get_commit_tree_in_graph(const struct commit *c)
> +{
> +	return get_commit_tree_in_graph_one(commit_graph, c);
>  }
>  
>  static void write_graph_chunk_fanout(struct hashfile *f,

Looks good to me.