From: "René Scharfe" <l.s.r@web•de>
To: Junio C Hamano <gitster@pobox•com>
Cc: Git List <git@vger•kernel.org>
Subject: Re: [PATCH v2] use strvec_pushv() to add another strvec
Date: Sat, 28 Mar 2026 00:07:38 +0100 [thread overview]
Message-ID: <b1a9ce9e-fe3f-4f1d-a603-7f484f1ba834@web.de> (raw)
In-Reply-To: <xmqqeclb91v6.fsf@gitster.g>
On 3/22/26 7:05 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web•de> writes:
>
>> diff --git a/contrib/coccinelle/strvec.cocci b/contrib/coccinelle/strvec.cocci
>> new file mode 100644
>> index 0000000000..64edb09f1c
>> --- /dev/null
>> +++ b/contrib/coccinelle/strvec.cocci
>> @@ -0,0 +1,46 @@
>> +@@
>> +type T;
>> +identifier i;
>> +expression dst;
>> +struct strvec *src_ptr;
>> +struct strvec src_arr;
>> +@@
>> +(
>> +- for (T i = 0; i < src_ptr->nr; i++) { strvec_push(dst, src_ptr->v[i]); }
>> ++ strvec_pushv(dst, src_ptr->v);
>> +|
>> +- for (T i = 0; i < src_arr.nr; i++) { strvec_push(dst, src_arr.v[i]); }
>> ++ strvec_pushv(dst, src_arr.v);
>> +)
>> +
>> +@ separate_loop_index @
>> +type T;
>> +identifier i;
>> +expression dst;
>> +struct strvec *src_ptr;
>> +struct strvec src_arr;
>> +@@
>> + T i;
>> + ...
>> +(
>> +- for (i = 0; i < src_ptr->nr; i++) { strvec_push(dst, src_ptr->v[i]); }
>> ++ strvec_pushv(dst, src_ptr->v);
>> +|
>> +- for (i = 0; i < src_arr.nr; i++) { strvec_push(dst, src_arr.v[i]); }
>> ++ strvec_pushv(dst, src_arr.v);
>> +)
>
> It is a bit unfortunate that we need to write these four cases separately.
An abstraction that matches both struct and struct pointer as well as
the appropriate member access operator would be nice.
"_arr" is a misnomer. struct strvec is not an array, even though it
contains one and its purpose is to store multiple items.
>> +@ unused_loop_index extends separate_loop_index @
>> +@@
>> + {
>> + ...
>> +- T i;
>> + ... when != i
>> + }
>
> I do not grok this one (not an objection, but a statement of fact
> that I have to look up what "when !=" is doing there and I haven't).
This line matches code that doesn't contain i, which is loop counter
from the rule above.
>> +@ depends on unused_loop_index @
>> +@@
>> + if (...)
>> +- {
>> + strvec_pushv(...);
>> +- }
>
> This is a bit questionable, in that we would probably want to remove
> excess {} around any simple single-statement block, and not limited
> to a call to strvec_pushv().
Perhaps, but this rule only exists to mop up after the one it depends
on, which can create such a thing.
Even if we had a general rule for that (and I'm not sure if that would
be a good idea), it would probably live in a different file. And
without this one here we'd need to run coccicheck and apply its results
twice -- first for strvec_pushv(), then again for brace removal.
Unnecessarily annoying.
> I think it leads to a philosophical question: should Coccinelle
> rules used in the context of this project aim to produce the ideal
> result that does not require any human clean-up, or is it OK to make
> humans notice there is a questionable construction without updating
> it to the final ideal form? I've been assuming the latter somehow
> but I do not recall we had a discussion or decision on this point
The README doesn't say, but mentions "cost-benefit ratio" twice.
Without brace-removal I get (with "make clean" between runs):
Benchmark 1: make tools/coccinelle/strvec.cocci.patch
Time (mean ± σ): 18.864 s ± 0.084 s [User: 51.865 s, System: 17.521 s]
Range (min … max): 18.677 s … 18.974 s 10 runs
... and with it:
Benchmark 1: make tools/coccinelle/strvec.cocci.patch
Time (mean ± σ): 19.066 s ± 0.132 s [User: 52.462 s, System: 17.665 s]
Range (min … max): 18.956 s … 19.300 s 10 runs
I think a more interesting question is whether we want to keep the
overall rule. If we do then automatic brace-removal doesn't add
too much a cost on top.
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 6ecd468ef7..a32224ed02 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -1024,12 +1024,8 @@ static int get_pack(struct fetch_pack_args *args,
>> fsck_msg_types.buf);
>> }
>>
>> - if (index_pack_args) {
>> - int i;
>> -
>> - for (i = 0; i < cmd.args.nr; i++)
>> - strvec_push(index_pack_args, cmd.args.v[i]);
>> - }
>> + if (index_pack_args)
>> + strvec_pushv(index_pack_args, cmd.args.v);
>
> This does lead to a great result, and I presume that this is the
> doing of the last two rules?
Right, and the one above them, of course.
René
prev parent reply other threads:[~2026-03-27 23:07 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-19 20:49 [PATCH] use strvec_pushv() to add another strvec René Scharfe
2026-03-19 21:14 ` Junio C Hamano
2026-03-20 0:46 ` René Scharfe
2026-03-20 0:46 ` [PATCH v2] " René Scharfe
2026-03-22 18:05 ` Junio C Hamano
2026-03-27 23:07 ` René Scharfe [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b1a9ce9e-fe3f-4f1d-a603-7f484f1ba834@web.de \
--to=l.s.r@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox