From: Jeff King <peff@peff.net>
Date: Mon, 12 Jun 2017 21:13:48 GMT
Subject: Re: [RFC PATCH 2/4] sha1_file: extract type and size from object_info
Message-ID: <20170612211348.xqawraeqacgw6iyx@sigill.intra.peff.net>
In-Reply-To: <20170612125254.33ba633a@twelve2.svl.corp.google.com>

On Mon, Jun 12, 2017 at 12:52:54PM -0700, Jonathan Tan wrote:

> > I like the idea of consolidating the logic. But I can't help but feel
> > that pulling these fields out of object_info is a step backwards. The
> > point of that struct is to let the caller specify which aspects of the
> > object they're interested in
> 
> My issue was that there are some parts that cannot be turned off (in
> particular, the object_info.u.packed part). Having said that, reading
> the packed object itself should give us enough information to populate
> that, so I'll take a look and see if this is possible.

I think in general that the parts of object_info which aren't optional
should be largely "free" to set (or at least O(1)).

> > Another approach to this whole mess is to have a single function for
> > acquiring a "handle" to an object, along with functions to query aspects
> [...]
> 
> There are a few safeguards that, I think, only work with the current
> get-everything-then-forget-about-it approach (the packed-loose-packed
> retry mechanism, and the desperate retry-if-corrupt-packed-object one).
> If we have a handle with a cache, then, for example, we would lose the
> ability to retry packed after loose if the handle has already declared
> that the object is loose.

Yes, the handle would have to make some guarantee that it could access
the object. Which would generally involve holding open a descriptor or
mmap. That would probably take some surgery to make it work with the way
pack mmap windows work.

So the whole "handle" thing is how it probably _ought_ to work, but I
agree we may be too far down the other path to make it worth switching.

-Peff