{"thread":{"id":"59502","subject":"How do we review changes made with coccinelle?","startedAt":"2023-03-30T18:23:47Z","lastAt":"2023-03-31T23:53:42Z","messageCount":6,"participants":["Glen Choo","Junio C Hamano","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"474474","messageId":"kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59502","inReplyTo":null,"subject":"How do we review changes made with coccinelle?","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-30T18:23:40Z","receivedAt":"2023-03-30T18:23:47Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nÆvar recently sent a series that made pretty extensive use of coccinelle\nto remove a lot of the_repository [1], but then we ended up in this\nweird spot where even though several reviewers thought the code changes\nlooked good, we weren't sure about giving Reviewed-By because we didn't\nknow coccinelle or how to review it.\n\nI'm pretty sure that the series could have been merged without a hitch\nif reviewers knew what to do about coccinelle. I expect more of these to\nappear as a result of the libification work, so it's probably a good\ntime for us to figure out some norms about coccinelle :)\n\nPerhaps we could start the discussion by sharing thoughts on the\nfollowing questions, which I'll summarize in a change to\ncontrib/coccinelle/README (where we can do final bikeshedding):\n\n- Is it okay to give Reviewed-By on the basis of _just_ the in-tree\n changes and ignore the .cocci patch?\n\n - If not, what should reviewers look for in .cocci? Do we have a\n style?\n\n- When do we introduce .pending.cocci vs .cocci?\n\n- What do we do with .cocci after they've been applied?\n\n- Do we care about new patches slowing down coccicheck?\n\nRelevant threads\n- How to learn cocci: https://lore.kernel.org/git/230326.86edpcw0yh.gmgdl@evledraar.gmail.com/ \n- https://lore.kernel.org/git/230328.86a5zxw31u.gmgdl@evledraar.gmail.com/\n- https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/\n\n[1] https://lore.kernel.org/git/cover-v2-00.17-00000000000-20230328T110946Z-avarab@gmail.com/\n"},{"id":"474478","messageId":"xmqqtty2hx30.fsf@gitster.g","threadId":"59502","inReplyTo":"kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: How do we review changes made with coccinelle?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T19:13:07Z","receivedAt":"2023-03-30T19:13:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> - Is it okay to give Reviewed-By on the basis of _just_ the in-tree\n> changes and ignore the .cocci patch?\n\nIf they were made in separate steps, sure. If not, not really. But\nwe can still say \"I've checked the changes the author made to the\ncode and they looked good.\" But we haven't reviewed the patch in\nits entirety in such a case to give a Reviewed-by, I would thihk.\n\n> - What do we do with .cocci after they've been applied?\n\nWhen we keep .cocci rules in tree, \"make coccicheck\" would complain\non any new code that matches the preimage pattern of these rules and\nadjust them. Your use of memcpy() may be rewritten to COPY_ARRAY()\nwhen appropriate. At least that is the theory---an overly wide or\nad-hoc rule that depends too much on heuristic may misconvert future\ncode, which needs to be caught by reviewers when .cocci files are\nadded to the tree.\n\n> - Do we care about new patches slowing down coccicheck?\n\nSurely.\n\nWe may want to cull rules from time to time. For example, a rule\nthat moves callers of an older API function to use a newer API\nfunction has to be kept while the older API function still exists in\nthe tree to help topics that are still in flight, but eventually\neverybody stops using it and the implementation of the older API\nfunction gets removed. We should make sure we remove the rule that\nis now stale.\n"},{"id":"474518","messageId":"kl6lzg7sc01z.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59502","inReplyTo":"xmqqtty2hx30.fsf@gitster.g","subject":"Re: How do we review changes made with coccinelle?","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-31T17:17:44Z","receivedAt":"2023-03-31T17:17:49Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> - Is it okay to give Reviewed-By on the basis of _just_ the in-tree\n>> changes and ignore the .cocci patch?\n>\n> If they were made in separate steps, sure. If not, not really. But\n> we can still say \"I've checked the changes the author made to the\n> code and they looked good.\" But we haven't reviewed the patch in\n> its entirety in such a case to give a Reviewed-by, I would thihk.\n\nI agree, but I wonder what this means in practice when .cocci fluency is\nso low. Maybe:\n\n- We increase .cocci fluency, partly by creating our own learning\n materials, partly by being more conscious about spreading knowledge\n from the folks who are relatively fluent in it. We might not have to\n do a lot to get the ball rolling either; just checking in a\n \"MyFirstCocci\" would help a lot, I think.\n\n- Define some guidelines for what it means for a \".cocci\" to be 'good\n enough'. E.g.:\n\n - Do rules need to target just the right things or is some collateral\n damage okay? Does this change between .cocci and .pending.cocci?\n\n - Do we allow copy-pasting in .cocci rules when a more 'elegant'\n alternative exists?\n\n>> - What do we do with .cocci after they've been applied?\n>\n> When we keep .cocci rules in tree, \"make coccicheck\" would complain\n> on any new code that matches the preimage pattern of these rules and\n> adjust them.\n\nOrthogonal to the in tree .cocci rules that enforce code style, there\nare .cocci meant for large scale refactoring.\n\nIIUC at least one other opinion there is that we keep such refactoring\nrules in tree temporarily for the benefit of in-flight or out-of-tree\nconflicts [1], but since they are defunct by definition, we remove them\neventually.\n\n[1] https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/\n"},{"id":"474523","messageId":"xmqqedp4g4z7.fsf@gitster.g","threadId":"59502","inReplyTo":"kl6lzg7sc01z.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: How do we review changes made with coccinelle?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-31T18:17:48Z","receivedAt":"2023-03-31T18:17:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> I agree, but I wonder what this means in practice when .cocci fluency is\n> so low. Maybe:\n>\n> - We increase .cocci fluency, partly by creating our own learning\n> materials, partly by being more conscious about spreading knowledge\n> from the folks who are relatively fluent in it. We might not have to\n> do a lot to get the ball rolling either; just checking in a\n> \"MyFirstCocci\" would help a lot, I think.\n\nI think it goes both ways. Readers of course must be willing to\nlearn, but whoever proposes to add a .cocci patch file should be\nable to explain what the rewrite rules in there are designed to do\nand why. It is pretty much the same deal as any other patches.\n\nI'll wait and prefer to hear from others, also on points that I did\nnot even quote back in my message because I didn't have anything to\nadd myself, before digging further on points I responded to.\n\nThanks for starting this topic.\n"},{"id":"474532","messageId":"ZCdxcKr2mQ5cBQ8u@nand.local","threadId":"59502","inReplyTo":"xmqqtty2hx30.fsf@gitster.g","subject":"Re: How do we review changes made with coccinelle?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-31T23:49:04Z","receivedAt":"2023-03-31T23:49:12Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 30, 2023 at 12:13:07PM -0700, Junio C Hamano wrote:\n> Glen Choo writes:\n>\n> > - Is it okay to give Reviewed-By on the basis of _just_ the in-tree\n> > changes and ignore the .cocci patch?\n>\n> If they were made in separate steps, sure. If not, not really. But\n> we can still say \"I've checked the changes the author made to the\n> code and they looked good.\" But we haven't reviewed the patch in\n> its entirety in such a case to give a Reviewed-by, I would thihk.\n\nI think that while none of us would probably call ourselves \"Coccinelle\nexperts\", we are probably reasonably capable of reviewing *.cocci files\nand their impact on the tree.\n\nWhat I meant when we were talking about this off-list was that I don't\nconsider myself an expert at writing idiomatic Coccinelle rules. But I\nfeel competent enough that I could review Ævar's patches by roughly\ngrokking the *.cocci changes, and then checking that the resulting tree\nstate matched my understanding of those changes.\n\n> > - Do we care about new patches slowing down coccicheck?\n\nI was the one who asked this question off-list, and I did so in a\nleading way that implied that the answer was \"no\".\n\n> Surely.\n\nBut I agree with Junio that we *do* care about slowing down the\nperformance of 'make coccicheck'. When I originally asked, I was under\nthe (false) impression that we didn't run 'make coccicheck' in CI. But\nwe do (see ci/run-static-analysis.sh), so we do care about the\nperformance there since we don't want to unnecessarily slow down CI.\n\nThanks,\nTaylor\n"},{"id":"474533","messageId":"ZCdyfaiFH5s5eYUc@nand.local","threadId":"59502","inReplyTo":"kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: How do we review changes made with coccinelle?","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-31T23:53:33Z","receivedAt":"2023-03-31T23:53:42Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 30, 2023 at 11:23:40AM -0700, Glen Choo wrote:\n> - When do we introduce .pending.cocci vs .cocci?\n\nThe documentation in contrib/coccinelle/README is authoritative. I'll\nrefer you there for the full details, but the rough idea is that:\n\n- *.cocci patches are used to guard against bad patterns in the code\n (e.g., that we should never write \"if (thing != NULL)\" and instead \"if\n (thing)\"). When applying these transformations leads to changes, they\n are designed to break the build to avoid letting these bad patterns\n enter our tree.\n\n- *.pending.cocci patches are used to guide large scale refactorings,\n like Ævar demonstrated in his last series. They don't cause us to fail\n the build in the meantime while the refactoring is in progress.\n\nThe latter is useful when the refactoring is either (a) too large to fit\nin a single patch, or (b) would cause too many conflicts with in-flight\nseries.\n\n(FWIW, I think that I'm mixing up my terminology, in that *.cocci files\nare typically referred to as \"transformations\" and the result of running\nCoccinelle on them is used to produce a patch. Shows you how much I know\nabout Coccinelle ;-).)\n\nThanks,\nTaylor\n"}]}