On Mon, Nov 22, 2021 at 07:19:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I did have this with a strvec_attach() locally, but figured I'd get
> comments about growing scope & with just one caller.
I do think I'd rather see us avoiding have to do this attach() by
refactoring the Windows code. But I sympathize with your pain in the
guess-and-wait-for-CI loop with Windows (I have an unrelated series I've
done that with, and it's rather awful).
> This version seems to be duplicating things in the existing API though,
> I just had the below, which I think works just as well without the
> duplication. Perhaps you missed strvec_push_nodup()?
>
> diff --git a/strvec.c b/strvec.c
> index 61a76ce6cb9..c10008d792f 100644
> --- a/strvec.c
> +++ b/strvec.c
> @@ -106,3 +106,9 @@ const char **strvec_detach(struct strvec *array)
> return ret;
> }
> }
> +
> +void strvec_attach(struct strvec *array, const char **items)
> +{
> + for (; *items; items++)
> + strvec_push_nodup(array, *items);
> +}
This isn't taking ownership of "items", though. We've attached the
things it points to, but not the array itself. It would perhaps be OK to
free() it here under the notion that we took ownership of it and it is
ours to do with as we please. Potentially a caller might expect that
we'd continue to use the attached buffer, but any such assumption is
invalid once another strvec_push() is called, since that could replace
the array anyway.
It's also slightly less efficient (it grows a new array unnecessarily),
but I doubt that matters much in practice.
-Peff
