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

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

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.

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.