Subject: Re: [PATCH v2 10/32] commit.c: add repo_get_commit_tree()

From: SZEDER Gábor <[email-masked]>
Date: Thu, 4 Apr 2019 19:04:27 +0200

On Wed, Apr 03, 2019 at 06:34:35PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>

Oh, look, an empty commit message ;)

> ---
>  commit.c                        | 5 +++--
>  commit.h                        | 3 ++-
>  contrib/coccinelle/commit.cocci | 4 ++--
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/commit.c b/commit.c
> index a5333c7ac6..f0a5506f04 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -340,7 +340,8 @@ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit)
>  	}
>  }
>  
> -struct tree *get_commit_tree(const struct commit *commit)
> +struct tree *repo_get_commit_tree(struct repository *r,
> +				  const struct commit *commit)

A rename to accomodate the additional 'struct repository*'
parameter.  Ok.

>  {
>  	if (commit->maybe_tree || !commit->object.parsed)
>  		return commit->maybe_tree;
> @@ -348,7 +349,7 @@ struct tree *get_commit_tree(const struct commit *commit)
>  	if (commit->graph_pos == COMMIT_NOT_FROM_GRAPH)
>  		BUG("commit has NULL tree, but was not loaded from commit-graph");
>  
> -	return get_commit_tree_in_graph(the_repository, commit);
> +	return get_commit_tree_in_graph(r, commit);
>  }
>  
>  struct object_id *get_commit_tree_oid(const struct commit *commit)
> diff --git a/commit.h b/commit.h
> index 42728c2906..b576201be8 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -143,7 +143,8 @@ void repo_unuse_commit_buffer(struct repository *r,
>   */
>  void free_commit_buffer(struct parsed_object_pool *pool, struct commit *);
>  
> -struct tree *get_commit_tree(const struct commit *);
> +struct tree *repo_get_commit_tree(struct repository *, const struct commit *);
> +#define get_commit_tree(c) repo_get_commit_tree(the_repository, c)

But we still keep 'get_commit_tree' around as a macro wrapper
defaulting to 'the_repository', leaving the 30+ callsites intact.
Good.

Furthermore, the comment describing the 'maybe_tree' field in 'struct
commit's declaration still suggest get_commit_tree():

  * If the commit is loaded from the commit-graph file, then this
  * member may be NULL. Only access it through get_commit_tree()
  * or get_commit_tree_oid().

>  struct object_id *get_commit_tree_oid(const struct commit *);
>  
>  /*
> diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
> index c49aa558f0..f5bc639981 100644
> --- a/contrib/coccinelle/commit.cocci
> +++ b/contrib/coccinelle/commit.cocci
> @@ -12,12 +12,12 @@ expression c;
>  
>  // These excluded functions must access c->maybe_tree direcly.
>  @@
> -identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
> +identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
>  expression c;
>  @@
>    f(...) {<...
>  - c->maybe_tree
> -+ get_commit_tree(c)
> ++ repo_get_commit_tree(the_repository, c)

So, why this change?

It would also require furher changes to 'commit.cocci', in particular
to the last semantic patch, which is supposed to ensure that
get_commit_tree() doesn't end up on the LHS of an assignment, but with
this change Coccinelle does suggest transfomations with
repo_get_commit_tree() on the LHS.

>    ...>}
>  @@
> -- 
> 2.21.0.479.g47ac719cd3