Re: [PATCH] Make object contents optionally available

From: Daniel Barkalow <barkalow@iabervon.org>
Date: May 17, 2005, 17:55 UTC
Message-ID: <Pine.LNX.4.21.0505171325150.30848-100000@iabervon.org>
In-Reply-To: <7vfywlhj3h.fsf@assigned-by-dhcp.cox.net>

On Tue, 17 May 2005, Junio C Hamano wrote:

Show 10 quoted lines

> >>>>> "DB" == Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> DB> I'm already going to add a per-type global to have the parse functions
> DB> also unpack the object contents user-visibly, for the case that Junio
> DB> pointed out. (Making it: parse_* doesn't change whether the contents are
> DB> unpacked, unless you tell it to unpack objects.)
> 
> That sounds better than the patch you sent last night, but are
> we sure that callers would be satisfied if you just make some
> _types_ more special than others?

It wouldn't make any types special; the caller can just control each type individually, so that code that only cares about commits could get commits unpacked, but not get trees unpacked even if it has them parsed.

> Parse functions need to unpack anyway, so it might make more
> sense to add an optional callback just after where unpacking
> happens to ask the main program if it wants the unpacked raw
> data to be kept.

I think that a callback is a bit excessive, and it wouldn't get the useful information anyway, which is really: "what is supposed to happen to this object, which you haven't seen before, after it's parsed?"

> So you would do something like:
>     struct object *parse_object(unsigned char *sha1)
>     {

That reminds me that I should also fix the parse_object path, which can now be simplified substantially.

Note that parse_object calls parse_<type>, rather than the reverse, because parse_<type> can skip the step of figuring out what routine to use. The parse_<type> version also has lookup_<type>, which can return the struct which will be filled out later; this isn't possible for an object of unknown type, since it can't tell what struct type to allocate.

> Another possibility is not to make double unpacking too costly
> by having an LRU of unpack_sha1_file(), but I am not sure how
> effective that would be.

It would work for pop_most_recent_commit, which parses the thing it's about to return, but I don't think it's a particularly good solution in general.

	-Daniel
*This .sig left intentionally blank*