On Sun, Aug 25, 2019 at 04:20:31PM +0900, Mike Hommey wrote: > > I think this is stronger than just "most of the time". It's an invariant > > for strbufs to have a NUL, so the only case where detaching isn't a noop > > is the empty slopbuf case you mention. > > If it's an invariant, why does detach actively tries to realloc and set > the nul terminator as if it can happen in more cases than when using the > slopbuf? It calls strbuf_grow() to handle the slopbuf case (we can't hand off the slopbuf, since the caller expects an allocated buffer). It just doesn't bother to distinguish that case itself, and lets strbuf_grow() handle it. I think it would be equally correct for strbuf_detach() to do: if (!sb->alloc) strbuf_grow(0); > > Splitting hairs, perhaps, but I think with that explanation, we could > > probably argue that this case will never come up: strbuf_getline will > > either have allocated a buffer or will have returned EOF. > > Note that the slopbuf case _does_ come up, and we always leak a 1 byte > buffer. Hmm, I suppose so, on the very first call before we've read anything (and likewise if parse_data() reset it then got an EOF, and we then tried to read another command). > I do agree the way fast-import works between cmd_hist and command_buf is > very brittle, as you've shown. I didn't feel like digging into it > though. Thanks for having gone further than I did. I'll see if I can shape my rambling into a patch. -Peff
