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

Jeff King [off-list ref] writes:

I think we'd want the patch below. It can be applied independently
(though if we do take the index-iterating version of Ævar's patch, I
think it should switch to size_t).

Yeah, I do not see a strong need for _pushvec(), especially the
variant that does not preallocate, when we have _pushv().  But the
type fix below does make sense.

On 8/29/2021 7:54 PM, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

I think we'd want the patch below. It can be applied independently
(though if we do take the index-iterating version of Ævar's patch, I
think it should switch to size_t).

Yeah, I do not see a strong need for _pushvec(), especially the
variant that does not preallocate, when we have _pushv().  But the
type fix below does make sense.

Thanks for chiming in. I was not aware of _pushv() when I asked
about the _pushvec() variant. Sorry, Ævar, for sending you down
an unnecessary direction.
 

quoted

-- >8 --
Subject: [PATCH] strvec: use size_t to store nr and alloc

We converted argv_array (which later became strvec) to use size_t in
819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in
order to avoid the possibility of integer overflow. But later, commit
d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally
converted these back to ints!

Those two commits were part of the same patch series. I'm pretty sure
what happened is that they were originally written in the opposite order
and then cleaned up and re-ordered during an interactive rebase. And
when resolving the inevitable conflict, I mistakenly took the "rename"
patch completely, accidentally dropping the type change.

We can correct it now; better late than never.

...

quoted

 struct strvec {
 	const char **v;
-	int nr;
-	int alloc;
+	size_t nr;
+	size_t alloc;
 };

This is also a good change to take.

Thanks,
-Stolee