On Wed, Dec 05, 2018 at 07:02:17AM +0100, René Scharfe wrote:
> > 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).
> > 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
