On Thu, Jun 12, 2014 at 11:22:31AM -0700, Junio C Hamano wrote:
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html