Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2021, 01:23 UTC
Message-ID: <xmqq8s0m9xbl.fsf@gitster.g>
In-Reply-To: <patch-v3-2.5-321b8ba3f0e-20210826T140414Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 9 quoted lines

> Add a strvec_pushvec() function to concatenate two "struct strvec *"
> together, and modify code added in 50d89ad6542 (submodule: use
> argv_array instead of hand-building arrays, 2012-09-01) to use it. In
> a subsequent commit we'll gain another API user.
>
> This could also have been named strvec_concat()[1], but I opted to
> make its name consistent with the strbuf_addbuf() function instead. We
> only name these sorts of functions *_concat() in one instance:
> parse_options_concat().

FWIW pushvec() is a much better name than concat() for this.

concat(A, B) could be an operation that is non-destructive for both A and B that returns a new vector C. I would imagine that it would be how, if not majority of, but at least a nontrivial percentage of, readers expect a function called concat() to behave. A better name for destructive version would probably be append(), if there is no other constraints on naming.

The name pushvec(A, B) makes it clear, thanks to similarity with other push*(A, ...) variants, that A is mutated using the other arguments. The name is much more clear with less chance of misunderstanding.

Show 7 quoted lines

> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
> +{
> +	int i;
> +
> +	for (i = 0; i < items->nr; i++)
> +		strvec_push(array, items->v[i]);
> +}

This implementation is not wrong per-se, but is somewhat disappointing. When items->nr is large, especially relative to the original array->alloc, it would incur unnecessary reallocations that we can easily avoid by pre-sizing the array before pushing the elements of items from it.

In the original code that became the first user of this helper, it may not have made much difference, but now it is becoming a more generally reusable API function, we should care.

Other than that, looks quite straight-forward.

Thanks.