summaryrefslogtreecommitdiff
path: root/contrib/coccinelle/object_id.cocci
AgeCommit message (Collapse)AuthorFilesLines
2018-10-15object_id.cocci: match only expressions of type 'struct object_id'Libravatar SZEDER Gábor1-54/+63
Most of our semantic patches in 'contrib/coccinelle/object_id.cocci' turn calls of SHA1-specific functions into calls of their corresponding object_id counterparts, e.g. sha1_to_hex() to oid_to_hex(). These semantic patches look something like this: @@ expression E1; @@ - sha1_to_hex(E1.hash) + oid_to_hex(&E1) and match the access to the 'hash' field in any data type, not only in 'struct object_id', and, consquently, can produce wrong transformations. Case in point is the recent hash function transition patch "rerere: convert to use the_hash_algo" [1], which, among other things, renamed 'struct rerere_dir's 'sha1' field to 'hash', and then 'make coccicheck' started to suggest the following wrong transformations for 'rerere.c' [2]: - return sha1_to_hex(id->collection->hash); + return oid_to_hex(id->collection); and - DIR *dir = opendir(git_path("rr-cache/%s", sha1_to_hex(rr_dir->hash))); + DIR *dir = opendir(git_path("rr-cache/%s", oid_to_hex(rr_dir))); Avoid such wrong transformations by tightening semantic patches in 'object_id.cocci' to match only type of or pointers to 'struct object_id'. [1] https://public-inbox.org/git/20181008215701.779099-15-sandals@crustytoothpaste.net/ [2] https://travis-ci.org/git/git/jobs/440463476#L580 Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-29convert "hashcmp() != 0" to "!hasheq()"Libravatar Jeff King1-0/+9
This rounds out the previous three patches, covering the inequality logic for the "hash" variant of the functions. As with the previous three, the accompanying code changes are the mechanical result of applying the coccinelle patch; see those patches for more discussion. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-29convert "oidcmp() != 0" to "!oideq()"Libravatar Jeff King1-0/+6
This is the flip side of the previous two patches: checking for a non-zero oidcmp() can be more strictly expressed as inequality. Like those patches, we write "!= 0" in the coccinelle transformation, which covers by isomorphism the more common: if (oidcmp(E1, E2)) As with the previous two patches, this patch can be achieved almost entirely by running "make coccicheck"; the only differences are manual line-wrap fixes to match the original code. There is one thing to note for anybody replicating this, though: coccinelle 1.0.4 seems to miss the case in builtin/tag.c, even though it's basically the same as all the others. Running with 1.0.7 does catch this, so presumably it's just a coccinelle bug that was fixed in the interim. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-29convert "hashcmp() == 0" to hasheq()Libravatar Jeff King1-0/+9
This is the partner patch to the previous one, but covering the "hash" variants instead of "oid". Note that our coccinelle rule is slightly more complex to avoid triggering the call in hasheq(). I didn't bother to add a new rule to convert: - hasheq(E1->hash, E2->hash) + oideq(E1, E2) Since these are new functions, there won't be any such existing callers. And since most of the code is already using oideq, we're not likely to introduce new ones. We might still see "!hashcmp(E1->hash, E2->hash)" from topics in flight. But because our new rule comes after the existing ones, that should first get converted to "!oidcmp(E1, E2)" and then to "oideq(E1, E2)". Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-29convert "oidcmp() == 0" to oideq()Libravatar Jeff King1-0/+6
Using the more restrictive oideq() should, in the long run, give the compiler more opportunities to optimize these callsites. For now, this conversion should be a complete noop with respect to the generated code. The result is also perhaps a little more readable, as it avoids the "zero is equal" idiom. Since it's so prevalent in C, I think seasoned programmers tend not to even notice it anymore, but it can sometimes make for awkward double negations (e.g., we can drop a few !!oidcmp() instances here). This patch was generated almost entirely by the included coccinelle patch. This mechanical conversion should be completely safe, because we check explicitly for cases where oidcmp() is compared to 0, which is what oideq() is doing under the hood. Note that we don't have to catch "!oidcmp()" separately; coccinelle's standard isomorphisms make sure the two are treated equivalently. I say "almost" because I did hand-edit the coccinelle output to fix up a few style violations (it mostly keeps the original formatting, but sometimes unwraps long lines). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2018-08-29coccinelle: use <...> for function exclusionLibravatar Jeff King1-10/+10
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>
2016-11-01cocci: avoid self-references in object_id transformationsLibravatar René Scharfe1-0/+15
The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex, sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure that the Coccinelle transformations for converting legacy function calls are not applied to these wrappers themselves, which would result in tautological declarations. Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-15contrib/coccinelle: fix semantic patch for oid_to_hex_r()Libravatar René Scharfe1-6/+6
Both sha1_to_hex_r() and oid_to_hex_r() take two parameters, so use two expressions in the semantic patch for transforming calls of the former to the latter one. Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-28contrib/coccinelle: add basic Coccinelle transformsLibravatar brian m. carlson1-0/+95
Coccinelle (http://coccinelle.lip6.fr/) is a program which performs mechanical transformations on C programs using semantic patches. These semantic patches can be used to implement automatic refactoring and maintenance tasks. Add a set of basic semantic patches to convert common patterns related to the struct object_id transformation, as well as a README. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>