On Thu, Apr 11, 2019 at 10:58:57PM +0200, SZEDER Gábor wrote:
> On Thu, Apr 11, 2019 at 10:51:46PM +0200, SZEDER Gábor wrote:
> > On Wed, Apr 10, 2019 at 10:56:52PM +0200, Johannes Schindelin wrote:
> > > > ...>}
> > > >
> > > > @@
> > > > + 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 make_virtual_commit() in 'merge-recursive.c'.
> and after a quick look these functions seem to be
> > rather fundamenal in the life-cycle of a commit object.
>
> Erm, not "commit object"; I meant the life-cycle of a 'struct commit'
> instance.
>
> > I think they deserve to be explicitly exempted, too, and then we could
> > remove this last semantic patch altogether.
And it would look like this. Yeah, that's a very long line there, but
I don't think we can break it up.
-- >8 --
diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
index 57c8f71479..fe814f313e 100644
--- a/contrib/coccinelle/commit.cocci
+++ b/contrib/coccinelle/commit.cocci
@@ -10,20 +10,15 @@ expression c;
- c->maybe_tree->object.oid.hash
+ get_commit_tree_oid(c)->hash
-// These excluded functions must access c->maybe_tree direcly.
+// These excluded functions must access/modify c->maybe_tree direcly.
+// Note that if c->maybe_tree is written somewhere outside of these
+// functions, then the recommended transformation will be bogus with
+// repo_get_commit_tree() on the LHS.
@@
-identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
-expression c;
+identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|fill_commit_in_graph|parse_commit_buffer|release_commit_memory|make_virtual_commit)$";
+struct commit *c;
@@
f(...) {<...
- c->maybe_tree
+ repo_get_commit_tree(the_repository, c)
...>}
-
-@@
-expression c;
-expression r;
-expression s;
-@@
-- repo_get_commit_tree(r, c) = s
-+ c->maybe_tree = s
