mirror/git
1
0
Fork 0
mirror of https://github.com/git/git.git synced 2024-11-15 17:03:14 +01:00
Code
11868978c7
git/contrib/coccinelle/commit.cocci

53 lines
1.0 KiB
Plaintext
Raw Normal View History

treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
@@
expression c;
@@
- &c->maybe_tree->object.oid
+ get_commit_tree_oid(c)
@@
expression c;
@@
- c->maybe_tree->object.oid.hash
+ get_commit_tree_oid(c)->hash
@@
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>
2019-04-16 11:33:18 +02:00
identifier f !~ "^set_commit_tree$";
treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
expression c;
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>
2019-04-16 11:33:18 +02:00
expression s;
treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
@@
coccinelle: use <...> for function exclusion Sometimes we want to suppress a coccinelle transformation inside a particular function. For example, in finding conversions of hashcmp() to oidcmp(), we should not convert the call in oidcmp() itself, since that would cause infinite recursion. We write that like this: @@ identifier f != oidcmp; expression E1, E2; @@ f(...) {... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) ...} to match the interior of any function _except_ oidcmp(). Unfortunately, this doesn't catch all cases (e.g., the one in sequencer.c that this patch fixes). The problem, as explained by one of the Coccinelle developers in [1], is: For transformation, A ... B requires that B occur on every execution path starting with A, unless that execution path ends up in error handling code. (eg, if (...) { ... return; }). Here your A is the start of the function. So you need a call to hashcmp on every path through the function, which fails when you add ifs. [...] Another issue with A ... B is that by default A and B should not appear in the matched region. So your original rule matches only the case where every execution path contains exactly one call to hashcmp, not more than one. One way to solve this is to put the pattern inside an angle-bracket pattern like "<... P ...>", which allows zero or more matches of P. That works (and is what this patch does), but it has one drawback: it matches more than we care about, and Coccinelle uses extra CPU. Here are timings for "make coccicheck" before and after this patch: [before] real 1m27.122s user 7m34.451s sys 0m37.330s [after] real 2m18.040s user 10m58.310s sys 0m41.549s That's not ideal, but it's more important for this to be correct than to be fast. And coccicheck is already fairly slow (and people don't run it for every single patch). So it's an acceptable tradeoff. There _is_ a better way to do it, which is to record the position at which we find hashcmp(), and then check it against the forbidden function list. Like: @@ position p : script:python() { p[0].current_element != "oidcmp" }; expression E1,E2; @@ - hashcmp@p(E1->hash, E2->hash) + oidcmp(E1, E2) This is only a little slower than the current code, and does the right thing in all cases. Unfortunately, not all builds of Coccinelle include python support (including the ones in Debian). Requiring it may mean that fewer people can easily run the tool, which is worse than it simply being a little slower. We may want to revisit this decision in the future if: - builds with python become more common - we find more uses for python support that tip the cost-benefit analysis But for now this patch sticks with the angle-bracket solution, and converts all existing cocci patches. This fixes only one missed case in the current code, though it makes a much better difference for some new rules I'm adding (converting "!hashcmp()" to "hasheq()" misses over half the possible conversions using the old form). [1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/ Helped-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-28 23:22:32 +02:00
f(...) {<...
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>
2019-04-16 11:33:18 +02:00
- c->maybe_tree = s
+ set_commit_tree(c, s)
coccinelle: use <...> for function exclusion Sometimes we want to suppress a coccinelle transformation inside a particular function. For example, in finding conversions of hashcmp() to oidcmp(), we should not convert the call in oidcmp() itself, since that would cause infinite recursion. We write that like this: @@ identifier f != oidcmp; expression E1, E2; @@ f(...) {... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) ...} to match the interior of any function _except_ oidcmp(). Unfortunately, this doesn't catch all cases (e.g., the one in sequencer.c that this patch fixes). The problem, as explained by one of the Coccinelle developers in [1], is: For transformation, A ... B requires that B occur on every execution path starting with A, unless that execution path ends up in error handling code. (eg, if (...) { ... return; }). Here your A is the start of the function. So you need a call to hashcmp on every path through the function, which fails when you add ifs. [...] Another issue with A ... B is that by default A and B should not appear in the matched region. So your original rule matches only the case where every execution path contains exactly one call to hashcmp, not more than one. One way to solve this is to put the pattern inside an angle-bracket pattern like "<... P ...>", which allows zero or more matches of P. That works (and is what this patch does), but it has one drawback: it matches more than we care about, and Coccinelle uses extra CPU. Here are timings for "make coccicheck" before and after this patch: [before] real 1m27.122s user 7m34.451s sys 0m37.330s [after] real 2m18.040s user 10m58.310s sys 0m41.549s That's not ideal, but it's more important for this to be correct than to be fast. And coccicheck is already fairly slow (and people don't run it for every single patch). So it's an acceptable tradeoff. There _is_ a better way to do it, which is to record the position at which we find hashcmp(), and then check it against the forbidden function list. Like: @@ position p : script:python() { p[0].current_element != "oidcmp" }; expression E1,E2; @@ - hashcmp@p(E1->hash, E2->hash) + oidcmp(E1, E2) This is only a little slower than the current code, and does the right thing in all cases. Unfortunately, not all builds of Coccinelle include python support (including the ones in Debian). Requiring it may mean that fewer people can easily run the tool, which is worse than it simply being a little slower. We may want to revisit this decision in the future if: - builds with python become more common - we find more uses for python support that tip the cost-benefit analysis But for now this patch sticks with the angle-bracket solution, and converts all existing cocci patches. This fixes only one missed case in the current code, though it makes a much better difference for some new rules I'm adding (converting "!hashcmp()" to "hasheq()" misses over half the possible conversions using the old form). [1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/ Helped-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-28 23:22:32 +02:00
...>}
treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
Fix spelling errors in code comments Reported-by: Jens Schleusener <Jens.Schleusener@fossies.org> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-05 18:07:23 +01:00
// These excluded functions must access c->maybe_tree directly.
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>
2019-04-16 11:33:18 +02:00
// Note that if c->maybe_tree is written somewhere outside of these
// functions, then the recommended transformation will be bogus with
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. Any access to "maybe_tree" field directly will result in _broken_ code after running through commit.cocci because we can't know what is the right repository to use. the_repository would be correct most of the time. But we're relying less and less on the_repository and that assumption may no longer be true. The transformation now is more of a poor man replacement for a C++ compiler catching access to private fields. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-04-16 11:33:19 +02:00
// repo_get_commit_tree() on the LHS.
treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
@@
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. Any access to "maybe_tree" field directly will result in _broken_ code after running through commit.cocci because we can't know what is the right repository to use. the_repository would be correct most of the time. But we're relying less and less on the_repository and that assumption may no longer be true. The transformation now is more of a poor man replacement for a C++ compiler catching access to private fields. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-04-16 11:33:19 +02:00
identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$";
treewide: replace maybe_tree with accessor methods In anticipation of making trees load lazily, create a Coccinelle script (contrib/coccinelle/commit.cocci) to ensure that all references to the 'maybe_tree' member of struct commit are either mutations or accesses through get_commit_tree() or get_commit_tree_oid(). Apply the Coccinelle script to create the rest of the patch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-04-06 21:09:38 +02:00
expression c;
@@
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>
2019-04-16 11:33:18 +02:00
f(...) {<...
- c->maybe_tree
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. Any access to "maybe_tree" field directly will result in _broken_ code after running through commit.cocci because we can't know what is the right repository to use. the_repository would be correct most of the time. But we're relying less and less on the_repository and that assumption may no longer be true. The transformation now is more of a poor man replacement for a C++ compiler catching access to private fields. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-04-16 11:33:19 +02:00
+ repo_get_commit_tree(specify_the_right_repo_here, c)
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>
2019-04-16 11:33:18 +02:00
...>}
commit: move members graph_pos, generation to a slab We remove members `graph_pos` and `generation` from the struct commit. The default assignments in init_commit_node() are no longer valid, which is fine as the slab helpers return appropriate default values and the assignments are removed. We will replace existing use of commit->generation and commit->graph_pos by commit_graph_data_slab helpers using `contrib/coccinelle/commit.cocci'. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-06-17 11:14:10 +02:00
@@
struct commit *c;
expression E;
@@
(
- c->generation = E;
+ commit_graph_data_at(c)->generation = E;
|
- c->graph_pos = E;
+ commit_graph_data_at(c)->graph_pos = E;
|
- c->generation
+ commit_graph_generation(c)
|
- c->graph_pos
+ commit_graph_position(c)
)
Copy Permalink