| Message ID | cover.1711360631.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | refs: introduce `--auto` to pack refs as needed | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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 > The range diff from this version looks good. Thanks!
Karthik Nayak <karthik.188@gmail.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> 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 >> > > The range diff from this version looks good. > Thanks! Let's mark the topic for 'next', then. Thanks, both. Will queue.
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`