Re: [PATCH 2/5] setup: detangle loading of loose object maps

On 26/07/24 05:48AM, Patrick Steinhardt wrote:
> When a repository is configured to use a compatibility hash function
> then we load the loose object map when we initialize the repository.
> This object map provides the mappings between the canonical object hash
> and the compatibility object hash.
> 
> Loading the object map happens in `repo_set_compat_hash_algo()`, which
> calls `repo_read_loose_object_map()` in case the compatibility object
> hash is non-zero. This setup sequence has two major downsides:
> 
>   - We assume that the primary object database is the "files" object
>     database so that we can extract its "loose" backend. This stops
>     working with pluggable object databases.

So IIUC, does this mean that `repo_set_compat_hash_algo()` is directly
reaching into the loose object source to load the compatibility object
map? I suppose it should be the responsibility of the respective ODB
backend to handle object compatibility.

>   - We require the object database to already have been initialized when
>     configuring the object database. This means that we must intermix
>     configuration of the repository and initialization of its
>     sub-structures in a weird way.

If there any reason we need to eagerly load compatibility object
mappings?

> Refactor the logic so that we instead load the loose object map via the
> "loose" backend, which fixes both of the above issues.

Sounds reasonable.

> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  loose.c            | 11 +++++------
>  loose.h            |  1 +
>  odb/source-loose.c |  2 ++
>  repository.c       |  2 --
>  setup.c            |  5 +++--
>  5 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/loose.c b/loose.c
> index 9dad75373b..a3b2dcedc2 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -61,7 +61,7 @@ static int insert_loose_map(struct odb_source_loose *loose,
>  	return inserted;
>  }
>  
> -static int load_one_loose_object_map(struct odb_source_loose *loose)
> +int loose_object_map_load(struct odb_source_loose *loose)
>  {
>  	struct repository *repo = loose->base.odb->repo;
>  	struct strbuf buf = STRBUF_INIT;
> @@ -69,6 +69,9 @@ static int load_one_loose_object_map(struct odb_source_loose *loose)
>  	FILE *fp;
>  	int ret = -1;
>  
> +	if (!should_use_loose_object_map(repo))
> +		return 0;

Previously the above condition has asserted in
`repo_read_loose_object_map()` which calls `loose_object_map_load()` for
each source. Do we expect each source to potentially answer differently
though?

> +
>  	if (!loose->map)
>  		loose_object_map_init(&loose->map);
>  	if (!loose->cache) {
> @@ -112,14 +115,10 @@ int repo_read_loose_object_map(struct repository *repo)
>  {
>  	struct odb_source *source;
>  
> -	if (!should_use_loose_object_map(repo))
> -		return 0;
> -
>  	odb_prepare_alternates(repo->objects);
> -
>  	for (source = repo->objects->sources; source; source = source->next) {
>  		struct odb_source_files *files = odb_source_files_downcast(source);
> -		if (load_one_loose_object_map(files->loose) < 0)
> +		if (loose_object_map_load(files->loose) < 0)
>  			return -1;
>  	}
>  
> diff --git a/loose.h b/loose.h
> index 6c9b3f4571..ed663ac550 100644
> --- a/loose.h
> +++ b/loose.h
> @@ -13,6 +13,7 @@ struct loose_object_map {
>  
>  void loose_object_map_init(struct loose_object_map **map);
>  void loose_object_map_clear(struct loose_object_map **map);
> +int loose_object_map_load(struct odb_source_loose *loose);
>  int repo_loose_object_map_oid(struct repository *repo,
>  			      const struct object_id *src,
>  			      const struct git_hash_algo *dest_algo,
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index 3f7d04a56e..812ca1c138 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -727,5 +727,7 @@ struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
>  	if (!is_absolute_path(loose->base.path))
>  		chdir_notify_register(NULL, odb_source_loose_reparent, loose);
>  
> +	loose_object_map_load(loose);

Now we load the loose object map for the specific source when its
created.

> +
>  	return loose;
>  }
> diff --git a/repository.c b/repository.c
> index 2ef0778846..6d633002b4 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -201,8 +201,6 @@ void repo_set_compat_hash_algo(struct repository *repo MAYBE_UNUSED, uint32_t al
>  	if (hash_algo_by_ptr(repo->hash_algo) == algo)
>  		BUG("hash_algo and compat_hash_algo match");
>  	repo->compat_hash_algo = algo ? &hash_algos[algo] : NULL;
> -	if (repo->compat_hash_algo)
> -		repo_read_loose_object_map(repo);

The loose object map is no longer read eagerly.

>  #else
>  	if (algo)
>  		die(_("compatibility hash algorithm support requires Rust"));
> diff --git a/setup.c b/setup.c
> index d31808130b..825572f5f1 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1788,8 +1788,6 @@ int apply_repository_format(struct repository *repo,
>  
>  	repo->bare_cfg = format->is_bare;
>  	repo_set_hash_algo(repo, format->hash_algo);
> -	repo->objects = odb_new(repo, object_directory,
> -				alternate_object_directories);
>  	repo_set_compat_hash_algo(repo, format->compat_hash_algo);
>  	repo_set_ref_storage_format(repo,
>  				    format->ref_storage_format,
> @@ -1805,6 +1803,9 @@ int apply_repository_format(struct repository *repo,
>  	repo->repository_format_precious_objects =
>  		format->precious_objects;
>  
> +	repo->objects = odb_new(repo, object_directory,
> +				alternate_object_directories);

We now defer creating the ODB until after the compat hash is configured.
Makes sense.

-Justin