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

Patrick Steinhardt <[email protected]> writes:

> 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.

I am not sure if I understand this sentence, especially "we can
extract its loose backend" part.  Do you mean 'extract the object
map from the loose backend'?  Or something else?

>   - 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.
>
> Refactor the logic so that we instead load the loose object map via the
> "loose" backend, which fixes both of the above issues.

It does make sense to have loose_object_map_load() that is very much
specific to the loose object odb source to odb_source_loose_new().
That way set_compat_hash_algo() does not have to assume that files
backend is used as the object store.

> @@ -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;

If this particular source in the list of sources is not backed by
the files backend, would downcast signal the fact (e.g., by
returning NULL) so that we can skip the next call instead?

Or would the next step in refactoring be to define "load object map"
method that is generic to odb_source so that this part does not have
to do any of these and instead simply do

	for (source = ...) {
		if (odb_source_object_map_load(source))
                	return -1;
        }

or something?