Re: [PATCH 08/17] provide a helper to free commit buffer

From: Jeff King <peff@peff.net>
Date: Jun 12, 2014, 20:08 UTC
Message-ID: <20140612200855.GB4468@sigill.intra.peff.net>
In-Reply-To: <xmqqzjhi56oo.fsf@gitster.dls.corp.google.com>

On Thu, Jun 12, 2014 at 11:22:31AM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> > Note that we also need to provide a "detach" mechanism for
> > the weird case in fsck which passes the buffer back to be
> > freed.
> 
> I find that last sentence a bit of white lie ;-).
> 
> The sole caller of "detach" is in index-pack, and discards the
> return value, which is not wrong per-se because it still has the
> pointer to the piece of memory it fed to parse_object_buffer(),
> knows and/or assumes that the return value must be the same as the
> one it already has, and it handles the freeing of that memory
> without relying on the object layer at all.
> 
> But that is an even more weird special case than the white-lie
> version.  As an API, "detach" that returns the buffer to be freed
> looks much less weird than what is really going on in the current
> caller.
> 
> I however have to wonder if
> 
> 	if (detach_commit_buffer(commit) != data)
>         	die("BUG");
> 
> might want to be there.

Yeah, it is a tricky site. It knows that parse_object_buffer may attach the buffer we hand it to "commit->buffer", even though we would prefer to keep the buffer ourselves. So the existing code really just wants to say "erase that attachment". And of course I started with:

  void detach_commit_buffer(struct commit *commit)
  {
	commit->buffer = NULL;
  }

Both before and after it's a bit of a layering violation; we know how the internals of buffer attaching work, and that we can detach to get our original.

I then expanded it to the strbuf-inspired detach you see, even though there are no callers who actually care about the return value. That makes more sense to me as a general API. I don't think it actually makes the layering violation worse (we are still making the exact same assumption), but I think it _seems_ worse, because the API now seems more fully formed. And note that we make the exact same assumptions abotu "struct tree" a few lines above.

The safety check you mention above makes sense to me. There are two things I'd _rather_ do, but they end up more complicated:

  1. It would be nice to ask parse_object_buffer not to attach the
     buffer in the first place; then we could get rid of the detach
     function entirely. But that attachment is necessary for all of the
     fsck sub-functions we call. Factoring those to take a separate
     buffer would be rather disruptive.

  2. Instead of confirming that detach returns the same buffer, just
     assume our buffer was eaten (as promised by set_commit_buffer),
     and continue on with whatever detach_commit_buffer returns.
     But it is not _our_ buffer in the first place. It is passed in by
     the caller, so this replacement would have to bubble back up to the
     original allocator.

So just putting in the safety check is probably the least-disruptive thing. It doesn't automatically adapt to a change in the underlying commit_buffer code, but it would at least notice it.

-Peff