commit.cocci: refactor code, avoid double rewrite

pclouds · gitster · commit a133c40b23c8 · 2019-04-16T18:56:51.000+09:00
"maybe" pointer in 'struct commit' is tricky because it can be lazily
initialized to take advantage of commit-graph if available. This makes
it not safe to access directly.

This leads to a rule in commit.cocci to rewrite 'x-&gt;maybe_tree' to
'get_commit_tree(x)'. But that rule alone could lead to incorrectly
rewrite assignments, e.g. from

    x-&gt;maybe_tree = yes

to

    get_commit_tree(x) = yes

Because of this we have a second rule to revert this effect. Szeder
found out that we could do better by performing the assignment rewrite
rule first, then the remaining is read-only access and handled by the
current first rule.

For this to work, we need to transform "x-&gt;maybe_tree = y" to something
that does NOT contain "x-&gt;maybe_tree" to avoid the original first
rule. This is where set_commit_tree() comes in.

Helped-by: SZEDER Gábor &lt;szeder.dev@gmail.com&gt;
Helped-by: Johannes Schindelin &lt;Johannes.Schindelin@gmx.de&gt;
Signed-off-by: Nguyễn Thái Ngọc Duy &lt;pclouds@gmail.com&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/commit-graph.c b/commit-graph.c
@@ -343,6 +343,11 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
 	item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
 }
 
+static inline void set_commit_tree(struct commit *c, struct tree *t)
+{
+	c->maybe_tree = t;
+}
+
 static int fill_commit_in_graph(struct repository *r,
 				struct commit *item,
 				struct commit_graph *g, uint32_t pos)
@@ -356,7 +361,7 @@ static int fill_commit_in_graph(struct repository *r,
 	item->object.parsed = 1;
 	item->graph_pos = pos;
 
-	item->maybe_tree = NULL;
+	set_commit_tree(item, NULL);
 
 	date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
 	date_low = get_be32(commit_data + g->hash_len + 12);
@@ -442,7 +447,7 @@ static struct tree *load_tree_for_commit(struct repository *r,
 					   GRAPH_DATA_WIDTH * (c->graph_pos);
 
 	hashcpy(oid.hash, commit_data);
-	c->maybe_tree = lookup_tree(r, &oid);
+	set_commit_tree(c, lookup_tree(r, &oid));
 
 	return c->maybe_tree;
 }
diff --git a/commit.c b/commit.c
@@ -340,6 +340,11 @@ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit)
 	}
 }
 
+static inline void set_commit_tree(struct commit *c, struct tree *t)
+{
+	c->maybe_tree = t;
+}
+
 struct tree *get_commit_tree(const struct commit *commit)
 {
 	if (commit->maybe_tree || !commit->object.parsed)
@@ -358,7 +363,7 @@ struct object_id *get_commit_tree_oid(const struct commit *commit)
 
 void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)
 {
-	c->maybe_tree = NULL;
+	set_commit_tree(c, NULL);
 	c->index = 0;
 	free_commit_buffer(pool, c);
 	free_commit_list(c->parents);
@@ -406,7 +411,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	if (get_oid_hex(bufptr + 5, &parent) < 0)
 		return error("bad tree pointer in commit %s",
 			     oid_to_hex(&item->object.oid));
-	item->maybe_tree = lookup_tree(r, &parent);
+	set_commit_tree(item, lookup_tree(r, &parent));
 	bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
 	pptr = &item->parents;
 
diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
@@ -10,19 +10,25 @@ 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_one|load_tree_for_commit)$";
+identifier f !~ "^set_commit_tree$";
 expression c;
+expression s;
 @@
   f(...) {<...
-- c->maybe_tree
-+ get_commit_tree(c)
+- c->maybe_tree = s
++ set_commit_tree(c, s)
   ...>}
 
+// These excluded functions must access 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
+// get_commit_tree() on the LHS.
 @@
+identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$";
 expression c;
-expression s;
 @@
-- get_commit_tree(c) = s
-+ c->maybe_tree = s
+  f(...) {<...
+- c->maybe_tree
++ get_commit_tree(c)
+  ...>}
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -163,14 +163,19 @@ static struct tree *shift_tree_object(struct repository *repo,
 	return lookup_tree(repo, &shifted);
 }
 
+static inline void set_commit_tree(struct commit *c, struct tree *t)
+{
+	c->maybe_tree = t;
+}
+
 static struct commit *make_virtual_commit(struct repository *repo,
 					  struct tree *tree,
 					  const char *comment)
 {
 	struct commit *commit = alloc_commit_node(repo);
 
 	set_merge_remote_desc(commit, comment, (struct object *)commit);
-	commit->maybe_tree = tree;
+	set_commit_tree(commit, tree);
 	commit->object.parsed = 1;
 	return commit;
 }

Original file line number	Diff line number	Diff line change
`@@ -163,14 +163,19 @@ static struct tree shift_tree_object(struct repository repo,`
`163`	`163`	`return lookup_tree(repo, &shifted);`
`164`	`164`	`}`
`165`	`165`
	`166`	`+static inline void set_commit_tree(struct commit c, struct tree t)`
	`167`	`+{`
	`168`	`+ c->maybe_tree = t;`
	`169`	`+}`
	`170`	`+`
`166`	`171`	`static struct commit make_virtual_commit(struct repository repo,`
`167`	`172`	`struct tree *tree,`
`168`	`173`	`const char *comment)`
`169`	`174`	`{`
`170`	`175`	`struct commit *commit = alloc_commit_node(repo);`
`171`	`176`
`172`	`177`	`set_merge_remote_desc(commit, comment, (struct object *)commit);`
`173`		`- commit->maybe_tree = tree;`
	`178`	`+ set_commit_tree(commit, tree);`
`174`	`179`	`commit->object.parsed = 1;`
`175`	`180`	`return commit;`
`176`	`181`	`}`