patchtmp-objdir: merge loose object index when migrating tmpdir

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(-)

On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:

You probably mean "object directory", not "database"?

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.

Sounds sensible.

Is that a theoretical scenario or do we actually have cases where a child process wants to write data into the quarantine directory, too?

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.

Curious. Why do we even expect that this file may be broken?

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.

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?

When merging, wouldn't we have to verify that the object IDs that we're adding don't already exist?

Makes sense.

Thanks!

Patrick

On 26/10/23 02:19PM, Patrick Steinhardt wrote:

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

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

[snip]

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.

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.

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.

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?

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

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

On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:

[snip]

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.

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

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.

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

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

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

On 26/10/26 07:39AM, Patrick Steinhardt wrote:

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

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

[snip]

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

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

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 and atomically write a new "loose-object-idx" 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 entries
    that may not ultimately get committed.

Changes since V1:
  - The loose object index write after the merging maps is now atomic.
  - Added a fixed timeout to all loose object index writers.
  - Duplicate entries are now dropped when writing the merged mapping file.

Thanks for the review, -Justin

Justin Tobler (4):
  loose: use finite timeout when locking "loose-object-idx" file
  loose: lift loose object index parsing logic into helper
  loose: lift loose object index writing logic into helper
  tmp-objdir: merge loose object index when migrating tmpdir

 loose.c                       | 207 ++++++++++++++++++++++++----------
 loose.h                       |   2 +
 t/t1016-compatObjectFormat.sh |  27 +++++
 tmp-objdir.c                  |  11 +-
 4 files changed, 189 insertions(+), 58 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  6f104c482e loose: use finite timeout when locking "loose-object-idx" file
-:  ---------- > 2:  4799421a74 loose: lift loose object index parsing logic into helper
-:  ---------- > 3:  4233e913ce loose: lift loose object index writing logic into helper
1:  34393e789c ! 4:  614aab27d1 tmp-objdir: merge loose object index when migrating tmpdir
    @@ Commit message
     
         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
    +    compatibility hash in a "loose-object-idx" file. This mapping file is
    +    located in the same object directory the object is written to. If batch
    +    fsync of loose objects is also enabled, the loose objects and mapping
    +    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:
    +    unable to be migrated if a mapping file 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.
    +    Furthermore, it is not sufficient to just replace the preexisting
    +    mapping file with the new one as the new file 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.
    +    "loose-object-idx" files explicitly by merging new entries and
    +    atomically writing the resulting mapping file.
     
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
    @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)
     +			   const char *dst)
     +{
     +	struct lock_file lock = LOCK_INIT;
    -+	struct strbuf buf = STRBUF_INIT;
    -+	struct stat st;
    -+	FILE *fp;
    -+	int fd = -1;
    ++	struct loose_object_map *map;
     +	int ret;
    ++	int fd;
     +
    -+	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;
    -+	}
    ++	loose_object_map_init(&map);
     +
    -+	if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
    ++	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;
     +	}
     +
    -+	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);
    ++	if (loose_object_map_read(repo, dst, map, NULL) < 0) {
    ++		ret = error(_("unable to read loose object index %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"));
    ++	if (loose_object_map_read(repo, src, map, NULL) < 0) {
    ++		ret = error(_("unable to read loose object index %s"), src);
     +		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;
    -+		}
    ++	if (loose_object_map_write(repo, map, fd) < 0) {
    ++		ret = error_errno(_("unable to write %s"),
    ++				  get_lock_file_path(&lock));
    ++		goto out;
     +	}
     +
    -+	ret = close(fd);
    -+	fd = -1;
    -+	if (ret) {
    -+		ret = error_errno(_("unable to close %s"), dst);
    ++	if (commit_lock_file(&lock) < 0) {
    ++		ret = error_errno(_("unable to write %s"), dst);
     +		goto out;
     +	}
     +
    -+	adjust_shared_perm(repo, dst);
    -+
     +	ret = 0;
     +
     +out:
    ++	loose_object_map_clear(&map);
     +	rollback_lock_file(&lock);
    -+	strbuf_release(&buf);
    -+	if (fd >= 0)
    -+		close(fd);
    -+	if (fp)
    -+		fclose(fp);
     +
     +	return ret;
     +}
    @@ t/t1016-compatObjectFormat.sh: compare_oids GPG2 'commit' signedcommit4 "$signed
     +
     +		# Write some additional objects and ensure the corresponding
     +		# temporary loose object index entries stored in the transaction
    -+		# are merged into the primary ODB.
    ++		# are merged into the primary object directory.
     +		echo bar >bar &&
     +		echo baz >baz &&
     +		git add bar baz &&

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Loose object index file writers first attempt to acquire a lock via `repo_hold_lock_file_for_update_timeout()` and specify a timeout value of -1. With a negative timeout value, the lock acquisition retries indefinitely while the lockfile still exists. If a process dies while holding the lock and leaves a stale lockfile behind, every subsequent writer of the mapping file hangs forever instead of reporting an error.

Introduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms and use it for all callers that lock a "loose-object-idx" file. A value of 1000ms matches the default timeout used for "core.packedRefsTimeout" and should provide concurrent writers sufficient time to acquire the lock.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

On Wed, Oct 28, 2026 at 07:24:10PM -0500, Justin Tobler wrote:

Right, aborting eventually is way better compared to blocking indefinitely. Ideally we should also make this setting configurable. But I think it's fine to skip that for now and fix that at a later point in time.

Patrick

Mapping entries from the "loose-object-idx" file are parsed via `loose_object_map_load()` and inserted into the in-memory loose object map of the corresponding object source. In a subsequent commit, a temporary loose object index file must also be parsed and have its contents stored in a map that is not attached to any object source.

Extract the logic for parsing a loose object index file into a separate `loose_object_map_read()` helper function that allows callers to explicitly specify the map to insert the entries into. The `insert_loose_map()` helper function is also updated accordingly to explicitly operate on the provided map and optional cache instead of the object source.

While at it, stop treating all errors related to opening the "loose-object-idx" file as success and instead only ignore ENOENT. It is expected that a mapping file may not always be present, but other errors should be signaled back to callers. Note that this slightly changes the behavior of `repo_read_loose_object_map()`, which now stops loading the mapping files of subsequent object sources when one is unreadable, the same way it already does when one is malformed.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 87 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

On Wed, Oct 28, 2026 at 07:24:11PM -0500, Justin Tobler wrote:

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.

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

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.

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

Patrick

On 26/10/29 09:05AM, Patrick Steinhardt wrote:

Yes, this is used in the final patch. I'll update the commit message to make this a bit more clear.

-Justin

Writing the in-memory loose object map to a "loose-object-idx" file is handled via `repo_write_loose_object_map()`, which always writes the map into the primary object directory. In a subsequent commit, a separate "loose-object-idx" file is staged prior to commit and must be constructed from a loose object map not attached to the primary loose object source.

Extract the logic for writing a loose object index file into a separate `loose_object_map_write()` helper function that allows callers to explicitly specify both the loose object map as well as the file descriptor to write the entries to.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 69 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 24 deletions(-)

On Wed, Oct 28, 2026 at 07:24:12PM -0500, Justin Tobler wrote:

Okay. This commit is essentially the equivalent to the preceding one, but for writes.

Curious that you're using a file descriptor instead of a path as you did in the preceding commit. But I guess that's because of how we first have to lock the file, which we of course don't have to do on the writing side.

I guess we could've also moved locking into this function here to make both interfaces match, but maybe the next commit will give a hint why we don't.

Patrick

On 26/10/29 09:05AM, Patrick Steinhardt wrote:

That is correct. When we lock the file, it gives us back the file descriptor back for the lockfile. I decided to use the file descriptor here because it was readily available to pass around.

Yup, this becomes more apparent in the next commit, but we don't want to acquire the lock in the helper, so that callers can lock the file prior to reading any preexisting version to get a consistent view. I may leave a note about this in the next versions commit message.

-Justin

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 mapping file is located in the same object directory the object is written to. If batch fsync of loose objects is also enabled, the loose objects and mapping 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 a mapping file 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 mapping file with the new one as the new file only contains mapping entries for the newly written objects.

Update the temporary directory migration logic to handle "loose-object-idx" files explicitly by merging new entries and atomically writing the resulting mapping file.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c                       | 47 +++++++++++++++++++++++++++++++++++
 loose.h                       |  2 ++
 t/t1016-compatObjectFormat.sh | 27 ++++++++++++++++++++
 tmp-objdir.c                  | 11 +++++++-
 4 files changed, 86 insertions(+), 1 deletion(-)

On Wed, Oct 28, 2026 at 07:24:13PM -0500, Justin Tobler wrote:

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.

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?

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

Thanks!

Patrick

On 26/10/29 09:05AM, Patrick Steinhardt wrote:

Ya good point. The src file can be read prior to acquiring the lock. Will update.

Will do!

Thanks, -Justin

Justin Tobler <jltobler@gmail.com> writes:

Thanks.

Merging this topic to 'seen' had a funny interaction with other topics in flight. repo_write_loose_object_map() function, from which loose_object_map_write() helper function was extracted out of, loses all callers, and will disappear in the merge. But the new helper, which gained a caller loose_object_idx_merge() in this series, remains.

On 26/10/29 03:22PM, Junio C Hamano wrote:

Thanks for the heads up. I see the function gets dropped by de2910a42b (loose: drop unused `repo_write_loose_object_map()`, 2026-10-27). Keeping the new helper and dropping `repo_write_loose_object_map()` is indeed the correct resolution.

It does look like in `seen` the declaration of `repo_write_loose_object_map()` is still left behind in "loose.h" though.

I have a few small changes queued up locally for a v3, which I will send out soon.

Thanks, -Justin

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 and atomically write a new "loose-object-idx" 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 entries
    that may not ultimately get committed.

Changes since V2:
  - Reading the src mapping file is now done prior to acquiring the lock
    to reduce the critical window.
  - Added a comment to `loose_object_idx_merge()` and updated some
    commit messages to improve clarity.

Changes since V1:
  - The loose object index write after the merging maps is now atomic.
  - Added a fixed timeout to all loose object index writers.
  - Duplicate entries are now dropped when writing the merged mapping file.

Thanks for the review, -Justin

Justin Tobler (4):
  loose: use finite timeout when locking "loose-object-idx" file
  loose: lift loose object index parsing logic into helper
  loose: lift loose object index writing logic into helper
  tmp-objdir: merge loose object index when migrating tmpdir

 loose.c                       | 207 ++++++++++++++++++++++++----------
 loose.h                       |   9 ++
 t/t1016-compatObjectFormat.sh |  27 +++++
 tmp-objdir.c                  |  11 +-
 4 files changed, 196 insertions(+), 58 deletions(-)

Range-diff against v2:
1:  6f104c482e = 1:  6f104c482e loose: use finite timeout when locking "loose-object-idx" file
2:  4799421a74 ! 2:  3ae068f364 loose: lift loose object index parsing logic into helper
    @@ Commit message
         explicitly specify the map to insert the entries into. The
         `insert_loose_map()` helper function is also updated accordingly to
         explicitly operate on the provided map and optional cache instead of the
    -    object source.
    +    object source. The cache is made optional for future callers that may
    +    only be interested in updating the map.
     
         While at it, stop treating all errors related to opening the
         "loose-object-idx" file as success and instead only ignore ENOENT. It is
3:  4233e913ce ! 3:  0a05eb5fb6 loose: lift loose object index writing logic into helper
    @@ Commit message
         Extract the logic for writing a loose object index file into a separate
         `loose_object_map_write()` helper function that allows callers to
         explicitly specify both the loose object map as well as the file
    -    descriptor to write the entries to.
    +    descriptor to write the entries to. Note that the responsibility of
    +    locking the file being written to is left to callers so that it is
    +    possible for a lock to be held for longer than just this single
    +    operation.
     
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
4:  614aab27d1 ! 4:  3c4a3332a5 tmp-objdir: merge loose object index when migrating tmpdir
    @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)
     +
     +	loose_object_map_init(&map);
     +
    ++	if (loose_object_map_read(repo, src, map, NULL) < 0) {
    ++		ret = error(_("unable to read loose object index %s"), src);
    ++		goto out;
    ++	}
    ++
     +	fd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,
     +						    LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);
     +	if (fd < 0) {
    @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)
     +		goto out;
     +	}
     +
    -+	if (loose_object_map_read(repo, src, map, NULL) < 0) {
    -+		ret = error(_("unable to read loose object index %s"), src);
    -+		goto out;
    -+	}
    -+
     +	if (loose_object_map_write(repo, map, fd) < 0) {
     +		ret = error_errno(_("unable to write %s"),
     +				  get_lock_file_path(&lock));
    @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)
     
      ## loose.h ##
     @@ loose.h: 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);
    + 
    ++/*
    ++ * Merge the mapping entries of the loose object index at `src` into the one at
    ++ * `dst`. The resulting mapping file is written atomically.
    ++ *
    ++ * Returns 0 on success, a negative error code otherwise.
    ++ */
     +int loose_object_idx_merge(struct repository *repo, const char *src,
     +			   const char *dst);
    - 
    ++
      #endif
     
      ## t/t1016-compatObjectFormat.sh ##

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Loose object index file writers first attempt to acquire a lock via `repo_hold_lock_file_for_update_timeout()` and specify a timeout value of -1. With a negative timeout value, the lock acquisition retries indefinitely while the lockfile still exists. If a process dies while holding the lock and leaves a stale lockfile behind, every subsequent writer of the mapping file hangs forever instead of reporting an error.

Introduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms and use it for all callers that lock a "loose-object-idx" file. A value of 1000ms matches the default timeout used for "core.packedRefsTimeout" and should provide concurrent writers sufficient time to acquire the lock.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Justin Tobler <jltobler@gmail.com> writes:

I was wondering why we don't use a config here too. Seems like Patrick also mentioned the same in his review. But this is a good middle step and we can add that in later.

#leftoverbits

[snip]

Mapping entries from the "loose-object-idx" file are parsed via `loose_object_map_load()` and inserted into the in-memory loose object map of the corresponding object source. In a subsequent commit, a temporary loose object index file must also be parsed and have its contents stored in a map that is not attached to any object source.

Extract the logic for parsing a loose object index file into a separate `loose_object_map_read()` helper function that allows callers to explicitly specify the map to insert the entries into. The `insert_loose_map()` helper function is also updated accordingly to explicitly operate on the provided map and optional cache instead of the object source. The cache is made optional for future callers that may only be interested in updating the map.

While at it, stop treating all errors related to opening the "loose-object-idx" file as success and instead only ignore ENOENT. It is expected that a mapping file may not always be present, but other errors should be signaled back to callers. Note that this slightly changes the behavior of `repo_read_loose_object_map()`, which now stops loading the mapping files of subsequent object sources when one is unreadable, the same way it already does when one is malformed.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 87 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

Justin Tobler <jltobler@gmail.com> writes:

Nit: with the introduction of `loose_object_map_read()`, perhaps makes
sense to keep naming consistent and rename this to
`loose_object_map_insert()`?

[snip]

The changes look good.

On 26/10/30 07:50PM, Karthik Nayak wrote:

I don't feel too strongly either way and am not sure this by itself it worth a reroll. I'll keep it as-is for now.

-Justin

Writing the in-memory loose object map to a "loose-object-idx" file is handled via `repo_write_loose_object_map()`, which always writes the map into the primary object directory. In a subsequent commit, a separate "loose-object-idx" file is staged prior to commit and must be constructed from a loose object map not attached to the primary loose object source.

Extract the logic for writing a loose object index file into a separate `loose_object_map_write()` helper function that allows callers to explicitly specify both the loose object map as well as the file descriptor to write the entries to. Note that the responsibility of locking the file being written to is left to callers so that it is possible for a lock to be held for longer than just this single operation.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c | 69 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 24 deletions(-)

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 mapping file is located in the same object directory the object is written to. If batch fsync of loose objects is also enabled, the loose objects and mapping 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 a mapping file 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 mapping file with the new one as the new file only contains mapping entries for the newly written objects.

Update the temporary directory migration logic to handle "loose-object-idx" files explicitly by merging new entries and atomically writing the resulting mapping file.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 loose.c                       | 47 +++++++++++++++++++++++++++++++++++
 loose.h                       |  9 +++++++
 t/t1016-compatObjectFormat.sh | 27 ++++++++++++++++++++
 tmp-objdir.c                  | 11 +++++++-
 4 files changed, 93 insertions(+), 1 deletion(-)

Justin Tobler <jltobler@gmail.com> writes:

Nit: should we name this `loose_object_map_merge()` to stay consistent
with the other `loose_object_map...()` functions?

Nit: Shouldn't this be unable to commit/rename? This will differentiate
from the above write error

The rest looks to be in order.

On 26/10/30 08:55PM, Karthik Nayak wrote:

I specifically did not call it `loose_object_map_merge()` because, unlike the other similarly named functions, this one does not update the in-memory loose object map for the object source. Here we are only merging the "loose-object-idx" files.

I choose "write" here because logically we are trying atomically write the final "loose-object-idx" file. IMO, I think the difference in filepath should be sufficient to differentiate between the two as well. Ultimately though, I don't feel strongly either way about this and am not sure it's worth a reroll, so I'm inclined to leave it as-is.

-Justin

On Thu, Oct 29, 2026 at 06:22:16PM -0500, Justin Tobler wrote:

The changes and range-diff both look exacty as expected, so I'm happy with this version. Thanks!

Patrick

On 26/10/30 07:20AM, Patrick Steinhardt wrote:

Thanks for the review!

-Justin

Justin Tobler <jltobler@gmail.com> writes:

Okay so reading a bit, and paraphrasing for my understanding: When we hae object compatibility setup in a repository. THe 'loose-object-idx' tracks the mapping from hash1<>hash2. The issue is batch fsync, where we use a temporary folder. I'm not sure what the 'error' here is, but I guess we'll see with the patches.

For pack files this is a non-issues as they're content addressed and individual packfiles have their own corresponding idx files.

This means we write a 'loose-object-idx' to the temporary directory and we need to merge it with the one in the main directory. Which seems to make sense.

So the child processes write the objects, so they write to the index too I presume. If not, we'd have to somehow get that information from all children to the parent which owns the transaction. That gets complicated / messy. Seems right.

Yeah this approach definitely doesn't make sense from a transaction point of view.

[snip]

Will dig into the patches...

On 26/10/30 07:38PM, Karthik Nayak wrote:

Thanks for the review! IMO, I think the suggestions left are probably not worth a reroll, so I plan to leave the series as-is, but please let me know if you feel differently. :)

Thanks, -Justin

Justin Tobler <jltobler@gmail.com> writes:

It's all good. It is in a good shape already!