From 630942a873ae0f1d067efaaf7b4d1b05cfb3a141 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:38 +0100 Subject: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()` In `reftable_stack_init_addition()` we call `stack_uptodate()` after having created the lockfile to check whether the stack was modified concurrently, which is indicated by a positive return code from the latter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller and abort the addition. The error handling has an off-by-one though because we check whether the error code is `> 1` instead of `> 0`. Thus, instead of returning the locking error, we would return a positive value. One of the callers of `reftable_stack_init_addition()` works around this bug by repeating the 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. 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 Signed-off-by: Junio C Hamano --- reftable/stack.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index 1ecf1b9751..92d9a7facb 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, err = stack_uptodate(st); if (err < 0) goto done; - - if (err > 1) { + if (err > 0) { err = REFTABLE_LOCK_ERROR; goto done; } @@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st, int err = reftable_stack_init_addition(&add, st); if (err < 0) goto done; - if (err > 0) { - err = REFTABLE_LOCK_ERROR; - goto done; - } err = reftable_addition_add(&add, write_table, arg); if (err < 0) From af18098c9d2b2e165aca127c35eeb98d157bd542 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:42 +0100 Subject: [PATCH 02/15] reftable/error: discern locked/outdated errors We currently throw two different errors into a similar-but-different error code: - Errors when trying to lock the reftable stack. - Errors when trying to write to the reftable stack which has been modified concurrently. This results in unclear error handling and user-visible error messages. Create a new `REFTABLE_OUTDATED_ERROR` so that those error conditions can be clearly told apart from each other. Adjust users of the old `REFTABLE_LOCK_ERROR` to use the new error code as required. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/error.c | 4 +++- reftable/reftable-error.h | 5 ++++- reftable/stack.c | 6 +++--- reftable/stack_test.c | 2 +- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/reftable/error.c b/reftable/error.c index 0d1766735e..cfb7a0fda4 100644 --- a/reftable/error.c +++ b/reftable/error.c @@ -22,7 +22,7 @@ const char *reftable_error_str(int err) case REFTABLE_NOT_EXIST_ERROR: return "file does not exist"; case REFTABLE_LOCK_ERROR: - return "data is outdated"; + return "data is locked"; case REFTABLE_API_ERROR: return "misuse of the reftable API"; case REFTABLE_ZLIB_ERROR: @@ -35,6 +35,8 @@ const char *reftable_error_str(int err) return "invalid refname"; case REFTABLE_ENTRY_TOO_BIG_ERROR: return "entry too large"; + case REFTABLE_OUTDATED_ERROR: + return "data concurrently modified"; case -1: return "general error"; default: diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h index 4c457aaaf8..e9b07c9f36 100644 --- a/reftable/reftable-error.h +++ b/reftable/reftable-error.h @@ -25,7 +25,7 @@ enum reftable_error { */ REFTABLE_NOT_EXIST_ERROR = -4, - /* Trying to write out-of-date data. */ + /* Trying to access locked data. */ REFTABLE_LOCK_ERROR = -5, /* Misuse of the API: @@ -57,6 +57,9 @@ enum reftable_error { /* Entry does not fit. This can happen when writing outsize reflog messages. */ REFTABLE_ENTRY_TOO_BIG_ERROR = -11, + + /* Trying to write out-of-date data. */ + REFTABLE_OUTDATED_ERROR = -12, }; /* convert the numeric error code to a string. The string should not be diff --git a/reftable/stack.c b/reftable/stack.c index 92d9a7facb..eaa8bb9c99 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -529,9 +529,9 @@ int reftable_stack_add(struct reftable_stack *st, { int err = stack_try_add(st, write, arg); if (err < 0) { - if (err == REFTABLE_LOCK_ERROR) { + if (err == REFTABLE_OUTDATED_ERROR) { /* Ignore error return, we want to propagate - REFTABLE_LOCK_ERROR. + REFTABLE_OUTDATED_ERROR. */ reftable_stack_reload(st); } @@ -591,7 +591,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, if (err < 0) goto done; if (err > 0) { - err = REFTABLE_LOCK_ERROR; + err = REFTABLE_OUTDATED_ERROR; goto done; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 509f486623..b0c7041a4f 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -232,7 +232,7 @@ static void test_reftable_stack_uptodate(void) EXPECT_ERR(err); err = reftable_stack_add(st2, &write_test_ref, &ref2); - EXPECT(err == REFTABLE_LOCK_ERROR); + EXPECT(err == REFTABLE_OUTDATED_ERROR); err = reftable_stack_reload(st2); EXPECT_ERR(err); From 33358350eb857a5fb273d143dcdfe15ac6d4db5d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:46 +0100 Subject: [PATCH 03/15] reftable/stack: use error codes when locking fails during compaction Compaction of a reftable stack may fail gracefully when there is a concurrent process that writes to the reftable stack and which has thus locked either the "tables.list" file or one of the tables. This is expected and can be handled gracefully by some of the callers which invoke compaction. Thus, to indicate this situation to our callers, we return a positive return code from `stack_compact_range()` and bubble it up to the caller. This kind of error handling is somewhat awkward though as many callers in the call chain never even think of handling positive return values. Thus, the result is either that such errors are swallowed by accident, or that we abort operations with an unhelpful error message. Make the code more robust by always using negative error codes when compaction fails, with `REFTABLE_LOCK_ERROR` for the described benign error case. Note that only a single callsite knew to handle positive error codes gracefully in the first place. Subsequent commits will touch up some of the other sites to handle those errors better. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index eaa8bb9c99..79856b6565 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -973,7 +973,15 @@ done: 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) @@ -1003,7 +1011,7 @@ static int stack_compact_range(struct reftable_stack *st, LOCK_NO_DEREF); if (err < 0) { if (errno == EEXIST) - err = 1; + err = REFTABLE_LOCK_ERROR; else err = REFTABLE_IO_ERROR; goto done; @@ -1025,7 +1033,7 @@ static int stack_compact_range(struct reftable_stack *st, table_name.buf, LOCK_NO_DEREF); if (err < 0) { if (errno == EEXIST) - err = 1; + err = REFTABLE_LOCK_ERROR; else err = REFTABLE_IO_ERROR; goto done; @@ -1075,7 +1083,7 @@ static int stack_compact_range(struct reftable_stack *st, LOCK_NO_DEREF); if (err < 0) { if (errno == EEXIST) - err = 1; + err = REFTABLE_LOCK_ERROR; else err = REFTABLE_IO_ERROR; goto done; @@ -1187,7 +1195,7 @@ static int stack_compact_range_stats(struct reftable_stack *st, struct reftable_log_expiry_config *config) { int err = stack_compact_range(st, first, last, config); - if (err > 0) + if (err == REFTABLE_LOCK_ERROR) st->stats.failures++; return err; } From a2f711ade0c4816a59155d72559cbc4759cd4699 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:50 +0100 Subject: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks Whenever we commit a new table to the reftable stack we will end up 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 `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 ignoring `REFTABLE_LOCK_ERROR`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 13 ++++++++++- reftable/stack_test.c | 44 ++++++++++++++++++++++++++++++++++++++ t/t0610-reftable-basics.sh | 20 +++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/reftable/stack.c b/reftable/stack.c index 79856b6565..dde50b61d6 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add) if (err) goto done; - if (!add->stack->disable_auto_compact) + if (!add->stack->disable_auto_compact) { + /* + * Auto-compact the stack to keep the number of tables in + * 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) + goto done; + err = 0; + } done: reftable_addition_close(add); diff --git a/reftable/stack_test.c b/reftable/stack_test.c index b0c7041a4f..887f458e5c 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -343,6 +343,49 @@ 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); + strbuf_release(&table_path); + clear_dir(dir); +} + static void test_reftable_stack_validate_refname(void) { struct reftable_write_options cfg = { 0 }; @@ -1089,6 +1132,7 @@ 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); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 686781192e..5f2f9baa9b 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' ' EOF ' +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + test_commit A && + for i in $(test_seq 10) + do + git branch branch-$i && + for table in .git/reftable/*.ref + do + touch "$table.lock" || exit 1 + done || + exit 1 + done && + test_line_count = 13 .git/reftable/tables.list + ) +' + test_expect_success 'pack-refs: compacts tables' ' test_when_finished "rm -rf repo" && git init repo && From 4ccf7060d8d0a3c08d1fb03b038a164eb4913d02 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:54 +0100 Subject: [PATCH 05/15] refs/reftable: print errors on compaction failure When git-pack-refs(1) fails in the reftable backend we end up printing no error message at all, leaving the caller puzzled as to why compaction has failed. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/reftable-backend.c | 6 +++++- t/t0610-reftable-basics.sh | 12 ++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 74dab18eda..66cdbbdb24 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1221,8 +1221,12 @@ static int reftable_be_pack_refs(struct ref_store *ref_store, stack = refs->main_stack; ret = reftable_stack_compact_all(stack, NULL); - if (ret) + if (ret < 0) { + ret = error(_("unable to compact stack: %s"), + reftable_error_str(ret)); goto out; + } + ret = reftable_stack_clean(stack); if (ret) goto out; diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 5f2f9baa9b..a53d1dc493 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -375,6 +375,18 @@ test_expect_success 'pack-refs: compacts tables' ' test_line_count = 1 repo/.git/reftable/tables.list ' +test_expect_success 'pack-refs: compaction raises locking errors' ' + test_when_finished "rm -rf repo" && + git init repo && + test_commit -C repo A && + touch repo/.git/reftable/tables.list.lock && + cat >expect <<-EOF && + error: unable to compact stack: data is locked + EOF + test_must_fail git -C repo pack-refs 2>err && + test_cmp expect err +' + test_expect_success 'pack-refs: prunes stale tables' ' test_when_finished "rm -rf repo" && git init repo && From ed12124d4a809c832ae7f963ecc71104ba5cd55b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:02:59 +0100 Subject: [PATCH 06/15] t/helper: drop pack-refs wrapper The test helper provides a "ref-store pack-refs" wrapper that more or less directly invokes `refs_pack_refs()`. This helper is only used in a single test with the "PACK_REFS_PRUNE" and "PACK_REFS_ALL" flags. Both of these flags can directly be accessed via git-pack-refs(1) though via the `--all` and `--prune` flags, which makes the helper superfluous. Refactor the test to use git-pack-refs(1) instead of the test helper. Drop the now-unused test helper command. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-ref-store.c | 20 -------------------- t/t0601-reffiles-pack-refs.sh | 13 +++++++++---- 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 7a0f6cac53..82bbf6e2e6 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -112,25 +112,6 @@ static const char **get_store(const char **argv, struct ref_store **refs) return argv + 1; } -static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE), - FLAG_DEF(PACK_REFS_ALL), - { NULL, 0 } }; - -static int cmd_pack_refs(struct ref_store *refs, const char **argv) -{ - unsigned int flags = arg_flags(*argv++, "flags", pack_flags); - static struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT; - static struct string_list included_refs = STRING_LIST_INIT_NODUP; - struct pack_refs_opts pack_opts = { .flags = flags, - .exclusions = &exclusions, - .includes = &included_refs }; - - if (pack_opts.flags & PACK_REFS_ALL) - string_list_append(pack_opts.includes, "*"); - - return refs_pack_refs(refs, &pack_opts); -} - static int cmd_create_symref(struct ref_store *refs, const char **argv) { const char *refname = notnull(*argv++, "refname"); @@ -326,7 +307,6 @@ struct command { }; static struct command commands[] = { - { "pack-refs", cmd_pack_refs }, { "create-symref", cmd_create_symref }, { "delete-refs", cmd_delete_refs }, { "rename-ref", cmd_rename_ref }, diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh index c309d2bae8..b1cf587347 100755 --- a/t/t0601-reffiles-pack-refs.sh +++ b/t/t0601-reffiles-pack-refs.sh @@ -32,11 +32,16 @@ test_expect_success 'prepare a trivial repository' ' HEAD=$(git rev-parse --verify HEAD) ' -test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' ' - N=`find .git/refs -type f | wc -l` && +test_expect_success 'pack-refs --prune --all' ' + test_path_is_missing .git/packed-refs && + git pack-refs --no-prune --all && + test_path_is_file .git/packed-refs && + N=$(find .git/refs -type f | wc -l) && test "$N" != 0 && - test-tool ref-store main pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL && - N=`find .git/refs -type f` && + + git pack-refs --prune --all && + test_path_is_file .git/packed-refs && + N=$(find .git/refs -type f) && test -z "$N" ' From 0f65c7a6761142c7abc6a2415325eda6f44edf49 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:03 +0100 Subject: [PATCH 07/15] refs: move `struct pack_refs_opts` to where it's used The declaration of `struct pack_refs_opts` is in a seemingly random place. Move it so that it's located right next to its flags and functions that use it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/refs.h b/refs.h index 298caf6c61..139ce7113b 100644 --- a/refs.h +++ b/refs.h @@ -66,12 +66,6 @@ const char *ref_storage_format_to_name(unsigned int ref_storage_format); #define RESOLVE_REF_NO_RECURSE 0x02 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04 -struct pack_refs_opts { - unsigned int flags; - struct ref_exclusions *exclusions; - struct string_list *includes; -}; - const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, int resolve_flags, @@ -433,6 +427,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, #define PACK_REFS_PRUNE 0x0001 #define PACK_REFS_ALL 0x0002 +struct pack_refs_opts { + unsigned int flags; + struct ref_exclusions *exclusions; + struct string_list *includes; +}; + /* * Write a packed-refs file for the current repository. * flags: Combination of the above PACK_REFS_* flags. From 35aeabd6c2845b89515f1fb374231dcc5d63d61d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:07 +0100 Subject: [PATCH 08/15] refs: remove `PACK_REFS_ALL` flag The intent of the `PACK_REFS_ALL` flag is to ask the backend to compact all refs instead of only a subset of them. Thus, this flag gets passed down to `refs_pack_refs()` via `struct pack_refs_opts::flags`. But starting with 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12), the flag's semantics have changed. Instead of being handled by the respective backends, this flag is now getting handled by the callers of `refs_pack_refs()` which will add a single glob ("*") to the list of refs-to-be-packed. Thus, the flag serves no purpose to the ref backends anymore. Remove the flag and replace it with a local variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-refs.c | 5 +++-- refs.h | 2 -- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c index bcf383cac9..97921beef2 100644 --- a/builtin/pack-refs.c +++ b/builtin/pack-refs.c @@ -21,9 +21,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) .flags = flags }; static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP; struct string_list_item *item; + int pack_all = 0; struct option opts[] = { - OPT_BIT(0, "all", &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL), + OPT_BOOL(0, "all", &pack_all, N_("pack everything")), OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE), OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"), N_("references to include")), @@ -38,7 +39,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) for_each_string_list_item(item, &option_excluded_refs) add_ref_exclusion(pack_refs_opts.exclusions, item->string); - if (pack_refs_opts.flags & PACK_REFS_ALL) + if (pack_all) string_list_append(pack_refs_opts.includes, "*"); if (!pack_refs_opts.includes->nr) diff --git a/refs.h b/refs.h index 139ce7113b..8c8994cb29 100644 --- a/refs.h +++ b/refs.h @@ -422,10 +422,8 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, /* * Flags for controlling behaviour of pack_refs() * PACK_REFS_PRUNE: Prune loose refs after packing - * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs */ #define PACK_REFS_PRUNE 0x0001 -#define PACK_REFS_ALL 0x0002 struct pack_refs_opts { unsigned int flags; From f89356db4a26eef47aceb2d03039fff6b1a4180d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:11 +0100 Subject: [PATCH 09/15] refs/reftable: expose auto compaction via new flag Under normal circumstances, the "reftable" backend will automatically perform compaction after appending to the stack. It is thus not necessary and may even be considered wasteful to run git-pack-refs(1) in "reftable"-backed repositories as it will cause the backend to compact all tables into a single one. We do exactly that though when running `git maintenance run --auto` or `git gc --auto`, which gets spawned by Git after running some specific commands. The `--auto` mode is typically only executing optimizations as needed. To do so, we already use several heuristics for the various different data structures in Git to determine whether to optimize them or not. We do not use any heuristics for refs though and instead always optimize them. Introduce a new `PACK_REFS_AUTO` flag that can be passed to the backend. When not handled by the backend we will continue to behave the exact same as we do right now, that is we optimize refs unconditionally. This is done for the "files" backend for now to retain current behaviour, even though we may eventually also want to introduce heuristics here. For the "reftable" backend though we already do have auto-compaction, so we can easily reuse that logic to implement the new auto-packing flag. Note that under normal circumstances, this should always end up being a no-op. After all, we already invoke the code for every single addition to the stack. But there are special cases where it can still be helpful to execute the auto-compaction code explicitly: - Concurrent writers may cause compaction to not run due to locks. - Callers may decide to disable compaction altogether and then pack refs at a later point due to various reasons. - Other implementations of the reftable format may do compaction differently or even not at all. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.h | 6 +++++- refs/reftable-backend.c | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/refs.h b/refs.h index 8c8994cb29..d278775e08 100644 --- a/refs.h +++ b/refs.h @@ -422,8 +422,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, /* * Flags for controlling behaviour of pack_refs() * PACK_REFS_PRUNE: Prune loose refs after packing + * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end + * result are decided by the ref backend. Backends may ignore + * this flag and fall back to a normal repack. */ -#define PACK_REFS_PRUNE 0x0001 +#define PACK_REFS_PRUNE (1 << 0) +#define PACK_REFS_AUTO (1 << 1) struct pack_refs_opts { unsigned int flags; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 66cdbbdb24..135bd4e268 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1220,7 +1220,10 @@ static int reftable_be_pack_refs(struct ref_store *ref_store, if (!stack) stack = refs->main_stack; - ret = reftable_stack_compact_all(stack, NULL); + if (opts->flags & PACK_REFS_AUTO) + ret = reftable_stack_auto_compact(stack); + else + ret = reftable_stack_compact_all(stack, NULL); if (ret < 0) { ret = error(_("unable to compact stack: %s"), reftable_error_str(ret)); From a75dc71f3772cee10dfb93861610d8ee413ad207 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:15 +0100 Subject: [PATCH 10/15] builtin/pack-refs: release allocated memory Some of the command line options in `cmd_pack_refs()` require us to allocate memory. This memory is never released and thus leaking, but we paper over this leak by declaring the respective variables as `static` function-level variables, which is somewhat awkward. Refactor the code to release the allocated memory and drop the `static` declaration. While at it, remove the useless `flags` variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/pack-refs.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c index 97921beef2..ea2baeec76 100644 --- a/builtin/pack-refs.c +++ b/builtin/pack-refs.c @@ -13,15 +13,17 @@ static char const * const pack_refs_usage[] = { int cmd_pack_refs(int argc, const char **argv, const char *prefix) { - unsigned int flags = PACK_REFS_PRUNE; - static struct ref_exclusions excludes = REF_EXCLUSIONS_INIT; - static struct string_list included_refs = STRING_LIST_INIT_NODUP; - struct pack_refs_opts pack_refs_opts = { .exclusions = &excludes, - .includes = &included_refs, - .flags = flags }; - static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP; + struct ref_exclusions excludes = REF_EXCLUSIONS_INIT; + struct string_list included_refs = STRING_LIST_INIT_NODUP; + struct pack_refs_opts pack_refs_opts = { + .exclusions = &excludes, + .includes = &included_refs, + .flags = PACK_REFS_PRUNE, + }; + struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP; struct string_list_item *item; int pack_all = 0; + int ret; struct option opts[] = { OPT_BOOL(0, "all", &pack_all, N_("pack everything")), @@ -45,5 +47,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) if (!pack_refs_opts.includes->nr) string_list_append(pack_refs_opts.includes, "refs/tags/*"); - return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts); + ret = refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts); + + clear_ref_exclusions(&excludes); + string_list_clear(&included_refs, 0); + string_list_clear(&option_excluded_refs, 0); + return ret; } From 6dcffc68f4cea97d4b4af8a9add0daae54a5c292 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:20 +0100 Subject: [PATCH 11/15] builtin/pack-refs: introduce new "--auto" flag Calling git-pack-refs(1) will unconditionally cause it to pack all requested refs regardless of the current state of the ref database. For example: - With the "files" backend we will end up rewriting the complete "packed-refs" file even if only a single ref would require compaction. - With the "reftable" backend we will end up always compacting all tables into a single table. This behaviour can be completely unnecessary depending on the backend and is thus wasteful. With the introduction of the `PACK_REFS_AUTO` flag in the preceding commit we can improve this and let the backends decide for themselves whether to pack refs in the first place. Expose this functionality via a new "--auto" flag in git-pack-refs(1), which mirrors the same flag in both git-gc(1) and git-maintenance(1). Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/git-pack-refs.txt | 15 ++++++++++++++- builtin/pack-refs.c | 3 ++- t/t0601-reffiles-pack-refs.sh | 7 +++++++ t/t0610-reftable-basics.sh | 34 +++++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt index 284956acb3..2dcabaf74c 100644 --- a/Documentation/git-pack-refs.txt +++ b/Documentation/git-pack-refs.txt @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access SYNOPSIS -------- [verse] -'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ] +'git pack-refs' [--all] [--no-prune] [--auto] [--include ] [--exclude ] DESCRIPTION ----------- @@ -60,6 +60,19 @@ with many branches of historical interests. The command usually removes loose refs under `$GIT_DIR/refs` hierarchy after packing them. This option tells it not to. +--auto:: + +Pack refs as needed depending on the current state of the ref database. The +behavior depends on the ref format used by the repository and may change in the +future. ++ + - "files": No special handling for `--auto` has been implemented. ++ + - "reftable": Tables are compacted such that they form a geometric + sequence. For two tables N and N+1, where N+1 is newer, this + maintains the property that N is at least twice as big as N+1. Only + tables that violate this property are compacted. + --include :: Pack refs based on a `glob(7)` pattern. Repetitions of this option diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c index ea2baeec76..db40825666 100644 --- a/builtin/pack-refs.c +++ b/builtin/pack-refs.c @@ -7,7 +7,7 @@ #include "revision.h" static char const * const pack_refs_usage[] = { - N_("git pack-refs [--all] [--no-prune] [--include ] [--exclude ]"), + N_("git pack-refs [--all] [--no-prune] [--auto] [--include ] [--exclude ]"), NULL }; @@ -28,6 +28,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) struct option opts[] = { OPT_BOOL(0, "all", &pack_all, N_("pack everything")), OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE), + OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), PACK_REFS_AUTO), OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"), N_("references to include")), OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"), diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh index b1cf587347..219a495451 100755 --- a/t/t0601-reffiles-pack-refs.sh +++ b/t/t0601-reffiles-pack-refs.sh @@ -164,6 +164,13 @@ test_expect_success 'test --exclude takes precedence over --include' ' git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" && test -f .git/refs/heads/dont_pack5' +test_expect_success '--auto packs and prunes refs as usual' ' + git branch auto && + test_path_is_file .git/refs/heads/auto && + git pack-refs --auto --all && + test_path_is_missing .git/refs/heads/auto +' + test_expect_success 'see if up-to-date packed refs are preserved' ' git branch q && git pack-refs --all --prune && diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index a53d1dc493..6de7529575 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -387,6 +387,40 @@ test_expect_success 'pack-refs: compaction raises locking errors' ' test_cmp expect err ' +test_expect_success 'pack-refs: auto compaction' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + test_commit A && + + # The tables should have been auto-compacted, and thus auto + # compaction should not have to do anything. + ls -1 .git/reftable >tables-expect && + test_line_count = 4 tables-expect && + git pack-refs --auto && + ls -1 .git/reftable >tables-actual && + test_cmp tables-expect tables-actual && + + # Lock all tables write some refs. Auto-compaction will be + # unable to compact tables and thus fails gracefully, leaving + # the stack in a sub-optimal state. + ls .git/reftable/*.ref | + while read table + do + touch "$table.lock" || exit 1 + done && + git branch B && + git branch C && + rm .git/reftable/*.lock && + test_line_count = 5 .git/reftable/tables.list && + + git pack-refs --auto && + test_line_count = 1 .git/reftable/tables.list + ) +' + test_expect_success 'pack-refs: prunes stale tables' ' test_when_finished "rm -rf repo" && git init repo && From 0e05d53992b85cd19ce5f3c5d476ded10e2b63e1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:24 +0100 Subject: [PATCH 12/15] builtin/gc: move `struct maintenance_run_opts` We're about to start using `struct maintenance_run_opts` in `maintenance_task_pack_refs()`. Move its definition up to prepare for this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/gc.c | 53 ++++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index cb80ced6cb..e0029c88f9 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -180,7 +180,32 @@ static void gc_config(void) git_config(git_default_config, NULL); } -struct maintenance_run_opts; +enum schedule_priority { + SCHEDULE_NONE = 0, + SCHEDULE_WEEKLY = 1, + SCHEDULE_DAILY = 2, + SCHEDULE_HOURLY = 3, +}; + +static enum schedule_priority parse_schedule(const char *value) +{ + if (!value) + return SCHEDULE_NONE; + if (!strcasecmp(value, "hourly")) + return SCHEDULE_HOURLY; + if (!strcasecmp(value, "daily")) + return SCHEDULE_DAILY; + if (!strcasecmp(value, "weekly")) + return SCHEDULE_WEEKLY; + return SCHEDULE_NONE; +} + +struct maintenance_run_opts { + int auto_flag; + int quiet; + enum schedule_priority schedule; +}; + static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts) { struct child_process cmd = CHILD_PROCESS_INIT; @@ -773,26 +798,6 @@ static const char *const builtin_maintenance_run_usage[] = { NULL }; -enum schedule_priority { - SCHEDULE_NONE = 0, - SCHEDULE_WEEKLY = 1, - SCHEDULE_DAILY = 2, - SCHEDULE_HOURLY = 3, -}; - -static enum schedule_priority parse_schedule(const char *value) -{ - if (!value) - return SCHEDULE_NONE; - if (!strcasecmp(value, "hourly")) - return SCHEDULE_HOURLY; - if (!strcasecmp(value, "daily")) - return SCHEDULE_DAILY; - if (!strcasecmp(value, "weekly")) - return SCHEDULE_WEEKLY; - return SCHEDULE_NONE; -} - static int maintenance_opt_schedule(const struct option *opt, const char *arg, int unset) { @@ -809,12 +814,6 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg, return 0; } -struct maintenance_run_opts { - int auto_flag; - int quiet; - enum schedule_priority schedule; -}; - /* Remember to update object flag allocation in object.h */ #define SEEN (1u<<0) From 77257e3c7e342acc0022c8c206db6c248cbfeb85 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:28 +0100 Subject: [PATCH 13/15] t6500: extract objects with "17" prefix The ".git/obects/17/" shard is somewhat special because it is used by git-gc(1) to estimate how many objects there are by extrapolating the number of objects in that shard, only. In t6500 we thus have a hard coded set of data that, when written to the object database, result in blobs starting with that prefix. We are about to need such "17"-prefixed objects in another test suite. Extract them into "t/oid-info/hash-info" so that they can be reused by other tests. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/oid-info/hash-info | 12 ++++++++++++ t/t6500-gc.sh | 30 +++++++----------------------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/t/oid-info/hash-info b/t/oid-info/hash-info index d0736dd1a0..b8a5bcb187 100644 --- a/t/oid-info/hash-info +++ b/t/oid-info/hash-info @@ -15,3 +15,15 @@ empty_blob sha256:473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a3037218 empty_tree sha1:4b825dc642cb6eb9a060e54bf8d69288fbee4904 empty_tree sha256:6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 + +blob17_1 sha1:263 +blob17_1 sha256:34 + +blob17_2 sha1:410 +blob17_2 sha256:174 + +blob17_3 sha1:523 +blob17_3 sha256:313 + +blob17_4 sha1:790 +blob17_4 sha256:481 diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 18fe1c25e6..43d40175f8 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -11,23 +11,7 @@ test_expect_success 'setup' ' # behavior, make sure we always pack everything to one pack by # default git config gc.bigPackThreshold 2g && - - # These are simply values which, when hashed as a blob with a newline, - # produce a hash where the first byte is 0x17 in their respective - # algorithms. - test_oid_cache <<-EOF - obj1 sha1:263 - obj1 sha256:34 - - obj2 sha1:410 - obj2 sha256:174 - - obj3 sha1:523 - obj3 sha256:313 - - obj4 sha1:790 - obj4 sha256:481 - EOF + test_oid_init ' test_expect_success 'gc empty repository' ' @@ -114,8 +98,8 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' ' # We need to create two object whose sha1s start with 17 # since this is what git gc counts. As it happens, these # two blobs will do so. - test_commit "$(test_oid obj1)" && - test_commit "$(test_oid obj2)" && + test_commit "$(test_oid blob17_1)" && + test_commit "$(test_oid blob17_2)" && git gc --auto >../out.actual 2>../err.actual ) && @@ -146,13 +130,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre # We need to create two object whose sha1s start with 17 # since this is what git gc counts. As it happens, these # two blobs will do so. - test_commit "$(test_oid obj1)" && - test_commit "$(test_oid obj2)" && + test_commit "$(test_oid blob17_1)" && + test_commit "$(test_oid blob17_2)" && # Our first gc will create a pack; our second will create a second pack git gc --auto && ls .git/objects/pack/pack-*.pack | sort >existing_packs && - test_commit "$(test_oid obj3)" && - test_commit "$(test_oid obj4)" && + test_commit "$(test_oid blob17_3)" && + test_commit "$(test_oid blob17_4)" && git gc --auto 2>err && test_grep ! "^warning:" err && From bfc2f9eb8e83f394cc397f978c833edced507923 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:32 +0100 Subject: [PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs Forward the `--auto` flag to git-pack-refs(1) when it has been invoked with this flag itself. This does not change anything for the "files" 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) 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 Signed-off-by: Junio C Hamano --- builtin/gc.c | 21 ++++++++++++--------- t/t0610-reftable-basics.sh | 19 ++++++++++++++++--- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index e0029c88f9..bf1f2a621a 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -212,6 +212,9 @@ static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts * cmd.git_cmd = 1; strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL); + if (opts->auto_flag) + strvec_push(&cmd.args, "--auto"); + return run_command(&cmd); } @@ -572,7 +575,7 @@ done: return ret; } -static void gc_before_repack(void) +static void gc_before_repack(struct maintenance_run_opts *opts) { /* * We may be called twice, as both the pre- and @@ -583,7 +586,7 @@ static void gc_before_repack(void) if (done++) return; - if (pack_refs && maintenance_task_pack_refs(NULL)) + if (pack_refs && maintenance_task_pack_refs(opts)) die(FAILED_RUN, "pack-refs"); if (prune_reflogs) { @@ -599,7 +602,6 @@ static void gc_before_repack(void) int cmd_gc(int argc, const char **argv, const char *prefix) { int aggressive = 0; - int auto_gc = 0; int quiet = 0; int force = 0; const char *name; @@ -608,6 +610,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) int keep_largest_pack = -1; timestamp_t dummy; struct child_process rerere_cmd = CHILD_PROCESS_INIT; + struct maintenance_run_opts opts = {0}; struct option builtin_gc_options[] = { OPT__QUIET(&quiet, N_("suppress progress reporting")), @@ -618,7 +621,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) OPT_MAGNITUDE(0, "max-cruft-size", &max_cruft_size, N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")), - OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"), + OPT_BOOL_F(0, "auto", &opts.auto_flag, N_("enable auto-gc mode"), PARSE_OPT_NOCOMPLETE), OPT_BOOL_F(0, "force", &force, N_("force running gc even if there may be another gc running"), @@ -663,7 +666,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) if (quiet) strvec_push(&repack, "-q"); - if (auto_gc) { + if (opts.auto_flag) { /* * Auto-gc should be least intrusive as possible. */ @@ -688,7 +691,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) if (lock_repo_for_gc(force, &pid)) return 0; - gc_before_repack(); /* dies on failure */ + gc_before_repack(&opts); /* dies on failure */ delete_tempfile(&pidfile); /* @@ -713,7 +716,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) name = lock_repo_for_gc(force, &pid); if (name) { - if (auto_gc) + if (opts.auto_flag) return 0; /* be quiet on --auto */ die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"), name, (uintmax_t)pid); @@ -728,7 +731,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) atexit(process_log_file_at_exit); } - gc_before_repack(); + gc_before_repack(&opts); if (!repository_format_precious_objects) { struct child_process repack_cmd = CHILD_PROCESS_INIT; @@ -783,7 +786,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0, NULL); - if (auto_gc && too_many_loose_objects()) + if (opts.auto_flag && too_many_loose_objects()) warning(_("There are too many unreachable loose objects; " "run 'git prune' to remove them.")); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 6de7529575..b37d8bf3b1 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -387,7 +387,9 @@ test_expect_success 'pack-refs: compaction raises locking errors' ' test_cmp expect err ' -test_expect_success 'pack-refs: auto compaction' ' +for command in pack-refs gc +do +test_expect_success "$command: auto compaction" ' test_when_finished "rm -rf repo" && git init repo && ( @@ -395,14 +397,24 @@ test_expect_success 'pack-refs: auto compaction' ' test_commit A && + # We need a bit of setup to ensure that git-gc(1) actually + # triggers, and that it does not write anything to the refdb. + git config gc.auto 1 && + git config gc.autoDetach 0 && + git config gc.reflogExpire never && + git config gc.reflogExpireUnreachable never && + test_oid blob17_1 | git hash-object -w --stdin && + # The tables should have been auto-compacted, and thus auto # compaction should not have to do anything. ls -1 .git/reftable >tables-expect && test_line_count = 4 tables-expect && - git pack-refs --auto && + git $command --auto && ls -1 .git/reftable >tables-actual && test_cmp tables-expect tables-actual && + test_oid blob17_2 | git hash-object -w --stdin && + # Lock all tables write some refs. Auto-compaction will be # unable to compact tables and thus fails gracefully, leaving # the stack in a sub-optimal state. @@ -416,10 +428,11 @@ test_expect_success 'pack-refs: auto compaction' ' rm .git/reftable/*.lock && test_line_count = 5 .git/reftable/tables.list && - git pack-refs --auto && + git $command --auto && test_line_count = 1 .git/reftable/tables.list ) ' +done test_expect_success 'pack-refs: prunes stale tables' ' test_when_finished "rm -rf repo" && From 9f6714ab3e61ad58c4532077d4b8dc807ff0410d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 25 Mar 2024 11:03:36 +0100 Subject: [PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto` When running `git maintenance run --auto`, then the various subtasks will only run as needed. Thus, we for example end up only packing loose objects if we hit a certain threshold. Interestingly enough, the "pack-refs" task is actually _never_ executed when the auto-flag is set because it does not have a condition at all. As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions: The 'auto_condition' function pointer is left NULL for now. We could extend this in the future to have a condition check if pack-refs should be run during 'git maintenance run --auto'. It is not quite clear from that quote whether it is actually intended that the task doesn't run at all in this mode. Also, no test was added to verify this behaviour. Ultimately though, it feels quite surprising that `git maintenance run --auto --task=pack-refs` would quietly never do anything at all. In any case, now that we do have the logic in place to let ref backends decide whether or not to repack refs, it does make sense to wire it up accordingly. With the "reftable" backend we will thus now perform auto-compaction, which optimizes the refdb as needed. But for the "files" backend we now unconditionally pack refs as it does not yet know to handle the "auto" flag. Arguably, this can be seen as a bug fix given that previously the task never did anything at all. Eventually though we should amend the "files" backend to use some heuristics for auto compaction, as well. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/gc.c | 12 +++++++++++- t/t0601-reffiles-pack-refs.sh | 10 ++++++++++ t/t0610-reftable-basics.sh | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index bf1f2a621a..3c874b248b 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -206,6 +206,16 @@ struct maintenance_run_opts { enum schedule_priority schedule; }; +static int pack_refs_condition(void) +{ + /* + * The auto-repacking logic for refs is handled by the ref backends and + * exposed via `git pack-refs --auto`. We thus always return truish + * here and let the backend decide for us. + */ + return 1; +} + static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts) { struct child_process cmd = CHILD_PROCESS_INIT; @@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = { [TASK_PACK_REFS] = { "pack-refs", maintenance_task_pack_refs, - NULL, + pack_refs_condition, }, }; diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh index 219a495451..7d4ab0b91a 100755 --- a/t/t0601-reffiles-pack-refs.sh +++ b/t/t0601-reffiles-pack-refs.sh @@ -370,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' ' test_cmp expect actual ' +test_expect_success 'maintenance --auto unconditionally packs loose refs' ' + git update-ref refs/heads/something HEAD && + test_path_is_file .git/refs/heads/something && + git rev-parse refs/heads/something >expect && + git maintenance run --task=pack-refs --auto && + test_path_is_missing .git/refs/heads/something && + git rev-parse refs/heads/something >actual && + test_cmp expect actual +' + test_done diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index b37d8bf3b1..931d888bbb 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' ' test_cmp expect err ' -for command in pack-refs gc +for command in pack-refs gc "maintenance run --task=pack-refs" do test_expect_success "$command: auto compaction" ' test_when_finished "rm -rf repo" &&