From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 29 Oct 2026 08:05:56 GMT
Subject: Re: [PATCH v2 4/4] tmp-objdir: merge loose object index when migrating tmpdir
Message-ID: <auL-ZBontaECu8vo@pks.im>
In-Reply-To: <20261029002413.3291184-5-jltobler@gmail.com>

On Wed, Oct 28, 2026 at 07:24:13PM -0500, Justin Tobler wrote:
> diff --git a/loose.c b/loose.c
> index f2e111c25e..36da63684f 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)
>  	free(m);
>  	*map = NULL;
>  }
> +
> +int loose_object_idx_merge(struct repository *repo, const char *src,
> +			   const char *dst)
> +{
> +	struct lock_file lock = LOCK_INIT;
> +	struct loose_object_map *map;
> +	int ret;
> +	int fd;
> +
> +	loose_object_map_init(&map);
> +
> +	fd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,
> +						    LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);
> +	if (fd < 0) {
> +		ret = error_errno(_("unable to lock %s"), dst);
> +		goto out;
> +	}

Okay, so this explains why you didn't want to pass a path to
`loose_object_map_write()`: we need the lock to span across multiple
other operations. Makes sense.

> +	if (loose_object_map_read(repo, dst, map, NULL) < 0) {
> +		ret = error(_("unable to read loose object index %s"), dst);
> +		goto out;
> +	}
> +
> +	if (loose_object_map_read(repo, src, map, NULL) < 0) {
> +		ret = error(_("unable to read loose object index %s"), src);
> +		goto out;
> +	}

It makes sense that we read `dst` under the lock so that we don't risk
losing data that a concurrent process writes into it. But to minimize
the critical section, can't we move `src` out of it and read it before
we lock?

> diff --git a/loose.h b/loose.h
> index ed663ac550..35b46f4be5 100644
> --- a/loose.h
> +++ b/loose.h
> @@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,
>  			      const struct object_id *compat_oid);
>  int repo_read_loose_object_map(struct repository *repo);
>  int repo_write_loose_object_map(struct repository *repo);
> +int loose_object_idx_merge(struct repository *repo, const char *src,
> +			   const char *dst);

Do we maybe want to add a comment to this new function?

Thanks!

Patrick