Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 23, 2026, 12:19 UTC
- Message-ID
- <attQtZ1dVoAnQznP@pks.im>
- In-Reply-To
- <20261022234007.3081825-1-jltobler@gmail.com>
On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:
> When a repository is configured with a compatibility hash, each loose > object write also records a mapping between the object and its > compatibility hash in a "loose-object-idx" file. This index is located > in the same object database the object is written to. If batch fsync of
You probably mean "object directory", not "database"?
Show 7 quoted lines
> loose objects is also enabled, the loose objects and index file are > instead written to a temporary object directory and expected to be > migrated to the primary object directory when the batch transaction > commits. > > While migrating the loose objects works, the "loose-object-idx" file is > unable to be migrated if an index already exists in the primary object
The naming really throws me off, "index" to me is our staging area. What this is is a map. But oh, well, cannot change that now.
Show 14 quoted lines
> directory. This occurs because `finalize_object_file_flags()` expects an > already existing file to have the same contents. Otherwise, the > following error is returned: > > error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents > error: unable to migrate temporary objects > > Furthermore, it is not sufficient to just replace the preexisting index > with the new one as the new index only contains mapping entries for the > newly written objects. > > Update the temporary directory migration logic to handle > "loose-object-idx" files explicitly by appending new mapping entries to > the primary index.
Sounds sensible.
Show 21 quoted lines
> Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- > > Greetings, > > While working on a separate patch series to convert the "files" ODB > transaction to be eagerly created instead of lazily, I encountered this > bug that causes an ODB transaction to fail on commit when a > compatibility hash is also configured. This occurs because the temporary > directory migration code does not know how to handle the loose object > index file. > > This patch addresses updating the migration logic to explicitly merge > mapping entries in the transaction by appending them to the primary > loose object index file. > > Some other approaches I considered: > - Storing the uncommitted object mapping entries in the transaction > without writing a file and only writing on transaction commit. I > chose to keep writing the temp index into the transaction so that > child processes that write to the tempdir could still append entries.
Is that a theoretical scenario or do we actually have cases where a child process wants to write data into the quarantine directory, too?
> - Also writing mapping entires to the main ODB loose object index. > This also didn't seem like a good apprach because it write entires > that may not ultimately get committed.
Agreed, we shouldn't commit this before the actual objects exist. I bet we don't properly handle this during housekeeping either -- or at least I never stumbled over logic that prunes mappings when pruning the corresponding objects. But that's a problem to solve another day.
Show 28 quoted lines
> diff --git a/loose.c b/loose.c
> index d6a2bb5ba3..79d4c256e0 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -265,3 +265,75 @@ 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 strbuf buf = STRBUF_INIT;
> + struct stat st;
> + FILE *fp;
> + int fd = -1;
> + int ret;
> +
> + fp = fopen(src, "r");
> + if (!fp) {
> + ret = error_errno(_("unable to open %s"), src);
> + goto out;
> + }
> + if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header)) {
> + ret = error(_("invalid loose object index %s"), src);
> + goto out;
> + }Curious. Why do we even expect that this file may be broken?
> + if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
> + ret = error_errno(_("unable to lock %s"), dst);
> + goto out;
> + }With a timeout of -1 we'll block indefinitely. That doesn't feel like the best option to me, as the lockfile may be left over from a previous process. And Git locking up indefinitely feels suboptimal to me.
> + fd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);
I don't think it's a good idea to append data under the lock. It may easily cause us to corrupt the file in case the write succeeds partially, only.
Our usual pattern here is to create the lockfile, write all data into the new file and then rename it into place.
I'm also a bit surprised that the file isn't sorted. Is a reader expected to always read _all_ mappings?
Show 23 quoted lines
> + if (fd < 0) {
> + ret = error_errno(_("unable to open %s"), dst);
> + goto out;
> + }
> + if (fstat(fd, &st) < 0) {
> + ret = error_errno(_("unable to stat %s"), dst);
> + goto out;
> + }
> +
> + /* Create the loose-object-idx if one does not already exist. */
> + if (!st.st_size && write_in_full(fd, loose_object_header,
> + strlen(loose_object_header)) < 0) {
> + ret = error_errno(_("failed to write loose object index header"));
> + goto out;
> + }
> +
> + while (!strbuf_getwholeline(&buf, fp, '\n')) {
> + if (write_in_full(fd, buf.buf, buf.len) < 0) {
> + ret = error_errno(_("failed to write loose object index %s"),
> + dst);
> + goto out;
> + }
> + }When merging, wouldn't we have to verify that the object IDs that we're adding don't already exist?
Show 13 quoted lines
> diff --git a/tmp-objdir.c b/tmp-objdir.c > index 3debb60270..5307717354 100644 > --- a/tmp-objdir.c > +++ b/tmp-objdir.c > @@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t, > if (is_loose_object_shard(name)) > flags_copy |= FOF_SKIP_COLLISION_CHECK; > > - ret |= migrate_one(t, src, dst, flags_copy); > + if (is_loose_object_idx(name)) > + ret |= loose_object_idx_merge(t->repo, src->buf, dst->buf); > + else > + ret |= migrate_one(t, src, dst, flags_copy);
Makes sense.
Thanks!
Patrick