[RFC] coccinelle: detect struct strbuf passed by value

3 messages between Mar 14, 2026 and Mar 15, 2026, from Deveshi Dwivedi, Jeff King.

Plain Markdown or JSON for tools and agents.

Deveshi DwivediMar 14, 2026, 17:12 UTC on lore

While reviewing the write_worktree_linking_files() fix [1], Jeff King suggested adding a coccinelle rule to catch functions that take struct strbuf by value. He noted that a reporting rule using coccinelle's Python scripting extensions could emit a descriptive warning.

A transformation rule achieves the same detection without the dependency. It rewrites a by-value strbuf parameter to a pointer. The resulting diff will not produce compilable code on its own (callers and the function body still need updating), but the spatch output alerts the developer that the signature needs attention. This is consistent with the other rules in strbuf.cocci, which also rewrite to the preferred form.

The rule itself:

    @@
    identifier fn, param;
    @@
      fn(...,
    - struct strbuf param
    + struct strbuf *param
      ,...)
      {
      ...
      }

Running 'make coccicheck COCCI=contrib/coccinelle/strbuf.cocci' on master catches two instances:

  - write_worktree_linking_files() in worktree.c, which is already
    fixed by the series in [1].

  - save_untracked_files() in builtin/stash.c, which takes
    'struct strbuf files' by value.  This is the same class of bug.

Sending this as an RFC to get feedback on whether this rule would be a reasonable addition before preparing a patch.

[1] https://lore.kernel.org/git/20260309192600.GC309867@coredump.intra.peff.net/

Deveshi Dwivedi

Jeff KingMar 15, 2026, 02:55 UTC in reply to Deveshi Dwivedi on lore

Re: [RFC] coccinelle: detect struct strbuf passed by value

On Sat, Mar 14, 2026 at 10:42:19PM +0530, Deveshi Dwivedi wrote:

Show 20 quoted lines

> A transformation rule achieves the same detection without the
> dependency.  It rewrites a by-value strbuf parameter to a pointer.
> The resulting diff will not produce compilable code on its own
> (callers and the function body still need updating), but the spatch
> output alerts the developer that the signature needs attention.
> This is consistent with the other rules in strbuf.cocci, which also
> rewrite to the preferred form.
> 
> The rule itself:
> 
>     @@
>     identifier fn, param;
>     @@
>       fn(...,
>     - struct strbuf param
>     + struct strbuf *param
>       ,...)
>       {
>       ...
>       }

This is much better than what I posted before. The real source of the problem is the functions which take strbufs by value, not the callsites that pass it to them (and mine was checking the latter).

And your use of "..." is better than what I had. I think mine insisted on having arguments after the strbuf, which is why it failed to find the case in save_untracked_files().

So the only question to me is whether people who hit the coccinelle suggestion might be confused by the patch output, since it doesn't carry any rationale. But I would rather catch the problem and risk confusion then have it go unnoticed.

-Peff

Deveshi DwivediMar 15, 2026, 07:29 UTC in reply to Jeff King on lore

Re: [RFC] coccinelle: detect struct strbuf passed by value

Show 6 quoted lines

> So the only question to me is whether people who hit the coccinelle
> suggestion might be confused by the patch output, since it doesn't carry
> any rationale. But I would rather catch the problem and risk confusion
> then have it go unnoticed.
>
> -Peff

Thanks for the review. I agree that the patch output may be a bit confusing, but it still seems useful to flag these cases. I will send a follow-up patch adding the rule and fixing the stash case.

Thanks, Deveshi