On Wed, Sep 20, 2017 at 08:49:00PM -0700, Jonathan Nieder wrote: > The patch I am replying to tightens the contract for parse_pathspec(). I disagree with this bit. In my view that was already the contract for parse_pathspec(), and it is simply poorly documented. You can see it being required elsewhere already (e.g., in line_log_init(), and quite probably in other places that feed custom argv-arrays to setup_revisions). > In other words, it was not at all obvious that "(2) The memory remains > useful until around the time of program exit" did not hold. To a > casual reader it instead looks like (2) does hold, and there's no > documentation short of delving into the implementation of > parse_pathspec() to convince a reader otherwise. The documentation > that is present leads to the opposite conclusion. I guess the point I was trying to make is that I _didn't_ have that opposite conclusion. A sane API does not create hidden memory ownership dependencies (though also as I said, it does not hurt to double check if you think it may be wrong, and certainly documenting also does not hurt). > The assertion (1) that this allocation is going to happen multiple > times in a program isn't true either. As you noted, we only read > stdin once. But that doesn't matter as long as (2) doesn't hold. I don't think I asserted that this allocation happens multiple times. I _did_ assert that the revision machinery may be called multiple times, and I'd rather fix the leak than have people figure out the maximum number of bytes we might leak in a given program. In other words, the more important point of what I wrote is that we should not be reaching for UNLEAK() at all in a function like this, even if you might be able to justify it after analysis. > TBH saying that I should write the one-sentence doc patch feels like > a cop-out. Just like it is not sustainable for those reviewers that > are interested in good test coverage to be the only ones who write > tests, I think we cannot avoid treating documentation of API contracts > as a shared responsibility. Hopefully the first paragraph I wrote above explains why I don't see this as changing the contract at all (and therefore making the documentation update orthogonal). I agree that it's a good practice to leave the whole codebase a little prettier than when you started, even if it's not strictly related to the small change you want to make. And I hope my record of patches shows that I follow through on that practice as a general rule. But I also think it's easy for reviewers to ask for those orthogonal changes, when it would be a similar amount of work for them to communicate their suggestion in the form of a patch. -Peff
