{"thread":{"id":"66637","subject":"[PATCH] tmp-objdir: merge loose object index when migrating tmpdir","startedAt":"2026-10-22T23:40:07Z","lastAt":"2026-11-03T16:37:50Z","messageCount":34,"participants":["Justin Tobler","Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"555937","messageId":"20261022234007.3081825-1-jltobler@gmail.com","threadId":"66637","inReplyTo":null,"subject":"[PATCH] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-22T23:40:07Z","receivedAt":"2026-10-22T23:40:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When a repository is configured with a compatibility hash, each loose\nobject write also records a mapping between the object and its\ncompatibility hash in a \"loose-object-idx\" file. This index is located\nin the same object database the object is written to. If batch fsync of\nloose objects is also enabled, the loose objects and index file are\ninstead written to a temporary object directory and expected to be\nmigrated to the primary object directory when the batch transaction\ncommits.\n\nWhile migrating the loose objects works, the \"loose-object-idx\" file is\nunable to be migrated if an index already exists in the primary object\ndirectory. This occurs because `finalize_object_file_flags()` expects an\nalready existing file to have the same contents. Otherwise, the\nfollowing error is returned:\n\n error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents\n error: unable to migrate temporary objects\n\nFurthermore, it is not sufficient to just replace the preexisting index\nwith the new one as the new index only contains mapping entries for the\nnewly written objects.\n\nUpdate the temporary directory migration logic to handle\n\"loose-object-idx\" files explicitly by appending new mapping entries to\nthe primary index.\n\nSigned-off-by: Justin Tobler \n---\n\nGreetings,\n\nWhile working on a separate patch series to convert the \"files\" ODB\ntransaction to be eagerly created instead of lazily, I encountered this\nbug that causes an ODB transaction to fail on commit when a\ncompatibility hash is also configured. This occurs because the temporary\ndirectory migration code does not know how to handle the loose object\nindex file.\n\nThis patch addresses updating the migration logic to explicitly merge\nmapping entries in the transaction by appending them to the primary\nloose object index file.\n\nSome other approaches I considered:\n - Storing the uncommitted object mapping entries in the transaction\n without writing a file and only writing on transaction commit. I\n chose to keep writing the temp index into the transaction so that\n child processes that write to the tempdir could still append entries.\n - Also writing mapping entires to the main ODB loose object index.\n This also didn't seem like a good apprach because it write entires\n that may not ultimately get committed.\n\nThanks for the review,\n-Justin\n\n---\n loose.c | 72 +++++++++++++++++++++++++++++++++++\n loose.h | 2 +\n t/t1016-compatObjectFormat.sh | 27 +++++++++++++\n tmp-objdir.c | 11 +++++-\n 4 files changed, 111 insertions(+), 1 deletion(-)\n\ndiff --git a/loose.c b/loose.c\nindex d6a2bb5ba3..79d4c256e0 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)\n \tfree(m);\n \t*map = NULL;\n }\n+\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst)\n+{\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct stat st;\n+\tFILE *fp;\n+\tint fd = -1;\n+\tint ret;\n+\n+\tfp = fopen(src, \"r\");\n+\tif (!fp) {\n+\t\tret = error_errno(_(\"unable to open %s\"), src);\n+\t\tgoto out;\n+\t}\n+\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n+\t\tret = error(_(\"invalid loose object index %s\"), src);\n+\t\tgoto out;\n+\t}\n+\n+\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n+\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tfd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);\n+\tif (fd < 0) {\n+\t\tret = error_errno(_(\"unable to open %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\tif (fstat(fd, &st) < 0) {\n+\t\tret = error_errno(_(\"unable to stat %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\t/* Create the loose-object-idx if one does not already exist. */\n+\tif (!st.st_size && write_in_full(fd, loose_object_header,\n+\t\t\t\t\t strlen(loose_object_header)) < 0) {\n+\t\tret = error_errno(_(\"failed to write loose object index header\"));\n+\t\tgoto out;\n+\t}\n+\n+\twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n+\t\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\t\tret = error_errno(_(\"failed to write loose object index %s\"),\n+\t\t\t\t\t dst);\n+\t\t\tgoto out;\n+\t\t}\n+\t}\n+\n+\tret = close(fd);\n+\tfd = -1;\n+\tif (ret) {\n+\t\tret = error_errno(_(\"unable to close %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tadjust_shared_perm(repo, dst);\n+\n+\tret = 0;\n+\n+out:\n+\trollback_lock_file(&lock);\n+\tstrbuf_release(&buf);\n+\tif (fd >= 0)\n+\t\tclose(fd);\n+\tif (fp)\n+\t\tfclose(fp);\n+\n+\treturn ret;\n+}\ndiff --git a/loose.h b/loose.h\nindex ed663ac550..35b46f4be5 100644\n--- a/loose.h\n+++ b/loose.h\n@@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n \t\t\t const struct object_id *compat_oid);\n int repo_read_loose_object_map(struct repository *repo);\n int repo_write_loose_object_map(struct repository *repo);\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst);\n \n #endif\ndiff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh\nindex 9cafcee509..bebf71bc29 100755\n--- a/t/t1016-compatObjectFormat.sh\n+++ b/t/t1016-compatObjectFormat.sh\n@@ -305,4 +305,31 @@ compare_oids GPG2 'commit' signedcommit4 \"$signedcommit4_sha1_oid\" \"$signedcommi\n compare_oids GPG2 'tag' signedtag3 \"$signedtag3_sha1_oid\" \"$signedtag3_sha256_oid\"\n compare_oids GPG2 'tag' signedtag4 \"$signedtag4_sha1_oid\" \"$signedtag4_sha256_oid\"\n \n+test_expect_success 'batched fsync migrates loose object index' '\n+\ttest_when_finished \"rm -rf batch-fsync\" &&\n+\tgit init --object-format=sha1 batch-fsync &&\n+\t(\n+\t\tcd batch-fsync &&\n+\t\tgit config core.repositoryFormatVersion 1 &&\n+\t\tgit config extensions.compatObjectFormat sha256 &&\n+\t\tgit config core.fsync loose-object &&\n+\t\tgit config core.fsyncMethod batch &&\n+\n+\t\t# Write object to generate initial loose object index.\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\n+\t\t# Write some additional objects and ensure the corresponding\n+\t\t# temporary loose object index entries stored in the transaction\n+\t\t# are merged into the primary ODB.\n+\t\techo bar >bar &&\n+\t\techo baz >baz &&\n+\t\tgit add bar baz &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :bar)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :baz)\" .git/objects/loose-object-idx\n+\t)\n+'\n+\n test_done\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 3debb60270..5307717354 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -4,6 +4,7 @@\n #include \"chdir-notify.h\"\n #include \"dir.h\"\n #include \"environment.h\"\n+#include \"loose.h\"\n #include \"object-file.h\"\n #include \"path.h\"\n #include \"string-list.h\"\n@@ -294,6 +295,11 @@ static int is_loose_object_shard(const char *name)\n \treturn strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);\n }\n \n+static int is_loose_object_idx(const char *name)\n+{\n+\treturn !strcmp(name, \"loose-object-idx\");\n+}\n+\n static int migrate_paths(struct tmp_objdir *t,\n \t\t\t struct strbuf *src, struct strbuf *dst,\n \t\t\t enum finalize_object_file_flags flags)\n@@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t,\n \t\tif (is_loose_object_shard(name))\n \t\t\tflags_copy |= FOF_SKIP_COLLISION_CHECK;\n \n-\t\tret |= migrate_one(t, src, dst, flags_copy);\n+\t\tif (is_loose_object_idx(name))\n+\t\t\tret |= loose_object_idx_merge(t->repo, src->buf, dst->buf);\n+\t\telse\n+\t\t\tret |= migrate_one(t, src, dst, flags_copy);\n \n \t\tstrbuf_setlen(src, src_len);\n \t\tstrbuf_setlen(dst, dst_len);\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"555997","messageId":"attQtZ1dVoAnQznP@pks.im","threadId":"66637","inReplyTo":"20261022234007.3081825-1-jltobler@gmail.com","subject":"Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-23T12:19:01Z","receivedAt":"2026-10-23T12:19:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:\n> When a repository is configured with a compatibility hash, each loose\n> object write also records a mapping between the object and its\n> compatibility hash in a \"loose-object-idx\" file. This index is located\n> in the same object database the object is written to. If batch fsync of\n\nYou probably mean \"object directory\", not \"database\"?\n\n> loose objects is also enabled, the loose objects and index file are\n> instead written to a temporary object directory and expected to be\n> migrated to the primary object directory when the batch transaction\n> commits.\n> \n> While migrating the loose objects works, the \"loose-object-idx\" file is\n> unable to be migrated if an index already exists in the primary object\n\nThe naming really throws me off, \"index\" to me is our staging area. What\nthis is is a map. But oh, well, cannot change that now.\n\n> directory. This occurs because `finalize_object_file_flags()` expects an\n> already existing file to have the same contents. Otherwise, the\n> following error is returned:\n> \n> error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents\n> error: unable to migrate temporary objects\n> \n> Furthermore, it is not sufficient to just replace the preexisting index\n> with the new one as the new index only contains mapping entries for the\n> newly written objects.\n> \n> Update the temporary directory migration logic to handle\n> \"loose-object-idx\" files explicitly by appending new mapping entries to\n> the primary index.\n\nSounds sensible.\n\n> Signed-off-by: Justin Tobler \n> ---\n> \n> Greetings,\n> \n> While working on a separate patch series to convert the \"files\" ODB\n> transaction to be eagerly created instead of lazily, I encountered this\n> bug that causes an ODB transaction to fail on commit when a\n> compatibility hash is also configured. This occurs because the temporary\n> directory migration code does not know how to handle the loose object\n> index file.\n> \n> This patch addresses updating the migration logic to explicitly merge\n> mapping entries in the transaction by appending them to the primary\n> loose object index file.\n> \n> Some other approaches I considered:\n> - Storing the uncommitted object mapping entries in the transaction\n> without writing a file and only writing on transaction commit. I\n> chose to keep writing the temp index into the transaction so that\n> child processes that write to the tempdir could still append entries.\n\nIs that a theoretical scenario or do we actually have cases where a\nchild process wants to write data into the quarantine directory, too?\n\n> - Also writing mapping entires to the main ODB loose object index.\n> This also didn't seem like a good apprach because it write entires\n> that may not ultimately get committed.\n\nAgreed, we shouldn't commit this before the actual objects exist. I bet\nwe don't properly handle this during housekeeping either -- or at least\nI never stumbled over logic that prunes mappings when pruning the\ncorresponding objects. But that's a problem to solve another day.\n\n> diff --git a/loose.c b/loose.c\n> index d6a2bb5ba3..79d4c256e0 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)\n> \tfree(m);\n> \t*map = NULL;\n> }\n> +\n> +int loose_object_idx_merge(struct repository *repo, const char *src,\n> +\t\t\t const char *dst)\n> +{\n> +\tstruct lock_file lock = LOCK_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct stat st;\n> +\tFILE *fp;\n> +\tint fd = -1;\n> +\tint ret;\n> +\n> +\tfp = fopen(src, \"r\");\n> +\tif (!fp) {\n> +\t\tret = error_errno(_(\"unable to open %s\"), src);\n> +\t\tgoto out;\n> +\t}\n> +\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n> +\t\tret = error(_(\"invalid loose object index %s\"), src);\n> +\t\tgoto out;\n> +\t}\n\nCurious. Why do we even expect that this file may be broken?\n\n> +\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n> +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n\nWith a timeout of -1 we'll block indefinitely. That doesn't feel like\nthe best option to me, as the lockfile may be left over from a previous\nprocess. And Git locking up indefinitely feels suboptimal to me.\n\n> +\tfd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);\n\nI don't think it's a good idea to append data under the lock. It may\neasily cause us to corrupt the file in case the write succeeds\npartially, only.\n\nOur usual pattern here is to create the lockfile, write all data into\nthe new file and then rename it into place.\n\nI'm also a bit surprised that the file isn't sorted. Is a reader\nexpected to always read _all_ mappings?\n\n> +\tif (fd < 0) {\n> +\t\tret = error_errno(_(\"unable to open %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n> +\tif (fstat(fd, &st) < 0) {\n> +\t\tret = error_errno(_(\"unable to stat %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n> +\n> +\t/* Create the loose-object-idx if one does not already exist. */\n> +\tif (!st.st_size && write_in_full(fd, loose_object_header,\n> +\t\t\t\t\t strlen(loose_object_header)) < 0) {\n> +\t\tret = error_errno(_(\"failed to write loose object index header\"));\n> +\t\tgoto out;\n> +\t}\n> +\n> +\twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> +\t\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n> +\t\t\tret = error_errno(_(\"failed to write loose object index %s\"),\n> +\t\t\t\t\t dst);\n> +\t\t\tgoto out;\n> +\t\t}\n> +\t}\n\nWhen merging, wouldn't we have to verify that the object IDs that we're\nadding don't already exist?\n\n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n> index 3debb60270..5307717354 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t,\n> \t\tif (is_loose_object_shard(name))\n> \t\t\tflags_copy |= FOF_SKIP_COLLISION_CHECK;\n> \n> -\t\tret |= migrate_one(t, src, dst, flags_copy);\n> +\t\tif (is_loose_object_idx(name))\n> +\t\t\tret |= loose_object_idx_merge(t->repo, src->buf, dst->buf);\n> +\t\telse\n> +\t\t\tret |= migrate_one(t, src, dst, flags_copy);\n\nMakes sense.\n\nThanks!\n\nPatrick\n\n"},{"id":"556112","messageId":"atufODWNPJMbvMZu@denethor","threadId":"66637","inReplyTo":"attQtZ1dVoAnQznP@pks.im","subject":"Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-23T21:25:12Z","receivedAt":"2026-10-23T21:25:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/23 02:19PM, Patrick Steinhardt wrote:\n> On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:\n> > When a repository is configured with a compatibility hash, each loose\n> > object write also records a mapping between the object and its\n> > compatibility hash in a \"loose-object-idx\" file. This index is located\n> > in the same object database the object is written to. If batch fsync of\n> \n> You probably mean \"object directory\", not \"database\"?\n\nYa, \"object directory\" would be more correct here. Will fix.\n\n> > loose objects is also enabled, the loose objects and index file are\n> > instead written to a temporary object directory and expected to be\n> > migrated to the primary object directory when the batch transaction\n> > commits.\n> > \n> > While migrating the loose objects works, the \"loose-object-idx\" file is\n> > unable to be migrated if an index already exists in the primary object\n> \n> The naming really throws me off, \"index\" to me is our staging area. What\n> this is is a map. But oh, well, cannot change that now.\n\nAgreed. I may change the commit message to call it a \"mapping file\" if\nthat helps in any way.\n\n[snip]\n> > Some other approaches I considered:\n> > - Storing the uncommitted object mapping entries in the transaction\n> > without writing a file and only writing on transaction commit. I\n> > chose to keep writing the temp index into the transaction so that\n> > child processes that write to the tempdir could still append entries.\n> \n> Is that a theoretical scenario or do we actually have cases where a\n> child process wants to write data into the quarantine directory, too?\n\nI think a git-receive-pack(1) can spawn a git-unpack-objects(1) child\nprocess and write loose objects into the quarantine directory.\nConsequently, if a repository is configured with a compatibility hash,\nthese loose objects would also end up writing mapping entries into the\nquarantine.\n\n> > - Also writing mapping entires to the main ODB loose object index.\n> > This also didn't seem like a good apprach because it write entires\n> > that may not ultimately get committed.\n> \n> Agreed, we shouldn't commit this before the actual objects exist. I bet\n> we don't properly handle this during housekeeping either -- or at least\n> I never stumbled over logic that prunes mappings when pruning the\n> corresponding objects. But that's a problem to solve another day.\n\nYep, there doesn't appear to be any existing logic to prune entries from\nthis mapping file. It can only grow in size. This seemed like a separate\nproblem altogether though, so I didn't bother tackling that here.\n\n> > diff --git a/loose.c b/loose.c\n> > index d6a2bb5ba3..79d4c256e0 100644\n> > --- a/loose.c\n> > +++ b/loose.c\n> > @@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)\n> > \tfree(m);\n> > \t*map = NULL;\n> > }\n> > +\n> > +int loose_object_idx_merge(struct repository *repo, const char *src,\n> > +\t\t\t const char *dst)\n> > +{\n> > +\tstruct lock_file lock = LOCK_INIT;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tstruct stat st;\n> > +\tFILE *fp;\n> > +\tint fd = -1;\n> > +\tint ret;\n> > +\n> > +\tfp = fopen(src, \"r\");\n> > +\tif (!fp) {\n> > +\t\tret = error_errno(_(\"unable to open %s\"), src);\n> > +\t\tgoto out;\n> > +\t}\n> > +\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n> > +\t\tret = error(_(\"invalid loose object index %s\"), src);\n> > +\t\tgoto out;\n> > +\t}\n> \n> Curious. Why do we even expect that this file may be broken?\n\nFor our intended use case of reading the \"loose-object-idx\" file from\nthe quarantine directory and merging its entries to the mapping file in\nthe primary object directory, I wouldn't expect it to ever be broken.\nThat is because realistically only a \"loose-object-idx\" that we create\nduring the transaction should be written into the quarantine directory.\n\nI chose to be more defensive here because the `loose_object_idx_merge()`\nfunction itself isn't aware of the circumstances regarding its use case.\nIn theory, another caller could use any mapping file as the source.\nUltimately though I don't feel too strongly either way and am happy to\nremove it if we don't like it.\n\n> > +\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n> > +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> > +\t\tgoto out;\n> > +\t}\n> \n> With a timeout of -1 we'll block indefinitely. That doesn't feel like\n> the best option to me, as the lockfile may be left over from a previous\n> process. And Git locking up indefinitely feels suboptimal to me.\n\nThat's fair. I picked this value purely because I was closely following\nhow `write_one_object()` implements its logic to update the\n\"loose-object-idx\" file.\n\nWould it make sense to make this value configurable? Or would it be fine\nto just set it to 1000ms? If we change this here, maybe we should also\nchange it for `write_one_object()` to match?\n\n> > +\tfd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);\n> \n> I don't think it's a good idea to append data under the lock. It may\n> easily cause us to corrupt the file in case the write succeeds\n> partially, only.\n> \n> Our usual pattern here is to create the lockfile, write all data into\n> the new file and then rename it into place.\n\nSo here I was also following how `write_one_object()` handles writing\nits mapping entries. I was trying to keep the `loose_object_idx_merge()`\nand `write_one_object()` approaches similar. If this really isn't the\nwrite approach though, I can explore adapting the patch to follow our\nmore typical pattern.\n\n> I'm also a bit surprised that the file isn't sorted. Is a reader\n> expected to always read _all_ mappings?\n\nI was surprised by this too. From my understanding, readers are not\nexpected to consult the \"loose-object-idx\" file directly, but instead\nconsult an in-memory hash map that is loaded from the file. When an\nentry is not found in map, the file is reloaded to try to pick up any\nmissing entries.\n\nI see this a preexisting quirk though and am not sure it is something\nthat needs to be addressed as part of this change.\n\n> > +\tif (fd < 0) {\n> > +\t\tret = error_errno(_(\"unable to open %s\"), dst);\n> > +\t\tgoto out;\n> > +\t}\n> > +\tif (fstat(fd, &st) < 0) {\n> > +\t\tret = error_errno(_(\"unable to stat %s\"), dst);\n> > +\t\tgoto out;\n> > +\t}\n> > +\n> > +\t/* Create the loose-object-idx if one does not already exist. */\n> > +\tif (!st.st_size && write_in_full(fd, loose_object_header,\n> > +\t\t\t\t\t strlen(loose_object_header)) < 0) {\n> > +\t\tret = error_errno(_(\"failed to write loose object index header\"));\n> > +\t\tgoto out;\n> > +\t}\n> > +\n> > +\twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> > +\t\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n> > +\t\t\tret = error_errno(_(\"failed to write loose object index %s\"),\n> > +\t\t\t\t\t dst);\n> > +\t\t\tgoto out;\n> > +\t\t}\n> > +\t}\n> \n> When merging, wouldn't we have to verify that the object IDs that we're\n> adding don't already exist?\n\nSo typically a loose object doesn't get written to the object directory\nat all if it already exists and thus doesn't create a new mapping entry.\nThere is a race though where concurrent writes of the same object could\ncause the same entry to be appended twice. This is already an existing\nproblem though as `write_one_object()` also doesn't verify whether the\nobject is already in the file.\n\nWith this patch, I mostly avoided many of these preexisting quirks and\njust focused on addressing the underlying bug. If we feel strongly about\nit though, I can change the approach.\n\nThanks,\n-Justin\n\n"},{"id":"556251","messageId":"at71hkqG7AbVCdnK@pks.im","threadId":"66637","inReplyTo":"atufODWNPJMbvMZu@denethor","subject":"Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-26T06:39:02Z","receivedAt":"2026-10-26T06:39:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:\n> On 26/10/23 02:19PM, Patrick Steinhardt wrote:\n> > On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:\n[snip]\n> > > diff --git a/loose.c b/loose.c\n> > > index d6a2bb5ba3..79d4c256e0 100644\n> > > --- a/loose.c\n> > > +++ b/loose.c\n> > > @@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)\n> > > \tfree(m);\n> > > \t*map = NULL;\n> > > }\n> > > +\n> > > +int loose_object_idx_merge(struct repository *repo, const char *src,\n> > > +\t\t\t const char *dst)\n> > > +{\n> > > +\tstruct lock_file lock = LOCK_INIT;\n> > > +\tstruct strbuf buf = STRBUF_INIT;\n> > > +\tstruct stat st;\n> > > +\tFILE *fp;\n> > > +\tint fd = -1;\n> > > +\tint ret;\n> > > +\n> > > +\tfp = fopen(src, \"r\");\n> > > +\tif (!fp) {\n> > > +\t\tret = error_errno(_(\"unable to open %s\"), src);\n> > > +\t\tgoto out;\n> > > +\t}\n> > > +\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n> > > +\t\tret = error(_(\"invalid loose object index %s\"), src);\n> > > +\t\tgoto out;\n> > > +\t}\n> > \n> > Curious. Why do we even expect that this file may be broken?\n> \n> For our intended use case of reading the \"loose-object-idx\" file from\n> the quarantine directory and merging its entries to the mapping file in\n> the primary object directory, I wouldn't expect it to ever be broken.\n> That is because realistically only a \"loose-object-idx\" that we create\n> during the transaction should be written into the quarantine directory.\n> \n> I chose to be more defensive here because the `loose_object_idx_merge()`\n> function itself isn't aware of the circumstances regarding its use case.\n> In theory, another caller could use any mapping file as the source.\n> Ultimately though I don't feel too strongly either way and am happy to\n> remove it if we don't like it.\n\nAh. You mean because we're called with arbitrary paths we want to verify\nwhether our callers know what they're doing, and consequently we check\nwhether the files look like a mapping file? Still a bit funny, but with\nthat context it at least doesn't feel too unreasonable.\n\n> > > +\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n> > > +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> > > +\t\tgoto out;\n> > > +\t}\n> > \n> > With a timeout of -1 we'll block indefinitely. That doesn't feel like\n> > the best option to me, as the lockfile may be left over from a previous\n> > process. And Git locking up indefinitely feels suboptimal to me.\n> \n> That's fair. I picked this value purely because I was closely following\n> how `write_one_object()` implements its logic to update the\n> \"loose-object-idx\" file.\n> \n> Would it make sense to make this value configurable? Or would it be fine\n> to just set it to 1000ms? If we change this here, maybe we should also\n> change it for `write_one_object()` to match?\n\nYeah, I think that'd be a good idea.\n\n> > > +\tfd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);\n> > \n> > I don't think it's a good idea to append data under the lock. It may\n> > easily cause us to corrupt the file in case the write succeeds\n> > partially, only.\n> > \n> > Our usual pattern here is to create the lockfile, write all data into\n> > the new file and then rename it into place.\n> \n> So here I was also following how `write_one_object()` handles writing\n> its mapping entries. I was trying to keep the `loose_object_idx_merge()`\n> and `write_one_object()` approaches similar. If this really isn't the\n> write approach though, I can explore adapting the patch to follow our\n> more typical pattern.\n\nI think this is quite broken. Right now nobody would care because we\ndon't really use the compatibility hash algorithm yet. But once it\nstarts becoming useful this _will_ lead to corrupted files. So I'd\nprefer to fix it rather than doubling down on the broken pattern.\n\n> > I'm also a bit surprised that the file isn't sorted. Is a reader\n> > expected to always read _all_ mappings?\n> \n> I was surprised by this too. From my understanding, readers are not\n> expected to consult the \"loose-object-idx\" file directly, but instead\n> consult an in-memory hash map that is loaded from the file. When an\n> entry is not found in map, the file is reloaded to try to pick up any\n> missing entries.\n\nSure. But that'd still require a linear scan through all mappings in\nhere.\n\n> I see this a preexisting quirk though and am not sure it is something\n> that needs to be addressed as part of this change.\n\nYeah, I think it's fine to ignore this for now. This part here is\ncertainly outside the scope of the patch series.\n\n> > > +\tif (fd < 0) {\n> > > +\t\tret = error_errno(_(\"unable to open %s\"), dst);\n> > > +\t\tgoto out;\n> > > +\t}\n> > > +\tif (fstat(fd, &st) < 0) {\n> > > +\t\tret = error_errno(_(\"unable to stat %s\"), dst);\n> > > +\t\tgoto out;\n> > > +\t}\n> > > +\n> > > +\t/* Create the loose-object-idx if one does not already exist. */\n> > > +\tif (!st.st_size && write_in_full(fd, loose_object_header,\n> > > +\t\t\t\t\t strlen(loose_object_header)) < 0) {\n> > > +\t\tret = error_errno(_(\"failed to write loose object index header\"));\n> > > +\t\tgoto out;\n> > > +\t}\n> > > +\n> > > +\twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n> > > +\t\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n> > > +\t\t\tret = error_errno(_(\"failed to write loose object index %s\"),\n> > > +\t\t\t\t\t dst);\n> > > +\t\t\tgoto out;\n> > > +\t\t}\n> > > +\t}\n> > \n> > When merging, wouldn't we have to verify that the object IDs that we're\n> > adding don't already exist?\n> \n> So typically a loose object doesn't get written to the object directory\n> at all if it already exists and thus doesn't create a new mapping entry.\n> There is a race though where concurrent writes of the same object could\n> cause the same entry to be appended twice. This is already an existing\n> problem though as `write_one_object()` also doesn't verify whether the\n> object is already in the file.\n> \n> With this patch, I mostly avoided many of these preexisting quirks and\n> just focused on addressing the underlying bug. If we feel strongly about\n> it though, I can change the approach.\n\nI feel strongly that this mechanism is quite broken in its current form:\n\n - The file is unsorted, making lookups inefficient.\n\n - We don't deduplicate at all.\n\n - We never prune entries.\n\n - Writes are not atomic.\n\nThe first three are conceptual issues that we'd have to fix to ever make\nthis feature usable. But that certainly doesn't have to happen as part\nof this patch series.\n\nThe last part though shouldn't be too hard to fix, and you make the\nproblem worse because you add a second writer that uses the same broken\npattern. So that's a part I'd like to see fixd as part of this series,\nbut I won't insist on it.\n\nThanks!\n\nPatrick\n\n"},{"id":"556385","messageId":"at92UF0G-6R89NPm@denethor","threadId":"66637","inReplyTo":"at71hkqG7AbVCdnK@pks.im","subject":"Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-26T17:58:24Z","receivedAt":"2026-10-26T17:58:24Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/26 07:39AM, Patrick Steinhardt wrote:\n> On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:\n> > On 26/10/23 02:19PM, Patrick Steinhardt wrote:\n> > > On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:\n> > > > +\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n> > > > +\t\tret = error(_(\"invalid loose object index %s\"), src);\n> > > > +\t\tgoto out;\n> > > > +\t}\n> > > \n> > > Curious. Why do we even expect that this file may be broken?\n> > \n> > For our intended use case of reading the \"loose-object-idx\" file from\n> > the quarantine directory and merging its entries to the mapping file in\n> > the primary object directory, I wouldn't expect it to ever be broken.\n> > That is because realistically only a \"loose-object-idx\" that we create\n> > during the transaction should be written into the quarantine directory.\n> > \n> > I chose to be more defensive here because the `loose_object_idx_merge()`\n> > function itself isn't aware of the circumstances regarding its use case.\n> > In theory, another caller could use any mapping file as the source.\n> > Ultimately though I don't feel too strongly either way and am happy to\n> > remove it if we don't like it.\n> \n> Ah. You mean because we're called with arbitrary paths we want to verify\n> whether our callers know what they're doing, and consequently we check\n> whether the files look like a mapping file? Still a bit funny, but with\n> that context it at least doesn't feel too unreasonable.\n\nYour understanding is correct. This may be more defensive that\nrealistically neccessary. Probably doesn't matter too much either way\nthough.\n\n> > > > +\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n> > > > +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> > > > +\t\tgoto out;\n> > > > +\t}\n> > > \n> > > With a timeout of -1 we'll block indefinitely. That doesn't feel like\n> > > the best option to me, as the lockfile may be left over from a previous\n> > > process. And Git locking up indefinitely feels suboptimal to me.\n> > \n> > That's fair. I picked this value purely because I was closely following\n> > how `write_one_object()` implements its logic to update the\n> > \"loose-object-idx\" file.\n> > \n> > Would it make sense to make this value configurable? Or would it be fine\n> > to just set it to 1000ms? If we change this here, maybe we should also\n> > change it for `write_one_object()` to match?\n> \n> Yeah, I think that'd be a good idea.\n\nOk, I'll update to use 1000ms in the next version.\n\n[snip]\n> > > When merging, wouldn't we have to verify that the object IDs that we're\n> > > adding don't already exist?\n> > \n> > So typically a loose object doesn't get written to the object directory\n> > at all if it already exists and thus doesn't create a new mapping entry.\n> > There is a race though where concurrent writes of the same object could\n> > cause the same entry to be appended twice. This is already an existing\n> > problem though as `write_one_object()` also doesn't verify whether the\n> > object is already in the file.\n> > \n> > With this patch, I mostly avoided many of these preexisting quirks and\n> > just focused on addressing the underlying bug. If we feel strongly about\n> > it though, I can change the approach.\n> \n> I feel strongly that this mechanism is quite broken in its current form:\n> \n> - The file is unsorted, making lookups inefficient.\n> \n> - We don't deduplicate at all.\n> \n> - We never prune entries.\n> \n> - Writes are not atomic.\n\nYa, I completely agree. There are quite a few existing problems here.\n\n> The first three are conceptual issues that we'd have to fix to ever make\n> this feature usable. But that certainly doesn't have to happen as part\n> of this patch series.\n> \n> The last part though shouldn't be too hard to fix, and you make the\n> problem worse because you add a second writer that uses the same broken\n> pattern. So that's a part I'd like to see fixd as part of this series,\n> but I won't insist on it.\n\nThat's fair. I'll update in the next version to make writing the\n\"loose-object-idx\" file here atomic. I'm not entirely sure if I should\ngo ahead and update the code in `write_one_object()` to also be atomic\nthough.\n\nOne concern I have is operations that may write many loose objects,\nwould rewrite the \"loose-object-idx\" file for each object written. If\nthe mapping file becomes quite large, I suppose this could create a\nsignificant performance regression. If these loose object writes were\nbatched in a transaction, we could wait to write the final mapping file\nat transaction commit, but that may not always be the case.\n\nRight now this feature isn't really usable to begin with, so maybe it\ndoesn't matter too much either way. For now, I'll proceed with just\nupdating the mapping file writer adding in this patch to be atomic.\n\n-Justin\n\n"},{"id":"556716","messageId":"20261029002413.3291184-1-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261022234007.3081825-1-jltobler@gmail.com","subject":"[PATCH v2 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T00:24:09Z","receivedAt":"2026-10-29T00:24:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nWhile working on a separate patch series to convert the \"files\" ODB\ntransaction to be eagerly created instead of lazily, I encountered this\nbug that causes an ODB transaction to fail on commit when a\ncompatibility hash is also configured. This occurs because the temporary\ndirectory migration code does not know how to handle the loose object\nindex file.\n\nThis patch addresses updating the migration logic to explicitly merge\nmapping entries in the transaction and atomically write a new\n\"loose-object-idx\" file.\n\nSome other approaches I considered:\n - Storing the uncommitted object mapping entries in the transaction\n without writing a file and only writing on transaction commit. I\n chose to keep writing the temp index into the transaction so that\n child processes that write to the tempdir could still append entries.\n - Also writing mapping entires to the main ODB loose object index.\n This also didn't seem like a good apprach because it write entries\n that may not ultimately get committed.\n\nChanges since V1:\n - The loose object index write after the merging maps is now atomic.\n - Added a fixed timeout to all loose object index writers.\n - Duplicate entries are now dropped when writing the merged mapping file.\n\nThanks for the review,\n-Justin\n\nJustin Tobler (4):\n loose: use finite timeout when locking \"loose-object-idx\" file\n loose: lift loose object index parsing logic into helper\n loose: lift loose object index writing logic into helper\n tmp-objdir: merge loose object index when migrating tmpdir\n\n loose.c | 207 ++++++++++++++++++++++++----------\n loose.h | 2 +\n t/t1016-compatObjectFormat.sh | 27 +++++\n tmp-objdir.c | 11 +-\n 4 files changed, 189 insertions(+), 58 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: 6f104c482e loose: use finite timeout when locking \"loose-object-idx\" file\n-: ---------- > 2: 4799421a74 loose: lift loose object index parsing logic into helper\n-: ---------- > 3: 4233e913ce loose: lift loose object index writing logic into helper\n1: 34393e789c ! 4: 614aab27d1 tmp-objdir: merge loose object index when migrating tmpdir\n @@ Commit message\n \n When a repository is configured with a compatibility hash, each loose\n object write also records a mapping between the object and its\n - compatibility hash in a \"loose-object-idx\" file. This index is located\n - in the same object database the object is written to. If batch fsync of\n - loose objects is also enabled, the loose objects and index file are\n - instead written to a temporary object directory and expected to be\n - migrated to the primary object directory when the batch transaction\n + compatibility hash in a \"loose-object-idx\" file. This mapping file is\n + located in the same object directory the object is written to. If batch\n + fsync of loose objects is also enabled, the loose objects and mapping\n + file are instead written to a temporary object directory and expected to\n + be migrated to the primary object directory when the batch transaction\n commits.\n \n While migrating the loose objects works, the \"loose-object-idx\" file is\n - unable to be migrated if an index already exists in the primary object\n - directory. This occurs because `finalize_object_file_flags()` expects an\n - already existing file to have the same contents. Otherwise, the\n - following error is returned:\n + unable to be migrated if a mapping file already exists in the primary\n + object directory. This occurs because `finalize_object_file_flags()`\n + expects an already existing file to have the same contents. Otherwise,\n + the following error is returned:\n \n error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents\n error: unable to migrate temporary objects\n \n - Furthermore, it is not sufficient to just replace the preexisting index\n - with the new one as the new index only contains mapping entries for the\n - newly written objects.\n + Furthermore, it is not sufficient to just replace the preexisting\n + mapping file with the new one as the new file only contains mapping\n + entries for the newly written objects.\n \n Update the temporary directory migration logic to handle\n - \"loose-object-idx\" files explicitly by appending new mapping entries to\n - the primary index.\n + \"loose-object-idx\" files explicitly by merging new entries and\n + atomically writing the resulting mapping file.\n \n Signed-off-by: Justin Tobler \n \n @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)\n +\t\t\t const char *dst)\n +{\n +\tstruct lock_file lock = LOCK_INIT;\n -+\tstruct strbuf buf = STRBUF_INIT;\n -+\tstruct stat st;\n -+\tFILE *fp;\n -+\tint fd = -1;\n ++\tstruct loose_object_map *map;\n +\tint ret;\n ++\tint fd;\n +\n -+\tfp = fopen(src, \"r\");\n -+\tif (!fp) {\n -+\t\tret = error_errno(_(\"unable to open %s\"), src);\n -+\t\tgoto out;\n -+\t}\n -+\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n -+\t\tret = error(_(\"invalid loose object index %s\"), src);\n -+\t\tgoto out;\n -+\t}\n ++\tloose_object_map_init(&map);\n +\n -+\tif (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {\n ++\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n ++\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n ++\tif (fd < 0) {\n +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n +\t\tgoto out;\n +\t}\n +\n -+\tfd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);\n -+\tif (fd < 0) {\n -+\t\tret = error_errno(_(\"unable to open %s\"), dst);\n -+\t\tgoto out;\n -+\t}\n -+\tif (fstat(fd, &st) < 0) {\n -+\t\tret = error_errno(_(\"unable to stat %s\"), dst);\n ++\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n ++\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n +\t\tgoto out;\n +\t}\n +\n -+\t/* Create the loose-object-idx if one does not already exist. */\n -+\tif (!st.st_size && write_in_full(fd, loose_object_header,\n -+\t\t\t\t\t strlen(loose_object_header)) < 0) {\n -+\t\tret = error_errno(_(\"failed to write loose object index header\"));\n ++\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n ++\t\tret = error(_(\"unable to read loose object index %s\"), src);\n +\t\tgoto out;\n +\t}\n +\n -+\twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n -+\t\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n -+\t\t\tret = error_errno(_(\"failed to write loose object index %s\"),\n -+\t\t\t\t\t dst);\n -+\t\t\tgoto out;\n -+\t\t}\n ++\tif (loose_object_map_write(repo, map, fd) < 0) {\n ++\t\tret = error_errno(_(\"unable to write %s\"),\n ++\t\t\t\t get_lock_file_path(&lock));\n ++\t\tgoto out;\n +\t}\n +\n -+\tret = close(fd);\n -+\tfd = -1;\n -+\tif (ret) {\n -+\t\tret = error_errno(_(\"unable to close %s\"), dst);\n ++\tif (commit_lock_file(&lock) < 0) {\n ++\t\tret = error_errno(_(\"unable to write %s\"), dst);\n +\t\tgoto out;\n +\t}\n +\n -+\tadjust_shared_perm(repo, dst);\n -+\n +\tret = 0;\n +\n +out:\n ++\tloose_object_map_clear(&map);\n +\trollback_lock_file(&lock);\n -+\tstrbuf_release(&buf);\n -+\tif (fd >= 0)\n -+\t\tclose(fd);\n -+\tif (fp)\n -+\t\tfclose(fp);\n +\n +\treturn ret;\n +}\n @@ t/t1016-compatObjectFormat.sh: compare_oids GPG2 'commit' signedcommit4 \"$signed\n +\n +\t\t# Write some additional objects and ensure the corresponding\n +\t\t# temporary loose object index entries stored in the transaction\n -+\t\t# are merged into the primary ODB.\n ++\t\t# are merged into the primary object directory.\n +\t\techo bar >bar &&\n +\t\techo baz >baz &&\n +\t\tgit add bar baz &&\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556717","messageId":"20261029002413.3291184-2-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"[PATCH v2 1/4] loose: use finite timeout when locking \"loose-object-idx\" file","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T00:24:10Z","receivedAt":"2026-10-29T00:24:10Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Loose object index file writers first attempt to acquire a lock via\n`repo_hold_lock_file_for_update_timeout()` and specify a timeout value\nof -1. With a negative timeout value, the lock acquisition retries\nindefinitely while the lockfile still exists. If a process dies while\nholding the lock and leaves a stale lockfile behind, every subsequent\nwriter of the mapping file hangs forever instead of reporting an error.\n\nIntroduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms\nand use it for all callers that lock a \"loose-object-idx\" file. A value\nof 1000ms matches the default timeout used for \"core.packedRefsTimeout\"\nand should provide concurrent writers sufficient time to acquire the\nlock.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex d6a2bb5ba3..c6f4ee0ec6 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -14,6 +14,8 @@\n \n static const char *loose_object_header = \"# loose-object-idx\\n\";\n \n+#define LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS 1000\n+\n static inline int should_use_loose_object_map(struct repository *repo)\n {\n \treturn repo->compat_hash_algo && repo->gitdir;\n@@ -137,7 +139,8 @@ int repo_write_loose_object_map(struct repository *repo)\n \n \trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n \tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n-\t\t\t\t\t\t LOCK_DIE_ON_ERROR, -1);\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \titer = kh_begin(map);\n \tif (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)\n \t\tgoto errout;\n@@ -180,7 +183,8 @@ static int write_one_object(struct odb_source_loose *loose,\n \n \tstrbuf_addf(&path, \"%s/loose-object-idx\", loose->base.path);\n \trepo_hold_lock_file_for_update_timeout(loose->base.odb->repo, &lock,\n-\t\t\t\t\t path.buf, LOCK_DIE_ON_ERROR, -1);\n+\t\t\t\t\t path.buf, LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \n \tfd = open(path.buf, O_WRONLY | O_CREAT | O_APPEND, 0666);\n \tif (fd < 0)\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556718","messageId":"20261029002413.3291184-3-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"[PATCH v2 2/4] loose: lift loose object index parsing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T00:24:11Z","receivedAt":"2026-10-29T00:24:11Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Mapping entries from the \"loose-object-idx\" file are parsed via\n`loose_object_map_load()` and inserted into the in-memory loose object\nmap of the corresponding object source. In a subsequent commit, a\ntemporary loose object index file must also be parsed and have its\ncontents stored in a map that is not attached to any object source.\n\nExtract the logic for parsing a loose object index file into a separate\n`loose_object_map_read()` helper function that allows callers to\nexplicitly specify the map to insert the entries into. The\n`insert_loose_map()` helper function is also updated accordingly to\nexplicitly operate on the provided map and optional cache instead of the\nobject source.\n\nWhile at it, stop treating all errors related to opening the\n\"loose-object-idx\" file as success and instead only ignore ENOENT. It is\nexpected that a mapping file may not always be present, but other errors\nshould be signaled back to callers. Note that this slightly changes the\nbehavior of `repo_read_loose_object_map()`, which now stops loading the\nmapping files of subsequent object sources when one is unreadable, the\nsame way it already does when one is malformed.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 87 +++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 54 insertions(+), 33 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex c6f4ee0ec6..5f72c644f9 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const\n \treturn 1;\n }\n \n-static int insert_loose_map(struct odb_source_loose *loose,\n+static int insert_loose_map(struct loose_object_map *map,\n+\t\t\t struct oidtree *cache,\n \t\t\t const struct object_id *oid,\n \t\t\t const struct object_id *compat_oid)\n {\n-\tstruct loose_object_map *map = loose->map;\n \tint inserted = 0;\n \n \tinserted |= insert_oid_pair(map->to_compat, oid, compat_oid);\n \tinserted |= insert_oid_pair(map->to_storage, compat_oid, oid);\n-\tif (inserted)\n-\t\toidtree_insert(loose->cache, compat_oid, NULL);\n+\tif (inserted && cache)\n+\t\toidtree_insert(cache, compat_oid, NULL);\n \n \treturn inserted;\n }\n \n-int loose_object_map_load(struct odb_source_loose *loose)\n+static int loose_object_map_read(struct repository *repo, const char *path,\n+\t\t\t\t struct loose_object_map *map,\n+\t\t\t\t struct oidtree *cache)\n {\n-\tstruct repository *repo = loose->base.odb->repo;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *path;\n \tFILE *fp;\n-\tint ret = -1;\n-\n-\tif (!should_use_loose_object_map(repo))\n-\t\treturn 0;\n-\n-\tif (!loose->map)\n-\t\tloose_object_map_init(&loose->map);\n-\tif (!loose->cache) {\n-\t\tALLOC_ARRAY(loose->cache, 1);\n-\t\toidtree_init(loose->cache);\n-\t}\n-\n-\tinsert_loose_map(loose, repo->hash_algo->empty_tree, repo->compat_hash_algo->empty_tree);\n-\tinsert_loose_map(loose, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);\n-\tinsert_loose_map(loose, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);\n+\tint ret;\n \n-\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n \tfp = fopen(path, \"rb\");\n \tif (!fp) {\n-\t\tfree(path);\n-\t\treturn 0;\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn -1;\n \t}\n \n-\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header))\n-\t\tgoto err;\n+\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \twhile (!strbuf_getline_lf(&buf, fp)) {\n \t\tconst char *p;\n \t\tstruct object_id oid, compat_oid;\n \t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, repo->hash_algo) ||\n \t\t *p++ != ' ' ||\n \t\t parse_oid_hex_algop(p, &compat_oid, &p, repo->compat_hash_algo) ||\n-\t\t p != buf.buf + buf.len)\n-\t\t\tgoto err;\n-\t\tinsert_loose_map(loose, &oid, &compat_oid);\n+\t\t p != buf.buf + buf.len) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tinsert_loose_map(map, cache, &oid, &compat_oid);\n \t}\n \n \tret = ferror(fp) ? -1 : 0;\n-err:\n-\tfclose(fp);\n+\n+out:\n \tstrbuf_release(&buf);\n+\tfclose(fp);\n+\n+\treturn ret;\n+}\n+\n+int loose_object_map_load(struct odb_source_loose *loose)\n+{\n+\tstruct repository *repo = loose->base.odb->repo;\n+\tchar *path;\n+\tint ret;\n+\n+\tif (!should_use_loose_object_map(repo))\n+\t\treturn 0;\n+\n+\tif (!loose->map)\n+\t\tloose_object_map_init(&loose->map);\n+\tif (!loose->cache) {\n+\t\tALLOC_ARRAY(loose->cache, 1);\n+\t\toidtree_init(loose->cache);\n+\t}\n+\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_tree,\n+\t\t\t repo->compat_hash_algo->empty_tree);\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_blob,\n+\t\t\t repo->compat_hash_algo->empty_blob);\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->null_oid,\n+\t\t\t repo->compat_hash_algo->null_oid);\n+\n+\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n+\tret = loose_object_map_read(repo, path, loose->map, loose->cache);\n+\n \tfree(path);\n \treturn ret;\n }\n@@ -223,7 +244,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n \tif (!should_use_loose_object_map(loose->base.odb->repo))\n \t\treturn 0;\n \n-\tinserted = insert_loose_map(loose, oid, compat_oid);\n+\tinserted = insert_loose_map(loose->map, loose->cache, oid, compat_oid);\n \tif (inserted)\n \t\treturn write_one_object(loose, oid, compat_oid);\n \treturn 0;\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556719","messageId":"20261029002413.3291184-4-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"[PATCH v2 3/4] loose: lift loose object index writing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T00:24:12Z","receivedAt":"2026-10-29T00:24:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Writing the in-memory loose object map to a \"loose-object-idx\" file is\nhandled via `repo_write_loose_object_map()`, which always writes the map\ninto the primary object directory. In a subsequent commit, a separate\n\"loose-object-idx\" file is staged prior to commit and must be\nconstructed from a loose object map not attached to the primary loose\nobject source.\n\nExtract the logic for writing a loose object index file into a separate\n`loose_object_map_write()` helper function that allows callers to\nexplicitly specify both the loose object map as well as the file\ndescriptor to write the entries to.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 69 +++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 45 insertions(+), 24 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex 5f72c644f9..f2e111c25e 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -146,25 +146,17 @@ int repo_read_loose_object_map(struct repository *repo)\n \treturn 0;\n }\n \n-int repo_write_loose_object_map(struct repository *repo)\n+static int loose_object_map_write(struct repository *repo,\n+\t\t\t\t struct loose_object_map *loose_map, int fd)\n {\n-\tstruct odb_source_files *files = odb_source_files_downcast(repo->objects->source);\n-\tkh_oid_map_t *map = files->dirs->loose->map->to_compat;\n-\tstruct lock_file lock;\n-\tint fd;\n+\tkh_oid_map_t *map = loose_map->to_compat;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tkhiter_t iter;\n-\tstruct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;\n-\n-\tif (!should_use_loose_object_map(repo))\n-\t\treturn 0;\n+\tint ret = -1;\n \n-\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n-\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n-\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n-\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \titer = kh_begin(map);\n \tif (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)\n-\t\tgoto errout;\n+\t\tgoto out;\n \n \tfor (; iter != kh_end(map); iter++) {\n \t\tif (kh_exist(map, iter)) {\n@@ -173,24 +165,53 @@ int repo_write_loose_object_map(struct repository *repo)\n \t\t\t\tcontinue;\n \t\t\tstrbuf_addf(&buf, \"%s %s\\n\", oid_to_hex(&kh_key(map, iter)), oid_to_hex(kh_value(map, iter)));\n \t\t\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\t\t\tgoto errout;\n+\t\t\t\tgoto out;\n \t\t\tstrbuf_reset(&buf);\n \t\t}\n \t}\n+\n+\tret = 0;\n+\n+out:\n \tstrbuf_release(&buf);\n+\n+\treturn ret;\n+}\n+\n+int repo_write_loose_object_map(struct repository *repo)\n+{\n+\tstruct odb_source_files *files = odb_source_files_downcast(repo->objects->source);\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct lock_file lock;\n+\tint ret;\n+\tint fd;\n+\n+\tif (!should_use_loose_object_map(repo))\n+\t\treturn 0;\n+\n+\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n+\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n+\n+\tif (loose_object_map_write(repo, files->dirs->loose->map, fd) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"),\n+\t\t\t\t get_lock_file_path(&lock));\n+\t\tgoto out;\n+\t}\n+\n \tif (commit_lock_file(&lock) < 0) {\n-\t\terror_errno(_(\"could not write loose object index %s\"), path.buf);\n-\t\tstrbuf_release(&path);\n-\t\treturn -1;\n+\t\tret = error_errno(_(\"unable to write %s\"), path.buf);\n+\t\tgoto out;\n \t}\n-\tstrbuf_release(&path);\n-\treturn 0;\n-errout:\n+\n+\tret = 0;\n+\n+out:\n \trollback_lock_file(&lock);\n-\tstrbuf_release(&buf);\n-\terror_errno(_(\"failed to write loose object index %s\"), path.buf);\n \tstrbuf_release(&path);\n-\treturn -1;\n+\n+\treturn ret;\n }\n \n static int write_one_object(struct odb_source_loose *loose,\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556720","messageId":"20261029002413.3291184-5-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"[PATCH v2 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T00:24:13Z","receivedAt":"2026-10-29T00:24:13Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When a repository is configured with a compatibility hash, each loose\nobject write also records a mapping between the object and its\ncompatibility hash in a \"loose-object-idx\" file. This mapping file is\nlocated in the same object directory the object is written to. If batch\nfsync of loose objects is also enabled, the loose objects and mapping\nfile are instead written to a temporary object directory and expected to\nbe migrated to the primary object directory when the batch transaction\ncommits.\n\nWhile migrating the loose objects works, the \"loose-object-idx\" file is\nunable to be migrated if a mapping file already exists in the primary\nobject directory. This occurs because `finalize_object_file_flags()`\nexpects an already existing file to have the same contents. Otherwise,\nthe following error is returned:\n\n error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents\n error: unable to migrate temporary objects\n\nFurthermore, it is not sufficient to just replace the preexisting\nmapping file with the new one as the new file only contains mapping\nentries for the newly written objects.\n\nUpdate the temporary directory migration logic to handle\n\"loose-object-idx\" files explicitly by merging new entries and\natomically writing the resulting mapping file.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 47 +++++++++++++++++++++++++++++++++++\n loose.h | 2 ++\n t/t1016-compatObjectFormat.sh | 27 ++++++++++++++++++++\n tmp-objdir.c | 11 +++++++-\n 4 files changed, 86 insertions(+), 1 deletion(-)\n\ndiff --git a/loose.c b/loose.c\nindex f2e111c25e..36da63684f 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)\n \tfree(m);\n \t*map = NULL;\n }\n+\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst)\n+{\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct loose_object_map *map;\n+\tint ret;\n+\tint fd;\n+\n+\tloose_object_map_init(&map);\n+\n+\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n+\tif (fd < 0) {\n+\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n+\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n+\t\tret = error(_(\"unable to read loose object index %s\"), src);\n+\t\tgoto out;\n+\t}\n+\n+\tif (loose_object_map_write(repo, map, fd) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"),\n+\t\t\t\t get_lock_file_path(&lock));\n+\t\tgoto out;\n+\t}\n+\n+\tif (commit_lock_file(&lock) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tret = 0;\n+\n+out:\n+\tloose_object_map_clear(&map);\n+\trollback_lock_file(&lock);\n+\n+\treturn ret;\n+}\ndiff --git a/loose.h b/loose.h\nindex ed663ac550..35b46f4be5 100644\n--- a/loose.h\n+++ b/loose.h\n@@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n \t\t\t const struct object_id *compat_oid);\n int repo_read_loose_object_map(struct repository *repo);\n int repo_write_loose_object_map(struct repository *repo);\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst);\n \n #endif\ndiff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh\nindex 9cafcee509..6d0f48226f 100755\n--- a/t/t1016-compatObjectFormat.sh\n+++ b/t/t1016-compatObjectFormat.sh\n@@ -305,4 +305,31 @@ compare_oids GPG2 'commit' signedcommit4 \"$signedcommit4_sha1_oid\" \"$signedcommi\n compare_oids GPG2 'tag' signedtag3 \"$signedtag3_sha1_oid\" \"$signedtag3_sha256_oid\"\n compare_oids GPG2 'tag' signedtag4 \"$signedtag4_sha1_oid\" \"$signedtag4_sha256_oid\"\n \n+test_expect_success 'batched fsync migrates loose object index' '\n+\ttest_when_finished \"rm -rf batch-fsync\" &&\n+\tgit init --object-format=sha1 batch-fsync &&\n+\t(\n+\t\tcd batch-fsync &&\n+\t\tgit config core.repositoryFormatVersion 1 &&\n+\t\tgit config extensions.compatObjectFormat sha256 &&\n+\t\tgit config core.fsync loose-object &&\n+\t\tgit config core.fsyncMethod batch &&\n+\n+\t\t# Write object to generate initial loose object index.\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\n+\t\t# Write some additional objects and ensure the corresponding\n+\t\t# temporary loose object index entries stored in the transaction\n+\t\t# are merged into the primary object directory.\n+\t\techo bar >bar &&\n+\t\techo baz >baz &&\n+\t\tgit add bar baz &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :bar)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :baz)\" .git/objects/loose-object-idx\n+\t)\n+'\n+\n test_done\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 3debb60270..5307717354 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -4,6 +4,7 @@\n #include \"chdir-notify.h\"\n #include \"dir.h\"\n #include \"environment.h\"\n+#include \"loose.h\"\n #include \"object-file.h\"\n #include \"path.h\"\n #include \"string-list.h\"\n@@ -294,6 +295,11 @@ static int is_loose_object_shard(const char *name)\n \treturn strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);\n }\n \n+static int is_loose_object_idx(const char *name)\n+{\n+\treturn !strcmp(name, \"loose-object-idx\");\n+}\n+\n static int migrate_paths(struct tmp_objdir *t,\n \t\t\t struct strbuf *src, struct strbuf *dst,\n \t\t\t enum finalize_object_file_flags flags)\n@@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t,\n \t\tif (is_loose_object_shard(name))\n \t\t\tflags_copy |= FOF_SKIP_COLLISION_CHECK;\n \n-\t\tret |= migrate_one(t, src, dst, flags_copy);\n+\t\tif (is_loose_object_idx(name))\n+\t\t\tret |= loose_object_idx_merge(t->repo, src->buf, dst->buf);\n+\t\telse\n+\t\t\tret |= migrate_one(t, src, dst, flags_copy);\n \n \t\tstrbuf_setlen(src, src_len);\n \t\tstrbuf_setlen(dst, dst_len);\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556732","messageId":"auL-Ta8DTicSLW7W@pks.im","threadId":"66637","inReplyTo":"20261029002413.3291184-2-jltobler@gmail.com","subject":"Re: [PATCH v2 1/4] loose: use finite timeout when locking \"loose-object-idx\" file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-29T08:05:42Z","receivedAt":"2026-10-29T08:05:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 28, 2026 at 07:24:10PM -0500, Justin Tobler wrote:\n> Loose object index file writers first attempt to acquire a lock via\n> `repo_hold_lock_file_for_update_timeout()` and specify a timeout value\n> of -1. With a negative timeout value, the lock acquisition retries\n> indefinitely while the lockfile still exists. If a process dies while\n> holding the lock and leaves a stale lockfile behind, every subsequent\n> writer of the mapping file hangs forever instead of reporting an error.\n> \n> Introduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms\n> and use it for all callers that lock a \"loose-object-idx\" file. A value\n> of 1000ms matches the default timeout used for \"core.packedRefsTimeout\"\n> and should provide concurrent writers sufficient time to acquire the\n> lock.\n\nRight, aborting eventually is way better compared to blocking\nindefinitely. Ideally we should also make this setting configurable. But\nI think it's fine to skip that for now and fix that at a later point in\ntime.\n\nPatrick\n\n"},{"id":"556733","messageId":"auL-W9Z2H9Ivn1RC@pks.im","threadId":"66637","inReplyTo":"20261029002413.3291184-3-jltobler@gmail.com","subject":"Re: [PATCH v2 2/4] loose: lift loose object index parsing logic into helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-29T08:05:47Z","receivedAt":"2026-10-29T08:05:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 28, 2026 at 07:24:11PM -0500, Justin Tobler wrote:\n> diff --git a/loose.c b/loose.c\n> index c6f4ee0ec6..5f72c644f9 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const\n> \treturn 1;\n> }\n> \n> -static int insert_loose_map(struct odb_source_loose *loose,\n> +static int insert_loose_map(struct loose_object_map *map,\n> +\t\t\t struct oidtree *cache,\n> \t\t\t const struct object_id *oid,\n> \t\t\t const struct object_id *compat_oid)\n> {\n> -\tstruct loose_object_map *map = loose->map;\n> \tint inserted = 0;\n> \n> \tinserted |= insert_oid_pair(map->to_compat, oid, compat_oid);\n> \tinserted |= insert_oid_pair(map->to_storage, compat_oid, oid);\n> -\tif (inserted)\n> -\t\toidtree_insert(loose->cache, compat_oid, NULL);\n> +\tif (inserted && cache)\n> +\t\toidtree_insert(cache, compat_oid, NULL);\n\nOkay, so the `cache` is optional now. You mention that fact in the\ncommit message, but you don't explain why that is. I assume a subsequent\ncommit will make use of this.\n\n> \treturn inserted;\n> }\n> \n> -int loose_object_map_load(struct odb_source_loose *loose)\n> +static int loose_object_map_read(struct repository *repo, const char *path,\n> +\t\t\t\t struct loose_object_map *map,\n> +\t\t\t\t struct oidtree *cache)\n> {\n> -\tstruct repository *repo = loose->base.odb->repo;\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tchar *path;\n> \tFILE *fp;\n> -\tint ret = -1;\n> -\n> -\tif (!should_use_loose_object_map(repo))\n> -\t\treturn 0;\n> -\n> -\tif (!loose->map)\n> -\t\tloose_object_map_init(&loose->map);\n> -\tif (!loose->cache) {\n> -\t\tALLOC_ARRAY(loose->cache, 1);\n> -\t\toidtree_init(loose->cache);\n> -\t}\n> -\n> -\tinsert_loose_map(loose, repo->hash_algo->empty_tree, repo->compat_hash_algo->empty_tree);\n> -\tinsert_loose_map(loose, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);\n> -\tinsert_loose_map(loose, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);\n\nOkay, all of this stays in `loose_object_map_load()` further down.\n\n> +\tint ret;\n> \n> -\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n> \tfp = fopen(path, \"rb\");\n> \tif (!fp) {\n> -\t\tfree(path);\n> -\t\treturn 0;\n> +\t\tif (errno == ENOENT)\n> +\t\t\treturn 0;\n> +\t\treturn -1;\n> \t}\n> \n> -\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header))\n> -\t\tgoto err;\n> +\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n> +\t\tret = -1;\n> +\t\tgoto out;\n> +\t}\n\nIdeally we'd of course print an error message telling the user what's\nhappened. But we didn't before either, so this is fine as-is.\n\n> \twhile (!strbuf_getline_lf(&buf, fp)) {\n> \t\tconst char *p;\n> \t\tstruct object_id oid, compat_oid;\n> \t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, repo->hash_algo) ||\n> \t\t *p++ != ' ' ||\n> \t\t parse_oid_hex_algop(p, &compat_oid, &p, repo->compat_hash_algo) ||\n> -\t\t p != buf.buf + buf.len)\n> -\t\t\tgoto err;\n> -\t\tinsert_loose_map(loose, &oid, &compat_oid);\n> +\t\t p != buf.buf + buf.len) {\n> +\t\t\tret = -1;\n> +\t\t\tgoto out;\n> +\t\t}\n> +\t\tinsert_loose_map(map, cache, &oid, &compat_oid);\n> \t}\n> \n> \tret = ferror(fp) ? -1 : 0;\n> -err:\n> -\tfclose(fp);\n> +\n> +out:\n> \tstrbuf_release(&buf);\n> +\tfclose(fp);\n> +\n> +\treturn ret;\n> +}\n> +\n> +int loose_object_map_load(struct odb_source_loose *loose)\n> +{\n> +\tstruct repository *repo = loose->base.odb->repo;\n> +\tchar *path;\n> +\tint ret;\n> +\n> +\tif (!should_use_loose_object_map(repo))\n> +\t\treturn 0;\n> +\n> +\tif (!loose->map)\n> +\t\tloose_object_map_init(&loose->map);\n> +\tif (!loose->cache) {\n> +\t\tALLOC_ARRAY(loose->cache, 1);\n> +\t\toidtree_init(loose->cache);\n> +\t}\n> +\n> +\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_tree,\n> +\t\t\t repo->compat_hash_algo->empty_tree);\n> +\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_blob,\n> +\t\t\t repo->compat_hash_algo->empty_blob);\n> +\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->null_oid,\n> +\t\t\t repo->compat_hash_algo->null_oid);\n> +\n> +\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n> +\tret = loose_object_map_read(repo, path, loose->map, loose->cache);\n> +\n> \tfree(path);\n> \treturn ret;\n> }\n\nRight, and this here is the original function that we had with the\nremaining bits that are specific to the source.\n\nPatrick\n\n"},{"id":"556734","messageId":"auL-YMoeDgx3wEv0@pks.im","threadId":"66637","inReplyTo":"20261029002413.3291184-4-jltobler@gmail.com","subject":"Re: [PATCH v2 3/4] loose: lift loose object index writing logic into helper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-29T08:05:52Z","receivedAt":"2026-10-29T08:05:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 28, 2026 at 07:24:12PM -0500, Justin Tobler wrote:\n> Writing the in-memory loose object map to a \"loose-object-idx\" file is\n> handled via `repo_write_loose_object_map()`, which always writes the map\n> into the primary object directory. In a subsequent commit, a separate\n> \"loose-object-idx\" file is staged prior to commit and must be\n> constructed from a loose object map not attached to the primary loose\n> object source.\n> \n> Extract the logic for writing a loose object index file into a separate\n> `loose_object_map_write()` helper function that allows callers to\n> explicitly specify both the loose object map as well as the file\n> descriptor to write the entries to.\n\nOkay. This commit is essentially the equivalent to the preceding one,\nbut for writes.\n\n> diff --git a/loose.c b/loose.c\n> index 5f72c644f9..f2e111c25e 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -146,25 +146,17 @@ int repo_read_loose_object_map(struct repository *repo)\n> \treturn 0;\n> }\n> \n> -int repo_write_loose_object_map(struct repository *repo)\n> +static int loose_object_map_write(struct repository *repo,\n> +\t\t\t\t struct loose_object_map *loose_map, int fd)\n\nCurious that you're using a file descriptor instead of a path as you did\nin the preceding commit. But I guess that's because of how we first have\nto lock the file, which we of course don't have to do on the writing\nside.\n\nI guess we could've also moved locking into this function here to make\nboth interfaces match, but maybe the next commit will give a hint why we\ndon't.\n\nPatrick\n\n"},{"id":"556735","messageId":"auL-ZBontaECu8vo@pks.im","threadId":"66637","inReplyTo":"20261029002413.3291184-5-jltobler@gmail.com","subject":"Re: [PATCH v2 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-29T08:05:56Z","receivedAt":"2026-10-29T08:05:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 28, 2026 at 07:24:13PM -0500, Justin Tobler wrote:\n> diff --git a/loose.c b/loose.c\n> index f2e111c25e..36da63684f 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)\n> \tfree(m);\n> \t*map = NULL;\n> }\n> +\n> +int loose_object_idx_merge(struct repository *repo, const char *src,\n> +\t\t\t const char *dst)\n> +{\n> +\tstruct lock_file lock = LOCK_INIT;\n> +\tstruct loose_object_map *map;\n> +\tint ret;\n> +\tint fd;\n> +\n> +\tloose_object_map_init(&map);\n> +\n> +\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n> +\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n> +\tif (fd < 0) {\n> +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n\nOkay, so this explains why you didn't want to pass a path to\n`loose_object_map_write()`: we need the lock to span across multiple\nother operations. Makes sense.\n\n> +\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n> +\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n> +\t\tret = error(_(\"unable to read loose object index %s\"), src);\n> +\t\tgoto out;\n> +\t}\n\nIt makes sense that we read `dst` under the lock so that we don't risk\nlosing data that a concurrent process writes into it. But to minimize\nthe critical section, can't we move `src` out of it and read it before\nwe lock?\n\n> diff --git a/loose.h b/loose.h\n> index ed663ac550..35b46f4be5 100644\n> --- a/loose.h\n> +++ b/loose.h\n> @@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n> \t\t\t const struct object_id *compat_oid);\n> int repo_read_loose_object_map(struct repository *repo);\n> int repo_write_loose_object_map(struct repository *repo);\n> +int loose_object_idx_merge(struct repository *repo, const char *src,\n> +\t\t\t const char *dst);\n\nDo we maybe want to add a comment to this new function?\n\nThanks!\n\nPatrick\n\n"},{"id":"556831","messageId":"auO2H-RBiRB_vwVl@denethor","threadId":"66637","inReplyTo":"auL-W9Z2H9Ivn1RC@pks.im","subject":"Re: [PATCH v2 2/4] loose: lift loose object index parsing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T21:10:48Z","receivedAt":"2026-10-29T21:10:48Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/29 09:05AM, Patrick Steinhardt wrote:\n> On Wed, Oct 28, 2026 at 07:24:11PM -0500, Justin Tobler wrote:\n> > -\tif (inserted)\n> > -\t\toidtree_insert(loose->cache, compat_oid, NULL);\n> > +\tif (inserted && cache)\n> > +\t\toidtree_insert(cache, compat_oid, NULL);\n> \n> Okay, so the `cache` is optional now. You mention that fact in the\n> commit message, but you don't explain why that is. I assume a subsequent\n> commit will make use of this.\n\nYes, this is used in the final patch. I'll update the commit message to\nmake this a bit more clear.\n\n-Justin\n\n"},{"id":"556832","messageId":"auO2XTTAz5RNlhvG@denethor","threadId":"66637","inReplyTo":"auL-YMoeDgx3wEv0@pks.im","subject":"Re: [PATCH v2 3/4] loose: lift loose object index writing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T21:18:51Z","receivedAt":"2026-10-29T21:18:51Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/29 09:05AM, Patrick Steinhardt wrote:\n> On Wed, Oct 28, 2026 at 07:24:12PM -0500, Justin Tobler wrote:\n> > -int repo_write_loose_object_map(struct repository *repo)\n> > +static int loose_object_map_write(struct repository *repo,\n> > +\t\t\t\t struct loose_object_map *loose_map, int fd)\n> \n> Curious that you're using a file descriptor instead of a path as you did\n> in the preceding commit. But I guess that's because of how we first have\n> to lock the file, which we of course don't have to do on the writing\n> side.\n\nThat is correct. When we lock the file, it gives us back the file\ndescriptor back for the lockfile. I decided to use the file descriptor\nhere because it was readily available to pass around.\n\n> I guess we could've also moved locking into this function here to make\n> both interfaces match, but maybe the next commit will give a hint why we\n> don't.\n\nYup, this becomes more apparent in the next commit, but we don't want to\nacquire the lock in the helper, so that callers can lock the file prior\nto reading any preexisting version to get a consistent view. I may leave\na note about this in the next versions commit message.\n\n-Justin\n\n"},{"id":"556833","messageId":"auO4Photxmne1R5Z@denethor","threadId":"66637","inReplyTo":"auL-ZBontaECu8vo@pks.im","subject":"Re: [PATCH v2 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T21:20:35Z","receivedAt":"2026-10-29T21:20:35Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/29 09:05AM, Patrick Steinhardt wrote:\n> On Wed, Oct 28, 2026 at 07:24:13PM -0500, Justin Tobler wrote:\n> > +\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n> > +\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n> > +\t\tgoto out;\n> > +\t}\n> > +\n> > +\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n> > +\t\tret = error(_(\"unable to read loose object index %s\"), src);\n> > +\t\tgoto out;\n> > +\t}\n> \n> It makes sense that we read `dst` under the lock so that we don't risk\n> losing data that a concurrent process writes into it. But to minimize\n> the critical section, can't we move `src` out of it and read it before\n> we lock?\n\nYa good point. The src file can be read prior to acquiring the lock.\nWill update.\n\n> > diff --git a/loose.h b/loose.h\n> > index ed663ac550..35b46f4be5 100644\n> > --- a/loose.h\n> > +++ b/loose.h\n> > @@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n> > \t\t\t const struct object_id *compat_oid);\n> > int repo_read_loose_object_map(struct repository *repo);\n> > int repo_write_loose_object_map(struct repository *repo);\n> > +int loose_object_idx_merge(struct repository *repo, const char *src,\n> > +\t\t\t const char *dst);\n> \n> Do we maybe want to add a comment to this new function?\n\nWill do!\n\nThanks,\n-Justin\n\n"},{"id":"556842","messageId":"xmqq8q3gywny.fsf@gitster.g","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"Re: [PATCH v2 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-29T22:22:25Z","receivedAt":"2026-10-29T22:22:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> Greetings,\n>\n> While working on a separate patch series to convert the \"files\" ODB\n> transaction to be eagerly created instead of lazily, I encountered this\n> bug that causes an ODB transaction to fail on commit when a\n> compatibility hash is also configured. This occurs because the temporary\n> directory migration code does not know how to handle the loose object\n> index file.\n>\n> This patch addresses updating the migration logic to explicitly merge\n> mapping entries in the transaction and atomically write a new\n> \"loose-object-idx\" file.\n>\n> Some other approaches I considered:\n> - Storing the uncommitted object mapping entries in the transaction\n> without writing a file and only writing on transaction commit. I\n> chose to keep writing the temp index into the transaction so that\n> child processes that write to the tempdir could still append entries.\n> - Also writing mapping entires to the main ODB loose object index.\n> This also didn't seem like a good apprach because it write entries\n> that may not ultimately get committed.\n>\n> Changes since V1:\n> - The loose object index write after the merging maps is now atomic.\n> - Added a fixed timeout to all loose object index writers.\n> - Duplicate entries are now dropped when writing the merged mapping file.\n>\n> Thanks for the review,\n> -Justin\n\nThanks.\n\nMerging this topic to 'seen' had a funny interaction with other\ntopics in flight. repo_write_loose_object_map() function, from\nwhich loose_object_map_write() helper function was extracted out of,\nloses all callers, and will disappear in the merge. But the new\nhelper, which gained a caller loose_object_idx_merge() in this\nseries, remains.\n\n"},{"id":"556843","messageId":"auPKJ-hUt22YaeyW@denethor","threadId":"66637","inReplyTo":"xmqq8q3gywny.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T22:45:54Z","receivedAt":"2026-10-29T22:45:54Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/29 03:22PM, Junio C Hamano wrote:\n> Merging this topic to 'seen' had a funny interaction with other\n> topics in flight. repo_write_loose_object_map() function, from\n> which loose_object_map_write() helper function was extracted out of,\n> loses all callers, and will disappear in the merge. But the new\n> helper, which gained a caller loose_object_idx_merge() in this\n> series, remains.\n\nThanks for the heads up. I see the function gets dropped by de2910a42b\n(loose: drop unused `repo_write_loose_object_map()`, 2026-10-27).\nKeeping the new helper and dropping `repo_write_loose_object_map()` is\nindeed the correct resolution.\n\nIt does look like in `seen` the declaration of\n`repo_write_loose_object_map()` is still left behind in \"loose.h\"\nthough.\n\nI have a few small changes queued up locally for a v3, which I will send\nout soon.\n\nThanks,\n-Justin\n\n"},{"id":"556846","messageId":"20261029232220.1245827-1-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029002413.3291184-1-jltobler@gmail.com","subject":"[PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T23:22:16Z","receivedAt":"2026-10-29T23:22:16Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nWhile working on a separate patch series to convert the \"files\" ODB\ntransaction to be eagerly created instead of lazily, I encountered this\nbug that causes an ODB transaction to fail on commit when a\ncompatibility hash is also configured. This occurs because the temporary\ndirectory migration code does not know how to handle the loose object\nindex file.\n\nThis patch addresses updating the migration logic to explicitly merge\nmapping entries in the transaction and atomically write a new\n\"loose-object-idx\" file.\n\nSome other approaches I considered:\n - Storing the uncommitted object mapping entries in the transaction\n without writing a file and only writing on transaction commit. I\n chose to keep writing the temp index into the transaction so that\n child processes that write to the tempdir could still append entries.\n - Also writing mapping entires to the main ODB loose object index.\n This also didn't seem like a good apprach because it write entries\n that may not ultimately get committed.\n\nChanges since V2:\n - Reading the src mapping file is now done prior to acquiring the lock\n to reduce the critical window.\n - Added a comment to `loose_object_idx_merge()` and updated some\n commit messages to improve clarity.\n\nChanges since V1:\n - The loose object index write after the merging maps is now atomic.\n - Added a fixed timeout to all loose object index writers.\n - Duplicate entries are now dropped when writing the merged mapping file.\n\nThanks for the review,\n-Justin\n\nJustin Tobler (4):\n loose: use finite timeout when locking \"loose-object-idx\" file\n loose: lift loose object index parsing logic into helper\n loose: lift loose object index writing logic into helper\n tmp-objdir: merge loose object index when migrating tmpdir\n\n loose.c | 207 ++++++++++++++++++++++++----------\n loose.h | 9 ++\n t/t1016-compatObjectFormat.sh | 27 +++++\n tmp-objdir.c | 11 +-\n 4 files changed, 196 insertions(+), 58 deletions(-)\n\nRange-diff against v2:\n1: 6f104c482e = 1: 6f104c482e loose: use finite timeout when locking \"loose-object-idx\" file\n2: 4799421a74 ! 2: 3ae068f364 loose: lift loose object index parsing logic into helper\n @@ Commit message\n explicitly specify the map to insert the entries into. The\n `insert_loose_map()` helper function is also updated accordingly to\n explicitly operate on the provided map and optional cache instead of the\n - object source.\n + object source. The cache is made optional for future callers that may\n + only be interested in updating the map.\n \n While at it, stop treating all errors related to opening the\n \"loose-object-idx\" file as success and instead only ignore ENOENT. It is\n3: 4233e913ce ! 3: 0a05eb5fb6 loose: lift loose object index writing logic into helper\n @@ Commit message\n Extract the logic for writing a loose object index file into a separate\n `loose_object_map_write()` helper function that allows callers to\n explicitly specify both the loose object map as well as the file\n - descriptor to write the entries to.\n + descriptor to write the entries to. Note that the responsibility of\n + locking the file being written to is left to callers so that it is\n + possible for a lock to be held for longer than just this single\n + operation.\n \n Signed-off-by: Justin Tobler \n \n4: 614aab27d1 ! 4: 3c4a3332a5 tmp-objdir: merge loose object index when migrating tmpdir\n @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)\n +\n +\tloose_object_map_init(&map);\n +\n ++\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n ++\t\tret = error(_(\"unable to read loose object index %s\"), src);\n ++\t\tgoto out;\n ++\t}\n ++\n +\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n +\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n +\tif (fd < 0) {\n @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)\n +\t\tgoto out;\n +\t}\n +\n -+\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n -+\t\tret = error(_(\"unable to read loose object index %s\"), src);\n -+\t\tgoto out;\n -+\t}\n -+\n +\tif (loose_object_map_write(repo, map, fd) < 0) {\n +\t\tret = error_errno(_(\"unable to write %s\"),\n +\t\t\t\t get_lock_file_path(&lock));\n @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)\n \n ## loose.h ##\n @@ loose.h: int repo_add_loose_object_map(struct odb_source_loose *loose,\n - \t\t\t const struct object_id *compat_oid);\n int repo_read_loose_object_map(struct repository *repo);\n int repo_write_loose_object_map(struct repository *repo);\n + \n ++/*\n ++ * Merge the mapping entries of the loose object index at `src` into the one at\n ++ * `dst`. The resulting mapping file is written atomically.\n ++ *\n ++ * Returns 0 on success, a negative error code otherwise.\n ++ */\n +int loose_object_idx_merge(struct repository *repo, const char *src,\n +\t\t\t const char *dst);\n - \n ++\n #endif\n \n ## t/t1016-compatObjectFormat.sh ##\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556847","messageId":"20261029232220.1245827-2-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"[PATCH v3 1/4] loose: use finite timeout when locking \"loose-object-idx\" file","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T23:22:17Z","receivedAt":"2026-10-29T23:22:17Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Loose object index file writers first attempt to acquire a lock via\n`repo_hold_lock_file_for_update_timeout()` and specify a timeout value\nof -1. With a negative timeout value, the lock acquisition retries\nindefinitely while the lockfile still exists. If a process dies while\nholding the lock and leaves a stale lockfile behind, every subsequent\nwriter of the mapping file hangs forever instead of reporting an error.\n\nIntroduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms\nand use it for all callers that lock a \"loose-object-idx\" file. A value\nof 1000ms matches the default timeout used for \"core.packedRefsTimeout\"\nand should provide concurrent writers sufficient time to acquire the\nlock.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex d6a2bb5ba3..c6f4ee0ec6 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -14,6 +14,8 @@\n \n static const char *loose_object_header = \"# loose-object-idx\\n\";\n \n+#define LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS 1000\n+\n static inline int should_use_loose_object_map(struct repository *repo)\n {\n \treturn repo->compat_hash_algo && repo->gitdir;\n@@ -137,7 +139,8 @@ int repo_write_loose_object_map(struct repository *repo)\n \n \trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n \tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n-\t\t\t\t\t\t LOCK_DIE_ON_ERROR, -1);\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \titer = kh_begin(map);\n \tif (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)\n \t\tgoto errout;\n@@ -180,7 +183,8 @@ static int write_one_object(struct odb_source_loose *loose,\n \n \tstrbuf_addf(&path, \"%s/loose-object-idx\", loose->base.path);\n \trepo_hold_lock_file_for_update_timeout(loose->base.odb->repo, &lock,\n-\t\t\t\t\t path.buf, LOCK_DIE_ON_ERROR, -1);\n+\t\t\t\t\t path.buf, LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \n \tfd = open(path.buf, O_WRONLY | O_CREAT | O_APPEND, 0666);\n \tif (fd < 0)\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556848","messageId":"20261029232220.1245827-3-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"[PATCH v3 2/4] loose: lift loose object index parsing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T23:22:18Z","receivedAt":"2026-10-29T23:22:18Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Mapping entries from the \"loose-object-idx\" file are parsed via\n`loose_object_map_load()` and inserted into the in-memory loose object\nmap of the corresponding object source. In a subsequent commit, a\ntemporary loose object index file must also be parsed and have its\ncontents stored in a map that is not attached to any object source.\n\nExtract the logic for parsing a loose object index file into a separate\n`loose_object_map_read()` helper function that allows callers to\nexplicitly specify the map to insert the entries into. The\n`insert_loose_map()` helper function is also updated accordingly to\nexplicitly operate on the provided map and optional cache instead of the\nobject source. The cache is made optional for future callers that may\nonly be interested in updating the map.\n\nWhile at it, stop treating all errors related to opening the\n\"loose-object-idx\" file as success and instead only ignore ENOENT. It is\nexpected that a mapping file may not always be present, but other errors\nshould be signaled back to callers. Note that this slightly changes the\nbehavior of `repo_read_loose_object_map()`, which now stops loading the\nmapping files of subsequent object sources when one is unreadable, the\nsame way it already does when one is malformed.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 87 +++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 54 insertions(+), 33 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex c6f4ee0ec6..5f72c644f9 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const\n \treturn 1;\n }\n \n-static int insert_loose_map(struct odb_source_loose *loose,\n+static int insert_loose_map(struct loose_object_map *map,\n+\t\t\t struct oidtree *cache,\n \t\t\t const struct object_id *oid,\n \t\t\t const struct object_id *compat_oid)\n {\n-\tstruct loose_object_map *map = loose->map;\n \tint inserted = 0;\n \n \tinserted |= insert_oid_pair(map->to_compat, oid, compat_oid);\n \tinserted |= insert_oid_pair(map->to_storage, compat_oid, oid);\n-\tif (inserted)\n-\t\toidtree_insert(loose->cache, compat_oid, NULL);\n+\tif (inserted && cache)\n+\t\toidtree_insert(cache, compat_oid, NULL);\n \n \treturn inserted;\n }\n \n-int loose_object_map_load(struct odb_source_loose *loose)\n+static int loose_object_map_read(struct repository *repo, const char *path,\n+\t\t\t\t struct loose_object_map *map,\n+\t\t\t\t struct oidtree *cache)\n {\n-\tstruct repository *repo = loose->base.odb->repo;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *path;\n \tFILE *fp;\n-\tint ret = -1;\n-\n-\tif (!should_use_loose_object_map(repo))\n-\t\treturn 0;\n-\n-\tif (!loose->map)\n-\t\tloose_object_map_init(&loose->map);\n-\tif (!loose->cache) {\n-\t\tALLOC_ARRAY(loose->cache, 1);\n-\t\toidtree_init(loose->cache);\n-\t}\n-\n-\tinsert_loose_map(loose, repo->hash_algo->empty_tree, repo->compat_hash_algo->empty_tree);\n-\tinsert_loose_map(loose, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);\n-\tinsert_loose_map(loose, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);\n+\tint ret;\n \n-\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n \tfp = fopen(path, \"rb\");\n \tif (!fp) {\n-\t\tfree(path);\n-\t\treturn 0;\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn -1;\n \t}\n \n-\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header))\n-\t\tgoto err;\n+\tif (strbuf_getwholeline(&buf, fp, '\\n') || strcmp(buf.buf, loose_object_header)) {\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \twhile (!strbuf_getline_lf(&buf, fp)) {\n \t\tconst char *p;\n \t\tstruct object_id oid, compat_oid;\n \t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, repo->hash_algo) ||\n \t\t *p++ != ' ' ||\n \t\t parse_oid_hex_algop(p, &compat_oid, &p, repo->compat_hash_algo) ||\n-\t\t p != buf.buf + buf.len)\n-\t\t\tgoto err;\n-\t\tinsert_loose_map(loose, &oid, &compat_oid);\n+\t\t p != buf.buf + buf.len) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tinsert_loose_map(map, cache, &oid, &compat_oid);\n \t}\n \n \tret = ferror(fp) ? -1 : 0;\n-err:\n-\tfclose(fp);\n+\n+out:\n \tstrbuf_release(&buf);\n+\tfclose(fp);\n+\n+\treturn ret;\n+}\n+\n+int loose_object_map_load(struct odb_source_loose *loose)\n+{\n+\tstruct repository *repo = loose->base.odb->repo;\n+\tchar *path;\n+\tint ret;\n+\n+\tif (!should_use_loose_object_map(repo))\n+\t\treturn 0;\n+\n+\tif (!loose->map)\n+\t\tloose_object_map_init(&loose->map);\n+\tif (!loose->cache) {\n+\t\tALLOC_ARRAY(loose->cache, 1);\n+\t\toidtree_init(loose->cache);\n+\t}\n+\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_tree,\n+\t\t\t repo->compat_hash_algo->empty_tree);\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->empty_blob,\n+\t\t\t repo->compat_hash_algo->empty_blob);\n+\tinsert_loose_map(loose->map, loose->cache, repo->hash_algo->null_oid,\n+\t\t\t repo->compat_hash_algo->null_oid);\n+\n+\tpath = xstrfmt(\"%s/loose-object-idx\", loose->base.path);\n+\tret = loose_object_map_read(repo, path, loose->map, loose->cache);\n+\n \tfree(path);\n \treturn ret;\n }\n@@ -223,7 +244,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n \tif (!should_use_loose_object_map(loose->base.odb->repo))\n \t\treturn 0;\n \n-\tinserted = insert_loose_map(loose, oid, compat_oid);\n+\tinserted = insert_loose_map(loose->map, loose->cache, oid, compat_oid);\n \tif (inserted)\n \t\treturn write_one_object(loose, oid, compat_oid);\n \treturn 0;\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556849","messageId":"20261029232220.1245827-4-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"[PATCH v3 3/4] loose: lift loose object index writing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T23:22:19Z","receivedAt":"2026-10-29T23:22:19Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Writing the in-memory loose object map to a \"loose-object-idx\" file is\nhandled via `repo_write_loose_object_map()`, which always writes the map\ninto the primary object directory. In a subsequent commit, a separate\n\"loose-object-idx\" file is staged prior to commit and must be\nconstructed from a loose object map not attached to the primary loose\nobject source.\n\nExtract the logic for writing a loose object index file into a separate\n`loose_object_map_write()` helper function that allows callers to\nexplicitly specify both the loose object map as well as the file\ndescriptor to write the entries to. Note that the responsibility of\nlocking the file being written to is left to callers so that it is\npossible for a lock to be held for longer than just this single\noperation.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 69 +++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 45 insertions(+), 24 deletions(-)\n\ndiff --git a/loose.c b/loose.c\nindex 5f72c644f9..f2e111c25e 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -146,25 +146,17 @@ int repo_read_loose_object_map(struct repository *repo)\n \treturn 0;\n }\n \n-int repo_write_loose_object_map(struct repository *repo)\n+static int loose_object_map_write(struct repository *repo,\n+\t\t\t\t struct loose_object_map *loose_map, int fd)\n {\n-\tstruct odb_source_files *files = odb_source_files_downcast(repo->objects->source);\n-\tkh_oid_map_t *map = files->dirs->loose->map->to_compat;\n-\tstruct lock_file lock;\n-\tint fd;\n+\tkh_oid_map_t *map = loose_map->to_compat;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tkhiter_t iter;\n-\tstruct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;\n-\n-\tif (!should_use_loose_object_map(repo))\n-\t\treturn 0;\n+\tint ret = -1;\n \n-\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n-\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n-\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n-\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n \titer = kh_begin(map);\n \tif (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)\n-\t\tgoto errout;\n+\t\tgoto out;\n \n \tfor (; iter != kh_end(map); iter++) {\n \t\tif (kh_exist(map, iter)) {\n@@ -173,24 +165,53 @@ int repo_write_loose_object_map(struct repository *repo)\n \t\t\t\tcontinue;\n \t\t\tstrbuf_addf(&buf, \"%s %s\\n\", oid_to_hex(&kh_key(map, iter)), oid_to_hex(kh_value(map, iter)));\n \t\t\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\t\t\tgoto errout;\n+\t\t\t\tgoto out;\n \t\t\tstrbuf_reset(&buf);\n \t\t}\n \t}\n+\n+\tret = 0;\n+\n+out:\n \tstrbuf_release(&buf);\n+\n+\treturn ret;\n+}\n+\n+int repo_write_loose_object_map(struct repository *repo)\n+{\n+\tstruct odb_source_files *files = odb_source_files_downcast(repo->objects->source);\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct lock_file lock;\n+\tint ret;\n+\tint fd;\n+\n+\tif (!should_use_loose_object_map(repo))\n+\t\treturn 0;\n+\n+\trepo_common_path_replace(repo, &path, \"objects/loose-object-idx\");\n+\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, path.buf,\n+\t\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n+\n+\tif (loose_object_map_write(repo, files->dirs->loose->map, fd) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"),\n+\t\t\t\t get_lock_file_path(&lock));\n+\t\tgoto out;\n+\t}\n+\n \tif (commit_lock_file(&lock) < 0) {\n-\t\terror_errno(_(\"could not write loose object index %s\"), path.buf);\n-\t\tstrbuf_release(&path);\n-\t\treturn -1;\n+\t\tret = error_errno(_(\"unable to write %s\"), path.buf);\n+\t\tgoto out;\n \t}\n-\tstrbuf_release(&path);\n-\treturn 0;\n-errout:\n+\n+\tret = 0;\n+\n+out:\n \trollback_lock_file(&lock);\n-\tstrbuf_release(&buf);\n-\terror_errno(_(\"failed to write loose object index %s\"), path.buf);\n \tstrbuf_release(&path);\n-\treturn -1;\n+\n+\treturn ret;\n }\n \n static int write_one_object(struct odb_source_loose *loose,\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556850","messageId":"20261029232220.1245827-5-jltobler@gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"[PATCH v3 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T23:22:20Z","receivedAt":"2026-10-29T23:22:20Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When a repository is configured with a compatibility hash, each loose\nobject write also records a mapping between the object and its\ncompatibility hash in a \"loose-object-idx\" file. This mapping file is\nlocated in the same object directory the object is written to. If batch\nfsync of loose objects is also enabled, the loose objects and mapping\nfile are instead written to a temporary object directory and expected to\nbe migrated to the primary object directory when the batch transaction\ncommits.\n\nWhile migrating the loose objects works, the \"loose-object-idx\" file is\nunable to be migrated if a mapping file already exists in the primary\nobject directory. This occurs because `finalize_object_file_flags()`\nexpects an already existing file to have the same contents. Otherwise,\nthe following error is returned:\n\n error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents\n error: unable to migrate temporary objects\n\nFurthermore, it is not sufficient to just replace the preexisting\nmapping file with the new one as the new file only contains mapping\nentries for the newly written objects.\n\nUpdate the temporary directory migration logic to handle\n\"loose-object-idx\" files explicitly by merging new entries and\natomically writing the resulting mapping file.\n\nSigned-off-by: Justin Tobler \n---\n loose.c | 47 +++++++++++++++++++++++++++++++++++\n loose.h | 9 +++++++\n t/t1016-compatObjectFormat.sh | 27 ++++++++++++++++++++\n tmp-objdir.c | 11 +++++++-\n 4 files changed, 93 insertions(+), 1 deletion(-)\n\ndiff --git a/loose.c b/loose.c\nindex f2e111c25e..0ce9c7486e 100644\n--- a/loose.c\n+++ b/loose.c\n@@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)\n \tfree(m);\n \t*map = NULL;\n }\n+\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst)\n+{\n+\tstruct lock_file lock = LOCK_INIT;\n+\tstruct loose_object_map *map;\n+\tint ret;\n+\tint fd;\n+\n+\tloose_object_map_init(&map);\n+\n+\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n+\t\tret = error(_(\"unable to read loose object index %s\"), src);\n+\t\tgoto out;\n+\t}\n+\n+\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n+\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n+\tif (fd < 0) {\n+\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n+\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tif (loose_object_map_write(repo, map, fd) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"),\n+\t\t\t\t get_lock_file_path(&lock));\n+\t\tgoto out;\n+\t}\n+\n+\tif (commit_lock_file(&lock) < 0) {\n+\t\tret = error_errno(_(\"unable to write %s\"), dst);\n+\t\tgoto out;\n+\t}\n+\n+\tret = 0;\n+\n+out:\n+\tloose_object_map_clear(&map);\n+\trollback_lock_file(&lock);\n+\n+\treturn ret;\n+}\ndiff --git a/loose.h b/loose.h\nindex ed663ac550..6ad82dc0bb 100644\n--- a/loose.h\n+++ b/loose.h\n@@ -24,4 +24,13 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,\n int repo_read_loose_object_map(struct repository *repo);\n int repo_write_loose_object_map(struct repository *repo);\n \n+/*\n+ * Merge the mapping entries of the loose object index at `src` into the one at\n+ * `dst`. The resulting mapping file is written atomically.\n+ *\n+ * Returns 0 on success, a negative error code otherwise.\n+ */\n+int loose_object_idx_merge(struct repository *repo, const char *src,\n+\t\t\t const char *dst);\n+\n #endif\ndiff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh\nindex 9cafcee509..6d0f48226f 100755\n--- a/t/t1016-compatObjectFormat.sh\n+++ b/t/t1016-compatObjectFormat.sh\n@@ -305,4 +305,31 @@ compare_oids GPG2 'commit' signedcommit4 \"$signedcommit4_sha1_oid\" \"$signedcommi\n compare_oids GPG2 'tag' signedtag3 \"$signedtag3_sha1_oid\" \"$signedtag3_sha256_oid\"\n compare_oids GPG2 'tag' signedtag4 \"$signedtag4_sha1_oid\" \"$signedtag4_sha256_oid\"\n \n+test_expect_success 'batched fsync migrates loose object index' '\n+\ttest_when_finished \"rm -rf batch-fsync\" &&\n+\tgit init --object-format=sha1 batch-fsync &&\n+\t(\n+\t\tcd batch-fsync &&\n+\t\tgit config core.repositoryFormatVersion 1 &&\n+\t\tgit config extensions.compatObjectFormat sha256 &&\n+\t\tgit config core.fsync loose-object &&\n+\t\tgit config core.fsyncMethod batch &&\n+\n+\t\t# Write object to generate initial loose object index.\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\n+\t\t# Write some additional objects and ensure the corresponding\n+\t\t# temporary loose object index entries stored in the transaction\n+\t\t# are merged into the primary object directory.\n+\t\techo bar >bar &&\n+\t\techo baz >baz &&\n+\t\tgit add bar baz &&\n+\t\ttest_grep \"$(git rev-parse :foo)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :bar)\" .git/objects/loose-object-idx &&\n+\t\ttest_grep \"$(git rev-parse :baz)\" .git/objects/loose-object-idx\n+\t)\n+'\n+\n test_done\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 3debb60270..5307717354 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -4,6 +4,7 @@\n #include \"chdir-notify.h\"\n #include \"dir.h\"\n #include \"environment.h\"\n+#include \"loose.h\"\n #include \"object-file.h\"\n #include \"path.h\"\n #include \"string-list.h\"\n@@ -294,6 +295,11 @@ static int is_loose_object_shard(const char *name)\n \treturn strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);\n }\n \n+static int is_loose_object_idx(const char *name)\n+{\n+\treturn !strcmp(name, \"loose-object-idx\");\n+}\n+\n static int migrate_paths(struct tmp_objdir *t,\n \t\t\t struct strbuf *src, struct strbuf *dst,\n \t\t\t enum finalize_object_file_flags flags)\n@@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t,\n \t\tif (is_loose_object_shard(name))\n \t\t\tflags_copy |= FOF_SKIP_COLLISION_CHECK;\n \n-\t\tret |= migrate_one(t, src, dst, flags_copy);\n+\t\tif (is_loose_object_idx(name))\n+\t\t\tret |= loose_object_idx_merge(t->repo, src->buf, dst->buf);\n+\t\telse\n+\t\t\tret |= migrate_one(t, src, dst, flags_copy);\n \n \t\tstrbuf_setlen(src, src_len);\n \t\tstrbuf_setlen(dst, dst_len);\n-- \n2.56.0.173.gf57c9a7388\n\n\n"},{"id":"556854","messageId":"auQ3FTXbn1awcP70@pks.im","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"Re: [PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-30T06:20:05Z","receivedAt":"2026-10-30T06:20:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 29, 2026 at 06:22:16PM -0500, Justin Tobler wrote:\n> Changes since V2:\n> - Reading the src mapping file is now done prior to acquiring the lock\n> to reduce the critical window.\n> - Added a comment to `loose_object_idx_merge()` and updated some\n> commit messages to improve clarity.\n\nThe changes and range-diff both look exacty as expected, so I'm happy\nwith this version. Thanks!\n\nPatrick\n\n"},{"id":"556890","messageId":"CAOLa=ZT_ZNcXztxE2gmTORD2o72f3u2i0Ptj5tgNzS54_V1MTg@mail.gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-1-jltobler@gmail.com","subject":"Re: [PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-30T10:38:37Z","receivedAt":"2026-10-30T10:38:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> Greetings,\n>\n> While working on a separate patch series to convert the \"files\" ODB\n> transaction to be eagerly created instead of lazily, I encountered this\n> bug that causes an ODB transaction to fail on commit when a\n> compatibility hash is also configured. This occurs because the temporary\n> directory migration code does not know how to handle the loose object\n> index file.\n\nOkay so reading a bit, and paraphrasing for my understanding: When we\nhae object compatibility setup in a repository. THe 'loose-object-idx'\ntracks the mapping from hash1<>hash2. The issue is batch fsync, where we\nuse a temporary folder. I'm not sure what the 'error' here is, but I\nguess we'll see with the patches.\n\nFor pack files this is a non-issues as they're content addressed and\nindividual packfiles have their own corresponding idx files.\n\n> This patch addresses updating the migration logic to explicitly merge\n> mapping entries in the transaction and atomically write a new\n> \"loose-object-idx\" file.\n>\n\nThis means we write a 'loose-object-idx' to the temporary directory and\nwe need to merge it with the one in the main directory. Which seems to\nmake sense.\n\n> Some other approaches I considered:\n> - Storing the uncommitted object mapping entries in the transaction\n> without writing a file and only writing on transaction commit. I\n> chose to keep writing the temp index into the transaction so that\n> child processes that write to the tempdir could still append entries.\n\nSo the child processes write the objects, so they write to the index too\nI presume. If not, we'd have to somehow get that information from all\nchildren to the parent which owns the transaction. That gets complicated\n/ messy. Seems right.\n\n> - Also writing mapping entires to the main ODB loose object index.\n> This also didn't seem like a good apprach because it write entries\n> that may not ultimately get committed.\n>\n\nYeah this approach definitely doesn't make sense from a transaction\npoint of view.\n\n[snip]\n\nWill dig into the patches...\n"},{"id":"556891","messageId":"CAOLa=ZSs05h9h+5+tdE=PA3ebj8U_8oj+qcstxZVcefpSY+Rcg@mail.gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-2-jltobler@gmail.com","subject":"Re: [PATCH v3 1/4] loose: use finite timeout when locking \"loose-object-idx\" file","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-30T10:43:12Z","receivedAt":"2026-10-30T10:43:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> Loose object index file writers first attempt to acquire a lock via\n> `repo_hold_lock_file_for_update_timeout()` and specify a timeout value\n> of -1. With a negative timeout value, the lock acquisition retries\n> indefinitely while the lockfile still exists. If a process dies while\n> holding the lock and leaves a stale lockfile behind, every subsequent\n> writer of the mapping file hangs forever instead of reporting an error.\n>\n> Introduce LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS to set the timeout to 1000ms\n> and use it for all callers that lock a \"loose-object-idx\" file. A value\n> of 1000ms matches the default timeout used for \"core.packedRefsTimeout\"\n> and should provide concurrent writers sufficient time to acquire the\n> lock.\n>\n\nI was wondering why we don't use a config here too. Seems like Patrick\nalso mentioned the same in his review. But this is a good middle step\nand we can add that in later.\n\n#leftoverbits\n\n[snip]\n"},{"id":"556894","messageId":"CAOLa=ZTNCcRCFDvUunqEqQCmDPE_2iu=rC4FwNmKAQujegC1Yg@mail.gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-3-jltobler@gmail.com","subject":"Re: [PATCH v3 2/4] loose: lift loose object index parsing logic into helper","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-30T10:50:44Z","receivedAt":"2026-10-30T10:50:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> diff --git a/loose.c b/loose.c\n> index c6f4ee0ec6..5f72c644f9 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const\n> \treturn 1;\n> }\n>\n> -static int insert_loose_map(struct odb_source_loose *loose,\n> +static int insert_loose_map(struct loose_object_map *map,\n> +\t\t\t struct oidtree *cache,\n> \t\t\t const struct object_id *oid,\n> \t\t\t const struct object_id *compat_oid)\n\nNit: with the introduction of `loose_object_map_read()`, perhaps makes\nsense to keep naming consistent and rename this to\n`loose_object_map_insert()`?\n\n[snip]\n\nThe changes look good.\n"},{"id":"556914","messageId":"CAOLa=ZR2AmC4EDM7V2A9UainUZ5md=dQT_pnJycxuqV7BhfxxA@mail.gmail.com","threadId":"66637","inReplyTo":"20261029232220.1245827-5-jltobler@gmail.com","subject":"Re: [PATCH v3 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-30T11:55:10Z","receivedAt":"2026-10-30T11:55:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> diff --git a/loose.c b/loose.c\n> index f2e111c25e..0ce9c7486e 100644\n> --- a/loose.c\n> +++ b/loose.c\n> @@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)\n> \tfree(m);\n> \t*map = NULL;\n> }\n> +\n> +int loose_object_idx_merge(struct repository *repo, const char *src,\n> +\t\t\t const char *dst)\n>\n\nNit: should we name this `loose_object_map_merge()` to stay consistent\nwith the other `loose_object_map...()` functions?\n\n> +{\n> +\tstruct lock_file lock = LOCK_INIT;\n> +\tstruct loose_object_map *map;\n> +\tint ret;\n> +\tint fd;\n> +\n> +\tloose_object_map_init(&map);\n> +\n> +\tif (loose_object_map_read(repo, src, map, NULL) < 0) {\n> +\t\tret = error(_(\"unable to read loose object index %s\"), src);\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tfd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,\n> +\t\t\t\t\t\t LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);\n> +\tif (fd < 0) {\n> +\t\tret = error_errno(_(\"unable to lock %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tif (loose_object_map_read(repo, dst, map, NULL) < 0) {\n> +\t\tret = error(_(\"unable to read loose object index %s\"), dst);\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tif (loose_object_map_write(repo, map, fd) < 0) {\n> +\t\tret = error_errno(_(\"unable to write %s\"),\n> +\t\t\t\t get_lock_file_path(&lock));\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tif (commit_lock_file(&lock) < 0) {\n> +\t\tret = error_errno(_(\"unable to write %s\"), dst);\n\nNit: Shouldn't this be unable to commit/rename? This will differentiate\nfrom the above write error\n\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tret = 0;\n> +\n> +out:\n> +\tloose_object_map_clear(&map);\n> +\trollback_lock_file(&lock);\n> +\n> +\treturn ret;\n> +}\n\nThe rest looks to be in order.\n"},{"id":"556925","messageId":"auS3XTwBDP004Gc9@denethor","threadId":"66637","inReplyTo":"CAOLa=ZTNCcRCFDvUunqEqQCmDPE_2iu=rC4FwNmKAQujegC1Yg@mail.gmail.com","subject":"Re: [PATCH v3 2/4] loose: lift loose object index parsing logic into helper","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-30T15:31:17Z","receivedAt":"2026-10-30T15:31:17Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/30 07:50PM, Karthik Nayak wrote:\n> Justin Tobler writes:\n> \n> > diff --git a/loose.c b/loose.c\n> > index c6f4ee0ec6..5f72c644f9 100644\n> > --- a/loose.c\n> > +++ b/loose.c\n> > @@ -48,67 +48,88 @@ static int insert_oid_pair(kh_oid_map_t *map, const struct object_id *key, const\n> > \treturn 1;\n> > }\n> >\n> > -static int insert_loose_map(struct odb_source_loose *loose,\n> > +static int insert_loose_map(struct loose_object_map *map,\n> > +\t\t\t struct oidtree *cache,\n> > \t\t\t const struct object_id *oid,\n> > \t\t\t const struct object_id *compat_oid)\n> \n> Nit: with the introduction of `loose_object_map_read()`, perhaps makes\n> sense to keep naming consistent and rename this to\n> `loose_object_map_insert()`?\n\nI don't feel too strongly either way and am not sure this by itself it\nworth a reroll. I'll keep it as-is for now.\n\n-Justin\n\n"},{"id":"556926","messageId":"auS4SzrVGYfAG8KA@denethor","threadId":"66637","inReplyTo":"CAOLa=ZR2AmC4EDM7V2A9UainUZ5md=dQT_pnJycxuqV7BhfxxA@mail.gmail.com","subject":"Re: [PATCH v3 4/4] tmp-objdir: merge loose object index when migrating tmpdir","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-30T15:40:01Z","receivedAt":"2026-10-30T15:40:01Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/30 08:55PM, Karthik Nayak wrote:\n> Justin Tobler writes:\n> \n> > diff --git a/loose.c b/loose.c\n> > index f2e111c25e..0ce9c7486e 100644\n> > --- a/loose.c\n> > +++ b/loose.c\n> > @@ -311,3 +311,50 @@ void loose_object_map_clear(struct loose_object_map **map)\n> > \tfree(m);\n> > \t*map = NULL;\n> > }\n> > +\n> > +int loose_object_idx_merge(struct repository *repo, const char *src,\n> > +\t\t\t const char *dst)\n> >\n> \n> Nit: should we name this `loose_object_map_merge()` to stay consistent\n> with the other `loose_object_map...()` functions?\n\nI specifically did not call it `loose_object_map_merge()` because,\nunlike the other similarly named functions, this one does not update the\nin-memory loose object map for the object source. Here we are only\nmerging the \"loose-object-idx\" files.\n\n> > +\tif (loose_object_map_write(repo, map, fd) < 0) {\n> > +\t\tret = error_errno(_(\"unable to write %s\"),\n> > +\t\t\t\t get_lock_file_path(&lock));\n> > +\t\tgoto out;\n> > +\t}\n> > +\n> > +\tif (commit_lock_file(&lock) < 0) {\n> > +\t\tret = error_errno(_(\"unable to write %s\"), dst);\n> \n> Nit: Shouldn't this be unable to commit/rename? This will differentiate\n> from the above write error\n\nI choose \"write\" here because logically we are trying atomically write\nthe final \"loose-object-idx\" file. IMO, I think the difference in\nfilepath should be sufficient to differentiate between the two as well.\nUltimately though, I don't feel strongly either way about this and am\nnot sure it's worth a reroll, so I'm inclined to leave it as-is.\n\n-Justin\n\n"},{"id":"556927","messageId":"auS6XMchhyuTF_VJ@denethor","threadId":"66637","inReplyTo":"auQ3FTXbn1awcP70@pks.im","subject":"Re: [PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-30T15:40:30Z","receivedAt":"2026-10-30T15:40:30Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/30 07:20AM, Patrick Steinhardt wrote:\n> On Thu, Oct 29, 2026 at 06:22:16PM -0500, Justin Tobler wrote:\n> > Changes since V2:\n> > - Reading the src mapping file is now done prior to acquiring the lock\n> > to reduce the critical window.\n> > - Added a comment to `loose_object_idx_merge()` and updated some\n> > commit messages to improve clarity.\n> \n> The changes and range-diff both look exacty as expected, so I'm happy\n> with this version. Thanks!\n\nThanks for the review!\n\n-Justin\n\n"},{"id":"556928","messageId":"auS6c_Ov1JOUtr4P@denethor","threadId":"66637","inReplyTo":"CAOLa=ZT_ZNcXztxE2gmTORD2o72f3u2i0Ptj5tgNzS54_V1MTg@mail.gmail.com","subject":"Re: [PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-30T15:43:49Z","receivedAt":"2026-10-30T15:43:49Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/30 07:38PM, Karthik Nayak wrote:\n> \n> Will dig into the patches...\n\nThanks for the review! IMO, I think the suggestions left are probably\nnot worth a reroll, so I plan to leave the series as-is, but please let\nme know if you feel differently. :)\n\nThanks,\n-Justin\n\n"},{"id":"557333","messageId":"CAOLa=ZRBdgNZ2D9B2ZFhMAOJc11ENZCcw2a7NhKtkv07t4VE-Q@mail.gmail.com","threadId":"66637","inReplyTo":"auS6c_Ov1JOUtr4P@denethor","subject":"Re: [PATCH v3 0/4] loose: fix migrating \"loose-object-idx\" with batch fsync","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-11-03T16:37:50Z","receivedAt":"2026-11-03T16:37:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> On 26/10/30 07:38PM, Karthik Nayak wrote:\n>>\n>> Will dig into the patches...\n>\n> Thanks for the review! IMO, I think the suggestions left are probably\n> not worth a reroll, so I plan to leave the series as-is, but please let\n> me know if you feel differently. :)\n>\n> Thanks,\n> -Justin\n\nIt's all good. It is in a good shape already!\n"}]}