[PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Justin Tobler]

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

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

Thanks for the review,
-Justin

---
 loose.c                       | 72 +++++++++++++++++++++++++++++++++++
 loose.h                       |  2 +
 t/t1016-compatObjectFormat.sh | 27 +++++++++++++
 tmp-objdir.c                  | 11 +++++-
 4 files changed, 111 insertions(+), 1 deletion(-)

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;
+	}
+
+	if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
+		ret = error_errno(_("unable to lock %s"), dst);
+		goto out;
+	}
+
+	fd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);
+	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;
+		}
+	}
+
+	ret = close(fd);
+	fd = -1;
+	if (ret) {
+		ret = error_errno(_("unable to close %s"), dst);
+		goto out;
+	}
+
+	adjust_shared_perm(repo, dst);
+
+	ret = 0;
+
+out:
+	rollback_lock_file(&lock);
+	strbuf_release(&buf);
+	if (fd >= 0)
+		close(fd);
+	if (fp)
+		fclose(fp);
+
+	return ret;
+}
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);
 
 #endif
diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh
index 9cafcee509..bebf71bc29 100755
--- a/t/t1016-compatObjectFormat.sh
+++ b/t/t1016-compatObjectFormat.sh
@@ -305,4 +305,31 @@ compare_oids GPG2 'commit' signedcommit4 "$signedcommit4_sha1_oid" "$signedcommi
 compare_oids GPG2 'tag' signedtag3 "$signedtag3_sha1_oid" "$signedtag3_sha256_oid"
 compare_oids GPG2 'tag' signedtag4 "$signedtag4_sha1_oid" "$signedtag4_sha256_oid"
 
+test_expect_success 'batched fsync migrates loose object index' '
+	test_when_finished "rm -rf batch-fsync" &&
+	git init --object-format=sha1 batch-fsync &&
+	(
+		cd batch-fsync &&
+		git config core.repositoryFormatVersion 1 &&
+		git config extensions.compatObjectFormat sha256 &&
+		git config core.fsync loose-object &&
+		git config core.fsyncMethod batch &&
+
+		# Write object to generate initial loose object index.
+		echo foo >foo &&
+		git add foo &&
+		test_grep "$(git rev-parse :foo)" .git/objects/loose-object-idx &&
+
+		# Write some additional objects and ensure the corresponding
+		# temporary loose object index entries stored in the transaction
+		# are merged into the primary ODB.
+		echo bar >bar &&
+		echo baz >baz &&
+		git add bar baz &&
+		test_grep "$(git rev-parse :foo)" .git/objects/loose-object-idx &&
+		test_grep "$(git rev-parse :bar)" .git/objects/loose-object-idx &&
+		test_grep "$(git rev-parse :baz)" .git/objects/loose-object-idx
+	)
+'
+
 test_done
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 3debb60270..5307717354 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -4,6 +4,7 @@
 #include "chdir-notify.h"
 #include "dir.h"
 #include "environment.h"
+#include "loose.h"
 #include "object-file.h"
 #include "path.h"
 #include "string-list.h"
@@ -294,6 +295,11 @@ static int is_loose_object_shard(const char *name)
 	return strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);
 }
 
+static int is_loose_object_idx(const char *name)
+{
+	return !strcmp(name, "loose-object-idx");
+}
+
 static int migrate_paths(struct tmp_objdir *t,
 			 struct strbuf *src, struct strbuf *dst,
 			 enum finalize_object_file_flags flags)
@@ -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);
 
 		strbuf_setlen(src, src_len);
 		strbuf_setlen(dst, dst_len);

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
-- 
2.56.0.173.gf57c9a7388

Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Patrick Steinhardt]

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"?

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

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

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

> 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?

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

> 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

Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Justin Tobler]

On 26/10/23 02:19PM, Patrick Steinhardt wrote:
> 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"?

Ya, "object directory" would be more correct here. Will fix.

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

Agreed. I may change the commit message to call it a "mapping file" if
that helps in any way.

[snip]
> > 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?

I think a git-receive-pack(1) can spawn a git-unpack-objects(1) child
process and write loose objects into the quarantine directory.
Consequently, if a repository is configured with a compatibility hash,
these loose objects would also end up writing mapping entries into the
quarantine.

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

Yep, there doesn't appear to be any existing logic to prune entries from
this mapping file. It can only grow in size. This seemed like a separate
problem altogether though, so I didn't bother tackling that here.

> > 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?

For our intended use case of reading the "loose-object-idx" file from
the quarantine directory and merging its entries to the mapping file in
the primary object directory, I wouldn't expect it to ever be broken.
That is because realistically only a "loose-object-idx" that we create
during the transaction should be written into the quarantine directory.

I chose to be more defensive here because the `loose_object_idx_merge()`
function itself isn't aware of the circumstances regarding its use case.
In theory, another caller could use any mapping file as the source.
Ultimately though I don't feel too strongly either way and am happy to
remove it if we don't like it.

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

That's fair. I picked this value purely because I was closely following
how `write_one_object()` implements its logic to update the
"loose-object-idx" file.

Would it make sense to make this value configurable? Or would it be fine
to just set it to 1000ms? If we change this here, maybe we should also
change it for `write_one_object()` to match?

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

So here I was also following how `write_one_object()` handles writing
its mapping entries. I was trying to keep the `loose_object_idx_merge()`
and `write_one_object()` approaches similar. If this really isn't the
write approach though, I can explore adapting the patch to follow our
more typical pattern.

> I'm also a bit surprised that the file isn't sorted. Is a reader
> expected to always read _all_ mappings?

I was surprised by this too. From my understanding, readers are not
expected to consult the "loose-object-idx" file directly, but instead
consult an in-memory hash map that is loaded from the file. When an
entry is not found in map, the file is reloaded to try to pick up any
missing entries.

I see this a preexisting quirk though and am not sure it is something
that needs to be addressed as part of this change.

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

So typically a loose object doesn't get written to the object directory
at all if it already exists and thus doesn't create a new mapping entry.
There is a race though where concurrent writes of the same object could
cause the same entry to be appended twice. This is already an existing
problem though as `write_one_object()` also doesn't verify whether the
object is already in the file.

With this patch, I mostly avoided many of these preexisting quirks and
just focused on addressing the underlying bug. If we feel strongly about
it though, I can change the approach.

Thanks,
-Justin

Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Patrick Steinhardt]

On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:
> On 26/10/23 02:19PM, Patrick Steinhardt wrote:
> > On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:
[snip]
> > > 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?
> 
> For our intended use case of reading the "loose-object-idx" file from
> the quarantine directory and merging its entries to the mapping file in
> the primary object directory, I wouldn't expect it to ever be broken.
> That is because realistically only a "loose-object-idx" that we create
> during the transaction should be written into the quarantine directory.
> 
> I chose to be more defensive here because the `loose_object_idx_merge()`
> function itself isn't aware of the circumstances regarding its use case.
> In theory, another caller could use any mapping file as the source.
> Ultimately though I don't feel too strongly either way and am happy to
> remove it if we don't like it.

Ah. You mean because we're called with arbitrary paths we want to verify
whether our callers know what they're doing, and consequently we check
whether the files look like a mapping file? Still a bit funny, but with
that context it at least doesn't feel too unreasonable.

> > > +	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.
> 
> That's fair. I picked this value purely because I was closely following
> how `write_one_object()` implements its logic to update the
> "loose-object-idx" file.
> 
> Would it make sense to make this value configurable? Or would it be fine
> to just set it to 1000ms? If we change this here, maybe we should also
> change it for `write_one_object()` to match?

Yeah, I think that'd be a good idea.

> > > +	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.
> 
> So here I was also following how `write_one_object()` handles writing
> its mapping entries. I was trying to keep the `loose_object_idx_merge()`
> and `write_one_object()` approaches similar. If this really isn't the
> write approach though, I can explore adapting the patch to follow our
> more typical pattern.

I think this is quite broken. Right now nobody would care because we
don't really use the compatibility hash algorithm yet. But once it
starts becoming useful this _will_ lead to corrupted files. So I'd
prefer to fix it rather than doubling down on the broken pattern.

> > I'm also a bit surprised that the file isn't sorted. Is a reader
> > expected to always read _all_ mappings?
> 
> I was surprised by this too. From my understanding, readers are not
> expected to consult the "loose-object-idx" file directly, but instead
> consult an in-memory hash map that is loaded from the file. When an
> entry is not found in map, the file is reloaded to try to pick up any
> missing entries.

Sure. But that'd still require a linear scan through all mappings in
here.

> I see this a preexisting quirk though and am not sure it is something
> that needs to be addressed as part of this change.

Yeah, I think it's fine to ignore this for now. This part here is
certainly outside the scope of the patch series.

> > > +	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?
> 
> So typically a loose object doesn't get written to the object directory
> at all if it already exists and thus doesn't create a new mapping entry.
> There is a race though where concurrent writes of the same object could
> cause the same entry to be appended twice. This is already an existing
> problem though as `write_one_object()` also doesn't verify whether the
> object is already in the file.
> 
> With this patch, I mostly avoided many of these preexisting quirks and
> just focused on addressing the underlying bug. If we feel strongly about
> it though, I can change the approach.

I feel strongly that this mechanism is quite broken in its current form:

  - The file is unsorted, making lookups inefficient.

  - We don't deduplicate at all.

  - We never prune entries.

  - Writes are not atomic.

The first three are conceptual issues that we'd have to fix to ever make
this feature usable. But that certainly doesn't have to happen as part
of this patch series.

The last part though shouldn't be too hard to fix, and you make the
problem worse because you add a second writer that uses the same broken
pattern. So that's a part I'd like to see fixd as part of this series,
but I won't insist on it.

Thanks!

Patrick

Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Justin Tobler]

On 26/10/26 07:39AM, Patrick Steinhardt wrote:
> On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:
> > On 26/10/23 02:19PM, Patrick Steinhardt wrote:
> > > On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:
> > > > +	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?
> > 
> > For our intended use case of reading the "loose-object-idx" file from
> > the quarantine directory and merging its entries to the mapping file in
> > the primary object directory, I wouldn't expect it to ever be broken.
> > That is because realistically only a "loose-object-idx" that we create
> > during the transaction should be written into the quarantine directory.
> > 
> > I chose to be more defensive here because the `loose_object_idx_merge()`
> > function itself isn't aware of the circumstances regarding its use case.
> > In theory, another caller could use any mapping file as the source.
> > Ultimately though I don't feel too strongly either way and am happy to
> > remove it if we don't like it.
> 
> Ah. You mean because we're called with arbitrary paths we want to verify
> whether our callers know what they're doing, and consequently we check
> whether the files look like a mapping file? Still a bit funny, but with
> that context it at least doesn't feel too unreasonable.

Your understanding is correct. This may be more defensive that
realistically neccessary. Probably doesn't matter too much either way
though.

> > > > +	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.
> > 
> > That's fair. I picked this value purely because I was closely following
> > how `write_one_object()` implements its logic to update the
> > "loose-object-idx" file.
> > 
> > Would it make sense to make this value configurable? Or would it be fine
> > to just set it to 1000ms? If we change this here, maybe we should also
> > change it for `write_one_object()` to match?
> 
> Yeah, I think that'd be a good idea.

Ok, I'll update to use 1000ms in the next version.

[snip]
> > > When merging, wouldn't we have to verify that the object IDs that we're
> > > adding don't already exist?
> > 
> > So typically a loose object doesn't get written to the object directory
> > at all if it already exists and thus doesn't create a new mapping entry.
> > There is a race though where concurrent writes of the same object could
> > cause the same entry to be appended twice. This is already an existing
> > problem though as `write_one_object()` also doesn't verify whether the
> > object is already in the file.
> > 
> > With this patch, I mostly avoided many of these preexisting quirks and
> > just focused on addressing the underlying bug. If we feel strongly about
> > it though, I can change the approach.
> 
> I feel strongly that this mechanism is quite broken in its current form:
> 
>   - The file is unsorted, making lookups inefficient.
> 
>   - We don't deduplicate at all.
> 
>   - We never prune entries.
> 
>   - Writes are not atomic.

Ya, I completely agree. There are quite a few existing problems here.

> The first three are conceptual issues that we'd have to fix to ever make
> this feature usable. But that certainly doesn't have to happen as part
> of this patch series.
> 
> The last part though shouldn't be too hard to fix, and you make the
> problem worse because you add a second writer that uses the same broken
> pattern. So that's a part I'd like to see fixd as part of this series,
> but I won't insist on it.

That's fair. I'll update in the next version to make writing the
"loose-object-idx" file here atomic. I'm not entirely sure if I should
go ahead and update the code in `write_one_object()` to also be atomic
though.

One concern I have is operations that may write many loose objects,
would rewrite the "loose-object-idx" file for each object written. If
the mapping file becomes quite large, I suppose this could create a
significant performance regression. If these loose object writes were
batched in a transaction, we could wait to write the final mapping file
at transaction commit, but that may not always be the case.

Right now this feature isn't really usable to begin with, so maybe it
doesn't matter too much either way. For now, I'll proceed with just
updating the mapping file writer adding in this patch to be atomic.

-Justin