From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 14 May 2025 04:31:48 GMT
Subject: Re: [PATCH v2 02/17] object-store: rename `object_directory` to `odb_alternate`
Message-ID: <aCQctNv5IkVJAfQL@pks.im>
In-Reply-To: <87bjrwjoov.fsf@iotcl.com>

On Tue, May 13, 2025 at 09:28:00PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 53da2116ddf..cd7db11d825 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1591,12 +1591,12 @@ static int add_possible_reference_from_superproject(
> >  	 * If the alternate object store is another repository, try the
> 
> This comment reads weirdly if an "alternate" is a location where objects
> are stored. Maybe say something like:
> 
>     * It the alternate resides in another repository, try the

Fair, but I'd prefer to not adapt too many of the surroundings yet.
Otherwise I fear that it'll open a can of worms that we can otherwise
address over time.

> > diff --git a/object-file.h b/object-file.h
> > index a85b2e5b494..f1601200938 100644
> > --- a/object-file.h
> > +++ b/object-file.h
> > @@ -24,23 +24,23 @@ enum {
> >  int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags);
> >  int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags);
> >  
> > -struct object_directory;
> > +struct odb_alternate;
> >  
> >  /*
> >   * Populate and return the loose object cache array corresponding to the
> >   * given object ID.
> >   */
> > -struct oidtree *odb_loose_cache(struct object_directory *odb,
> > +struct oidtree *odb_loose_cache(struct odb_alternate *alternate,
> >  				const struct object_id *oid);
> >  
> >  /* Empty the loose object cache for the specified object directory. */
> 
> Also here s/directory/alternate/ ? Or database?

This one remains valid even with the new terminology. The loose cache is
specific to loose object files, which does have an associated object
directory. Refactoring loose objects to become a bit more self-contained
will be one of the next steps after these refactoring land.

> > -void odb_clear_loose_cache(struct object_directory *odb);
> > +void odb_clear_loose_cache(struct odb_alternate *alternate);
> >  
> >  /*
> >   * Put in `buf` the name of the file in the local object database that
> >   * would be used to store a loose object with the specified oid.
> >   */
> 
> Not sure if we should use "alternate" here?

Probably rather "loose object directory", as it is again specific to
loose objects.

> > diff --git a/object-store.h b/object-store.h
> > index 34b8efbbb83..2dda6e85388 100644
> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -12,8 +12,11 @@ struct oidtree;
> >  struct strbuf;
> >  struct repository;
> >  
> > -struct object_directory {
> > -	struct object_directory *next;
> > +/*
> > + * An alternate part of an object database that stores the actual objects.
> 
> I'm having a hard time trying to understand what you're saying here.
> What do you think about:
> 
>     * The alternate is the part of the object database that stores the actual objects.

That reads better. I'll extend this even further to give a bit more
context around primary alternates.

> > @@ -97,16 +100,16 @@ struct object_database {
> >  	 * cannot be NULL after initialization). Subsequent directories are
> >  	 * alternates.
> >  	 */
> 
> The full original comment can use some love too:
> 
>     *
>     * Set of all object directories; the main directory is first (and
>     * cannot be NULL after initialization). Subsequent directories are
>     * alternates.
>     */
> 
> How about:
> 
>     *
>     * Set of all alternates, storing the actual objects.
>     * The primary alternate is first (and cannot be NULL after
>     * & initialization). More alternates are optional.
>     */

Same here, I'd rather not touch this comment for now. We'd only catch a
small subset of outdated comments anyway, so I'd rather focus on the
required changes given that this patch is already quite large.

Patrick