On 4/30/2018 5:31 AM, SZEDER Gábor wrote:
The semantic patch 'contrib/coccinelle/commit.cocci' added in
2e27bd7731 (treewide: replace maybe_tree with accessor methods,
2018-04-06) is supposed to "ensure that all references to the
'maybe_tree' member of struct commit are either mutations or accesses
through get_commit_tree()". So get_commit_tree() clearly must be able
to directly access the 'maybe_tree' member, and 'commit.cocci' has a
bit of a roundabout workaround to ensure that get_commit_tree()'s
direct access in its return statement is not transformed: after all
references to 'maybe_tree' have been transformed to a call to
get_commit_tree(), including the reference in get_commit_tree()
itself, the last rule transforms back a 'return get_commit_tree()'
statement, back then found only in get_commit_tree() itself, to a
direct access.
Unfortunately, already the very next commit shows that this workaround
is insufficient: 7b8a21dba1 (commit-graph: lazy-load trees for
commits, 2018-04-06) extends get_commit_tree() with a condition
directly accessing the 'maybe_tree' member, and Coccinelle with
'commit.cocci' promptly detects it and suggests a transformation to
avoid it. This transformation is clearly wrong, because calling
get_commit_tree() to access 'maybe_tree' _in_ get_commit_tree() would
obviously lead to recursion. Furthermore, the same commit added
another, more specialized getter function get_commit_tree_in_graph(),
whose legitimate direct access to 'maybe_tree' triggers a similar
wrong transformation suggestion.
Thanks for catching this, Szeder. Sorry for the noise.
Exclude both of these getter functions from the general rule in
'commit.cocci' that matches their direct accesses to 'maybe_tree'.
Also exclude load_tree_for_commit(), which, as static helper funcion
of get_commit_tree_in_graph(), has legitimate direct access to
'maybe_tree' as well.
This is an interesting feature of Coccinelle. Happy to learn it.
The last rule transforming back 'return get_commit_tree()' statements
to direct accesses thus became unnecessary, remove it.
Signed-off-by: SZEDER Gábor <szeder....@gmail.com>
I applied this locally on 'next' and ran the check. I succeeded with no
changes.
Thanks!
Reviewed-by: Derrick Stolee <dsto...@microsoft.com>
---
contrib/coccinelle/commit.cocci | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
index ac38525941..a7e9215ffc 100644
--- a/contrib/coccinelle/commit.cocci
+++ b/contrib/coccinelle/commit.cocci
@@ -10,11 +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.
@@
+identifier f !~
"^(get_commit_tree|get_commit_tree_in_graph|load_tree_for_commit)$";
expression c;
@@
+ f(...) {...
- c->maybe_tree
+ get_commit_tree(c)
+ ...}
@@
expression c;
@@ -22,9 +26,3 @@ expression s;
@@
- get_commit_tree(c) = s
+ c->maybe_tree = s
-
-@@
-expression c;
-@@
-- return get_commit_tree(c);
-+ return c->maybe_tree;
