commit.cocci: refactor code, avoid double rewrite
"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->maybe_tree' to 'get_commit_tree(x)'. But that rule alone could lead to incorrectly rewrite assignments, e.g. from x->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->maybe_tree = y" to something that does NOT contain "x->maybe_tree" to avoid the original first rule. This is where set_commit_tree() comes in. Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nguyễn Thái Ngọc Duy committed
Apr 16, 2019 at 16:33 UTC
a133c40b23c80ed77cfe077213a45af67be28f74
4 files changed
+33
-12
commit-graph.c
+7
-2
@@ -343,6 +343,11 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
343
item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
344
}
345
346
+static inline void set_commit_tree(struct commit *c, struct tree *t)
347
+{
348
+ c->maybe_tree = t;
349
+}
350
+
351
static int fill_commit_in_graph(struct repository *r,
352
struct commit *item,
353
struct commit_graph *g, uint32_t pos)
@@ -356,7 +361,7 @@ static int fill_commit_in_graph(struct repository *r,
361
item->object.parsed = 1;
362
item->graph_pos = pos;
363
359
- item->maybe_tree = NULL;
364
+ set_commit_tree(item, NULL);
365
366
date_high = get_be32(commit_data + g->hash_len + 8) & 0x3;
367
date_low = get_be32(commit_data + g->hash_len + 12);
@@ -442,7 +447,7 @@ static struct tree *load_tree_for_commit(struct repository *r,
447
GRAPH_DATA_WIDTH * (c->graph_pos);
448
449
hashcpy(oid.hash, commit_data);
445
- c->maybe_tree = lookup_tree(r, &oid);
450
+ set_commit_tree(c, lookup_tree(r, &oid));
451
452
return c->maybe_tree;
453
}
commit.c
+7
-2
@@ -340,6 +340,11 @@ void free_commit_buffer(struct parsed_object_pool *pool, struct commit *commit)
340
}
341
}
342
343
+static inline void set_commit_tree(struct commit *c, struct tree *t)
344
+{
345
+ c->maybe_tree = t;
346
+}
347
+
348
struct tree *get_commit_tree(const struct commit *commit)
349
{
350
if (commit->maybe_tree || !commit->object.parsed)
@@ -358,7 +363,7 @@ struct object_id *get_commit_tree_oid(const struct commit *commit)
363
364
void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)
365
{
361
- c->maybe_tree = NULL;
366
+ set_commit_tree(c, NULL);
367
c->index = 0;
368
free_commit_buffer(pool, c);
369
free_commit_list(c->parents);
@@ -406,7 +411,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
411
if (get_oid_hex(bufptr + 5, &parent) < 0)
412
return error("bad tree pointer in commit %s",
413
oid_to_hex(&item->object.oid));
409
- item->maybe_tree = lookup_tree(r, &parent);
414
+ set_commit_tree(item, lookup_tree(r, &parent));
415
bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
416
pptr = &item->parents;
417
contrib/coccinelle/commit.cocci
+13
-7
@@ -10,19 +10,25 @@ expression c;
10
- c->maybe_tree->object.oid.hash
11
+ get_commit_tree_oid(c)->hash
12
13
-// These excluded functions must access c->maybe_tree direcly.
13
@@
15
-identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
14
+identifier f !~ "^set_commit_tree$";
15
expression c;
16
+expression s;
17
@@
18
f(...) {<...
19
-- c->maybe_tree
20
-+ get_commit_tree(c)
19
+- c->maybe_tree = s
20
++ set_commit_tree(c, s)
21
...>}
22
23
+// These excluded functions must access c->maybe_tree direcly.
24
+// Note that if c->maybe_tree is written somewhere outside of these
25
+// functions, then the recommended transformation will be bogus with
26
+// get_commit_tree() on the LHS.
27
@@
28
+identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$";
29
expression c;
25
-expression s;
30
@@
27
-- get_commit_tree(c) = s
28
-+ c->maybe_tree = s
31
+ f(...) {<...
32
+- c->maybe_tree
33
++ get_commit_tree(c)
34
+ ...>}
merge-recursive.c
+6
-1
@@ -163,6 +163,11 @@ static struct tree *shift_tree_object(struct repository *repo,
163
return lookup_tree(repo, &shifted);
164
}
165
166
+static inline void set_commit_tree(struct commit *c, struct tree *t)
167
+{
168
+ c->maybe_tree = t;
169
+}
170
+
171
static struct commit *make_virtual_commit(struct repository *repo,
172
struct tree *tree,
173
const char *comment)
@@ -170,7 +175,7 @@ static struct commit *make_virtual_commit(struct repository *repo,
175
struct commit *commit = alloc_commit_node(repo);
176
177
set_merge_remote_desc(commit, comment, (struct object *)commit);
173
- commit->maybe_tree = tree;
178
+ set_commit_tree(commit, tree);
179
commit->object.parsed = 1;
180
return commit;
181
}