[PATCH v2 0/4] loose: fix migrating "loose-object-idx" with batch fsync

Greetings,

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

This patch addresses updating the migration logic to explicitly merge mapping entries in the transaction and atomically write a new "loose-object-idx" file.

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

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

Thanks for the review, -Justin

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

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

Range-diff against v1:
-:  ---------- > 1:  6f104c482e loose: use finite timeout when locking "loose-object-idx" file
-:  ---------- > 2:  4799421a74 loose: lift loose object index parsing logic into helper
-:  ---------- > 3:  4233e913ce loose: lift loose object index writing logic into helper
1:  34393e789c ! 4:  614aab27d1 tmp-objdir: merge loose object index when migrating tmpdir
    @@ Commit message
     
         When a repository is configured with a compatibility hash, each loose
         object write also records a mapping between the object and its
    -    compatibility hash in a "loose-object-idx" file. This index is located
    -    in the same object database the object is written to. If batch fsync of
    -    loose objects is also enabled, the loose objects and index file are
    -    instead written to a temporary object directory and expected to be
    -    migrated to the primary object directory when the batch transaction
    +    compatibility hash in a "loose-object-idx" file. This mapping file is
    +    located in the same object directory the object is written to. If batch
    +    fsync of loose objects is also enabled, the loose objects and mapping
    +    file are instead written to a temporary object directory and expected to
    +    be migrated to the primary object directory when the batch transaction
         commits.
     
         While migrating the loose objects works, the "loose-object-idx" file is
    -    unable to be migrated if an index already exists in the primary object
    -    directory. This occurs because `finalize_object_file_flags()` expects an
    -    already existing file to have the same contents. Otherwise, the
    -    following error is returned:
    +    unable to be migrated if a mapping file already exists in the primary
    +    object directory. This occurs because `finalize_object_file_flags()`
    +    expects an already existing file to have the same contents. Otherwise,
    +    the following error is returned:
     
           error: files '.git/objects/tmp_objdir-bulk-fsync-XXXXXX/loose-object-idx' and '.git/objects/loose-object-idx' differ in contents
           error: unable to migrate temporary objects
     
    -    Furthermore, it is not sufficient to just replace the preexisting index
    -    with the new one as the new index only contains mapping entries for the
    -    newly written objects.
    +    Furthermore, it is not sufficient to just replace the preexisting
    +    mapping file with the new one as the new file only contains mapping
    +    entries for the newly written objects.
     
         Update the temporary directory migration logic to handle
    -    "loose-object-idx" files explicitly by appending new mapping entries to
    -    the primary index.
    +    "loose-object-idx" files explicitly by merging new entries and
    +    atomically writing the resulting mapping file.
     
         Signed-off-by: Justin Tobler <jltobler@gmail.com>
     
    @@ loose.c: void loose_object_map_clear(struct loose_object_map **map)
     +			   const char *dst)
     +{
     +	struct lock_file lock = LOCK_INIT;
    -+	struct strbuf buf = STRBUF_INIT;
    -+	struct stat st;
    -+	FILE *fp;
    -+	int fd = -1;
    ++	struct loose_object_map *map;
     +	int ret;
    ++	int fd;
     +
    -+	fp = fopen(src, "r");
    -+	if (!fp) {
    -+		ret = error_errno(_("unable to open %s"), src);
    -+		goto out;
    -+	}
    -+	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header)) {
    -+		ret = error(_("invalid loose object index %s"), src);
    -+		goto out;
    -+	}
    ++	loose_object_map_init(&map);
     +
    -+	if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
    ++	fd = repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0,
    ++						    LOOSE_OBJECT_IDX_LOCK_TIMEOUT_MS);
    ++	if (fd < 0) {
     +		ret = error_errno(_("unable to lock %s"), dst);
     +		goto out;
     +	}
     +
    -+	fd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);
    -+	if (fd < 0) {
    -+		ret = error_errno(_("unable to open %s"), dst);
    -+		goto out;
    -+	}
    -+	if (fstat(fd, &st) < 0) {
    -+		ret = error_errno(_("unable to stat %s"), dst);
    ++	if (loose_object_map_read(repo, dst, map, NULL) < 0) {
    ++		ret = error(_("unable to read loose object index %s"), dst);
     +		goto out;
     +	}
     +
    -+	/* Create the loose-object-idx if one does not already exist. */
    -+	if (!st.st_size && write_in_full(fd, loose_object_header,
    -+					 strlen(loose_object_header)) < 0) {
    -+		ret = error_errno(_("failed to write loose object index header"));
    ++	if (loose_object_map_read(repo, src, map, NULL) < 0) {
    ++		ret = error(_("unable to read loose object index %s"), src);
     +		goto out;
     +	}
     +
    -+	while (!strbuf_getwholeline(&buf, fp, '\n')) {
    -+		if (write_in_full(fd, buf.buf, buf.len) < 0) {
    -+			ret = error_errno(_("failed to write loose object index %s"),
    -+					  dst);
    -+			goto out;
    -+		}
    ++	if (loose_object_map_write(repo, map, fd) < 0) {
    ++		ret = error_errno(_("unable to write %s"),
    ++				  get_lock_file_path(&lock));
    ++		goto out;
     +	}
     +
    -+	ret = close(fd);
    -+	fd = -1;
    -+	if (ret) {
    -+		ret = error_errno(_("unable to close %s"), dst);
    ++	if (commit_lock_file(&lock) < 0) {
    ++		ret = error_errno(_("unable to write %s"), dst);
     +		goto out;
     +	}
     +
    -+	adjust_shared_perm(repo, dst);
    -+
     +	ret = 0;
     +
     +out:
    ++	loose_object_map_clear(&map);
     +	rollback_lock_file(&lock);
    -+	strbuf_release(&buf);
    -+	if (fd >= 0)
    -+		close(fd);
    -+	if (fp)
    -+		fclose(fp);
     +
     +	return ret;
     +}
    @@ t/t1016-compatObjectFormat.sh: compare_oids GPG2 'commit' signedcommit4 "$signed
     +
     +		# Write some additional objects and ensure the corresponding
     +		# temporary loose object index entries stored in the transaction
    -+		# are merged into the primary ODB.
    ++		# are merged into the primary object directory.
     +		echo bar >bar &&
     +		echo baz >baz &&
     +		git add bar baz &&

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e