Re: [PATCH] fast-import: Reinitialize command_buf rather than detach it.

From: Jeff King <peff@peff.net>
Date: Aug 25, 2019, 07:28 UTC
Message-ID: <20190825072809.GD23806@sigill.intra.peff.net>
In-Reply-To: <20190825072031.2m2go6ssshww6tup@glandium.org>

On Sun, Aug 25, 2019 at 04:20:31PM +0900, Mike Hommey wrote:

Show 7 quoted lines

> > 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);

Show 6 quoted lines

> > 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