Re: [PATCH v2 2/4] loose: lift loose object index parsing logic into helper
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 29, 2026, 08:05 UTC
- 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:
Show 23 quoted lines
> 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.
Show 27 quoted lines
> 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.
Show 18 quoted lines
> + 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.
Show 56 quoted lines
> 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