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

[Toon Claes <toon@iotcl.com>]

Justin Tobler <jltobler@gmail.com> writes:

> 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 <ps@pks.im>
>> ---
>>  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?

I've been wondering about this as well. The reason for this change is to
also have this guard when odb_source_loose_new(), in source-loose.c (see
further down in the patch), calls this function too.

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

Here.

-- 
Cheers,
Toon