> 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 */

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

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

> 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);

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

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