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

On Fri, Jul 24, 2026 at 11:41:41AM -0700, Junio C Hamano wrote:
> 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?

Yeah, this is a bit awkward. Rewritten like this:

  - We assume that the primary object database is the "files" object
    database and unconditionally downcast it. This will BUG in case a
    different object database type was used together with a compat hash
    algorithm.

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

No, the downcast will BUG in case it's not the "files" backend.

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

This patch series is rather moving into the direction of making the
object map an internal implementation detail. Ideally, callers shouldn't
even have to be aware that such an object map exists. And by making the
loose object source load it automatically we get closer to that state.

There's only one more caller that calls `repo_read_loose_object_map()`
directly, in "object-file-convert.c", and that caller only calls it to
reload the map in case a concurrent process may have rewritten it. If we
make the backends handle this via `odb_source_prepare(FLUSH_CACHES)`
then we could also get rid of that caller.

Patrick