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

[Junio C Hamano <gitster@pobox.com>]

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?