On Tue, May 13, 2025 at 09:28:00PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
