From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 29 Oct 2026 08:05:47 GMT
Subject: Re: [PATCH v2 2/4] loose: lift loose object index parsing logic into helper
Message-ID: <auL-W9Z2H9Ivn1RC@pks.im>
In-Reply-To: <20261029002413.3291184-3-jltobler@gmail.com>

On Wed, Oct 28, 2026 at 07:24:11PM -0500, Justin Tobler wrote:
> diff --git a/loose.c b/loose.c
> index c6f4ee0ec6..5f72c644f9 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const
>  	return 1;
>  }
>  
> -static int insert_loose_map(struct odb_source_loose *loose,
> +static int insert_loose_map(struct loose_object_map *map,
> +			    struct oidtree *cache,
>  			    const struct object_id *oid,
>  			    const struct object_id *compat_oid)
>  {
> -	struct loose_object_map *map = loose->map;
>  	int inserted = 0;
>  
>  	inserted |= insert_oid_pair(map->to_compat, oid, compat_oid);
>  	inserted |= insert_oid_pair(map->to_storage, compat_oid, oid);
> -	if (inserted)
> -		oidtree_insert(loose->cache, compat_oid, NULL);
> +	if (inserted && cache)
> +		oidtree_insert(cache, compat_oid, NULL);

Okay, so the `cache` is optional now. You mention that fact in the
commit message, but you don't explain why that is. I assume a subsequent
commit will make use of this.

>  	return inserted;
>  }
>  
> -int loose_object_map_load(struct odb_source_loose *loose)
> +static int loose_object_map_read(struct repository *repo, const char *path,
> +				 struct loose_object_map *map,
> +				 struct oidtree *cache)
>  {
> -	struct repository *repo = loose->base.odb->repo;
>  	struct strbuf buf = STRBUF_INIT;
> -	char *path;
>  	FILE *fp;
> -	int ret = -1;
> -
> -	if (!should_use_loose_object_map(repo))
> -		return 0;
> -
> -	if (!loose->map)
> -		loose_object_map_init(&loose->map);
> -	if (!loose->cache) {
> -		ALLOC_ARRAY(loose->cache, 1);
> -		oidtree_init(loose->cache);
> -	}
> -
> -	insert_loose_map(loose, repo->hash_algo->empty_tree, repo->compat_hash_algo->empty_tree);
> -	insert_loose_map(loose, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);
> -	insert_loose_map(loose, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);

Okay, all of this stays in `loose_object_map_load()` further down.

> +	int ret;
>  
> -	path = xstrfmt("%s/loose-object-idx", loose->base.path);
>  	fp = fopen(path, "rb");
>  	if (!fp) {
> -		free(path);
> -		return 0;
> +		if (errno == ENOENT)
> +			return 0;
> +		return -1;
>  	}
>  
> -	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
> -		goto err;
> +	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header)) {
> +		ret = -1;
> +		goto out;
> +	}

Ideally we'd of course print an error message telling the user what's
happened. But we didn't before either, so this is fine as-is.

>  	while (!strbuf_getline_lf(&buf, fp)) {
>  		const char *p;
>  		struct object_id oid, compat_oid;
>  		if (parse_oid_hex_algop(buf.buf, &oid, &p, repo->hash_algo) ||
>  		    *p++ != ' ' ||
>  		    parse_oid_hex_algop(p, &compat_oid, &p, repo->compat_hash_algo) ||
> -		    p != buf.buf + buf.len)
> -			goto err;
> -		insert_loose_map(loose, &oid, &compat_oid);
> +		    p != buf.buf + buf.len) {
> +			ret = -1;
> +			goto out;
> +		}
> +		insert_loose_map(map, cache, &oid, &compat_oid);
>  	}
>  
>  	ret = ferror(fp) ? -1 : 0;
> -err:
> -	fclose(fp);
> +
> +out:
>  	strbuf_release(&buf);
> +	fclose(fp);
> +
> +	return ret;
> +}
> +
> +int loose_object_map_load(struct odb_source_loose *loose)
> +{
> +	struct repository *repo = loose->base.odb->repo;
> +	char *path;
> +	int ret;
> +
> +	if (!should_use_loose_object_map(repo))
> +		return 0;
> +
> +	if (!loose->map)
> +		loose_object_map_init(&loose->map);
> +	if (!loose->cache) {
> +		ALLOC_ARRAY(loose->cache, 1);
> +		oidtree_init(loose->cache);
> +	}
> +
> +	insert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_tree,
> +			 repo->compat_hash_algo->empty_tree);
> +	insert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_blob,
> +			 repo->compat_hash_algo->empty_blob);
> +	insert_loose_map(loose->map, loose->cache, repo->hash_algo->null_oid,
> +			 repo->compat_hash_algo->null_oid);
> +
> +	path = xstrfmt("%s/loose-object-idx", loose->base.path);
> +	ret = loose_object_map_read(repo, path, loose->map, loose->cache);
> +
>  	free(path);
>  	return ret;
>  }

Right, and this here is the original function that we had with the
remaining bits that are specific to the source.

Patrick