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@xxxxxx>
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@xxxxxx>
## 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@xxxxxx>
@@ 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@xxxxxx>
15: 71f4800d36 = 15: 1bdea3b316 builtin/gc: pack refs when using `git maintenance run --auto`
--
2.44.GIT
Attachment:
signature.asc
Description: PGP signature
