Patrick Steinhardt <ps@xxxxxx> writes:
> The `object_directory` structure is used as an access point for a single
> object directory like ".git/objects". While the structure isn't yet
> fully self-contained, the intent is for it to eventually contain all
> information required to access objects in one specific location.
>
> While the name "object directory" is a good fit for now, this will
> change over time as we continue with the agenda to make pluggable object
> databases a thing. Eventually, objects may not be accessed via any kind
> of directory at all anymore, but they could instead be backed by any
> kind of durable storage mechanism. While it seems quite far-fetched for
> now, it is thinkable that eventually this might even be some form of a
> database, for example.
>
> As such, the current name of this structure will become worse over time
> as we evolve into the direction of pluggable ODBs. Immediate next steps
> will start to carve out proper self-contained object directories, which
> requires us to pass in these object directories as parameters. Based on
> our modern naming schema this means that those functions should then be
> named after their subsystem, which means that we would start to bake the
> current name into the codebase more and more.
>
> Let's preempt this by renaming the structure to `odb_alternate` now
> already. This name is agnostic of how exactly objects are stored while
> still specifically pinpointing that this is about an alternate object
> database. In the future, it allows us to easily introduce e.g. a
> `odb_files_alternate` and other specific implementations over time.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/commit-graph.c | 18 +++----
> builtin/count-objects.c | 4 +-
> builtin/fetch.c | 2 +-
> builtin/fsck.c | 14 ++---
> builtin/gc.c | 14 ++---
> builtin/grep.c | 2 +-
> builtin/multi-pack-index.c | 4 +-
> builtin/submodule--helper.c | 6 +--
> bundle.c | 2 +-
> commit-graph.c | 94 +++++++++++++++++-----------------
> commit-graph.h | 14 ++---
> diagnose.c | 8 +--
> http-walker.c | 2 +-
> http.c | 4 +-
> loose.c | 42 +++++++--------
> midx.c | 6 +--
> object-file.c | 80 ++++++++++++++---------------
> object-file.h | 8 +--
> object-name.c | 6 +--
> object-store.c | 122 ++++++++++++++++++++++----------------------
> object-store.h | 25 +++++----
> packfile.c | 16 +++---
> path.c | 2 +-
> refs.c | 2 +-
> repository.c | 14 ++---
> submodule-config.c | 2 +-
> t/helper/test-read-graph.c | 6 +--
> tmp-objdir.c | 24 ++++-----
> 28 files changed, 273 insertions(+), 270 deletions(-)
[snip]
> 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
> @@ -1582,7 +1582,7 @@ static const char alternate_error_advice[] = N_(
> );
>
> static int add_possible_reference_from_superproject(
> - struct object_directory *odb, void *sas_cb)
> + struct odb_alternate *alt_odb, void *sas_cb)
> {
> struct submodule_alternate_setup *sas = sas_cb;
> size_t len;
> @@ -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
> * standard layout with .git/(modules/<name>)+/objects
> */
> - if (strip_suffix(odb->path, "/objects", &len)) {
> + if (strip_suffix(alt_odb->path, "/objects", &len)) {
> struct repository alternate;
> char *sm_alternate;
> struct strbuf sb = STRBUF_INIT;
> struct strbuf err = STRBUF_INIT;
> - strbuf_add(&sb, odb->path, len);
> + strbuf_add(&sb, alt_odb->path, len);
>
> if (repo_init(&alternate, sb.buf, NULL) < 0)
> die(_("could not get a repository handle for gitdir '%s'"),
[snip]
> 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?
> -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?
> -const char *odb_loose_path(struct object_directory *odb,
> +const char *odb_loose_path(struct odb_alternate *alternate,
> struct strbuf *buf,
> const struct object_id *oid);
>
> diff --git a/object-name.c b/object-name.c
> index 9288b2dd245..b83ba882b9e 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -112,10 +112,10 @@ static enum cb_next match_prefix(const struct object_id *oid, void *arg)
>
> static void find_short_object_filename(struct disambiguate_state *ds)
> {
> - struct object_directory *odb;
> + struct odb_alternate *alternate;
>
> - for (odb = ds->repo->objects->odb; odb && !ds->ambiguous; odb = odb->next)
> - oidtree_each(odb_loose_cache(odb, &ds->bin_pfx),
> + for (alternate = ds->repo->objects->alternates; alternate && !ds->ambiguous; alternate = alternate->next)
> + oidtree_each(odb_loose_cache(alternate, &ds->bin_pfx),
> &ds->bin_pfx, ds->len, match_prefix, ds);
> }
[snip]
> 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.
> + */
> +struct odb_alternate {
> + struct odb_alternate *next;
>
> /*
> * Used to store the results of readdir(3) calls when we are OK
> @@ -52,8 +55,8 @@ struct object_directory {
> void prepare_alt_odb(struct repository *r);
> int has_alt_odb(struct repository *r);
> char *compute_alternate_path(const char *path, struct strbuf *err);
> -struct object_directory *find_odb(struct repository *r, const char *obj_dir);
> -typedef int alt_odb_fn(struct object_directory *, void *);
> +struct odb_alternate *find_odb(struct repository *r, const char *obj_dir);
> +typedef int alt_odb_fn(struct odb_alternate *, void *);
> int foreach_alt_odb(alt_odb_fn, void*);
> typedef void alternate_ref_fn(const struct object_id *oid, void *);
> void for_each_alternate_ref(alternate_ref_fn, void *);
> @@ -75,12 +78,12 @@ void add_to_alternates_memory(const char *dir);
> * Replace the current writable object directory with the specified temporary
> * object directory; returns the former primary object directory.
> */
> -struct object_directory *set_temporary_primary_odb(const char *dir, int will_destroy);
> +struct odb_alternate *set_temporary_primary_odb(const char *dir, int will_destroy);
>
> /*
> * Restore a previous ODB replaced by set_temporary_main_odb.
> */
> -void restore_primary_odb(struct object_directory *restore_odb, const char *old_path);
> +void restore_primary_odb(struct odb_alternate *restore_alternate, const char *old_path);
>
> struct packed_git;
> struct multi_pack_index;
> @@ -88,7 +91,7 @@ struct cached_object_entry;
>
> /*
> * The object database encapsulates access to objects in a repository. It
> - * manages one or more backends that store the actual objects which are
> + * manages one or more alternates that store the actual objects which are
> * configured via alternates.
> */
> struct object_database {
> @@ -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.
*/
> - struct object_directory *odb;
> - struct object_directory **odb_tail;
> - struct kh_odb_path_map *odb_by_path;
> + struct odb_alternate *alternates;
> + struct odb_alternate **alternates_tail;
> + struct kh_odb_path_map *alternate_by_path;
>
> int loaded_alternates;
>
> /*
> * A list of alternate object directories loaded from the environment;
> * this should not generally need to be accessed directly, but will
> - * populate the "odb" list when prepare_alt_odb() is run.
> + * populate the "alternates" list when prepare_alt_odb() is run.
> */
> char *alternate_db;
>
[snip]
Huge patch, I scrolled through the changes, but focused on some naming
and comments.
--
Toon
