All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tmp-objdir: merge loose object index when migrating tmpdir
@ 2026-10-22 23:40 Justin Tobler
  2026-10-23 12:19 ` Patrick Steinhardt
  2026-10-29  0:24 ` [PATCH v2 0/4] loose: fix migrating "loose-object-idx" with batch fsync Justin Tobler
  0 siblings, 2 replies; 24+ messages in thread
From: Justin Tobler @ 2026-10-22 23:40 UTC (permalink / raw)
  To: git; +Cc: Justin Tobler

When a repository is configured with a compatibility hash, each loose
object write also records a mapping between the object and its
compatibility hash in a "loose-object-idx" file. This index is located
in the same object database the object is written to. If batch fsync of
loose objects is also enabled, the loose objects and index file are
instead written to a temporary object directory and expected to be
migrated to the primary object directory when the batch transaction
commits.

While migrating the loose objects works, the "loose-object-idx" file is
unable to be migrated if an index already exists in the primary object
directory. This occurs because `finalize_object_file_flags()` expects an
already existing file to have the same contents. Otherwise, the
following error is returned:

  error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents
  error: unable to migrate temporary objects

Furthermore, it is not sufficient to just replace the preexisting index
with the new one as the new index only contains mapping entries for the
newly written objects.

Update the temporary directory migration logic to handle
"loose-object-idx" files explicitly by appending new mapping entries to
the primary index.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---

Greetings,

While working on a separate patch series to convert the "files" ODB
transaction to be eagerly created instead of lazily, I encountered this
bug that causes an ODB transaction to fail on commit when a
compatibility hash is also configured. This occurs because the temporary
directory migration code does not know how to handle the loose object
index file.

This patch addresses updating the migration logic to explicitly merge
mapping entries in the transaction by appending them to the primary
loose object index file.

Some other approaches I considered:
  - Storing the uncommitted object mapping entries in the transaction
    without writing a file and only writing on transaction commit. I
    chose to keep writing the temp index into the transaction so that
    child processes that write to the tempdir could still append entries.
  - Also writing mapping entires to the main ODB loose object index.
    This also didn't seem like a good apprach because it write entires
    that may not ultimately get committed.

Thanks for the review,
-Justin

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

diff --git a/loose.c b/loose.c
index d6a2bb5ba3..79d4c256e0 100644
--- a/loose.c
+++ b/loose.c
@@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)
 	free(m);
 	*map = NULL;
 }
+
+int loose_object_idx_merge(struct repository *repo, const char *src,
+			   const char *dst)
+{
+	struct lock_file lock = LOCK_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct stat st;
+	FILE *fp;
+	int fd = -1;
+	int ret;
+
+	fp = fopen(src, "r");
+	if (!fp) {
+		ret = error_errno(_("unable to open %s"), src);
+		goto out;
+	}
+	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header)) {
+		ret = error(_("invalid loose object index %s"), src);
+		goto out;
+	}
+
+	if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
+		ret = error_errno(_("unable to lock %s"), dst);
+		goto out;
+	}
+
+	fd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);
+	if (fd < 0) {
+		ret = error_errno(_("unable to open %s"), dst);
+		goto out;
+	}
+	if (fstat(fd, &st) < 0) {
+		ret = error_errno(_("unable to stat %s"), dst);
+		goto out;
+	}
+
+	/* Create the loose-object-idx if one does not already exist. */
+	if (!st.st_size && write_in_full(fd, loose_object_header,
+					 strlen(loose_object_header)) < 0) {
+		ret = error_errno(_("failed to write loose object index header"));
+		goto out;
+	}
+
+	while (!strbuf_getwholeline(&buf, fp, '\n')) {
+		if (write_in_full(fd, buf.buf, buf.len) < 0) {
+			ret = error_errno(_("failed to write loose object index %s"),
+					  dst);
+			goto out;
+		}
+	}
+
+	ret = close(fd);
+	fd = -1;
+	if (ret) {
+		ret = error_errno(_("unable to close %s"), dst);
+		goto out;
+	}
+
+	adjust_shared_perm(repo, dst);
+
+	ret = 0;
+
+out:
+	rollback_lock_file(&lock);
+	strbuf_release(&buf);
+	if (fd >= 0)
+		close(fd);
+	if (fp)
+		fclose(fp);
+
+	return ret;
+}
diff --git a/loose.h b/loose.h
index ed663ac550..35b46f4be5 100644
--- a/loose.h
+++ b/loose.h
@@ -23,5 +23,7 @@ int repo_add_loose_object_map(struct odb_source_loose *loose,
 			      const struct object_id *compat_oid);
 int repo_read_loose_object_map(struct repository *repo);
 int repo_write_loose_object_map(struct repository *repo);
+int loose_object_idx_merge(struct repository *repo, const char *src,
+			   const char *dst);
 
 #endif
diff --git a/t/t1016-compatObjectFormat.sh b/t/t1016-compatObjectFormat.sh
index 9cafcee509..bebf71bc29 100755
--- a/t/t1016-compatObjectFormat.sh
+++ b/t/t1016-compatObjectFormat.sh
@@ -305,4 +305,31 @@ compare_oids GPG2 'commit' signedcommit4 "$signedcommit4_sha1_oid" "$signedcommi
 compare_oids GPG2 'tag' signedtag3 "$signedtag3_sha1_oid" "$signedtag3_sha256_oid"
 compare_oids GPG2 'tag' signedtag4 "$signedtag4_sha1_oid" "$signedtag4_sha256_oid"
 
+test_expect_success 'batched fsync migrates loose object index' '
+	test_when_finished "rm -rf batch-fsync" &&
+	git init --object-format=sha1 batch-fsync &&
+	(
+		cd batch-fsync &&
+		git config core.repositoryFormatVersion 1 &&
+		git config extensions.compatObjectFormat sha256 &&
+		git config core.fsync loose-object &&
+		git config core.fsyncMethod batch &&
+
+		# Write object to generate initial loose object index.
+		echo foo >foo &&
+		git add foo &&
+		test_grep "$(git rev-parse :foo)" .git/objects/loose-object-idx &&
+
+		# Write some additional objects and ensure the corresponding
+		# temporary loose object index entries stored in the transaction
+		# are merged into the primary ODB.
+		echo bar >bar &&
+		echo baz >baz &&
+		git add bar baz &&
+		test_grep "$(git rev-parse :foo)" .git/objects/loose-object-idx &&
+		test_grep "$(git rev-parse :bar)" .git/objects/loose-object-idx &&
+		test_grep "$(git rev-parse :baz)" .git/objects/loose-object-idx
+	)
+'
+
 test_done
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 3debb60270..5307717354 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -4,6 +4,7 @@
 #include "chdir-notify.h"
 #include "dir.h"
 #include "environment.h"
+#include "loose.h"
 #include "object-file.h"
 #include "path.h"
 #include "string-list.h"
@@ -294,6 +295,11 @@ static int is_loose_object_shard(const char *name)
 	return strlen(name) == 2 && isxdigit(name[0]) && isxdigit(name[1]);
 }
 
+static int is_loose_object_idx(const char *name)
+{
+	return !strcmp(name, "loose-object-idx");
+}
+
 static int migrate_paths(struct tmp_objdir *t,
 			 struct strbuf *src, struct strbuf *dst,
 			 enum finalize_object_file_flags flags)
@@ -317,7 +323,10 @@ static int migrate_paths(struct tmp_objdir *t,
 		if (is_loose_object_shard(name))
 			flags_copy |= FOF_SKIP_COLLISION_CHECK;
 
-		ret |= migrate_one(t, src, dst, flags_copy);
+		if (is_loose_object_idx(name))
+			ret |= loose_object_idx_merge(t->repo, src->buf, dst->buf);
+		else
+			ret |= migrate_one(t, src, dst, flags_copy);
 
 		strbuf_setlen(src, src_len);
 		strbuf_setlen(dst, dst_len);

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
-- 
2.56.0.173.gf57c9a7388


^ permalink raw reply related	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2026-10-29 23:22 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22 23:40 [PATCH] tmp-objdir: merge loose object index when migrating tmpdir Justin Tobler
2026-10-23 12:19 ` Patrick Steinhardt
2026-10-23 21:25   ` Justin Tobler
2026-10-26  6:39     ` Patrick Steinhardt
2026-10-26 17:58       ` Justin Tobler
2026-10-29  0:24 ` [PATCH v2 0/4] loose: fix migrating "loose-object-idx" with batch fsync Justin Tobler
2026-10-29  0:24   ` [PATCH v2 1/4] loose: use finite timeout when locking "loose-object-idx" file Justin Tobler
2026-10-29  8:05     ` Patrick Steinhardt
2026-10-29  0:24   ` [PATCH v2 2/4] loose: lift loose object index parsing logic into helper Justin Tobler
2026-10-29  8:05     ` Patrick Steinhardt
2026-10-29 21:10       ` Justin Tobler
2026-10-29  0:24   ` [PATCH v2 3/4] loose: lift loose object index writing " Justin Tobler
2026-10-29  8:05     ` Patrick Steinhardt
2026-10-29 21:18       ` Justin Tobler
2026-10-29  0:24   ` [PATCH v2 4/4] tmp-objdir: merge loose object index when migrating tmpdir Justin Tobler
2026-10-29  8:05     ` Patrick Steinhardt
2026-10-29 21:20       ` Justin Tobler
2026-10-29 22:22   ` [PATCH v2 0/4] loose: fix migrating "loose-object-idx" with batch fsync Junio C Hamano
2026-10-29 22:45     ` Justin Tobler
2026-10-29 23:22   ` [PATCH v3 " Justin Tobler
2026-10-29 23:22     ` [PATCH v3 1/4] loose: use finite timeout when locking "loose-object-idx" file Justin Tobler
2026-10-29 23:22     ` [PATCH v3 2/4] loose: lift loose object index parsing logic into helper Justin Tobler
2026-10-29 23:22     ` [PATCH v3 3/4] loose: lift loose object index writing " Justin Tobler
2026-10-29 23:22     ` [PATCH v3 4/4] tmp-objdir: merge loose object index when migrating tmpdir Justin Tobler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.