Jeff King <peff@xxxxxxxx> 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. 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. But with that "bias", many of the recent patches from you look more like pointless churn, mixed with fixes to theoretical problems, than clean-ups with real benefits. What makes it worse is that there are occasional real gems among these "meh" patches, which means I have to read all of them anyway to sift wheat from chaff X-<.
