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

From: Jeff King <peff@peff.net>
Date: Dec 5, 2018, 06:51 UTC
Message-ID: <20181205065136.GA27263@sigill.intra.peff.net>
In-Reply-To: <54e7a97d-501c-1aa7-55cd-83f070d05a8c@web.de>

On Wed, Dec 05, 2018 at 07:02:17AM +0100, René Scharfe wrote:

Show 6 quoted lines

> > I actually wrote it that way initially, but doing the hashcpy() in the
> > caller is a bit more awkward. My thought was to punt on that until the
> > rest of the surrounding code starts handling oids.
> 
> The level of awkwardness is the same to me, but sha1_loose_object_info()
> is much longer already, so it might feel worse to add it there.

Right, what I meant was that in sha1_loose_object_info():

  if (special_case)
	handle_special_case();

is easier to follow than a block setting up the special case and then calling the function.

> This
> function is easily converted to struct object_id, though, as its single
> caller can pass one on -- this makes the copy unnecessary.

If you mean modifying sha1_loose_object_info() to take an oid, then sure, I agree that is a good step forward (and that is exactly the "punt until" moment I meant).

Show 11 quoted lines

> > This patch looks sane. How do you want to handle it? I'd assumed your
> > earlier one would be for applying on top, but this one doesn't have a
> > commit message. Did you want me to squash down the individual hunks?
> 
> I'm waiting for the first one (object-store: use one oid_array per
> subdirectory for loose cache) to be accepted, as it aims to solve a
> user-visible performance regression, i.e. that's where the meat is.
> I'm particularly interested in performance numbers from Ævar for it.
> 
> I can send the last one properly later, and add patches for converting
> quick_has_loose() to struct object_id.  Those are just cosmetic..

Great, thanks. I just wanted to make sure these improvements weren't going to slip through the cracks.

-Peff