[PATCH] use strvec_pushv() to add another strvec

[PATCH] use strvec_pushv() to add another strvec

[René Scharfe]

Simplify the code by letting strvec_pushv() add the items of a second
strvec instead of pushing them one by one.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 fetch-pack.c | 8 ++------
 git.c        | 3 +--
 submodule.c  | 4 +---
 3 files changed, 4 insertions(+), 11 deletions(-)

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);
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
diff --git a/git.c b/git.c
index 2b212e6675..5a40eab8a2 100644
--- a/git.c
+++ b/git.c
@@ -877,8 +877,7 @@ static int run_argv(struct strvec *args)
 			commit_pager_choice();
 
 			strvec_push(&cmd.args, "git");
-			for (size_t i = 0; i < args->nr; i++)
-				strvec_push(&cmd.args, args->v[i]);
+			strvec_pushv(&cmd.args, args->v);
 
 			trace_argv_printf(cmd.args.v, "trace: exec:");
 
diff --git a/submodule.c b/submodule.c
index cd879a5cfe..4c8c674aa4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1815,7 +1815,6 @@ int fetch_submodules(struct repository *r,
 		     int default_option,
 		     int quiet, int max_parallel_jobs)
 {
-	int i;
 	struct submodule_parallel_fetch spf = SPF_INIT;
 	const struct run_process_parallel_opts opts = {
 		.tr2_category = "submodule",
@@ -1842,8 +1841,7 @@ int fetch_submodules(struct repository *r,
 		die(_("index file corrupt"));
 
 	strvec_push(&spf.args, "fetch");
-	for (i = 0; i < options->nr; i++)
-		strvec_push(&spf.args, options->v[i]);
+	strvec_pushv(&spf.args, options->v);
 	strvec_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
-- 
2.53.0

Re: [PATCH] use strvec_pushv() to add another strvec

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Simplify the code by letting strvec_pushv() add the items of a second
> strvec instead of pushing them one by one.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  fetch-pack.c | 8 ++------
>  git.c        | 3 +--
>  submodule.c  | 4 +---
>  3 files changed, 4 insertions(+), 11 deletions(-)

Nice.  Is this something we can make a coccinelle rule for?

Something like

@@
struct strvec SRC;
struct strvec DST;
size_t I;
@@
-	for (size_t I = 0; I < SRC.nr; I++)
-		strvec_push(&DST, SRC.v[I]);
+	strvec_pushv(&DST, SRC.v);

perhaps?

Will queue.

> 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);
>  
>  	sigchain_push(SIGPIPE, SIG_IGN);
>  
> diff --git a/git.c b/git.c
> index 2b212e6675..5a40eab8a2 100644
> --- a/git.c
> +++ b/git.c
> @@ -877,8 +877,7 @@ static int run_argv(struct strvec *args)
>  			commit_pager_choice();
>  
>  			strvec_push(&cmd.args, "git");
> -			for (size_t i = 0; i < args->nr; i++)
> -				strvec_push(&cmd.args, args->v[i]);
> +			strvec_pushv(&cmd.args, args->v);
>  
>  			trace_argv_printf(cmd.args.v, "trace: exec:");
>  
> diff --git a/submodule.c b/submodule.c
> index cd879a5cfe..4c8c674aa4 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1815,7 +1815,6 @@ int fetch_submodules(struct repository *r,
>  		     int default_option,
>  		     int quiet, int max_parallel_jobs)
>  {
> -	int i;
>  	struct submodule_parallel_fetch spf = SPF_INIT;
>  	const struct run_process_parallel_opts opts = {
>  		.tr2_category = "submodule",
> @@ -1842,8 +1841,7 @@ int fetch_submodules(struct repository *r,
>  		die(_("index file corrupt"));
>  
>  	strvec_push(&spf.args, "fetch");
> -	for (i = 0; i < options->nr; i++)
> -		strvec_push(&spf.args, options->v[i]);
> +	strvec_pushv(&spf.args, options->v);
>  	strvec_push(&spf.args, "--recurse-submodules-default");
>  	/* default value, "--submodule-prefix" and its value are added later */

Re: [PATCH] use strvec_pushv() to add another strvec

[René Scharfe]

On 3/19/26 10:14 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Simplify the code by letting strvec_pushv() add the items of a second
>> strvec instead of pushing them one by one.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  fetch-pack.c | 8 ++------
>>  git.c        | 3 +--
>>  submodule.c  | 4 +---
>>  3 files changed, 4 insertions(+), 11 deletions(-)
> 
> Nice.  Is this something we can make a coccinelle rule for?
> 
> Something like
> 
> @@
> struct strvec SRC;
> struct strvec DST;
> size_t I;
> @@
> -	for (size_t I = 0; I < SRC.nr; I++)
> -		strvec_push(&DST, SRC.v[I]);
> +	strvec_pushv(&DST, SRC.v);
> 
> perhaps?
We can.  Should we?  It does find a fourth case from the 18th batch that
has landed a few hours ago, at least.  Will send as v2.

René

[PATCH v2] use strvec_pushv() to add another strvec

[René Scharfe]

Add and apply a semantic patch that simplifies the code by letting
strvec_pushv() append the items of a second strvec instead of pushing
them one by one.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Changes since v1:
- Semantic patch.
- New conversion in builtin/rebase.c.

 builtin/rebase.c                |  3 +--
 contrib/coccinelle/strvec.cocci | 46 +++++++++++++++++++++++++++++++++
 fetch-pack.c                    |  8 ++----
 git.c                           |  3 +--
 submodule.c                     |  4 +--
 5 files changed, 51 insertions(+), 13 deletions(-)
 create mode 100644 contrib/coccinelle/strvec.cocci

diff --git a/builtin/rebase.c b/builtin/rebase.c
index a1c7d78196..fa4f5d9306 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -182,8 +182,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 
 	replay.signoff = opts->signoff;
 
-	for (size_t i = 0; i < opts->trailer_args.nr; i++)
-		strvec_push(&replay.trailer_args, opts->trailer_args.v[i]);
+	strvec_pushv(&replay.trailer_args, opts->trailer_args.v);
 
 	replay.allow_ff = !(opts->flags & REBASE_FORCE);
 	if (opts->allow_rerere_autoupdate)
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);
+)
+
+@ unused_loop_index extends separate_loop_index @
+@@
+  {
+  ...
+- T i;
+  ... when != i
+  }
+
+@ depends on unused_loop_index @
+@@
+  if (...)
+- {
+  strvec_pushv(...);
+- }
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);
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
diff --git a/git.c b/git.c
index 2b212e6675..5a40eab8a2 100644
--- a/git.c
+++ b/git.c
@@ -877,8 +877,7 @@ static int run_argv(struct strvec *args)
 			commit_pager_choice();
 
 			strvec_push(&cmd.args, "git");
-			for (size_t i = 0; i < args->nr; i++)
-				strvec_push(&cmd.args, args->v[i]);
+			strvec_pushv(&cmd.args, args->v);
 
 			trace_argv_printf(cmd.args.v, "trace: exec:");
 
diff --git a/submodule.c b/submodule.c
index cd879a5cfe..4c8c674aa4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1815,7 +1815,6 @@ int fetch_submodules(struct repository *r,
 		     int default_option,
 		     int quiet, int max_parallel_jobs)
 {
-	int i;
 	struct submodule_parallel_fetch spf = SPF_INIT;
 	const struct run_process_parallel_opts opts = {
 		.tr2_category = "submodule",
@@ -1842,8 +1841,7 @@ int fetch_submodules(struct repository *r,
 		die(_("index file corrupt"));
 
 	strvec_push(&spf.args, "fetch");
-	for (i = 0; i < options->nr; i++)
-		strvec_push(&spf.args, options->v[i]);
+	strvec_pushv(&spf.args, options->v);
 	strvec_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
-- 
2.53.0

Re: [PATCH v2] use strvec_pushv() to add another strvec

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Add and apply a semantic patch that simplifies the code by letting
> strvec_pushv() append the items of a second strvec instead of pushing
> them one by one.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Changes since v1:
> - Semantic patch.
> - New conversion in builtin/rebase.c.

Thanks.  The fixup does not apply to plain vanilla v2.53 but that is
OK, as a code-cleanup patch like this is not maint material.

> 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.

> +@ 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).

> +@ 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().

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.

> 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?

Re: [PATCH v2] use strvec_pushv() to add another strvec

[René Scharfe]

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é