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.
