[PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 10110 bytes --]

Hi,

this is the second version of my patch series that introduces the
`--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)
and git-maintenance(1).

Changes compared to v1:

    - Clarified some of the commit messages.

    - Adjusted the stale comment of `stack_compact_range()`.

    - Added a unit test for failing auto-compaction.

    - Clarified a comment to explain why it is fine for auto-compaction
      to fail.

Thanks!

Patrick

Patrick Steinhardt (15):
  reftable/stack: fix error handling in `reftable_stack_init_addition()`
  reftable/error: discern locked/outdated errors
  reftable/stack: use error codes when locking fails during compaction
  reftable/stack: gracefully handle failed auto-compaction due to locks
  refs/reftable: print errors on compaction failure
  t/helper: drop pack-refs wrapper
  refs: move `struct pack_refs_opts` to where it's used
  refs: remove `PACK_REFS_ALL` flag
  refs/reftable: expose auto compaction via new flag
  builtin/pack-refs: release allocated memory
  builtin/pack-refs: introduce new "--auto" flag
  builtin/gc: move `struct maintenance_run_opts`
  t6500: extract objects with "17" prefix
  builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs
  builtin/gc: pack refs when using `git maintenance run --auto`

 Documentation/git-pack-refs.txt | 15 +++++-
 builtin/gc.c                    | 86 +++++++++++++++++++--------------
 builtin/pack-refs.c             | 31 +++++++-----
 refs.h                          | 20 ++++----
 refs/reftable-backend.c         | 11 ++++-
 reftable/error.c                |  4 +-
 reftable/reftable-error.h       |  5 +-
 reftable/stack.c                | 44 +++++++++++------
 reftable/stack_test.c           | 45 ++++++++++++++++-
 t/helper/test-ref-store.c       | 20 --------
 t/oid-info/hash-info            | 12 +++++
 t/t0601-reffiles-pack-refs.sh   | 30 ++++++++++--
 t/t0610-reftable-basics.sh      | 79 ++++++++++++++++++++++++++++++
 t/t6500-gc.sh                   | 30 +++---------
 14 files changed, 307 insertions(+), 125 deletions(-)

Range-diff against v1:
 1:  1e39d93a45 !  1:  b41b9b27cb reftable/stack: fix error handling in `reftable_stack_init_addition()`
    @@ Commit message
         error code check without the off-by-one. But other callers are subtly
         broken by this bug.
     
    -    Fix this by checking for `err > 0` instead.
    +    Fix this by checking for `err > 0` instead. This has the consequence
    +    that `reftable_stack_init_addition()` won't ever return a positive error
    +    code anymore, but will instead return `REFTABLE_LOCK_ERROR` now. Thus,
    +    we can drop the check for a positive error code in `stack_try_add()`
    +    now.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 2:  e837703ca1 =  2:  be7212006b reftable/error: discern locked/outdated errors
 3:  ae2130ffda !  3:  95dda44672 reftable/stack: use error codes when locking fails during compaction
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## reftable/stack.c ##
    +@@ reftable/stack.c: static int stack_write_compact(struct reftable_stack *st,
    + 	return err;
    + }
    + 
    +-/* <  0: error. 0 == OK, > 0 attempt failed; could retry. */
    ++/*
    ++ * Compact all tables in the range `[first, last)` into a single new table.
    ++ *
    ++ * This function returns `0` on success or a code `< 0` on failure. When the
    ++ * stack or any of the tables in the specified range are already locked then
    ++ * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that
    ++ * callers can either ignore, or they may choose to retry compaction after some
    ++ * amount of time.
    ++ */
    + static int stack_compact_range(struct reftable_stack *st,
    + 			       size_t first, size_t last,
    + 			       struct reftable_log_expiry_config *expiry)
     @@ reftable/stack.c: static int stack_compact_range(struct reftable_stack *st,
      					LOCK_NO_DEREF);
      	if (err < 0) {
 4:  37a18b91ca !  4:  50a3c37f92 reftable/stack: gracefully handle failed auto-compaction due to locks
    @@ Commit message
         invoking auto-compaction of the stack to keep the total number of tables
         at bay. This auto-compaction may fail though in case at least one of the
         tables which we are about to compact is locked. This is indicated by the
    -    compaction function returning a positive value. We do not handle this
    -    case though, and thus bubble that return value up the calling chain,
    -    which will ultimately cause a failure.
    +    compaction function returning `REFTABLE_LOCK_ERROR`. We do not handle
    +    this case though, and thus bubble that return value up the calling
    +    chain, which will ultimately cause a failure.
     
    -    Fix this bug by handling positive return values.
    +    Fix this bug by ignoring `REFTABLE_LOCK_ERROR`.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)
     +	if (!add->stack->disable_auto_compact) {
     +		/*
     +		 * Auto-compact the stack to keep the number of tables in
    -+		 * control. Note that we explicitly ignore locking issues which
    -+		 * may indicate that a concurrent process is already trying to
    -+		 * compact tables. This is fine, so we simply ignore that error
    -+		 * condition.
    ++		 * control. It is possible that a concurrent writer is already
    ++		 * trying to compact parts of the stack, which would lead to a
    ++		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
    ++		 * already. This is a benign error though, so we ignore it.
     +		 */
      		err = reftable_stack_auto_compact(add->stack);
     +		if (err < 0 && err != REFTABLE_LOCK_ERROR)
    @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)
      done:
      	reftable_addition_close(add);
     
    + ## reftable/stack_test.c ##
    +@@ reftable/stack_test.c: static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
    + 	clear_dir(dir);
    + }
    + 
    ++static void test_reftable_stack_auto_compaction_fails_gracefully(void)
    ++{
    ++	struct reftable_ref_record ref = {
    ++		.refname = "refs/heads/master",
    ++		.update_index = 1,
    ++		.value_type = REFTABLE_REF_VAL1,
    ++		.value.val1 = {0x01},
    ++	};
    ++	struct reftable_write_options cfg = {0};
    ++	struct reftable_stack *st;
    ++	struct strbuf table_path = STRBUF_INIT;
    ++	char *dir = get_tmp_dir(__LINE__);
    ++	int err;
    ++
    ++	err = reftable_new_stack(&st, dir, cfg);
    ++	EXPECT_ERR(err);
    ++
    ++	err = reftable_stack_add(st, write_test_ref, &ref);
    ++	EXPECT_ERR(err);
    ++	EXPECT(st->merged->stack_len == 1);
    ++	EXPECT(st->stats.attempts == 0);
    ++	EXPECT(st->stats.failures == 0);
    ++
    ++	/*
    ++	 * Lock the newly written table such that it cannot be compacted.
    ++	 * Adding a new table to the stack should not be impacted by this, even
    ++	 * though auto-compaction will now fail.
    ++	 */
    ++	strbuf_addf(&table_path, "%s/%s.lock", dir, st->readers[0]->name);
    ++	write_file_buf(table_path.buf, "", 0);
    ++
    ++	ref.update_index = 2;
    ++	err = reftable_stack_add(st, write_test_ref, &ref);
    ++	EXPECT_ERR(err);
    ++	EXPECT(st->merged->stack_len == 2);
    ++	EXPECT(st->stats.attempts == 1);
    ++	EXPECT(st->stats.failures == 1);
    ++
    ++	reftable_stack_destroy(st);
    ++	clear_dir(dir);
    ++}
    ++
    + static void test_reftable_stack_validate_refname(void)
    + {
    + 	struct reftable_write_options cfg = { 0 };
    +@@ reftable/stack_test.c: int stack_test_main(int argc, const char *argv[])
    + 	RUN_TEST(test_reftable_stack_tombstone);
    + 	RUN_TEST(test_reftable_stack_transaction_api);
    + 	RUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);
    ++	RUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);
    + 	RUN_TEST(test_reftable_stack_update_index_check);
    + 	RUN_TEST(test_reftable_stack_uptodate);
    + 	RUN_TEST(test_reftable_stack_validate_refname);
    +
      ## t/t0610-reftable-basics.sh ##
     @@ t/t0610-reftable-basics.sh: test_expect_success 'ref transaction: empty transaction in empty repo' '
      	EOF
 5:  f336db817c =  5:  6615f25b08 refs/reftable: print errors on compaction failure
 6:  999d0c2bb8 =  6:  e84b797728 t/helper: drop pack-refs wrapper
 7:  072627d82c =  7:  809094ffec refs: move `struct pack_refs_opts` to where it's used
 8:  919abe8eb9 =  8:  cf966fc584 refs: remove `PACK_REFS_ALL` flag
 9:  61ebcb2d52 =  9:  5d7af236d4 refs/reftable: expose auto compaction via new flag
10:  ff163a621d = 10:  23c6c20e4e builtin/pack-refs: release allocated memory
11:  8727f08bab = 11:  eb48ac0329 builtin/pack-refs: introduce new "--auto" flag
12:  65c9ff3ee5 = 12:  94cb036345 builtin/gc: move `struct maintenance_run_opts`
13:  817de1a88f = 13:  9657c67b3b t6500: extract objects with "17" prefix
14:  474cf66b26 ! 14:  3eaff8289b builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs
    @@ Commit message
         backend, which will continue to eagerly pack refs. But it does ensure
         that the "reftable" backend only compacts refs as required.
     
    -    This change does not impact git-maintenance(1) as it won't ever end up
    -    executing the pack-refs task when run with `--auto`.
    +    This change does not impact git-maintenance(1) because this command will
    +    in fact never run the pack-refs task when run with `--auto`. This issue
    +    will be addressed in a subsequent commit.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
15:  71f4800d36 = 15:  1bdea3b316 builtin/gc: pack refs when using `git maintenance run --auto`
-- 
2.44.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]