Subject: Re: [PATCH v3 00/33] nd/sha1-name-c-wo-the-repository updates

From: SZEDER Gábor <[email-masked]>
Date: Thu, 11 Apr 2019 22:51:46 +0200

On Wed, Apr 10, 2019 at 10:56:52PM +0200, Johannes Schindelin wrote:
> Hi Duy,
> 
> On Sat, 6 Apr 2019, Nguyễn Thái Ngọc Duy wrote:
> 
> > 10:  68876a150f ! 11:  848456f59c commit.c: add repo_get_commit_tree()
> >     @@ -2,6 +2,11 @@
> >
> >          commit.c: add repo_get_commit_tree()
> >
> >     +    Remove the implicit dependency on the_repository in this function.
> >     +    It will be used in sha1-name.c functions when they are updated to take
> >     +    any 'struct repository'. get_commit_tree() remains as a compat wrapper,
> >     +    to be slowly replaced later.
> >     +
> >       diff --git a/commit.c b/commit.c
> >       --- a/commit.c
> >       +++ b/commit.c
> >     @@ -29,6 +34,15 @@
> >       --- a/commit.h
> >       +++ b/commit.h
> >      @@
> >     +
> >     + 	/*
> >     + 	 * If the commit is loaded from the commit-graph file, then this
> >     +-	 * member may be NULL. Only access it through get_commit_tree()
> >     ++	 * member may be NULL. Only access it through repo_get_commit_tree()
> >     + 	 * or get_commit_tree_oid().
> >     + 	 */
> >     + 	struct tree *maybe_tree;
> >     +@@
> >        */
> >       void free_commit_buffer(struct parsed_object_pool *pool, struct commit *);
> >
> >     @@ -57,3 +71,10 @@
> >         ...>}
> >
> >       @@
> >     + expression c;
> >     ++expression r;
> >     + expression s;
> >     + @@
> >     +-- get_commit_tree(c) = s
> >     ++- repo_get_commit_tree(r, c) = s
> >     + + c->maybe_tree = s
> 
> I think this is wrong, and admittedly I had the very same version
> originally.
> 
> When you have an arbitrary `r` in any `repo_get_commit_tree(r, c)` (as
> opposed to `the_repository`), the conversion to `c->maybe_tree` is most
> likely incorrect.
> 
> Therefore, I don't think that we can do that.

So, as far as I understand, the goal of these 'c->maybe_tree'-related
semantic patches is to prevent "generic" parts of Git from accessing
this field directly, as it might not be initialized in a
commit-graph-enabled repository.

Only three functions are explicitly exempt, while this last semantic
patch in question implicitly allows a few more that assign a value to
'c->maybe_tree'.  These functions are release_commit_memory() and
parse_commit_buffer() in 'commit.c' and fill_commit_in_graph() in
'commit-graph.c', and after a quick look these functions seem to be
rather fundamenal in the life-cycle of a commit object.

I think they deserve to be explicitly exempted, too, and then we could
remove this last semantic patch altogether.