Re: [PATCH v2 0/7] strvec: use size_t to store nr and alloc

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 13, 2021, 12:29 UTC
Message-ID: <87mtog4pai.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqlf41ghmk.fsf@gitster.g>

On Sun, Sep 12 2021, Junio C Hamano wrote:

Show 32 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> I'm a little "meh" on some of these, for a few reasons:
>>
>>  - anything calling into setup_revisions() eventually is just kicking
>>    the can anyway. And these are generally not buggy in the first place,
>>    since they're bounded argv creations.
>>
>>  - passing a strvec instead of the broken-down pair is a less flexible
>>    interface. It's one thing if the callee benefits from seeing the
>>    strvec (say, because they may push more items onto it). But I think
>>    with strbufs, we have a general guideline that if a function _can_
>>    take the bare pointer, then it should. (Sorry, I don't have a
>>    succinct reference to CodingGuidelines or anything like that; I feel
>>    like this is wisdom we came up with on the list in the early days of
>>    strbufs).
>>
>>  - if we are going to pass a strvec, it should almost certainly be
>>    const, to make it clear how we intend to use it.
>
> All true.
>
>> These cases are largely stupid things that real people would never come
>> across. The real goal is making sure we don't get hit with a memory
>> safety bug (under-allocation, converting a big size_t to a negative int,
>> etc).
>
> Yes.  
>
> Ævar, I do not mean any disrespect to you, but I have to say that
> topics like this one are starting to wear my concentration and
> patience down really thin and making me really slow down.

I'm sorry. I'll try to lay off on the patch firehose until the delta I've got in master..seen is way down from what it is now.

Show 5 quoted lines

> Perhaps I am biased by not yet having seen what you eventually want
> to build on top, and because of that I do not understood why and how
> these "clean ups" are so valuable to have right now (as opposed to
> just letting the sleeping dog lie), which you would of course have a
> much better chance to know than I do.

I could go into that, but it's probably best to leave it at: Yeah for these seemingly going nowhere small changes I'm generally taking them somewhere interesting.

But figuring out how to split that is still a hard problem. E.g. I've had one series (the fsck error message improvements) that's been stalled for ~3 months due to size/including the "interesting" part, but recently relatively small increments of prep code changes seem to get a lot of review traction.

As for this strvec.h s/int/size_t/ topic. I'm not taking that anywhere, Jeff suggested it and came up with the patch, I figured more helpful than "if we change s/int/size_t/g for x, shouldn't we change that for y which whe assign x to?" would be patches I had to do that, which I'd come up with after Jeff suggested this direction in response to another topic.