{"thread":{"id":"65246","subject":"[RFC] coccinelle: detect struct strbuf passed by value","startedAt":"2026-03-14T17:12:33Z","lastAt":"2026-03-15T07:29:23Z","messageCount":3,"participants":["Deveshi Dwivedi","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"538980","messageId":"CAG7UgESKLMnO_4+PSJUt-TXJxFQyxEEfpCmJfMmTw2+rhT-HWw@mail.gmail.com","threadId":"65246","inReplyTo":null,"subject":"[RFC] coccinelle: detect struct strbuf passed by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-14T17:12:19Z","receivedAt":"2026-03-14T17:12:33Z","isPatch":false,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"While reviewing the write_worktree_linking_files() fix [1], Jeff King\nsuggested adding a coccinelle rule to catch functions that take\nstruct strbuf by value.  He noted that a reporting rule using\ncoccinelle's Python scripting extensions could emit a descriptive\nwarning.\n\nA transformation rule achieves the same detection without the\ndependency.  It rewrites a by-value strbuf parameter to a pointer.\nThe resulting diff will not produce compilable code on its own\n(callers and the function body still need updating), but the spatch\noutput alerts the developer that the signature needs attention.\nThis is consistent with the other rules in strbuf.cocci, which also\nrewrite to the preferred form.\n\nThe rule itself:\n\n    @@\n    identifier fn, param;\n    @@\n      fn(...,\n    - struct strbuf param\n    + struct strbuf *param\n      ,...)\n      {\n      ...\n      }\n\nRunning 'make coccicheck COCCI=contrib/coccinelle/strbuf.cocci' on\nmaster catches two instances:\n\n  - write_worktree_linking_files() in worktree.c, which is already\n    fixed by the series in [1].\n\n  - save_untracked_files() in builtin/stash.c, which takes\n    'struct strbuf files' by value.  This is the same class of bug.\n\nSending this as an RFC to get feedback on whether this rule would be\na reasonable addition before preparing a patch.\n\n[1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/\n\nDeveshi Dwivedi\n"},{"id":"539007","messageId":"20260315025508.GA926820@coredump.intra.peff.net","threadId":"65246","inReplyTo":"CAG7UgESKLMnO_4+PSJUt-TXJxFQyxEEfpCmJfMmTw2+rhT-HWw@mail.gmail.com","subject":"Re: [RFC] coccinelle: detect struct strbuf passed by value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-15T02:55:08Z","receivedAt":"2026-03-15T02:55:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 14, 2026 at 10:42:19PM +0530, Deveshi Dwivedi wrote:\n\n> A transformation rule achieves the same detection without the\n> dependency.  It rewrites a by-value strbuf parameter to a pointer.\n> The resulting diff will not produce compilable code on its own\n> (callers and the function body still need updating), but the spatch\n> output alerts the developer that the signature needs attention.\n> This is consistent with the other rules in strbuf.cocci, which also\n> rewrite to the preferred form.\n> \n> The rule itself:\n> \n>     @@\n>     identifier fn, param;\n>     @@\n>       fn(...,\n>     - struct strbuf param\n>     + struct strbuf *param\n>       ,...)\n>       {\n>       ...\n>       }\n\nThis is much better than what I posted before. The real source of the\nproblem is the functions which take strbufs by value, not the callsites\nthat pass it to them (and mine was checking the latter).\n\nAnd your use of \"...\" is better than what I had. I think mine insisted\non having arguments after the strbuf, which is why it failed to find the\ncase in save_untracked_files().\n\nSo the only question to me is whether people who hit the coccinelle\nsuggestion might be confused by the patch output, since it doesn't carry\nany rationale. But I would rather catch the problem and risk confusion\nthen have it go unnoticed.\n\n-Peff\n"},{"id":"539014","messageId":"CAG7UgETB7uPvWuZL08i48JvVgefJAoUqjt96C0dPNV_Qpj8jfw@mail.gmail.com","threadId":"65246","inReplyTo":"20260315025508.GA926820@coredump.intra.peff.net","subject":"Re: [RFC] coccinelle: detect struct strbuf passed by value","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-03-15T07:29:08Z","receivedAt":"2026-03-15T07:29:23Z","isPatch":false,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"> So the only question to me is whether people who hit the coccinelle\n> suggestion might be confused by the patch output, since it doesn't carry\n> any rationale. But I would rather catch the problem and risk confusion\n> then have it go unnoticed.\n>\n> -Peff\nThanks for the review. I agree that the patch output may be a bit\nconfusing, but it still seems useful to flag these cases. I will send\na follow-up patch adding the rule and fixing the stash case.\n\nThanks,\nDeveshi\n"}]}