Re: [PATCH 8/9] sha1-file: use loose object cache for quick existence check

From: Jeff King <peff@peff.net>
Date: Dec 5, 2018, 20:17 UTC
Message-ID: <20181205201728.GA19936@sigill.intra.peff.net>
In-Reply-To: <48ab861b-46c8-2bd5-0508-fa106efd8e56@web.de>

On Wed, Dec 05, 2018 at 07:41:44PM +0100, René Scharfe wrote:

Show 6 quoted lines

> > If we start to convert those, there's a
> > little bit of a rabbit hole, but it's actually not too bad.
> 
> You don't need to crawl in just for quick_has_loose(), but eventually
> everything has to be converted.  It seems a bit much for one patch, but
> perhaps that's just my ever-decreasing attention span speaking.

Yeah, my normal process here is to dig to the bottom of the rabbit hole, and then break it into actual patches. I just shared the middle state here. ;)

I suspect the http bits could be split off into their own thing. The bits in sha1-file.c I'd plan to mostly do all together, as they are a family of related functions.

Mostly I wasn't sure how to wrap this up with the other changes. It's obviously pretty invasive, and I don't want it to get in the way of actual functional changes we've been discussing.

Show 11 quoted lines

> > @@ -879,15 +879,15 @@ int git_open_cloexec(const char *name, int flags)
> >   * Note that it may point to static storage and is only valid until another
> >   * call to stat_sha1_file().
> >   */
> > -static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
> > -			  struct stat *st, const char **path)
> > +static int stat_loose_object(struct repository *r, const struct object_id *oid,
> > +			     struct stat *st, const char **path)
> 
> Hmm, read_sha1_file() was renamed to read_object_file() earlier this
> year, and I'd have expected this to become stat_object_file().  Names..

read_object_file() is about reading an object from _any_ source. These are specifically about loose objects, and I think that distinction is important (both here and for map_loose_object, etc).

I'd actually argue that read_object_file() should just be read_object(), but that already exists. Sigh. (I think it's fixable, but obviously orthogonal to this topic).

> Anyway, the comment above and one a few lines below should be updated
> as well.

Thanks, fixed.

Show 5 quoted lines

> >  static int open_sha1_file(struct repository *r,
> > -			  const unsigned char *sha1, const char **path)
> > +			  const struct object_id *oid, const char **path)
> 
> That function should lose the "sha1" in its name as well.

Yep, fixed.

Show 6 quoted lines

> > -static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
> > +static void *unpack_loose_rest(git_zstream *stream,
> > +			       void *buffer, unsigned long size,
> > +			       const struct object_id *oid)
> 
> Hmm, both old and new name here look weird to me at this point.

It makes more sense in the pairing of unpack_sha1_header() and unpack_sha1_rest(). Maybe "body" would be better than "rest".

At any rate, it probably makes sense to rename them together (but I didn't touch the "header" one here). Maybe the name changes should come as a separate patch. I was mostly changing them here because I was changing the signatures anyway, and had to touch all of the callers.

-Peff