[PATCH v3 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 V2:
  - Reading the src mapping file is now done prior to acquiring the lock
    to reduce the critical window.
  - Added a comment to `loose_object_idx_merge()` and updated some
    commit messages to improve clarity.

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

Thanks for the review, -Justin

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

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

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

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e