On Tue, Dec 04, 2018 at 10:45:13PM +0100, René Scharfe wrote:
> > The comment in the context there is warning callers to remember to load
> > the cache first. Now that we have individual caches, might it make sense
> > to change the interface a bit, and make these members private. I.e.,
> > something like:
> >
> > struct oid_array *odb_loose_cache(struct object_directory *odb,
> > int subdir_nr)
> > {
> > if (!loose_objects_subdir_seen[subdir_nr])
> > odb_load_loose_cache(odb, subdir_nr); /* or just inline it here */
> >
> > return &odb->loose_objects_cache[subdir_nr];
> > }
>
> Sure. And it should take an object_id pointer instead of a subdir_nr --
> less duplication, nicer interface (patch below).
I had considered that initially, but it's a little less flexible if a
caller doesn't actually have an oid. Though both of the proposed callers
do, so it's probably OK to worry about that if and when it ever comes up
(the most plausible thing in my mind is doing some abbrev-like analysis
without looking to abbreviate a _particular_ oid).
> It would be nice if it could return a const pointer to discourage
> messing up the cache, but that's not compatible with oid_array_lookup().
Yeah.
> And quick_has_loose() should be converted to object_id as well -- adding
> a function that takes a SHA-1 is a regression. :D
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.
> ---
> object-store.h | 8 ++++----
> sha1-file.c | 19 ++++++++-----------
> sha1-name.c | 4 +---
> 3 files changed, 13 insertions(+), 18 deletions(-)
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?
-Peff
