{"thread":{"id":"61142","subject":"[PATCH 00/15] refs: introduce `--auto` to pack refs as needed","startedAt":"2024-03-18T10:52:29Z","lastAt":"2024-03-25T16:56:43Z","messageCount":49,"participants":["Patrick Steinhardt","Karthik Nayak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"490843","messageId":"cover.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":null,"subject":"[PATCH 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:22Z","receivedAt":"2024-03-18T10:52:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nright now, it is left to the caller of `refs_pack_refs()` to decide\nwhether or not to repack refs and how exactly to do it. This is\ninefficient at times given that the caller typically has no idea whether\nor not the refdb is close to optimal already. So even if the refdb is\nalready in a close-to-optimal state we would end up repacking all of it.\n\nThis patch series aims to address this shortcoming. It introduces a new\nflag `PACK_REFS_AUTO` that tells the ref backend to repack refs as\nneeded. For the \"files\" backend we don't honor this flag yet and thus\nend up behaving the exact same as if that flag wasn't set. But for the\n\"reftable\" backend we will use the same auto-compaction algorithm that\nis already used during writes to the refdb. Thus, in most of the cases\nit wouldn't actually do anything except when the refdb is suboptimally\npacked.\n\nEventually we'll probably also want to wire up heuristics for the\n\"files\" backend to honor the `PACK_REFS_AUTO` flag. Using something like\na ratio of packed-refs size/number of loose refs might be viable. I\npunted on that though as I feared that it might lead to bikeshedding and\nthus distract from the main goal of this topic, which is to prepare the\nref code and relevant commands to perform optimizations as required. I'm\nhappy to add such a patch to this series though in case anybody feels\nstrongly about this.\n\nThe `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`\nflag. It is wired up in both `git gc --auto` and `git maintenance run\n--auto`.\n\nThe series is structured as follows:\n\n - Patches 1 - 5: Bugfixes and improvements for the reftable-specific\n compaction code. These are issues that I've found while working on\n this series.\n\n - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,\n which isn't actually used by the ref backends anymore and confused\n me multiple times.\n\n - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),\n git-gc(1) and git-maintenance(1).\n\nThe patch series is built on top of 2953d95d40 (The eighth batch,\n2024-03-15) with \"ps/reftable-stack-tempfile\" at 60c4c42515\n(reftable/stack: register compacted tables as tempfiles, 2024-03-07)\nmerged into it due to a merge conflict in \"reftable/stack.c\".\n\nPatrick\n\nPS: I polished this patch series while traveling and am still out of\n office until Thursday. I'll thus only get to respond to threads that\n are waiting for my input at the end of this week.\n\nPatrick Steinhardt (15):\n reftable/stack: fix error handling in `reftable_stack_init_addition()`\n reftable/error: discern locked/outdated errors\n reftable/stack: use error codes when locking fails during compaction\n reftable/stack: gracefully handle failed auto-compaction due to locks\n refs/reftable: print errors on compaction failure\n t/helper: drop pack-refs wrapper\n refs: move `struct pack_refs_opts` to where it's used\n refs: remove `PACK_REFS_ALL` flag\n refs/reftable: expose auto compaction via new flag\n builtin/pack-refs: release allocated memory\n builtin/pack-refs: introduce new \"--auto\" flag\n builtin/gc: move `struct maintenance_run_opts`\n t6500: extract objects with \"17\" prefix\n builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs\n builtin/gc: pack refs when using `git maintenance run --auto`\n\n Documentation/git-pack-refs.txt | 15 +++++-\n builtin/gc.c | 86 +++++++++++++++++++--------------\n builtin/pack-refs.c | 31 +++++++-----\n refs.h | 20 ++++----\n refs/reftable-backend.c | 11 ++++-\n reftable/error.c | 4 +-\n reftable/reftable-error.h | 5 +-\n reftable/stack.c | 34 +++++++------\n reftable/stack_test.c | 2 +-\n t/helper/test-ref-store.c | 20 --------\n t/oid-info/hash-info | 12 +++++\n t/t0601-reffiles-pack-refs.sh | 30 ++++++++++--\n t/t0610-reftable-basics.sh | 79 ++++++++++++++++++++++++++++++\n t/t6500-gc.sh | 30 +++---------\n 14 files changed, 255 insertions(+), 124 deletions(-)\n\n\nbase-commit: f94fee98b456ef39a5f85fff5802f4e538a4dc7b\n-- \n2.44.0\n\n"},{"id":"490844","messageId":"1e39d93a45db502280ecff383d53e0294f969719.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:27Z","receivedAt":"2024-03-18T10:52:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_init_addition()` we call `stack_uptodate()` after\nhaving created the lockfile to check whether the stack was modified\nconcurrently, which is indicated by a positive return code from the\nlatter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller\nand abort the addition.\n\nThe error handling has an off-by-one though because we check whether the\nerror code is `> 1` instead of `> 0`. Thus, instead of returning the\nlocking error, we would return a positive value. One of the callers of\n`reftable_stack_init_addition()` works around this bug by repeating the\nerror code check without the off-by-one. But other callers are subtly\nbroken by this bug.\n\nFix this by checking for `err > 0` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ecf1b9751..92d9a7facb 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n-\n-\tif (err > 1) {\n+\tif (err > 0) {\n \t\terr = REFTABLE_LOCK_ERROR;\n \t\tgoto done;\n \t}\n@@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st,\n \tint err = reftable_stack_init_addition(&add, st);\n \tif (err < 0)\n \t\tgoto done;\n-\tif (err > 0) {\n-\t\terr = REFTABLE_LOCK_ERROR;\n-\t\tgoto done;\n-\t}\n \n \terr = reftable_addition_add(&add, write_table, arg);\n \tif (err < 0)\n-- \n2.44.0\n\n"},{"id":"490845","messageId":"e837703ca1955fc2cafb49ccede12e926150a70c.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 02/15] reftable/error: discern locked/outdated errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:31Z","receivedAt":"2024-03-18T10:52:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently throw two different errors into a similar-but-different\nerror code:\n\n - Errors when trying to lock the reftable stack.\n\n - Errors when trying to write to the reftable stack which has been\n modified concurrently.\n\nThis results in unclear error handling and user-visible error messages.\n\nCreate a new `REFTABLE_OUTDATED_ERROR` so that those error conditions\ncan be clearly told apart from each other. Adjust users of the old\n`REFTABLE_LOCK_ERROR` to use the new error code as required.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/error.c | 4 +++-\n reftable/reftable-error.h | 5 ++++-\n reftable/stack.c | 6 +++---\n reftable/stack_test.c | 2 +-\n 4 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/reftable/error.c b/reftable/error.c\nindex 0d1766735e..cfb7a0fda4 100644\n--- a/reftable/error.c\n+++ b/reftable/error.c\n@@ -22,7 +22,7 @@ const char *reftable_error_str(int err)\n \tcase REFTABLE_NOT_EXIST_ERROR:\n \t\treturn \"file does not exist\";\n \tcase REFTABLE_LOCK_ERROR:\n-\t\treturn \"data is outdated\";\n+\t\treturn \"data is locked\";\n \tcase REFTABLE_API_ERROR:\n \t\treturn \"misuse of the reftable API\";\n \tcase REFTABLE_ZLIB_ERROR:\n@@ -35,6 +35,8 @@ const char *reftable_error_str(int err)\n \t\treturn \"invalid refname\";\n \tcase REFTABLE_ENTRY_TOO_BIG_ERROR:\n \t\treturn \"entry too large\";\n+\tcase REFTABLE_OUTDATED_ERROR:\n+\t\treturn \"data concurrently modified\";\n \tcase -1:\n \t\treturn \"general error\";\n \tdefault:\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex 4c457aaaf8..e9b07c9f36 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -25,7 +25,7 @@ enum reftable_error {\n \t */\n \tREFTABLE_NOT_EXIST_ERROR = -4,\n \n-\t/* Trying to write out-of-date data. */\n+\t/* Trying to access locked data. */\n \tREFTABLE_LOCK_ERROR = -5,\n \n \t/* Misuse of the API:\n@@ -57,6 +57,9 @@ enum reftable_error {\n \t/* Entry does not fit. This can happen when writing outsize reflog\n \t messages. */\n \tREFTABLE_ENTRY_TOO_BIG_ERROR = -11,\n+\n+\t/* Trying to write out-of-date data. */\n+\tREFTABLE_OUTDATED_ERROR = -12,\n };\n \n /* convert the numeric error code to a string. The string should not be\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 92d9a7facb..eaa8bb9c99 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -529,9 +529,9 @@ int reftable_stack_add(struct reftable_stack *st,\n {\n \tint err = stack_try_add(st, write, arg);\n \tif (err < 0) {\n-\t\tif (err == REFTABLE_LOCK_ERROR) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_LOCK_ERROR.\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n \t\t\t*/\n \t\t\treftable_stack_reload(st);\n \t\t}\n@@ -591,7 +591,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n \tif (err > 0) {\n-\t\terr = REFTABLE_LOCK_ERROR;\n+\t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n \t}\n \ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 509f486623..b0c7041a4f 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -232,7 +232,7 @@ static void test_reftable_stack_uptodate(void)\n \tEXPECT_ERR(err);\n \n \terr = reftable_stack_add(st2, &write_test_ref, &ref2);\n-\tEXPECT(err == REFTABLE_LOCK_ERROR);\n+\tEXPECT(err == REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tEXPECT_ERR(err);\n-- \n2.44.0\n\n"},{"id":"490846","messageId":"ae2130ffda2135bdea11ea6764c08e98a2cc4905.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 03/15] reftable/stack: use error codes when locking fails during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:36Z","receivedAt":"2024-03-18T10:52:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Compaction of a reftable stack may fail gracefully when there is a\nconcurrent process that writes to the reftable stack and which has thus\nlocked either the \"tables.list\" file or one of the tables. This is\nexpected and can be handled gracefully by some of the callers which\ninvoke compaction. Thus, to indicate this situation to our callers, we\nreturn a positive return code from `stack_compact_range()` and bubble it\nup to the caller.\n\nThis kind of error handling is somewhat awkward though as many callers\nin the call chain never even think of handling positive return values.\nThus, the result is either that such errors are swallowed by accident,\nor that we abort operations with an unhelpful error message.\n\nMake the code more robust by always using negative error codes when\ncompaction fails, with `REFTABLE_LOCK_ERROR` for the described benign\nerror case.\n\nNote that only a single callsite knew to handle positive error codes\ngracefully in the first place. Subsequent commits will touch up some of\nthe other sites to handle those errors better.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex eaa8bb9c99..ba15c48ddd 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1003,7 +1003,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\tLOCK_NO_DEREF);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n-\t\t\terr = 1;\n+\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -1025,7 +1025,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\t\ttable_name.buf, LOCK_NO_DEREF);\n \t\tif (err < 0) {\n \t\t\tif (errno == EEXIST)\n-\t\t\t\terr = 1;\n+\t\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\t\telse\n \t\t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n@@ -1075,7 +1075,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\tLOCK_NO_DEREF);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n-\t\t\terr = 1;\n+\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -1187,7 +1187,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,\n \t\t\t\t struct reftable_log_expiry_config *config)\n {\n \tint err = stack_compact_range(st, first, last, config);\n-\tif (err > 0)\n+\tif (err == REFTABLE_LOCK_ERROR)\n \t\tst->stats.failures++;\n \treturn err;\n }\n-- \n2.44.0\n\n"},{"id":"490847","messageId":"37a18b91ca998ca5ad27d17e86d286040b6e6ee1.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:40Z","receivedAt":"2024-03-18T10:52:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever we commit a new table to the reftable stack we will end up\ninvoking auto-compaction of the stack to keep the total number of tables\nat bay. This auto-compaction may fail though in case at least one of the\ntables which we are about to compact is locked. This is indicated by the\ncompaction function returning a positive value. We do not handle this\ncase though, and thus bubble that return value up the calling chain,\nwhich will ultimately cause a failure.\n\nFix this bug by handling positive return values.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 13 ++++++++++++-\n t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ba15c48ddd..dd5160d751 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tif (err)\n \t\tgoto done;\n \n-\tif (!add->stack->disable_auto_compact)\n+\tif (!add->stack->disable_auto_compact) {\n+\t\t/*\n+\t\t * Auto-compact the stack to keep the number of tables in\n+\t\t * control. Note that we explicitly ignore locking issues which\n+\t\t * may indicate that a concurrent process is already trying to\n+\t\t * compact tables. This is fine, so we simply ignore that error\n+\t\t * condition.\n+\t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n+\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n+\t\t\tgoto done;\n+\t\terr = 0;\n+\t}\n \n done:\n \treftable_addition_close(add);\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 686781192e..5f2f9baa9b 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '\n \tEOF\n '\n \n+test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\t\tfor i in $(test_seq 10)\n+\t\tdo\n+\t\t\tgit branch branch-$i &&\n+\t\t\tfor table in .git/reftable/*.ref\n+\t\t\tdo\n+\t\t\t\ttouch \"$table.lock\" || exit 1\n+\t\t\tdone ||\n+\t\t\texit 1\n+\t\tdone &&\n+\t\ttest_line_count = 13 .git/reftable/tables.list\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.0\n\n"},{"id":"490848","messageId":"f336db817c76339647424f316fa3d106bb90416d.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 05/15] refs/reftable: print errors on compaction failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:45Z","receivedAt":"2024-03-18T10:52:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-pack-refs(1) fails in the reftable backend we end up printing\nno error message at all, leaving the caller puzzled as to why compaction\nhas failed. Fix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 6 +++++-\n t/t0610-reftable-basics.sh | 12 ++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 74dab18eda..66cdbbdb24 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1221,8 +1221,12 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n \t\tstack = refs->main_stack;\n \n \tret = reftable_stack_compact_all(stack, NULL);\n-\tif (ret)\n+\tif (ret < 0) {\n+\t\tret = error(_(\"unable to compact stack: %s\"),\n+\t\t\t reftable_error_str(ret));\n \t\tgoto out;\n+\t}\n+\n \tret = reftable_stack_clean(stack);\n \tif (ret)\n \t\tgoto out;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 5f2f9baa9b..a53d1dc493 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -375,6 +375,18 @@ test_expect_success 'pack-refs: compacts tables' '\n \ttest_line_count = 1 repo/.git/reftable/tables.list\n '\n \n+test_expect_success 'pack-refs: compaction raises locking errors' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo A &&\n+\ttouch repo/.git/reftable/tables.list.lock &&\n+\tcat >expect <<-EOF &&\n+\terror: unable to compact stack: data is locked\n+\tEOF\n+\ttest_must_fail git -C repo pack-refs 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.0\n\n"},{"id":"490849","messageId":"999d0c2bb8fe11766a20d102dcef0f4f38a5fbd4.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 06/15] t/helper: drop pack-refs wrapper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:50Z","receivedAt":"2024-03-18T10:52:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The test helper provides a \"ref-store pack-refs\" wrapper that\nmore or less directly invokes `refs_pack_refs()`. This helper is only\nused in a single test with the \"PACK_REFS_PRUNE\" and \"PACK_REFS_ALL\"\nflags. Both of these flags can directly be accessed via git-pack-refs(1)\nthough via the `--all` and `--prune` flags, which makes the helper\nsuperfluous.\n\nRefactor the test to use git-pack-refs(1) instead of the test helper.\nDrop the now-unused test helper command.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-ref-store.c | 20 --------------------\n t/t0601-reffiles-pack-refs.sh | 13 +++++++++----\n 2 files changed, 9 insertions(+), 24 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 7a0f6cac53..82bbf6e2e6 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -112,25 +112,6 @@ static const char **get_store(const char **argv, struct ref_store **refs)\n \treturn argv + 1;\n }\n \n-static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n-\t\t\t\t\t FLAG_DEF(PACK_REFS_ALL),\n-\t\t\t\t\t { NULL, 0 } };\n-\n-static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n-{\n-\tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n-\tstatic struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;\n-\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n-\tstruct pack_refs_opts pack_opts = { .flags = flags,\n-\t\t\t\t\t .exclusions = &exclusions,\n-\t\t\t\t\t .includes = &included_refs };\n-\n-\tif (pack_opts.flags & PACK_REFS_ALL)\n-\t\tstring_list_append(pack_opts.includes, \"*\");\n-\n-\treturn refs_pack_refs(refs, &pack_opts);\n-}\n-\n static int cmd_create_symref(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n@@ -326,7 +307,6 @@ struct command {\n };\n \n static struct command commands[] = {\n-\t{ \"pack-refs\", cmd_pack_refs },\n \t{ \"create-symref\", cmd_create_symref },\n \t{ \"delete-refs\", cmd_delete_refs },\n \t{ \"rename-ref\", cmd_rename_ref },\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex c309d2bae8..b1cf587347 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -32,11 +32,16 @@ test_expect_success 'prepare a trivial repository' '\n \tHEAD=$(git rev-parse --verify HEAD)\n '\n \n-test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '\n-\tN=`find .git/refs -type f | wc -l` &&\n+test_expect_success 'pack-refs --prune --all' '\n+\ttest_path_is_missing .git/packed-refs &&\n+\tgit pack-refs --no-prune --all &&\n+\ttest_path_is_file .git/packed-refs &&\n+\tN=$(find .git/refs -type f | wc -l) &&\n \ttest \"$N\" != 0 &&\n-\ttest-tool ref-store main pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&\n-\tN=`find .git/refs -type f` &&\n+\n+\tgit pack-refs --prune --all &&\n+\ttest_path_is_file .git/packed-refs &&\n+\tN=$(find .git/refs -type f) &&\n \ttest -z \"$N\"\n '\n \n-- \n2.44.0\n\n"},{"id":"490850","messageId":"072627d82cc71a56a4e0884a15380a9df5d1d03d.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 07/15] refs: move `struct pack_refs_opts` to where it's used","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:54Z","receivedAt":"2024-03-18T10:52:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The declaration of `struct pack_refs_opts` is in a seemingly random\nplace. Move it so that it's located right next to its flags and\nfunctions that use it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.h | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 298caf6c61..139ce7113b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -66,12 +66,6 @@ const char *ref_storage_format_to_name(unsigned int ref_storage_format);\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n-struct pack_refs_opts {\n-\tunsigned int flags;\n-\tstruct ref_exclusions *exclusions;\n-\tstruct string_list *includes;\n-};\n-\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -433,6 +427,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n #define PACK_REFS_PRUNE 0x0001\n #define PACK_REFS_ALL 0x0002\n \n+struct pack_refs_opts {\n+\tunsigned int flags;\n+\tstruct ref_exclusions *exclusions;\n+\tstruct string_list *includes;\n+};\n+\n /*\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n-- \n2.44.0\n\n"},{"id":"490851","messageId":"919abe8eb993dbfab36136c7d9a4fd5987270519.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 08/15] refs: remove `PACK_REFS_ALL` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:52:59Z","receivedAt":"2024-03-18T10:53:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The intent of the `PACK_REFS_ALL` flag is to ask the backend to compact\nall refs instead of only a subset of them. Thus, this flag gets passed\ndown to `refs_pack_refs()` via `struct pack_refs_opts::flags`.\n\nBut starting with 4fe42f326e (pack-refs: teach pack-refs --include\noption, 2023-05-12), the flag's semantics have changed. Instead of being\nhandled by the respective backends, this flag is now getting handled by\nthe callers of `refs_pack_refs()` which will add a single glob (\"*\") to\nthe list of refs-to-be-packed. Thus, the flag serves no purpose to the\nref backends anymore.\n\nRemove the flag and replace it with a local variable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-refs.c | 5 +++--\n refs.h | 2 --\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex bcf383cac9..97921beef2 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -21,9 +21,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n+\tint pack_all = 0;\n \n \tstruct option opts[] = {\n-\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n+\t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n \t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n \t\t\tN_(\"references to include\")),\n@@ -38,7 +39,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &option_excluded_refs)\n \t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n \n-\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n+\tif (pack_all)\n \t\tstring_list_append(pack_refs_opts.includes, \"*\");\n \n \tif (!pack_refs_opts.includes->nr)\ndiff --git a/refs.h b/refs.h\nindex 139ce7113b..8c8994cb29 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -422,10 +422,8 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n /*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n- * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n */\n #define PACK_REFS_PRUNE 0x0001\n-#define PACK_REFS_ALL 0x0002\n \n struct pack_refs_opts {\n \tunsigned int flags;\n-- \n2.44.0\n\n"},{"id":"490852","messageId":"61ebcb2d52e829cf8d8a5b24f16a79b8ff4d0f3b.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 09/15] refs/reftable: expose auto compaction via new flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:04Z","receivedAt":"2024-03-18T10:53:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Under normal circumstances, the \"reftable\" backend will automatically\nperform compaction after appending to the stack. It is thus not\nnecessary and may even be considered wasteful to run git-pack-refs(1) in\n\"reftable\"-backed repositories as it will cause the backend to compact\nall tables into a single one. We do exactly that though when running\n`git maintenance run --auto` or `git gc --auto`, which gets spawned by\nGit after running some specific commands.\n\nThe `--auto` mode is typically only executing optimizations as needed.\nTo do so, we already use several heuristics for the various different\ndata structures in Git to determine whether to optimize them or not.\nWe do not use any heuristics for refs though and instead always optimize\nthem.\n\nIntroduce a new `PACK_REFS_AUTO` flag that can be passed to the backend.\nWhen not handled by the backend we will continue to behave the exact\nsame as we do right now, that is we optimize refs unconditionally. This\nis done for the \"files\" backend for now to retain current behaviour,\neven though we may eventually also want to introduce heuristics here.\nFor the \"reftable\" backend though we already do have auto-compaction, so\nwe can easily reuse that logic to implement the new auto-packing flag.\n\nNote that under normal circumstances, this should always end up being a\nno-op. After all, we already invoke the code for every single addition\nto the stack. But there are special cases where it can still be helpful\nto execute the auto-compaction code explicitly:\n\n - Concurrent writers may cause compaction to not run due to locks.\n\n - Callers may decide to disable compaction altogether and then pack\n refs at a later point due to various reasons.\n\n - Other implementations of the reftable format may do compaction\n differently or even not at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.h | 6 +++++-\n refs/reftable-backend.c | 5 ++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 8c8994cb29..d278775e08 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -422,8 +422,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n /*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n+ * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end\n+ * result are decided by the ref backend. Backends may ignore\n+ * this flag and fall back to a normal repack.\n */\n-#define PACK_REFS_PRUNE 0x0001\n+#define PACK_REFS_PRUNE (1 << 0)\n+#define PACK_REFS_AUTO (1 << 1)\n \n struct pack_refs_opts {\n \tunsigned int flags;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 66cdbbdb24..135bd4e268 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1220,7 +1220,10 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n \tif (!stack)\n \t\tstack = refs->main_stack;\n \n-\tret = reftable_stack_compact_all(stack, NULL);\n+\tif (opts->flags & PACK_REFS_AUTO)\n+\t\tret = reftable_stack_auto_compact(stack);\n+\telse\n+\t\tret = reftable_stack_compact_all(stack, NULL);\n \tif (ret < 0) {\n \t\tret = error(_(\"unable to compact stack: %s\"),\n \t\t\t reftable_error_str(ret));\n-- \n2.44.0\n\n"},{"id":"490853","messageId":"ff163a621d3b55924882cea1d1c51c074ce2cae9.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 10/15] builtin/pack-refs: release allocated memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:08Z","receivedAt":"2024-03-18T10:53:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some of the command line options in `cmd_pack_refs()` require us to\nallocate memory. This memory is never released and thus leaking, but we\npaper over this leak by declaring the respective variables as `static`\nfunction-level variables, which is somewhat awkward.\n\nRefactor the code to release the allocated memory and drop the `static`\ndeclaration. While at it, remove the useless `flags` variable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-refs.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 97921beef2..ea2baeec76 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -13,15 +13,17 @@ static char const * const pack_refs_usage[] = {\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n-\tunsigned int flags = PACK_REFS_PRUNE;\n-\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n-\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n-\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n-\t\t\t\t\t\t .includes = &included_refs,\n-\t\t\t\t\t\t .flags = flags };\n-\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct string_list included_refs = STRING_LIST_INIT_NODUP;\n+\tstruct pack_refs_opts pack_refs_opts = {\n+\t\t.exclusions = &excludes,\n+\t\t.includes = &included_refs,\n+\t\t.flags = PACK_REFS_PRUNE,\n+\t};\n+\tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \tint pack_all = 0;\n+\tint ret;\n \n \tstruct option opts[] = {\n \t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n@@ -45,5 +47,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tif (!pack_refs_opts.includes->nr)\n \t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n \n-\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n+\tret = refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n+\n+\tclear_ref_exclusions(&excludes);\n+\tstring_list_clear(&included_refs, 0);\n+\tstring_list_clear(&option_excluded_refs, 0);\n+\treturn ret;\n }\n-- \n2.44.0\n\n"},{"id":"490854","messageId":"8727f08bab8459ca608915a6320c817711742a87.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 11/15] builtin/pack-refs: introduce new \"--auto\" flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:13Z","receivedAt":"2024-03-18T10:53:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Calling git-pack-refs(1) will unconditionally cause it to pack all\nrequested refs regardless of the current state of the ref database. For\nexample:\n\n - With the \"files\" backend we will end up rewriting the complete\n \"packed-refs\" file even if only a single ref would require\n compaction.\n\n - With the \"reftable\" backend we will end up always compacting all\n tables into a single table.\n\nThis behaviour can be completely unnecessary depending on the backend\nand is thus wasteful.\n\nWith the introduction of the `PACK_REFS_AUTO` flag in the preceding\ncommit we can improve this and let the backends decide for themselves\nwhether to pack refs in the first place. Expose this functionality via a\nnew \"--auto\" flag in git-pack-refs(1), which mirrors the same flag in\nboth git-gc(1) and git-maintenance(1).\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-pack-refs.txt | 15 ++++++++++++++-\n builtin/pack-refs.c | 3 ++-\n t/t0601-reffiles-pack-refs.sh | 7 +++++++\n t/t0610-reftable-basics.sh | 34 +++++++++++++++++++++++++++++++++\n 4 files changed, 57 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 284956acb3..2dcabaf74c 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n+'git pack-refs' [--all] [--no-prune] [--auto] [--include ] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,19 @@ with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--auto::\n+\n+Pack refs as needed depending on the current state of the ref database. The\n+behavior depends on the ref format used by the repository and may change in the\n+future.\n++\n+\t- \"files\": No special handling for `--auto` has been implemented.\n++\n+\t- \"reftable\": Tables are compacted such that they form a geometric\n+\t sequence. For two tables N and N+1, where N+1 is newer, this\n+\t maintains the property that N is at least twice as big as N+1. Only\n+\t tables that violate this property are compacted.\n+\n --include ::\n \n Pack refs based on a `glob(7)` pattern. Repetitions of this option\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex ea2baeec76..db40825666 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -7,7 +7,7 @@\n #include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--auto] [--include ] [--exclude ]\"),\n \tNULL\n };\n \n@@ -28,6 +28,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tstruct option opts[] = {\n \t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), PACK_REFS_AUTO),\n \t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n \t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex b1cf587347..219a495451 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -164,6 +164,13 @@ test_expect_success 'test --exclude takes precedence over --include' '\n \tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n \ttest -f .git/refs/heads/dont_pack5'\n \n+test_expect_success '--auto packs and prunes refs as usual' '\n+\tgit branch auto &&\n+\ttest_path_is_file .git/refs/heads/auto &&\n+\tgit pack-refs --auto --all &&\n+\ttest_path_is_missing .git/refs/heads/auto\n+'\n+\n test_expect_success 'see if up-to-date packed refs are preserved' '\n \tgit branch q &&\n \tgit pack-refs --all --prune &&\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex a53d1dc493..6de7529575 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,6 +387,40 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'pack-refs: auto compaction' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\n+\t\t# The tables should have been auto-compacted, and thus auto\n+\t\t# compaction should not have to do anything.\n+\t\tls -1 .git/reftable >tables-expect &&\n+\t\ttest_line_count = 4 tables-expect &&\n+\t\tgit pack-refs --auto &&\n+\t\tls -1 .git/reftable >tables-actual &&\n+\t\ttest_cmp tables-expect tables-actual &&\n+\n+\t\t# Lock all tables write some refs. Auto-compaction will be\n+\t\t# unable to compact tables and thus fails gracefully, leaving\n+\t\t# the stack in a sub-optimal state.\n+\t\tls .git/reftable/*.ref |\n+\t\twhile read table\n+\t\tdo\n+\t\t\ttouch \"$table.lock\" || exit 1\n+\t\tdone &&\n+\t\tgit branch B &&\n+\t\tgit branch C &&\n+\t\trm .git/reftable/*.lock &&\n+\t\ttest_line_count = 5 .git/reftable/tables.list &&\n+\n+\t\tgit pack-refs --auto &&\n+\t\ttest_line_count = 1 .git/reftable/tables.list\n+\t)\n+'\n+\n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.0\n\n"},{"id":"490855","messageId":"65c9ff3ee51c982203e8b76378536958836db7d5.1710706118.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 12/15] builtin/gc: move `struct maintenance_run_opts`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:18Z","receivedAt":"2024-03-18T10:53:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to start using `struct maintenance_run_opts` in\n`maintenance_task_pack_refs()`. Move its definition up to prepare for\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 53 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 26 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb..e0029c88f9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -180,7 +180,32 @@ static void gc_config(void)\n \tgit_config(git_default_config, NULL);\n }\n \n-struct maintenance_run_opts;\n+enum schedule_priority {\n+\tSCHEDULE_NONE = 0,\n+\tSCHEDULE_WEEKLY = 1,\n+\tSCHEDULE_DAILY = 2,\n+\tSCHEDULE_HOURLY = 3,\n+};\n+\n+static enum schedule_priority parse_schedule(const char *value)\n+{\n+\tif (!value)\n+\t\treturn SCHEDULE_NONE;\n+\tif (!strcasecmp(value, \"hourly\"))\n+\t\treturn SCHEDULE_HOURLY;\n+\tif (!strcasecmp(value, \"daily\"))\n+\t\treturn SCHEDULE_DAILY;\n+\tif (!strcasecmp(value, \"weekly\"))\n+\t\treturn SCHEDULE_WEEKLY;\n+\treturn SCHEDULE_NONE;\n+}\n+\n+struct maintenance_run_opts {\n+\tint auto_flag;\n+\tint quiet;\n+\tenum schedule_priority schedule;\n+};\n+\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -773,26 +798,6 @@ static const char *const builtin_maintenance_run_usage[] = {\n \tNULL\n };\n \n-enum schedule_priority {\n-\tSCHEDULE_NONE = 0,\n-\tSCHEDULE_WEEKLY = 1,\n-\tSCHEDULE_DAILY = 2,\n-\tSCHEDULE_HOURLY = 3,\n-};\n-\n-static enum schedule_priority parse_schedule(const char *value)\n-{\n-\tif (!value)\n-\t\treturn SCHEDULE_NONE;\n-\tif (!strcasecmp(value, \"hourly\"))\n-\t\treturn SCHEDULE_HOURLY;\n-\tif (!strcasecmp(value, \"daily\"))\n-\t\treturn SCHEDULE_DAILY;\n-\tif (!strcasecmp(value, \"weekly\"))\n-\t\treturn SCHEDULE_WEEKLY;\n-\treturn SCHEDULE_NONE;\n-}\n-\n static int maintenance_opt_schedule(const struct option *opt, const char *arg,\n \t\t\t\t int unset)\n {\n@@ -809,12 +814,6 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-struct maintenance_run_opts {\n-\tint auto_flag;\n-\tint quiet;\n-\tenum schedule_priority schedule;\n-};\n-\n /* Remember to update object flag allocation in object.h */\n #define SEEN\t\t(1u<<0)\n \n-- \n2.44.0\n\n"},{"id":"490856","messageId":"817de1a88f2fc5edd8c113da2c49636a12ce0725.1710706119.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 13/15] t6500: extract objects with \"17\" prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:22Z","receivedAt":"2024-03-18T10:53:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \".git/obects/17/\" shard is somewhat special because it is used by\ngit-gc(1) to estimate how many objects there are by extrapolating the\nnumber of objects in that shard, only. In t6500 we thus have a hard\ncoded set of data that, when written to the object database, result in\nblobs starting with that prefix.\n\nWe are about to need such \"17\"-prefixed objects in another test suite.\nExtract them into \"t/oid-info/hash-info\" so that they can be reused by\nother tests.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/oid-info/hash-info | 12 ++++++++++++\n t/t6500-gc.sh | 30 +++++++-----------------------\n 2 files changed, 19 insertions(+), 23 deletions(-)\n\ndiff --git a/t/oid-info/hash-info b/t/oid-info/hash-info\nindex d0736dd1a0..b8a5bcb187 100644\n--- a/t/oid-info/hash-info\n+++ b/t/oid-info/hash-info\n@@ -15,3 +15,15 @@ empty_blob sha256:473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a3037218\n \n empty_tree sha1:4b825dc642cb6eb9a060e54bf8d69288fbee4904\n empty_tree sha256:6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321\n+\n+blob17_1 sha1:263\n+blob17_1 sha256:34\n+\n+blob17_2 sha1:410\n+blob17_2 sha256:174\n+\n+blob17_3 sha1:523\n+blob17_3 sha256:313\n+\n+blob17_4 sha1:790\n+blob17_4 sha256:481\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6..43d40175f8 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -11,23 +11,7 @@ test_expect_success 'setup' '\n \t# behavior, make sure we always pack everything to one pack by\n \t# default\n \tgit config gc.bigPackThreshold 2g &&\n-\n-\t# These are simply values which, when hashed as a blob with a newline,\n-\t# produce a hash where the first byte is 0x17 in their respective\n-\t# algorithms.\n-\ttest_oid_cache <<-EOF\n-\tobj1 sha1:263\n-\tobj1 sha256:34\n-\n-\tobj2 sha1:410\n-\tobj2 sha256:174\n-\n-\tobj3 sha1:523\n-\tobj3 sha256:313\n-\n-\tobj4 sha1:790\n-\tobj4 sha256:481\n-\tEOF\n+\ttest_oid_init\n '\n \n test_expect_success 'gc empty repository' '\n@@ -114,8 +98,8 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' '\n \t\t# We need to create two object whose sha1s start with 17\n \t\t# since this is what git gc counts. As it happens, these\n \t\t# two blobs will do so.\n-\t\ttest_commit \"$(test_oid obj1)\" &&\n-\t\ttest_commit \"$(test_oid obj2)\" &&\n+\t\ttest_commit \"$(test_oid blob17_1)\" &&\n+\t\ttest_commit \"$(test_oid blob17_2)\" &&\n \n \t\tgit gc --auto >../out.actual 2>../err.actual\n \t) &&\n@@ -146,13 +130,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \t# We need to create two object whose sha1s start with 17\n \t# since this is what git gc counts. As it happens, these\n \t# two blobs will do so.\n-\ttest_commit \"$(test_oid obj1)\" &&\n-\ttest_commit \"$(test_oid obj2)\" &&\n+\ttest_commit \"$(test_oid blob17_1)\" &&\n+\ttest_commit \"$(test_oid blob17_2)\" &&\n \t# Our first gc will create a pack; our second will create a second pack\n \tgit gc --auto &&\n \tls .git/objects/pack/pack-*.pack | sort >existing_packs &&\n-\ttest_commit \"$(test_oid obj3)\" &&\n-\ttest_commit \"$(test_oid obj4)\" &&\n+\ttest_commit \"$(test_oid blob17_3)\" &&\n+\ttest_commit \"$(test_oid blob17_4)\" &&\n \n \tgit gc --auto 2>err &&\n \ttest_grep ! \"^warning:\" err &&\n-- \n2.44.0\n\n"},{"id":"490857","messageId":"474cf66b26f5e459dccd60a2dcd1a09eff86178c.1710706119.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:28Z","receivedAt":"2024-03-18T10:53:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Forward the `--auto` flag to git-pack-refs(1) when it has been invoked\nwith this flag itself. This does not change anything for the \"files\"\nbackend, which will continue to eagerly pack refs. But it does ensure\nthat the \"reftable\" backend only compacts refs as required.\n\nThis change does not impact git-maintenance(1) as it won't ever end up\nexecuting the pack-refs task when run with `--auto`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 21 ++++++++++++---------\n t/t0610-reftable-basics.sh | 19 ++++++++++++++++---\n 2 files changed, 28 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex e0029c88f9..bf1f2a621a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -212,6 +212,9 @@ static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *\n \n \tcmd.git_cmd = 1;\n \tstrvec_pushl(&cmd.args, \"pack-refs\", \"--all\", \"--prune\", NULL);\n+\tif (opts->auto_flag)\n+\t\tstrvec_push(&cmd.args, \"--auto\");\n+\n \treturn run_command(&cmd);\n }\n \n@@ -572,7 +575,7 @@ static int report_last_gc_error(void)\n \treturn ret;\n }\n \n-static void gc_before_repack(void)\n+static void gc_before_repack(struct maintenance_run_opts *opts)\n {\n \t/*\n \t * We may be called twice, as both the pre- and\n@@ -583,7 +586,7 @@ static void gc_before_repack(void)\n \tif (done++)\n \t\treturn;\n \n-\tif (pack_refs && maintenance_task_pack_refs(NULL))\n+\tif (pack_refs && maintenance_task_pack_refs(opts))\n \t\tdie(FAILED_RUN, \"pack-refs\");\n \n \tif (prune_reflogs) {\n@@ -599,7 +602,6 @@ static void gc_before_repack(void)\n int cmd_gc(int argc, const char **argv, const char *prefix)\n {\n \tint aggressive = 0;\n-\tint auto_gc = 0;\n \tint quiet = 0;\n \tint force = 0;\n \tconst char *name;\n@@ -608,6 +610,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tint keep_largest_pack = -1;\n \ttimestamp_t dummy;\n \tstruct child_process rerere_cmd = CHILD_PROCESS_INIT;\n+\tstruct maintenance_run_opts opts = {0};\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -618,7 +621,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &max_cruft_size,\n \t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n-\t\tOPT_BOOL_F(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\"),\n+\t\tOPT_BOOL_F(0, \"auto\", &opts.auto_flag, N_(\"enable auto-gc mode\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL_F(0, \"force\", &force,\n \t\t\t N_(\"force running gc even if there may be another gc running\"),\n@@ -663,7 +666,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (quiet)\n \t\tstrvec_push(&repack, \"-q\");\n \n-\tif (auto_gc) {\n+\tif (opts.auto_flag) {\n \t\t/*\n \t\t * Auto-gc should be least intrusive as possible.\n \t\t */\n@@ -688,7 +691,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\n-\t\t\tgc_before_repack(); /* dies on failure */\n+\t\t\tgc_before_repack(&opts); /* dies on failure */\n \t\t\tdelete_tempfile(&pidfile);\n \n \t\t\t/*\n@@ -713,7 +716,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tname = lock_repo_for_gc(force, &pid);\n \tif (name) {\n-\t\tif (auto_gc)\n+\t\tif (opts.auto_flag)\n \t\t\treturn 0; /* be quiet on --auto */\n \t\tdie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n \t\t name, (uintmax_t)pid);\n@@ -728,7 +731,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tatexit(process_log_file_at_exit);\n \t}\n \n-\tgc_before_repack();\n+\tgc_before_repack(&opts);\n \n \tif (!repository_format_precious_objects) {\n \t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n@@ -783,7 +786,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t NULL);\n \n-\tif (auto_gc && too_many_loose_objects())\n+\tif (opts.auto_flag && too_many_loose_objects())\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n \ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 6de7529575..b37d8bf3b1 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,7 +387,9 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n-test_expect_success 'pack-refs: auto compaction' '\n+for command in pack-refs gc\n+do\n+test_expect_success \"$command: auto compaction\" '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n@@ -395,14 +397,24 @@ test_expect_success 'pack-refs: auto compaction' '\n \n \t\ttest_commit A &&\n \n+\t\t# We need a bit of setup to ensure that git-gc(1) actually\n+\t\t# triggers, and that it does not write anything to the refdb.\n+\t\tgit config gc.auto 1 &&\n+\t\tgit config gc.autoDetach 0 &&\n+\t\tgit config gc.reflogExpire never &&\n+\t\tgit config gc.reflogExpireUnreachable never &&\n+\t\ttest_oid blob17_1 | git hash-object -w --stdin &&\n+\n \t\t# The tables should have been auto-compacted, and thus auto\n \t\t# compaction should not have to do anything.\n \t\tls -1 .git/reftable >tables-expect &&\n \t\ttest_line_count = 4 tables-expect &&\n-\t\tgit pack-refs --auto &&\n+\t\tgit $command --auto &&\n \t\tls -1 .git/reftable >tables-actual &&\n \t\ttest_cmp tables-expect tables-actual &&\n \n+\t\ttest_oid blob17_2 | git hash-object -w --stdin &&\n+\n \t\t# Lock all tables write some refs. Auto-compaction will be\n \t\t# unable to compact tables and thus fails gracefully, leaving\n \t\t# the stack in a sub-optimal state.\n@@ -416,10 +428,11 @@ test_expect_success 'pack-refs: auto compaction' '\n \t\trm .git/reftable/*.lock &&\n \t\ttest_line_count = 5 .git/reftable/tables.list &&\n \n-\t\tgit pack-refs --auto &&\n+\t\tgit $command --auto &&\n \t\ttest_line_count = 1 .git/reftable/tables.list\n \t)\n '\n+done\n \n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n-- \n2.44.0\n\n"},{"id":"490858","messageId":"71f4800d36bcc77d8f36c5fc7b48eccbb90e6a93.1710706119.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-18T10:53:32Z","receivedAt":"2024-03-18T10:53:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When running `git maintenance run --auto`, then the various subtasks\nwill only run as needed. Thus, we for example end up only packing loose\nobjects if we hit a certain threshold.\n\nInterestingly enough, the \"pack-refs\" task is actually _never_ executed\nwhen the auto-flag is set because it does not have a condition at all.\nAs 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:\n\n The 'auto_condition' function pointer is left NULL for now. We could\n extend this in the future to have a condition check if pack-refs\n should be run during 'git maintenance run --auto'.\n\nIt is not quite clear from that quote whether it is actually intended\nthat the task doesn't run at all in this mode. Also, no test was added\nto verify this behaviour. Ultimately though, it feels quite surprising\nthat `git maintenance run --auto --task=pack-refs` would quietly never\ndo anything at all.\n\nIn any case, now that we do have the logic in place to let ref backends\ndecide whether or not to repack refs, it does make sense to wire it up\naccordingly. With the \"reftable\" backend we will thus now perform\nauto-compaction, which optimizes the refdb as needed.\n\nBut for the \"files\" backend we now unconditionally pack refs as it does\nnot yet know to handle the \"auto\" flag. Arguably, this can be seen as a\nbug fix given that previously the task never did anything at all.\nEventually though we should amend the \"files\" backend to use some\nheuristics for auto compaction, as well.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 12 +++++++++++-\n t/t0601-reffiles-pack-refs.sh | 10 ++++++++++\n t/t0610-reftable-basics.sh | 2 +-\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bf1f2a621a..3c874b248b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -206,6 +206,16 @@ struct maintenance_run_opts {\n \tenum schedule_priority schedule;\n };\n \n+static int pack_refs_condition(void)\n+{\n+\t/*\n+\t * The auto-repacking logic for refs is handled by the ref backends and\n+\t * exposed via `git pack-refs --auto`. We thus always return truish\n+\t * here and let the backend decide for us.\n+\t */\n+\treturn 1;\n+}\n+\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = {\n \t[TASK_PACK_REFS] = {\n \t\t\"pack-refs\",\n \t\tmaintenance_task_pack_refs,\n-\t\tNULL,\n+\t\tpack_refs_condition,\n \t},\n };\n \ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex 219a495451..7d4ab0b91a 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -370,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'maintenance --auto unconditionally packs loose refs' '\n+\tgit update-ref refs/heads/something HEAD &&\n+\ttest_path_is_file .git/refs/heads/something &&\n+\tgit rev-parse refs/heads/something >expect &&\n+\tgit maintenance run --task=pack-refs --auto &&\n+\ttest_path_is_missing .git/refs/heads/something &&\n+\tgit rev-parse refs/heads/something >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex b37d8bf3b1..931d888bbb 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n-for command in pack-refs gc\n+for command in pack-refs gc \"maintenance run --task=pack-refs\"\n do\n test_expect_success \"$command: auto compaction\" '\n \ttest_when_finished \"rm -rf repo\" &&\n-- \n2.44.0\n\n"},{"id":"491029","messageId":"CAOLa=ZTgJj=moQC2Ve40KzSinmNGFwTSx_RcKaiCEQArcg8NoQ@mail.gmail.com","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"Re: [PATCH 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T19:30:58Z","receivedAt":"2024-03-20T19:31:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n> The `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`\n> flag. It is wired up in both `git gc --auto` and `git maintenance run\n> --auto`.\n>\n\nWondering if this is also exposed as a config option.\n\n> The series is structured as follows:\n>\n> - Patches 1 - 5: Bugfixes and improvements for the reftable-specific\n> compaction code. These are issues that I've found while working on\n> this series.\n>\n> - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,\n> which isn't actually used by the ref backends anymore and confused\n> me multiple times.\n>\n> - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),\n> git-gc(1) and git-maintenance(1).\n>\n\nI'm assuming this means `PACK_REFS_AUTO` in the last sentence.\n"},{"id":"491042","messageId":"CAOLa=ZQ5wFCqWZ4yjMzUg5Yz1UeVpwUap0z9Lx9EfRVKnzHttQ@mail.gmail.com","threadId":"61142","inReplyTo":"1e39d93a45db502280ecff383d53e0294f969719.1710706118.git.ps@pks.im","subject":"Re: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T21:50:43Z","receivedAt":"2024-03-20T21:50:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 1ecf1b9751..92d9a7facb 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> \terr = stack_uptodate(st);\n> \tif (err < 0)\n> \t\tgoto done;\n> -\n> -\tif (err > 1) {\n> +\tif (err > 0) {\n> \t\terr = REFTABLE_LOCK_ERROR;\n> \t\tgoto done;\n> \t}\n> @@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st,\n> \tint err = reftable_stack_init_addition(&add, st);\n> \tif (err < 0)\n> \t\tgoto done;\n> -\tif (err > 0) {\n> -\t\terr = REFTABLE_LOCK_ERROR;\n> -\t\tgoto done;\n> -\t}\n\nThis changes the behavior though, since now we skip the `goto done`. It\nwould be best to change the previous line to `if (err)`, which is what\nthe other function (`reftable_stack_new_addition`) does.\n"},{"id":"491068","messageId":"CAOLa=ZS6uF3fuor3V2F_GM__JNRpUAyLcrVDbCHsJ6JStL96cA@mail.gmail.com","threadId":"61142","inReplyTo":"ae2130ffda2135bdea11ea6764c08e98a2cc4905.1710706118.git.ps@pks.im","subject":"Re: [PATCH 03/15] reftable/stack: use error codes when locking fails during compaction","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T22:14:10Z","receivedAt":"2024-03-20T22:14:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index eaa8bb9c99..ba15c48ddd 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -1003,7 +1003,7 @@ static int stack_compact_range(struct reftable_stack *st,\n> \t\t\t\t\tLOCK_NO_DEREF);\n> \tif (err < 0) {\n> \t\tif (errno == EEXIST)\n> -\t\t\terr = 1;\n> +\t\t\terr = REFTABLE_LOCK_ERROR;\n> \t\telse\n> \t\t\terr = REFTABLE_IO_ERROR;\n> \t\tgoto done;\n>\n\nThe comment at the top of the function now needs to be re-written with\nthis change.\n"},{"id":"491069","messageId":"CAOLa=ZRxt7rwmU4UJAZra605zyqk5QJD5OD1oM_BidRpJbZP1w@mail.gmail.com","threadId":"61142","inReplyTo":"37a18b91ca998ca5ad27d17e86d286040b6e6ee1.1710706118.git.ps@pks.im","subject":"Re: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T22:22:55Z","receivedAt":"2024-03-20T22:22:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Whenever we commit a new table to the reftable stack we will end up\n> invoking auto-compaction of the stack to keep the total number of tables\n> at bay. This auto-compaction may fail though in case at least one of the\n> tables which we are about to compact is locked. This is indicated by the\n> compaction function returning a positive value. We do not handle this\n\nWe no longer return a positive value, do we?\n\n> case though, and thus bubble that return value up the calling chain,\n> which will ultimately cause a failure.\n>\n> Fix this bug by handling positive return values.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> reftable/stack.c | 13 ++++++++++++-\n> t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++\n> 2 files changed, 32 insertions(+), 1 deletion(-)\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index ba15c48ddd..dd5160d751 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)\n> \tif (err)\n> \t\tgoto done;\n>\n> -\tif (!add->stack->disable_auto_compact)\n> +\tif (!add->stack->disable_auto_compact) {\n> +\t\t/*\n> +\t\t * Auto-compact the stack to keep the number of tables in\n> +\t\t * control. Note that we explicitly ignore locking issues which\n> +\t\t * may indicate that a concurrent process is already trying to\n> +\t\t * compact tables. This is fine, so we simply ignore that error\n> +\t\t * condition.\n> +\t\t */\n>\n\nNit: The last two sentences are somewhat the same, it'd be perhaps nicer\nto explain why \"this is fine\".\n\n> \t\terr = reftable_stack_auto_compact(add->stack);\n> +\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n> +\t\t\tgoto done;\n> +\t\terr = 0;\n> +\t}\n>\n> done:\n> \treftable_addition_close(add);\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 686781192e..5f2f9baa9b 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '\n> \tEOF\n> '\n>\n> +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\ttest_commit A &&\n> +\t\tfor i in $(test_seq 10)\n> +\t\tdo\n> +\t\t\tgit branch branch-$i &&\n> +\t\t\tfor table in .git/reftable/*.ref\n> +\t\t\tdo\n> +\t\t\t\ttouch \"$table.lock\" || exit 1\n> +\t\t\tdone ||\n> +\t\t\texit 1\n> +\t\tdone &&\n> +\t\ttest_line_count = 13 .git/reftable/tables.list\n> +\t)\n> +'\n\nI'm not sure about the testing setup for reftables, but maybe if we\nmoved this to the unit tests, we could've also checked the\n`reftable_stack_compaction_stats(st)->failures` value. This is fine, but\nit doesn't really tell us if the compaction was even attempted.\n"},{"id":"491072","messageId":"CAOLa=ZQbkW1_X3oaipbTNMTy3z5jvbFoFp4W9wPSh8gHSAFYzg@mail.gmail.com","threadId":"61142","inReplyTo":"ff163a621d3b55924882cea1d1c51c074ce2cae9.1710706118.git.ps@pks.im","subject":"Re: [PATCH 10/15] builtin/pack-refs: release allocated memory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T23:23:54Z","receivedAt":"2024-03-20T23:23:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Some of the command line options in `cmd_pack_refs()` require us to\n> allocate memory. This memory is never released and thus leaking, but we\n> paper over this leak by declaring the respective variables as `static`\n> function-level variables, which is somewhat awkward.\n>\n> Refactor the code to release the allocated memory and drop the `static`\n> declaration. While at it, remove the useless `flags` variable.\n>\n\nTangent: I was looking at `files_pack_refs` and it seems like there are\na bunch of `die(...)` calls there. I wonder if it would be nicer to use\n`error()` instead so we could do a better job at cleanup.\n"},{"id":"491081","messageId":"CAOLa=ZTWvLkzdJMOS4nU=zGhmXS2nSEXzK3Wo2+=jRgV56atrA@mail.gmail.com","threadId":"61142","inReplyTo":"474cf66b26f5e459dccd60a2dcd1a09eff86178c.1710706119.git.ps@pks.im","subject":"Re: [PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T23:56:31Z","receivedAt":"2024-03-20T23:56:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Forward the `--auto` flag to git-pack-refs(1) when it has been invoked\n> with this flag itself. This does not change anything for the \"files\"\n> backend, which will continue to eagerly pack refs. But it does ensure\n> that the \"reftable\" backend only compacts refs as required.\n>\n> This change does not impact git-maintenance(1) as it won't ever end up\n> executing the pack-refs task when run with `--auto`.\n>\n\nI'm not sure I follow this one, the man page for git-maintenance(1)\ndoesn't mention anything about not running pack-refs when `--auto` is\nused.\n"},{"id":"491082","messageId":"CAOLa=ZRRCLOcXvn=F8eXH5N0kncPhu968_se8tG43VDEjZruwQ@mail.gmail.com","threadId":"61142","inReplyTo":"71f4800d36bcc77d8f36c5fc7b48eccbb90e6a93.1710706119.git.ps@pks.im","subject":"Re: [PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-20T23:59:06Z","receivedAt":"2024-03-20T23:59:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> When running `git maintenance run --auto`, then the various subtasks\n> will only run as needed. Thus, we for example end up only packing loose\n> objects if we hit a certain threshold.\n>\n> Interestingly enough, the \"pack-refs\" task is actually _never_ executed\n> when the auto-flag is set because it does not have a condition at all.\n> As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:\n>\n> The 'auto_condition' function pointer is left NULL for now. We could\n> extend this in the future to have a condition check if pack-refs\n> should be run during 'git maintenance run --auto'.\n>\n\nOk, this answers my question in the previous email.\n\n> It is not quite clear from that quote whether it is actually intended\n> that the task doesn't run at all in this mode. Also, no test was added\n> to verify this behaviour. Ultimately though, it feels quite surprising\n> that `git maintenance run --auto --task=pack-refs` would quietly never\n> do anything at all.\n>\n> In any case, now that we do have the logic in place to let ref backends\n> decide whether or not to repack refs, it does make sense to wire it up\n> accordingly. With the \"reftable\" backend we will thus now perform\n> auto-compaction, which optimizes the refdb as needed.\n>\n> But for the \"files\" backend we now unconditionally pack refs as it does\n> not yet know to handle the \"auto\" flag. Arguably, this can be seen as a\n> bug fix given that previously the task never did anything at all.\n> Eventually though we should amend the \"files\" backend to use some\n> heuristics for auto compaction, as well.\n>\n\nAgreed, thanks for the clear explanation here.\n"},{"id":"491406","messageId":"ZgE_fJpufp463Bp7@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZTgJj=moQC2Ve40KzSinmNGFwTSx_RcKaiCEQArcg8NoQ@mail.gmail.com","subject":"Re: [PATCH 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:20Z","receivedAt":"2024-03-25T09:10:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 12:30:58PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> > The `PACK_REFS_AUTO` flag is exposed via a new `git pack-refs --auto`\n> > flag. It is wired up in both `git gc --auto` and `git maintenance run\n> > --auto`.\n> >\n> \n> Wondering if this is also exposed as a config option.\n\nIt's not, no, and I don't quite think it would make sense. We don't\nexpose the current `git gc --auto` flag as config option, either. While\nwe have \"gc.auto\", it doesn't configure whether or not the `--auto` flag\nis set, but rather configures the boundary at which objects would be\nrepacked.\n\nIn hindsight that config option is too narrowly focussed on objects\nwhile pretending to be quite general. It would've been great if this was\ncalled \"gc.autoLooseObjectLimit\" or something similar instead to not\ncause confusion and to be more easily extendable in the future.\n\nI can certainly see that we might eventually want to have ref backend\nspecific configuration here. For the \"files\" backend this could be be\nthe number of loose refs that are allowed to exist before repacking,\nwhereas for the \"reftable\" backend this could be the geometric factor.\nBut I'll leave that for a future series.\n\n> > The series is structured as follows:\n> >\n> > - Patches 1 - 5: Bugfixes and improvements for the reftable-specific\n> > compaction code. These are issues that I've found while working on\n> > this series.\n> >\n> > - Patches 6 - 8: Refactorings to drop the `PACK_REFS_ALL` flag,\n> > which isn't actually used by the ref backends anymore and confused\n> > me multiple times.\n> >\n> > - Patches 9 - 15: Handling of `PACK_REFS_ALL` in git-pack-refs(1),\n> > git-gc(1) and git-maintenance(1).\n> >\n> \n> I'm assuming this means `PACK_REFS_AUTO` in the last sentence.\n\nAh, yes.\n\nPatrick\n"},{"id":"491404","messageId":"ZgE_ggdkO6ejjuOG@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZQ5wFCqWZ4yjMzUg5Yz1UeVpwUap0z9Lx9EfRVKnzHttQ@mail.gmail.com","subject":"Re: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:26Z","receivedAt":"2024-03-25T09:10:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 02:50:43PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index 1ecf1b9751..92d9a7facb 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n> > \terr = stack_uptodate(st);\n> > \tif (err < 0)\n> > \t\tgoto done;\n> > -\n> > -\tif (err > 1) {\n> > +\tif (err > 0) {\n> > \t\terr = REFTABLE_LOCK_ERROR;\n> > \t\tgoto done;\n> > \t}\n> > @@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st,\n> > \tint err = reftable_stack_init_addition(&add, st);\n> > \tif (err < 0)\n> > \t\tgoto done;\n> > -\tif (err > 0) {\n> > -\t\terr = REFTABLE_LOCK_ERROR;\n> > -\t\tgoto done;\n> > -\t}\n> \n> This changes the behavior though, since now we skip the `goto done`. It\n> would be best to change the previous line to `if (err)`, which is what\n> the other function (`reftable_stack_new_addition`) does.\n\nIt actually doesn't because `reftable_stack_init_addition()` does not\nreturn a positive value anymore. And as it returns `REFTABLE_LOCK_ERROR`\nin the case where it previously did return a positive value the\nbehaviour is exactly the same because we go into thee `if (err < 0)`\nbranch.\n\nI'll amend the commit message to clariyf.\n\nPatrick\n"},{"id":"491405","messageId":"ZgE_h_su_Y_HfGXR@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZS6uF3fuor3V2F_GM__JNRpUAyLcrVDbCHsJ6JStL96cA@mail.gmail.com","subject":"Re: [PATCH 03/15] reftable/stack: use error codes when locking fails during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:31Z","receivedAt":"2024-03-25T09:10:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 03:14:10PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index eaa8bb9c99..ba15c48ddd 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -1003,7 +1003,7 @@ static int stack_compact_range(struct reftable_stack *st,\n> > \t\t\t\t\tLOCK_NO_DEREF);\n> > \tif (err < 0) {\n> > \t\tif (errno == EEXIST)\n> > -\t\t\terr = 1;\n> > +\t\t\terr = REFTABLE_LOCK_ERROR;\n> > \t\telse\n> > \t\t\terr = REFTABLE_IO_ERROR;\n> > \t\tgoto done;\n> >\n> \n> The comment at the top of the function now needs to be re-written with\n> this change.\n\nGood catch, will fix.\n\nPatrick\n"},{"id":"491407","messageId":"ZgE_kJldM-Rh9tKl@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZRxt7rwmU4UJAZra605zyqk5QJD5OD1oM_BidRpJbZP1w@mail.gmail.com","subject":"Re: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:40Z","receivedAt":"2024-03-25T09:10:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 03:22:55PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Whenever we commit a new table to the reftable stack we will end up\n> > invoking auto-compaction of the stack to keep the total number of tables\n> > at bay. This auto-compaction may fail though in case at least one of the\n> > tables which we are about to compact is locked. This is indicated by the\n> > compaction function returning a positive value. We do not handle this\n> \n> We no longer return a positive value, do we?\n\nYup, this is stale.\n\n> > case though, and thus bubble that return value up the calling chain,\n> > which will ultimately cause a failure.\n> >\n> > Fix this bug by handling positive return values.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > reftable/stack.c | 13 ++++++++++++-\n> > t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++\n> > 2 files changed, 32 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index ba15c48ddd..dd5160d751 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)\n> > \tif (err)\n> > \t\tgoto done;\n> >\n> > -\tif (!add->stack->disable_auto_compact)\n> > +\tif (!add->stack->disable_auto_compact) {\n> > +\t\t/*\n> > +\t\t * Auto-compact the stack to keep the number of tables in\n> > +\t\t * control. Note that we explicitly ignore locking issues which\n> > +\t\t * may indicate that a concurrent process is already trying to\n> > +\t\t * compact tables. This is fine, so we simply ignore that error\n> > +\t\t * condition.\n> > +\t\t */\n> >\n> \n> Nit: The last two sentences are somewhat the same, it'd be perhaps nicer\n> to explain why \"this is fine\".\n\nFair enough.\n\n> > \t\terr = reftable_stack_auto_compact(add->stack);\n> > +\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n> > +\t\t\tgoto done;\n> > +\t\terr = 0;\n> > +\t}\n> >\n> > done:\n> > \treftable_addition_close(add);\n> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> > index 686781192e..5f2f9baa9b 100755\n> > --- a/t/t0610-reftable-basics.sh\n> > +++ b/t/t0610-reftable-basics.sh\n> > @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '\n> > \tEOF\n> > '\n> >\n> > +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\n> > +\t\ttest_commit A &&\n> > +\t\tfor i in $(test_seq 10)\n> > +\t\tdo\n> > +\t\t\tgit branch branch-$i &&\n> > +\t\t\tfor table in .git/reftable/*.ref\n> > +\t\t\tdo\n> > +\t\t\t\ttouch \"$table.lock\" || exit 1\n> > +\t\t\tdone ||\n> > +\t\t\texit 1\n> > +\t\tdone &&\n> > +\t\ttest_line_count = 13 .git/reftable/tables.list\n> > +\t)\n> > +'\n> \n> I'm not sure about the testing setup for reftables, but maybe if we\n> moved this to the unit tests, we could've also checked the\n> `reftable_stack_compaction_stats(st)->failures` value. This is fine, but\n> it doesn't really tell us if the compaction was even attempted.\n\nBoth tests have their merit, I'd say. Let's thus just add both.\n\nPatrick\n"},{"id":"491408","messageId":"ZgE_lSXhGlHNiaCM@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZQbkW1_X3oaipbTNMTy3z5jvbFoFp4W9wPSh8gHSAFYzg@mail.gmail.com","subject":"Re: [PATCH 10/15] builtin/pack-refs: release allocated memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:45Z","receivedAt":"2024-03-25T09:10:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 04:23:54PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Some of the command line options in `cmd_pack_refs()` require us to\n> > allocate memory. This memory is never released and thus leaking, but we\n> > paper over this leak by declaring the respective variables as `static`\n> > function-level variables, which is somewhat awkward.\n> >\n> > Refactor the code to release the allocated memory and drop the `static`\n> > declaration. While at it, remove the useless `flags` variable.\n> >\n> \n> Tangent: I was looking at `files_pack_refs` and it seems like there are\n> a bunch of `die(...)` calls there. I wonder if it would be nicer to use\n> `error()` instead so we could do a better job at cleanup.\n\nI agree that this would be a nice refactoring.\n\nPatrick\n"},{"id":"491409","messageId":"ZgE_m8qP8Ng7omhR@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZTWvLkzdJMOS4nU=zGhmXS2nSEXzK3Wo2+=jRgV56atrA@mail.gmail.com","subject":"Re: [PATCH 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:51Z","receivedAt":"2024-03-25T09:10:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 04:56:31PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Forward the `--auto` flag to git-pack-refs(1) when it has been invoked\n> > with this flag itself. This does not change anything for the \"files\"\n> > backend, which will continue to eagerly pack refs. But it does ensure\n> > that the \"reftable\" backend only compacts refs as required.\n> >\n> > This change does not impact git-maintenance(1) as it won't ever end up\n> > executing the pack-refs task when run with `--auto`.\n> >\n> \n> I'm not sure I follow this one, the man page for git-maintenance(1)\n> doesn't mention anything about not running pack-refs when `--auto` is\n> used.\n\nI'm clarifying this further in patch 15. It's quite confusing in the\nfirst place that git-maintenance(1) doesn't ever pack refs at all when\nusing `--auto`. I will amend the commit message to explain this better.\n\nPatrick\n"},{"id":"491410","messageId":"ZgE_ofR50OWmFumK@tanuki","threadId":"61142","inReplyTo":"CAOLa=ZRRCLOcXvn=F8eXH5N0kncPhu968_se8tG43VDEjZruwQ@mail.gmail.com","subject":"Re: [PATCH 15/15] builtin/gc: pack refs when using `git maintenance run --auto`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T09:10:57Z","receivedAt":"2024-03-25T09:11:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 20, 2024 at 04:59:06PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > When running `git maintenance run --auto`, then the various subtasks\n> > will only run as needed. Thus, we for example end up only packing loose\n> > objects if we hit a certain threshold.\n> >\n> > Interestingly enough, the \"pack-refs\" task is actually _never_ executed\n> > when the auto-flag is set because it does not have a condition at all.\n> > As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:\n> >\n> > The 'auto_condition' function pointer is left NULL for now. We could\n> > extend this in the future to have a condition check if pack-refs\n> > should be run during 'git maintenance run --auto'.\n> >\n> \n> Ok, this answers my question in the previous email.\n> \n> > It is not quite clear from that quote whether it is actually intended\n> > that the task doesn't run at all in this mode. Also, no test was added\n> > to verify this behaviour. Ultimately though, it feels quite surprising\n> > that `git maintenance run --auto --task=pack-refs` would quietly never\n> > do anything at all.\n> >\n> > In any case, now that we do have the logic in place to let ref backends\n> > decide whether or not to repack refs, it does make sense to wire it up\n> > accordingly. With the \"reftable\" backend we will thus now perform\n> > auto-compaction, which optimizes the refdb as needed.\n> >\n> > But for the \"files\" backend we now unconditionally pack refs as it does\n> > not yet know to handle the \"auto\" flag. Arguably, this can be seen as a\n> > bug fix given that previously the task never did anything at all.\n> > Eventually though we should amend the \"files\" backend to use some\n> > heuristics for auto compaction, as well.\n> >\n> \n> Agreed, thanks for the clear explanation here.\n\nThanks for your thorough review!\n\nPatrick\n"},{"id":"491415","messageId":"cover.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1710706118.git.ps@pks.im","subject":"[PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:34Z","receivedAt":"2024-03-25T10:02:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that introduces the\n`--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)\nand git-maintenance(1).\n\nChanges compared to v1:\n\n - Clarified some of the commit messages.\n\n - Adjusted the stale comment of `stack_compact_range()`.\n\n - Added a unit test for failing auto-compaction.\n\n - Clarified a comment to explain why it is fine for auto-compaction\n to fail.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (15):\n reftable/stack: fix error handling in `reftable_stack_init_addition()`\n reftable/error: discern locked/outdated errors\n reftable/stack: use error codes when locking fails during compaction\n reftable/stack: gracefully handle failed auto-compaction due to locks\n refs/reftable: print errors on compaction failure\n t/helper: drop pack-refs wrapper\n refs: move `struct pack_refs_opts` to where it's used\n refs: remove `PACK_REFS_ALL` flag\n refs/reftable: expose auto compaction via new flag\n builtin/pack-refs: release allocated memory\n builtin/pack-refs: introduce new \"--auto\" flag\n builtin/gc: move `struct maintenance_run_opts`\n t6500: extract objects with \"17\" prefix\n builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs\n builtin/gc: pack refs when using `git maintenance run --auto`\n\n Documentation/git-pack-refs.txt | 15 +++++-\n builtin/gc.c | 86 +++++++++++++++++++--------------\n builtin/pack-refs.c | 31 +++++++-----\n refs.h | 20 ++++----\n refs/reftable-backend.c | 11 ++++-\n reftable/error.c | 4 +-\n reftable/reftable-error.h | 5 +-\n reftable/stack.c | 44 +++++++++++------\n reftable/stack_test.c | 45 ++++++++++++++++-\n t/helper/test-ref-store.c | 20 --------\n t/oid-info/hash-info | 12 +++++\n t/t0601-reffiles-pack-refs.sh | 30 ++++++++++--\n t/t0610-reftable-basics.sh | 79 ++++++++++++++++++++++++++++++\n t/t6500-gc.sh | 30 +++---------\n 14 files changed, 307 insertions(+), 125 deletions(-)\n\nRange-diff against v1:\n 1: 1e39d93a45 ! 1: b41b9b27cb reftable/stack: fix error handling in `reftable_stack_init_addition()`\n @@ Commit message\n error code check without the off-by-one. But other callers are subtly\n broken by this bug.\n \n - Fix this by checking for `err > 0` instead.\n + Fix this by checking for `err > 0` instead. This has the consequence\n + that `reftable_stack_init_addition()` won't ever return a positive error\n + code anymore, but will instead return `REFTABLE_LOCK_ERROR` now. Thus,\n + we can drop the check for a positive error code in `stack_try_add()`\n + now.\n \n Signed-off-by: Patrick Steinhardt \n \n 2: e837703ca1 = 2: be7212006b reftable/error: discern locked/outdated errors\n 3: ae2130ffda ! 3: 95dda44672 reftable/stack: use error codes when locking fails during compaction\n @@ Commit message\n Signed-off-by: Patrick Steinhardt \n \n ## reftable/stack.c ##\n +@@ reftable/stack.c: static int stack_write_compact(struct reftable_stack *st,\n + \treturn err;\n + }\n + \n +-/* < 0: error. 0 == OK, > 0 attempt failed; could retry. */\n ++/*\n ++ * Compact all tables in the range `[first, last)` into a single new table.\n ++ *\n ++ * This function returns `0` on success or a code `< 0` on failure. When the\n ++ * stack or any of the tables in the specified range are already locked then\n ++ * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that\n ++ * callers can either ignore, or they may choose to retry compaction after some\n ++ * amount of time.\n ++ */\n + static int stack_compact_range(struct reftable_stack *st,\n + \t\t\t size_t first, size_t last,\n + \t\t\t struct reftable_log_expiry_config *expiry)\n @@ reftable/stack.c: static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\tLOCK_NO_DEREF);\n \tif (err < 0) {\n 4: 37a18b91ca ! 4: 50a3c37f92 reftable/stack: gracefully handle failed auto-compaction due to locks\n @@ Commit message\n invoking auto-compaction of the stack to keep the total number of tables\n at bay. This auto-compaction may fail though in case at least one of the\n tables which we are about to compact is locked. This is indicated by the\n - compaction function returning a positive value. We do not handle this\n - case though, and thus bubble that return value up the calling chain,\n - which will ultimately cause a failure.\n + compaction function returning `REFTABLE_LOCK_ERROR`. We do not handle\n + this case though, and thus bubble that return value up the calling\n + chain, which will ultimately cause a failure.\n \n - Fix this bug by handling positive return values.\n + Fix this bug by ignoring `REFTABLE_LOCK_ERROR`.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)\n +\tif (!add->stack->disable_auto_compact) {\n +\t\t/*\n +\t\t * Auto-compact the stack to keep the number of tables in\n -+\t\t * control. Note that we explicitly ignore locking issues which\n -+\t\t * may indicate that a concurrent process is already trying to\n -+\t\t * compact tables. This is fine, so we simply ignore that error\n -+\t\t * condition.\n ++\t\t * control. It is possible that a concurrent writer is already\n ++\t\t * trying to compact parts of the stack, which would lead to a\n ++\t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n ++\t\t * already. This is a benign error though, so we ignore it.\n +\t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n +\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)\n done:\n \treftable_addition_close(add);\n \n + ## reftable/stack_test.c ##\n +@@ reftable/stack_test.c: static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n + \tclear_dir(dir);\n + }\n + \n ++static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n ++{\n ++\tstruct reftable_ref_record ref = {\n ++\t\t.refname = \"refs/heads/master\",\n ++\t\t.update_index = 1,\n ++\t\t.value_type = REFTABLE_REF_VAL1,\n ++\t\t.value.val1 = {0x01},\n ++\t};\n ++\tstruct reftable_write_options cfg = {0};\n ++\tstruct reftable_stack *st;\n ++\tstruct strbuf table_path = STRBUF_INIT;\n ++\tchar *dir = get_tmp_dir(__LINE__);\n ++\tint err;\n ++\n ++\terr = reftable_new_stack(&st, dir, cfg);\n ++\tEXPECT_ERR(err);\n ++\n ++\terr = reftable_stack_add(st, write_test_ref, &ref);\n ++\tEXPECT_ERR(err);\n ++\tEXPECT(st->merged->stack_len == 1);\n ++\tEXPECT(st->stats.attempts == 0);\n ++\tEXPECT(st->stats.failures == 0);\n ++\n ++\t/*\n ++\t * Lock the newly written table such that it cannot be compacted.\n ++\t * Adding a new table to the stack should not be impacted by this, even\n ++\t * though auto-compaction will now fail.\n ++\t */\n ++\tstrbuf_addf(&table_path, \"%s/%s.lock\", dir, st->readers[0]->name);\n ++\twrite_file_buf(table_path.buf, \"\", 0);\n ++\n ++\tref.update_index = 2;\n ++\terr = reftable_stack_add(st, write_test_ref, &ref);\n ++\tEXPECT_ERR(err);\n ++\tEXPECT(st->merged->stack_len == 2);\n ++\tEXPECT(st->stats.attempts == 1);\n ++\tEXPECT(st->stats.failures == 1);\n ++\n ++\treftable_stack_destroy(st);\n ++\tclear_dir(dir);\n ++}\n ++\n + static void test_reftable_stack_validate_refname(void)\n + {\n + \tstruct reftable_write_options cfg = { 0 };\n +@@ reftable/stack_test.c: int stack_test_main(int argc, const char *argv[])\n + \tRUN_TEST(test_reftable_stack_tombstone);\n + \tRUN_TEST(test_reftable_stack_transaction_api);\n + \tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n ++\tRUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);\n + \tRUN_TEST(test_reftable_stack_update_index_check);\n + \tRUN_TEST(test_reftable_stack_uptodate);\n + \tRUN_TEST(test_reftable_stack_validate_refname);\n +\n ## t/t0610-reftable-basics.sh ##\n @@ t/t0610-reftable-basics.sh: test_expect_success 'ref transaction: empty transaction in empty repo' '\n \tEOF\n 5: f336db817c = 5: 6615f25b08 refs/reftable: print errors on compaction failure\n 6: 999d0c2bb8 = 6: e84b797728 t/helper: drop pack-refs wrapper\n 7: 072627d82c = 7: 809094ffec refs: move `struct pack_refs_opts` to where it's used\n 8: 919abe8eb9 = 8: cf966fc584 refs: remove `PACK_REFS_ALL` flag\n 9: 61ebcb2d52 = 9: 5d7af236d4 refs/reftable: expose auto compaction via new flag\n10: ff163a621d = 10: 23c6c20e4e builtin/pack-refs: release allocated memory\n11: 8727f08bab = 11: eb48ac0329 builtin/pack-refs: introduce new \"--auto\" flag\n12: 65c9ff3ee5 = 12: 94cb036345 builtin/gc: move `struct maintenance_run_opts`\n13: 817de1a88f = 13: 9657c67b3b t6500: extract objects with \"17\" prefix\n14: 474cf66b26 ! 14: 3eaff8289b builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs\n @@ Commit message\n backend, which will continue to eagerly pack refs. But it does ensure\n that the \"reftable\" backend only compacts refs as required.\n \n - This change does not impact git-maintenance(1) as it won't ever end up\n - executing the pack-refs task when run with `--auto`.\n + This change does not impact git-maintenance(1) because this command will\n + in fact never run the pack-refs task when run with `--auto`. This issue\n + will be addressed in a subsequent commit.\n \n Signed-off-by: Patrick Steinhardt \n \n15: 71f4800d36 = 15: 1bdea3b316 builtin/gc: pack refs when using `git maintenance run --auto`\n-- \n2.44.GIT\n\n"},{"id":"491419","messageId":"b41b9b27cb8a7f7865cd9e128cb31d2a1a2e5cc5.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:38Z","receivedAt":"2024-03-25T10:02:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_init_addition()` we call `stack_uptodate()` after\nhaving created the lockfile to check whether the stack was modified\nconcurrently, which is indicated by a positive return code from the\nlatter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller\nand abort the addition.\n\nThe error handling has an off-by-one though because we check whether the\nerror code is `> 1` instead of `> 0`. Thus, instead of returning the\nlocking error, we would return a positive value. One of the callers of\n`reftable_stack_init_addition()` works around this bug by repeating the\nerror code check without the off-by-one. But other callers are subtly\nbroken by this bug.\n\nFix this by checking for `err > 0` instead. This has the consequence\nthat `reftable_stack_init_addition()` won't ever return a positive error\ncode anymore, but will instead return `REFTABLE_LOCK_ERROR` now. Thus,\nwe can drop the check for a positive error code in `stack_try_add()`\nnow.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ecf1b9751..92d9a7facb 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n-\n-\tif (err > 1) {\n+\tif (err > 0) {\n \t\terr = REFTABLE_LOCK_ERROR;\n \t\tgoto done;\n \t}\n@@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st,\n \tint err = reftable_stack_init_addition(&add, st);\n \tif (err < 0)\n \t\tgoto done;\n-\tif (err > 0) {\n-\t\terr = REFTABLE_LOCK_ERROR;\n-\t\tgoto done;\n-\t}\n \n \terr = reftable_addition_add(&add, write_table, arg);\n \tif (err < 0)\n-- \n2.44.GIT\n\n"},{"id":"491416","messageId":"be7212006ba983be056834e29fc36f4d9a80413a.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 02/15] reftable/error: discern locked/outdated errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:42Z","receivedAt":"2024-03-25T10:02:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently throw two different errors into a similar-but-different\nerror code:\n\n - Errors when trying to lock the reftable stack.\n\n - Errors when trying to write to the reftable stack which has been\n modified concurrently.\n\nThis results in unclear error handling and user-visible error messages.\n\nCreate a new `REFTABLE_OUTDATED_ERROR` so that those error conditions\ncan be clearly told apart from each other. Adjust users of the old\n`REFTABLE_LOCK_ERROR` to use the new error code as required.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/error.c | 4 +++-\n reftable/reftable-error.h | 5 ++++-\n reftable/stack.c | 6 +++---\n reftable/stack_test.c | 2 +-\n 4 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/reftable/error.c b/reftable/error.c\nindex 0d1766735e..cfb7a0fda4 100644\n--- a/reftable/error.c\n+++ b/reftable/error.c\n@@ -22,7 +22,7 @@ const char *reftable_error_str(int err)\n \tcase REFTABLE_NOT_EXIST_ERROR:\n \t\treturn \"file does not exist\";\n \tcase REFTABLE_LOCK_ERROR:\n-\t\treturn \"data is outdated\";\n+\t\treturn \"data is locked\";\n \tcase REFTABLE_API_ERROR:\n \t\treturn \"misuse of the reftable API\";\n \tcase REFTABLE_ZLIB_ERROR:\n@@ -35,6 +35,8 @@ const char *reftable_error_str(int err)\n \t\treturn \"invalid refname\";\n \tcase REFTABLE_ENTRY_TOO_BIG_ERROR:\n \t\treturn \"entry too large\";\n+\tcase REFTABLE_OUTDATED_ERROR:\n+\t\treturn \"data concurrently modified\";\n \tcase -1:\n \t\treturn \"general error\";\n \tdefault:\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex 4c457aaaf8..e9b07c9f36 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -25,7 +25,7 @@ enum reftable_error {\n \t */\n \tREFTABLE_NOT_EXIST_ERROR = -4,\n \n-\t/* Trying to write out-of-date data. */\n+\t/* Trying to access locked data. */\n \tREFTABLE_LOCK_ERROR = -5,\n \n \t/* Misuse of the API:\n@@ -57,6 +57,9 @@ enum reftable_error {\n \t/* Entry does not fit. This can happen when writing outsize reflog\n \t messages. */\n \tREFTABLE_ENTRY_TOO_BIG_ERROR = -11,\n+\n+\t/* Trying to write out-of-date data. */\n+\tREFTABLE_OUTDATED_ERROR = -12,\n };\n \n /* convert the numeric error code to a string. The string should not be\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 92d9a7facb..eaa8bb9c99 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -529,9 +529,9 @@ int reftable_stack_add(struct reftable_stack *st,\n {\n \tint err = stack_try_add(st, write, arg);\n \tif (err < 0) {\n-\t\tif (err == REFTABLE_LOCK_ERROR) {\n+\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n \t\t\t/* Ignore error return, we want to propagate\n-\t\t\t REFTABLE_LOCK_ERROR.\n+\t\t\t REFTABLE_OUTDATED_ERROR.\n \t\t\t*/\n \t\t\treftable_stack_reload(st);\n \t\t}\n@@ -591,7 +591,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n \tif (err > 0) {\n-\t\terr = REFTABLE_LOCK_ERROR;\n+\t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n \t}\n \ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 7336757cf5..2c3540d9e6 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -232,7 +232,7 @@ static void test_reftable_stack_uptodate(void)\n \tEXPECT_ERR(err);\n \n \terr = reftable_stack_add(st2, &write_test_ref, &ref2);\n-\tEXPECT(err == REFTABLE_LOCK_ERROR);\n+\tEXPECT(err == REFTABLE_OUTDATED_ERROR);\n \n \terr = reftable_stack_reload(st2);\n \tEXPECT_ERR(err);\n-- \n2.44.GIT\n\n"},{"id":"491422","messageId":"95dda44672a253927d625685a3eb911463318f3a.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 03/15] reftable/stack: use error codes when locking fails during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:46Z","receivedAt":"2024-03-25T10:02:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Compaction of a reftable stack may fail gracefully when there is a\nconcurrent process that writes to the reftable stack and which has thus\nlocked either the \"tables.list\" file or one of the tables. This is\nexpected and can be handled gracefully by some of the callers which\ninvoke compaction. Thus, to indicate this situation to our callers, we\nreturn a positive return code from `stack_compact_range()` and bubble it\nup to the caller.\n\nThis kind of error handling is somewhat awkward though as many callers\nin the call chain never even think of handling positive return values.\nThus, the result is either that such errors are swallowed by accident,\nor that we abort operations with an unhelpful error message.\n\nMake the code more robust by always using negative error codes when\ncompaction fails, with `REFTABLE_LOCK_ERROR` for the described benign\nerror case.\n\nNote that only a single callsite knew to handle positive error codes\ngracefully in the first place. Subsequent commits will touch up some of\nthe other sites to handle those errors better.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex eaa8bb9c99..79856b6565 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -973,7 +973,15 @@ static int stack_write_compact(struct reftable_stack *st,\n \treturn err;\n }\n \n-/* < 0: error. 0 == OK, > 0 attempt failed; could retry. */\n+/*\n+ * Compact all tables in the range `[first, last)` into a single new table.\n+ *\n+ * This function returns `0` on success or a code `< 0` on failure. When the\n+ * stack or any of the tables in the specified range are already locked then\n+ * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that\n+ * callers can either ignore, or they may choose to retry compaction after some\n+ * amount of time.\n+ */\n static int stack_compact_range(struct reftable_stack *st,\n \t\t\t size_t first, size_t last,\n \t\t\t struct reftable_log_expiry_config *expiry)\n@@ -1003,7 +1011,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\tLOCK_NO_DEREF);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n-\t\t\terr = 1;\n+\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -1025,7 +1033,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\t\ttable_name.buf, LOCK_NO_DEREF);\n \t\tif (err < 0) {\n \t\t\tif (errno == EEXIST)\n-\t\t\t\terr = 1;\n+\t\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\t\telse\n \t\t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n@@ -1075,7 +1083,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t\t\tLOCK_NO_DEREF);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n-\t\t\terr = 1;\n+\t\t\terr = REFTABLE_LOCK_ERROR;\n \t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -1187,7 +1195,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,\n \t\t\t\t struct reftable_log_expiry_config *config)\n {\n \tint err = stack_compact_range(st, first, last, config);\n-\tif (err > 0)\n+\tif (err == REFTABLE_LOCK_ERROR)\n \t\tst->stats.failures++;\n \treturn err;\n }\n-- \n2.44.GIT\n\n"},{"id":"491414","messageId":"50a3c37f92a28876c0db24e515826485c863ecc3.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:50Z","receivedAt":"2024-03-25T10:02:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever we commit a new table to the reftable stack we will end up\ninvoking auto-compaction of the stack to keep the total number of tables\nat bay. This auto-compaction may fail though in case at least one of the\ntables which we are about to compact is locked. This is indicated by the\ncompaction function returning `REFTABLE_LOCK_ERROR`. We do not handle\nthis case though, and thus bubble that return value up the calling\nchain, which will ultimately cause a failure.\n\nFix this bug by ignoring `REFTABLE_LOCK_ERROR`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 13 +++++++++++-\n reftable/stack_test.c | 43 ++++++++++++++++++++++++++++++++++++++\n t/t0610-reftable-basics.sh | 20 ++++++++++++++++++\n 3 files changed, 75 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 79856b6565..dde50b61d6 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)\n \tif (err)\n \t\tgoto done;\n \n-\tif (!add->stack->disable_auto_compact)\n+\tif (!add->stack->disable_auto_compact) {\n+\t\t/*\n+\t\t * Auto-compact the stack to keep the number of tables in\n+\t\t * control. It is possible that a concurrent writer is already\n+\t\t * trying to compact parts of the stack, which would lead to a\n+\t\t * `REFTABLE_LOCK_ERROR` because parts of the stack are locked\n+\t\t * already. This is a benign error though, so we ignore it.\n+\t\t */\n \t\terr = reftable_stack_auto_compact(add->stack);\n+\t\tif (err < 0 && err != REFTABLE_LOCK_ERROR)\n+\t\t\tgoto done;\n+\t\terr = 0;\n+\t}\n \n done:\n \treftable_addition_close(add);\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 2c3540d9e6..822e681028 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -343,6 +343,48 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n+static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n+{\n+\tstruct reftable_ref_record ref = {\n+\t\t.refname = \"refs/heads/master\",\n+\t\t.update_index = 1,\n+\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t.value.val1 = {0x01},\n+\t};\n+\tstruct reftable_write_options cfg = {0};\n+\tstruct reftable_stack *st;\n+\tstruct strbuf table_path = STRBUF_INIT;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tint err;\n+\n+\terr = reftable_new_stack(&st, dir, cfg);\n+\tEXPECT_ERR(err);\n+\n+\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st->merged->stack_len == 1);\n+\tEXPECT(st->stats.attempts == 0);\n+\tEXPECT(st->stats.failures == 0);\n+\n+\t/*\n+\t * Lock the newly written table such that it cannot be compacted.\n+\t * Adding a new table to the stack should not be impacted by this, even\n+\t * though auto-compaction will now fail.\n+\t */\n+\tstrbuf_addf(&table_path, \"%s/%s.lock\", dir, st->readers[0]->name);\n+\twrite_file_buf(table_path.buf, \"\", 0);\n+\n+\tref.update_index = 2;\n+\terr = reftable_stack_add(st, write_test_ref, &ref);\n+\tEXPECT_ERR(err);\n+\tEXPECT(st->merged->stack_len == 2);\n+\tEXPECT(st->stats.attempts == 1);\n+\tEXPECT(st->stats.failures == 1);\n+\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n static void test_reftable_stack_validate_refname(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n@@ -1085,6 +1127,7 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_tombstone);\n \tRUN_TEST(test_reftable_stack_transaction_api);\n \tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n+\tRUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n \tRUN_TEST(test_reftable_stack_validate_refname);\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 686781192e..5f2f9baa9b 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '\n \tEOF\n '\n \n+test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\t\tfor i in $(test_seq 10)\n+\t\tdo\n+\t\t\tgit branch branch-$i &&\n+\t\t\tfor table in .git/reftable/*.ref\n+\t\t\tdo\n+\t\t\t\ttouch \"$table.lock\" || exit 1\n+\t\t\tdone ||\n+\t\t\texit 1\n+\t\tdone &&\n+\t\ttest_line_count = 13 .git/reftable/tables.list\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"491417","messageId":"6615f25b0890c934a31902bed024eaa9146ec641.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 05/15] refs/reftable: print errors on compaction failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:54Z","receivedAt":"2024-03-25T10:02:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-pack-refs(1) fails in the reftable backend we end up printing\nno error message at all, leaving the caller puzzled as to why compaction\nhas failed. Fix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 6 +++++-\n t/t0610-reftable-basics.sh | 12 ++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e206d5a073..694dc4845f 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1204,8 +1204,12 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n \t\tstack = refs->main_stack;\n \n \tret = reftable_stack_compact_all(stack, NULL);\n-\tif (ret)\n+\tif (ret < 0) {\n+\t\tret = error(_(\"unable to compact stack: %s\"),\n+\t\t\t reftable_error_str(ret));\n \t\tgoto out;\n+\t}\n+\n \tret = reftable_stack_clean(stack);\n \tif (ret)\n \t\tgoto out;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 5f2f9baa9b..a53d1dc493 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -375,6 +375,18 @@ test_expect_success 'pack-refs: compacts tables' '\n \ttest_line_count = 1 repo/.git/reftable/tables.list\n '\n \n+test_expect_success 'pack-refs: compaction raises locking errors' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo A &&\n+\ttouch repo/.git/reftable/tables.list.lock &&\n+\tcat >expect <<-EOF &&\n+\terror: unable to compact stack: data is locked\n+\tEOF\n+\ttest_must_fail git -C repo pack-refs 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"491418","messageId":"e84b7977287d5e2b9fbdbe1cceadf33c109a55e7.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 06/15] t/helper: drop pack-refs wrapper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:02:59Z","receivedAt":"2024-03-25T10:03:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The test helper provides a \"ref-store pack-refs\" wrapper that\nmore or less directly invokes `refs_pack_refs()`. This helper is only\nused in a single test with the \"PACK_REFS_PRUNE\" and \"PACK_REFS_ALL\"\nflags. Both of these flags can directly be accessed via git-pack-refs(1)\nthough via the `--all` and `--prune` flags, which makes the helper\nsuperfluous.\n\nRefactor the test to use git-pack-refs(1) instead of the test helper.\nDrop the now-unused test helper command.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-ref-store.c | 20 --------------------\n t/t0601-reffiles-pack-refs.sh | 13 +++++++++----\n 2 files changed, 9 insertions(+), 24 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 7a0f6cac53..82bbf6e2e6 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -112,25 +112,6 @@ static const char **get_store(const char **argv, struct ref_store **refs)\n \treturn argv + 1;\n }\n \n-static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n-\t\t\t\t\t FLAG_DEF(PACK_REFS_ALL),\n-\t\t\t\t\t { NULL, 0 } };\n-\n-static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n-{\n-\tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n-\tstatic struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;\n-\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n-\tstruct pack_refs_opts pack_opts = { .flags = flags,\n-\t\t\t\t\t .exclusions = &exclusions,\n-\t\t\t\t\t .includes = &included_refs };\n-\n-\tif (pack_opts.flags & PACK_REFS_ALL)\n-\t\tstring_list_append(pack_opts.includes, \"*\");\n-\n-\treturn refs_pack_refs(refs, &pack_opts);\n-}\n-\n static int cmd_create_symref(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n@@ -326,7 +307,6 @@ struct command {\n };\n \n static struct command commands[] = {\n-\t{ \"pack-refs\", cmd_pack_refs },\n \t{ \"create-symref\", cmd_create_symref },\n \t{ \"delete-refs\", cmd_delete_refs },\n \t{ \"rename-ref\", cmd_rename_ref },\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex c309d2bae8..b1cf587347 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -32,11 +32,16 @@ test_expect_success 'prepare a trivial repository' '\n \tHEAD=$(git rev-parse --verify HEAD)\n '\n \n-test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '\n-\tN=`find .git/refs -type f | wc -l` &&\n+test_expect_success 'pack-refs --prune --all' '\n+\ttest_path_is_missing .git/packed-refs &&\n+\tgit pack-refs --no-prune --all &&\n+\ttest_path_is_file .git/packed-refs &&\n+\tN=$(find .git/refs -type f | wc -l) &&\n \ttest \"$N\" != 0 &&\n-\ttest-tool ref-store main pack-refs PACK_REFS_PRUNE,PACK_REFS_ALL &&\n-\tN=`find .git/refs -type f` &&\n+\n+\tgit pack-refs --prune --all &&\n+\ttest_path_is_file .git/packed-refs &&\n+\tN=$(find .git/refs -type f) &&\n \ttest -z \"$N\"\n '\n \n-- \n2.44.GIT\n\n"},{"id":"491421","messageId":"809094ffec47db9c2f95850106448ad3b2e83430.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 07/15] refs: move `struct pack_refs_opts` to where it's used","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:03Z","receivedAt":"2024-03-25T10:03:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The declaration of `struct pack_refs_opts` is in a seemingly random\nplace. Move it so that it's located right next to its flags and\nfunctions that use it.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.h | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 298caf6c61..139ce7113b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -66,12 +66,6 @@ const char *ref_storage_format_to_name(unsigned int ref_storage_format);\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n-struct pack_refs_opts {\n-\tunsigned int flags;\n-\tstruct ref_exclusions *exclusions;\n-\tstruct string_list *includes;\n-};\n-\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -433,6 +427,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n #define PACK_REFS_PRUNE 0x0001\n #define PACK_REFS_ALL 0x0002\n \n+struct pack_refs_opts {\n+\tunsigned int flags;\n+\tstruct ref_exclusions *exclusions;\n+\tstruct string_list *includes;\n+};\n+\n /*\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n-- \n2.44.GIT\n\n"},{"id":"491420","messageId":"cf966fc5846e87f191118c9b115774b36e8e9e9a.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 08/15] refs: remove `PACK_REFS_ALL` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:07Z","receivedAt":"2024-03-25T10:03:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The intent of the `PACK_REFS_ALL` flag is to ask the backend to compact\nall refs instead of only a subset of them. Thus, this flag gets passed\ndown to `refs_pack_refs()` via `struct pack_refs_opts::flags`.\n\nBut starting with 4fe42f326e (pack-refs: teach pack-refs --include\noption, 2023-05-12), the flag's semantics have changed. Instead of being\nhandled by the respective backends, this flag is now getting handled by\nthe callers of `refs_pack_refs()` which will add a single glob (\"*\") to\nthe list of refs-to-be-packed. Thus, the flag serves no purpose to the\nref backends anymore.\n\nRemove the flag and replace it with a local variable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-refs.c | 5 +++--\n refs.h | 2 --\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex bcf383cac9..97921beef2 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -21,9 +21,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n+\tint pack_all = 0;\n \n \tstruct option opts[] = {\n-\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n+\t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n \t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n \t\t\tN_(\"references to include\")),\n@@ -38,7 +39,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &option_excluded_refs)\n \t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n \n-\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n+\tif (pack_all)\n \t\tstring_list_append(pack_refs_opts.includes, \"*\");\n \n \tif (!pack_refs_opts.includes->nr)\ndiff --git a/refs.h b/refs.h\nindex 139ce7113b..8c8994cb29 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -422,10 +422,8 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n /*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n- * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n */\n #define PACK_REFS_PRUNE 0x0001\n-#define PACK_REFS_ALL 0x0002\n \n struct pack_refs_opts {\n \tunsigned int flags;\n-- \n2.44.GIT\n\n"},{"id":"491423","messageId":"5d7af236d4a7fd9d133f400a3b814b03d772aacc.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 09/15] refs/reftable: expose auto compaction via new flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:11Z","receivedAt":"2024-03-25T10:03:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Under normal circumstances, the \"reftable\" backend will automatically\nperform compaction after appending to the stack. It is thus not\nnecessary and may even be considered wasteful to run git-pack-refs(1) in\n\"reftable\"-backed repositories as it will cause the backend to compact\nall tables into a single one. We do exactly that though when running\n`git maintenance run --auto` or `git gc --auto`, which gets spawned by\nGit after running some specific commands.\n\nThe `--auto` mode is typically only executing optimizations as needed.\nTo do so, we already use several heuristics for the various different\ndata structures in Git to determine whether to optimize them or not.\nWe do not use any heuristics for refs though and instead always optimize\nthem.\n\nIntroduce a new `PACK_REFS_AUTO` flag that can be passed to the backend.\nWhen not handled by the backend we will continue to behave the exact\nsame as we do right now, that is we optimize refs unconditionally. This\nis done for the \"files\" backend for now to retain current behaviour,\neven though we may eventually also want to introduce heuristics here.\nFor the \"reftable\" backend though we already do have auto-compaction, so\nwe can easily reuse that logic to implement the new auto-packing flag.\n\nNote that under normal circumstances, this should always end up being a\nno-op. After all, we already invoke the code for every single addition\nto the stack. But there are special cases where it can still be helpful\nto execute the auto-compaction code explicitly:\n\n - Concurrent writers may cause compaction to not run due to locks.\n\n - Callers may decide to disable compaction altogether and then pack\n refs at a later point due to various reasons.\n\n - Other implementations of the reftable format may do compaction\n differently or even not at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.h | 6 +++++-\n refs/reftable-backend.c | 5 ++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 8c8994cb29..d278775e08 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -422,8 +422,12 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n /*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n+ * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end\n+ * result are decided by the ref backend. Backends may ignore\n+ * this flag and fall back to a normal repack.\n */\n-#define PACK_REFS_PRUNE 0x0001\n+#define PACK_REFS_PRUNE (1 << 0)\n+#define PACK_REFS_AUTO (1 << 1)\n \n struct pack_refs_opts {\n \tunsigned int flags;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 694dc4845f..0bed6d2ab4 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1203,7 +1203,10 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n \tif (!stack)\n \t\tstack = refs->main_stack;\n \n-\tret = reftable_stack_compact_all(stack, NULL);\n+\tif (opts->flags & PACK_REFS_AUTO)\n+\t\tret = reftable_stack_auto_compact(stack);\n+\telse\n+\t\tret = reftable_stack_compact_all(stack, NULL);\n \tif (ret < 0) {\n \t\tret = error(_(\"unable to compact stack: %s\"),\n \t\t\t reftable_error_str(ret));\n-- \n2.44.GIT\n\n"},{"id":"491429","messageId":"23c6c20e4ee62bea22bb35c86baad0119985f426.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 10/15] builtin/pack-refs: release allocated memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:15Z","receivedAt":"2024-03-25T10:03:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some of the command line options in `cmd_pack_refs()` require us to\nallocate memory. This memory is never released and thus leaking, but we\npaper over this leak by declaring the respective variables as `static`\nfunction-level variables, which is somewhat awkward.\n\nRefactor the code to release the allocated memory and drop the `static`\ndeclaration. While at it, remove the useless `flags` variable.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-refs.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 97921beef2..ea2baeec76 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -13,15 +13,17 @@ static char const * const pack_refs_usage[] = {\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n-\tunsigned int flags = PACK_REFS_PRUNE;\n-\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n-\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n-\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n-\t\t\t\t\t\t .includes = &included_refs,\n-\t\t\t\t\t\t .flags = flags };\n-\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct string_list included_refs = STRING_LIST_INIT_NODUP;\n+\tstruct pack_refs_opts pack_refs_opts = {\n+\t\t.exclusions = &excludes,\n+\t\t.includes = &included_refs,\n+\t\t.flags = PACK_REFS_PRUNE,\n+\t};\n+\tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \tint pack_all = 0;\n+\tint ret;\n \n \tstruct option opts[] = {\n \t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n@@ -45,5 +47,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tif (!pack_refs_opts.includes->nr)\n \t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n \n-\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n+\tret = refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n+\n+\tclear_ref_exclusions(&excludes);\n+\tstring_list_clear(&included_refs, 0);\n+\tstring_list_clear(&option_excluded_refs, 0);\n+\treturn ret;\n }\n-- \n2.44.GIT\n\n"},{"id":"491424","messageId":"eb48ac0329fb23fd781fca8cc421ea3e9bf781a0.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 11/15] builtin/pack-refs: introduce new \"--auto\" flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:20Z","receivedAt":"2024-03-25T10:03:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Calling git-pack-refs(1) will unconditionally cause it to pack all\nrequested refs regardless of the current state of the ref database. For\nexample:\n\n - With the \"files\" backend we will end up rewriting the complete\n \"packed-refs\" file even if only a single ref would require\n compaction.\n\n - With the \"reftable\" backend we will end up always compacting all\n tables into a single table.\n\nThis behaviour can be completely unnecessary depending on the backend\nand is thus wasteful.\n\nWith the introduction of the `PACK_REFS_AUTO` flag in the preceding\ncommit we can improve this and let the backends decide for themselves\nwhether to pack refs in the first place. Expose this functionality via a\nnew \"--auto\" flag in git-pack-refs(1), which mirrors the same flag in\nboth git-gc(1) and git-maintenance(1).\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-pack-refs.txt | 15 ++++++++++++++-\n builtin/pack-refs.c | 3 ++-\n t/t0601-reffiles-pack-refs.sh | 7 +++++++\n t/t0610-reftable-basics.sh | 34 +++++++++++++++++++++++++++++++++\n 4 files changed, 57 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 284956acb3..2dcabaf74c 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n+'git pack-refs' [--all] [--no-prune] [--auto] [--include ] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,19 @@ with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--auto::\n+\n+Pack refs as needed depending on the current state of the ref database. The\n+behavior depends on the ref format used by the repository and may change in the\n+future.\n++\n+\t- \"files\": No special handling for `--auto` has been implemented.\n++\n+\t- \"reftable\": Tables are compacted such that they form a geometric\n+\t sequence. For two tables N and N+1, where N+1 is newer, this\n+\t maintains the property that N is at least twice as big as N+1. Only\n+\t tables that violate this property are compacted.\n+\n --include ::\n \n Pack refs based on a `glob(7)` pattern. Repetitions of this option\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex ea2baeec76..db40825666 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -7,7 +7,7 @@\n #include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--auto] [--include ] [--exclude ]\"),\n \tNULL\n };\n \n@@ -28,6 +28,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tstruct option opts[] = {\n \t\tOPT_BOOL(0, \"all\", &pack_all, N_(\"pack everything\")),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), PACK_REFS_AUTO),\n \t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n \t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex b1cf587347..219a495451 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -164,6 +164,13 @@ test_expect_success 'test --exclude takes precedence over --include' '\n \tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n \ttest -f .git/refs/heads/dont_pack5'\n \n+test_expect_success '--auto packs and prunes refs as usual' '\n+\tgit branch auto &&\n+\ttest_path_is_file .git/refs/heads/auto &&\n+\tgit pack-refs --auto --all &&\n+\ttest_path_is_missing .git/refs/heads/auto\n+'\n+\n test_expect_success 'see if up-to-date packed refs are preserved' '\n \tgit branch q &&\n \tgit pack-refs --all --prune &&\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex a53d1dc493..6de7529575 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,6 +387,40 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'pack-refs: auto compaction' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\n+\t\t# The tables should have been auto-compacted, and thus auto\n+\t\t# compaction should not have to do anything.\n+\t\tls -1 .git/reftable >tables-expect &&\n+\t\ttest_line_count = 4 tables-expect &&\n+\t\tgit pack-refs --auto &&\n+\t\tls -1 .git/reftable >tables-actual &&\n+\t\ttest_cmp tables-expect tables-actual &&\n+\n+\t\t# Lock all tables write some refs. Auto-compaction will be\n+\t\t# unable to compact tables and thus fails gracefully, leaving\n+\t\t# the stack in a sub-optimal state.\n+\t\tls .git/reftable/*.ref |\n+\t\twhile read table\n+\t\tdo\n+\t\t\ttouch \"$table.lock\" || exit 1\n+\t\tdone &&\n+\t\tgit branch B &&\n+\t\tgit branch C &&\n+\t\trm .git/reftable/*.lock &&\n+\t\ttest_line_count = 5 .git/reftable/tables.list &&\n+\n+\t\tgit pack-refs --auto &&\n+\t\ttest_line_count = 1 .git/reftable/tables.list\n+\t)\n+'\n+\n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"491425","messageId":"94cb036345230691f802f4afae4e801f74f90ab2.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 12/15] builtin/gc: move `struct maintenance_run_opts`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:24Z","receivedAt":"2024-03-25T10:03:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to start using `struct maintenance_run_opts` in\n`maintenance_task_pack_refs()`. Move its definition up to prepare for\nthis.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 53 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 26 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb..e0029c88f9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -180,7 +180,32 @@ static void gc_config(void)\n \tgit_config(git_default_config, NULL);\n }\n \n-struct maintenance_run_opts;\n+enum schedule_priority {\n+\tSCHEDULE_NONE = 0,\n+\tSCHEDULE_WEEKLY = 1,\n+\tSCHEDULE_DAILY = 2,\n+\tSCHEDULE_HOURLY = 3,\n+};\n+\n+static enum schedule_priority parse_schedule(const char *value)\n+{\n+\tif (!value)\n+\t\treturn SCHEDULE_NONE;\n+\tif (!strcasecmp(value, \"hourly\"))\n+\t\treturn SCHEDULE_HOURLY;\n+\tif (!strcasecmp(value, \"daily\"))\n+\t\treturn SCHEDULE_DAILY;\n+\tif (!strcasecmp(value, \"weekly\"))\n+\t\treturn SCHEDULE_WEEKLY;\n+\treturn SCHEDULE_NONE;\n+}\n+\n+struct maintenance_run_opts {\n+\tint auto_flag;\n+\tint quiet;\n+\tenum schedule_priority schedule;\n+};\n+\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -773,26 +798,6 @@ static const char *const builtin_maintenance_run_usage[] = {\n \tNULL\n };\n \n-enum schedule_priority {\n-\tSCHEDULE_NONE = 0,\n-\tSCHEDULE_WEEKLY = 1,\n-\tSCHEDULE_DAILY = 2,\n-\tSCHEDULE_HOURLY = 3,\n-};\n-\n-static enum schedule_priority parse_schedule(const char *value)\n-{\n-\tif (!value)\n-\t\treturn SCHEDULE_NONE;\n-\tif (!strcasecmp(value, \"hourly\"))\n-\t\treturn SCHEDULE_HOURLY;\n-\tif (!strcasecmp(value, \"daily\"))\n-\t\treturn SCHEDULE_DAILY;\n-\tif (!strcasecmp(value, \"weekly\"))\n-\t\treturn SCHEDULE_WEEKLY;\n-\treturn SCHEDULE_NONE;\n-}\n-\n static int maintenance_opt_schedule(const struct option *opt, const char *arg,\n \t\t\t\t int unset)\n {\n@@ -809,12 +814,6 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-struct maintenance_run_opts {\n-\tint auto_flag;\n-\tint quiet;\n-\tenum schedule_priority schedule;\n-};\n-\n /* Remember to update object flag allocation in object.h */\n #define SEEN\t\t(1u<<0)\n \n-- \n2.44.GIT\n\n"},{"id":"491426","messageId":"9657c67b3b9136be27e469da40ef0520bea6598d.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 13/15] t6500: extract objects with \"17\" prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:28Z","receivedAt":"2024-03-25T10:03:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \".git/obects/17/\" shard is somewhat special because it is used by\ngit-gc(1) to estimate how many objects there are by extrapolating the\nnumber of objects in that shard, only. In t6500 we thus have a hard\ncoded set of data that, when written to the object database, result in\nblobs starting with that prefix.\n\nWe are about to need such \"17\"-prefixed objects in another test suite.\nExtract them into \"t/oid-info/hash-info\" so that they can be reused by\nother tests.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/oid-info/hash-info | 12 ++++++++++++\n t/t6500-gc.sh | 30 +++++++-----------------------\n 2 files changed, 19 insertions(+), 23 deletions(-)\n\ndiff --git a/t/oid-info/hash-info b/t/oid-info/hash-info\nindex d0736dd1a0..b8a5bcb187 100644\n--- a/t/oid-info/hash-info\n+++ b/t/oid-info/hash-info\n@@ -15,3 +15,15 @@ empty_blob sha256:473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a3037218\n \n empty_tree sha1:4b825dc642cb6eb9a060e54bf8d69288fbee4904\n empty_tree sha256:6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321\n+\n+blob17_1 sha1:263\n+blob17_1 sha256:34\n+\n+blob17_2 sha1:410\n+blob17_2 sha256:174\n+\n+blob17_3 sha1:523\n+blob17_3 sha256:313\n+\n+blob17_4 sha1:790\n+blob17_4 sha256:481\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 18fe1c25e6..43d40175f8 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -11,23 +11,7 @@ test_expect_success 'setup' '\n \t# behavior, make sure we always pack everything to one pack by\n \t# default\n \tgit config gc.bigPackThreshold 2g &&\n-\n-\t# These are simply values which, when hashed as a blob with a newline,\n-\t# produce a hash where the first byte is 0x17 in their respective\n-\t# algorithms.\n-\ttest_oid_cache <<-EOF\n-\tobj1 sha1:263\n-\tobj1 sha256:34\n-\n-\tobj2 sha1:410\n-\tobj2 sha256:174\n-\n-\tobj3 sha1:523\n-\tobj3 sha256:313\n-\n-\tobj4 sha1:790\n-\tobj4 sha256:481\n-\tEOF\n+\ttest_oid_init\n '\n \n test_expect_success 'gc empty repository' '\n@@ -114,8 +98,8 @@ test_expect_success 'pre-auto-gc hook can stop auto gc' '\n \t\t# We need to create two object whose sha1s start with 17\n \t\t# since this is what git gc counts. As it happens, these\n \t\t# two blobs will do so.\n-\t\ttest_commit \"$(test_oid obj1)\" &&\n-\t\ttest_commit \"$(test_oid obj2)\" &&\n+\t\ttest_commit \"$(test_oid blob17_1)\" &&\n+\t\ttest_commit \"$(test_oid blob17_2)\" &&\n \n \t\tgit gc --auto >../out.actual 2>../err.actual\n \t) &&\n@@ -146,13 +130,13 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \t# We need to create two object whose sha1s start with 17\n \t# since this is what git gc counts. As it happens, these\n \t# two blobs will do so.\n-\ttest_commit \"$(test_oid obj1)\" &&\n-\ttest_commit \"$(test_oid obj2)\" &&\n+\ttest_commit \"$(test_oid blob17_1)\" &&\n+\ttest_commit \"$(test_oid blob17_2)\" &&\n \t# Our first gc will create a pack; our second will create a second pack\n \tgit gc --auto &&\n \tls .git/objects/pack/pack-*.pack | sort >existing_packs &&\n-\ttest_commit \"$(test_oid obj3)\" &&\n-\ttest_commit \"$(test_oid obj4)\" &&\n+\ttest_commit \"$(test_oid blob17_3)\" &&\n+\ttest_commit \"$(test_oid blob17_4)\" &&\n \n \tgit gc --auto 2>err &&\n \ttest_grep ! \"^warning:\" err &&\n-- \n2.44.GIT\n\n"},{"id":"491428","messageId":"3eaff8289be5032cfd425272f63a6a02ae432eb1.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 14/15] builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:32Z","receivedAt":"2024-03-25T10:03:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Forward the `--auto` flag to git-pack-refs(1) when it has been invoked\nwith this flag itself. This does not change anything for the \"files\"\nbackend, which will continue to eagerly pack refs. But it does ensure\nthat the \"reftable\" backend only compacts refs as required.\n\nThis change does not impact git-maintenance(1) because this command will\nin fact never run the pack-refs task when run with `--auto`. This issue\nwill be addressed in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 21 ++++++++++++---------\n t/t0610-reftable-basics.sh | 19 ++++++++++++++++---\n 2 files changed, 28 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex e0029c88f9..bf1f2a621a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -212,6 +212,9 @@ static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *\n \n \tcmd.git_cmd = 1;\n \tstrvec_pushl(&cmd.args, \"pack-refs\", \"--all\", \"--prune\", NULL);\n+\tif (opts->auto_flag)\n+\t\tstrvec_push(&cmd.args, \"--auto\");\n+\n \treturn run_command(&cmd);\n }\n \n@@ -572,7 +575,7 @@ static int report_last_gc_error(void)\n \treturn ret;\n }\n \n-static void gc_before_repack(void)\n+static void gc_before_repack(struct maintenance_run_opts *opts)\n {\n \t/*\n \t * We may be called twice, as both the pre- and\n@@ -583,7 +586,7 @@ static void gc_before_repack(void)\n \tif (done++)\n \t\treturn;\n \n-\tif (pack_refs && maintenance_task_pack_refs(NULL))\n+\tif (pack_refs && maintenance_task_pack_refs(opts))\n \t\tdie(FAILED_RUN, \"pack-refs\");\n \n \tif (prune_reflogs) {\n@@ -599,7 +602,6 @@ static void gc_before_repack(void)\n int cmd_gc(int argc, const char **argv, const char *prefix)\n {\n \tint aggressive = 0;\n-\tint auto_gc = 0;\n \tint quiet = 0;\n \tint force = 0;\n \tconst char *name;\n@@ -608,6 +610,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tint keep_largest_pack = -1;\n \ttimestamp_t dummy;\n \tstruct child_process rerere_cmd = CHILD_PROCESS_INIT;\n+\tstruct maintenance_run_opts opts = {0};\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -618,7 +621,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tOPT_MAGNITUDE(0, \"max-cruft-size\", &max_cruft_size,\n \t\t\t N_(\"with --cruft, limit the size of new cruft packs\")),\n \t\tOPT_BOOL(0, \"aggressive\", &aggressive, N_(\"be more thorough (increased runtime)\")),\n-\t\tOPT_BOOL_F(0, \"auto\", &auto_gc, N_(\"enable auto-gc mode\"),\n+\t\tOPT_BOOL_F(0, \"auto\", &opts.auto_flag, N_(\"enable auto-gc mode\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL_F(0, \"force\", &force,\n \t\t\t N_(\"force running gc even if there may be another gc running\"),\n@@ -663,7 +666,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (quiet)\n \t\tstrvec_push(&repack, \"-q\");\n \n-\tif (auto_gc) {\n+\tif (opts.auto_flag) {\n \t\t/*\n \t\t * Auto-gc should be least intrusive as possible.\n \t\t */\n@@ -688,7 +691,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\n-\t\t\tgc_before_repack(); /* dies on failure */\n+\t\t\tgc_before_repack(&opts); /* dies on failure */\n \t\t\tdelete_tempfile(&pidfile);\n \n \t\t\t/*\n@@ -713,7 +716,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tname = lock_repo_for_gc(force, &pid);\n \tif (name) {\n-\t\tif (auto_gc)\n+\t\tif (opts.auto_flag)\n \t\t\treturn 0; /* be quiet on --auto */\n \t\tdie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n \t\t name, (uintmax_t)pid);\n@@ -728,7 +731,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tatexit(process_log_file_at_exit);\n \t}\n \n-\tgc_before_repack();\n+\tgc_before_repack(&opts);\n \n \tif (!repository_format_precious_objects) {\n \t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n@@ -783,7 +786,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,\n \t\t\t\t\t NULL);\n \n-\tif (auto_gc && too_many_loose_objects())\n+\tif (opts.auto_flag && too_many_loose_objects())\n \t\twarning(_(\"There are too many unreachable loose objects; \"\n \t\t\t\"run 'git prune' to remove them.\"));\n \ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 6de7529575..b37d8bf3b1 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,7 +387,9 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n-test_expect_success 'pack-refs: auto compaction' '\n+for command in pack-refs gc\n+do\n+test_expect_success \"$command: auto compaction\" '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n@@ -395,14 +397,24 @@ test_expect_success 'pack-refs: auto compaction' '\n \n \t\ttest_commit A &&\n \n+\t\t# We need a bit of setup to ensure that git-gc(1) actually\n+\t\t# triggers, and that it does not write anything to the refdb.\n+\t\tgit config gc.auto 1 &&\n+\t\tgit config gc.autoDetach 0 &&\n+\t\tgit config gc.reflogExpire never &&\n+\t\tgit config gc.reflogExpireUnreachable never &&\n+\t\ttest_oid blob17_1 | git hash-object -w --stdin &&\n+\n \t\t# The tables should have been auto-compacted, and thus auto\n \t\t# compaction should not have to do anything.\n \t\tls -1 .git/reftable >tables-expect &&\n \t\ttest_line_count = 4 tables-expect &&\n-\t\tgit pack-refs --auto &&\n+\t\tgit $command --auto &&\n \t\tls -1 .git/reftable >tables-actual &&\n \t\ttest_cmp tables-expect tables-actual &&\n \n+\t\ttest_oid blob17_2 | git hash-object -w --stdin &&\n+\n \t\t# Lock all tables write some refs. Auto-compaction will be\n \t\t# unable to compact tables and thus fails gracefully, leaving\n \t\t# the stack in a sub-optimal state.\n@@ -416,10 +428,11 @@ test_expect_success 'pack-refs: auto compaction' '\n \t\trm .git/reftable/*.lock &&\n \t\ttest_line_count = 5 .git/reftable/tables.list &&\n \n-\t\tgit pack-refs --auto &&\n+\t\tgit $command --auto &&\n \t\ttest_line_count = 1 .git/reftable/tables.list\n \t)\n '\n+done\n \n test_expect_success 'pack-refs: prunes stale tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n-- \n2.44.GIT\n\n"},{"id":"491427","messageId":"1bdea3b316b6ab88d5e9c8541e8f9348fc36c998.1711360631.git.ps@pks.im","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"[PATCH v2 15/15] builtin/gc: pack refs when using `git maintenance run --auto`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T10:03:36Z","receivedAt":"2024-03-25T10:03:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When running `git maintenance run --auto`, then the various subtasks\nwill only run as needed. Thus, we for example end up only packing loose\nobjects if we hit a certain threshold.\n\nInterestingly enough, the \"pack-refs\" task is actually _never_ executed\nwhen the auto-flag is set because it does not have a condition at all.\nAs 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:\n\n The 'auto_condition' function pointer is left NULL for now. We could\n extend this in the future to have a condition check if pack-refs\n should be run during 'git maintenance run --auto'.\n\nIt is not quite clear from that quote whether it is actually intended\nthat the task doesn't run at all in this mode. Also, no test was added\nto verify this behaviour. Ultimately though, it feels quite surprising\nthat `git maintenance run --auto --task=pack-refs` would quietly never\ndo anything at all.\n\nIn any case, now that we do have the logic in place to let ref backends\ndecide whether or not to repack refs, it does make sense to wire it up\naccordingly. With the \"reftable\" backend we will thus now perform\nauto-compaction, which optimizes the refdb as needed.\n\nBut for the \"files\" backend we now unconditionally pack refs as it does\nnot yet know to handle the \"auto\" flag. Arguably, this can be seen as a\nbug fix given that previously the task never did anything at all.\nEventually though we should amend the \"files\" backend to use some\nheuristics for auto compaction, as well.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/gc.c | 12 +++++++++++-\n t/t0601-reffiles-pack-refs.sh | 10 ++++++++++\n t/t0610-reftable-basics.sh | 2 +-\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex bf1f2a621a..3c874b248b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -206,6 +206,16 @@ struct maintenance_run_opts {\n \tenum schedule_priority schedule;\n };\n \n+static int pack_refs_condition(void)\n+{\n+\t/*\n+\t * The auto-repacking logic for refs is handled by the ref backends and\n+\t * exposed via `git pack-refs --auto`. We thus always return truish\n+\t * here and let the backend decide for us.\n+\t */\n+\treturn 1;\n+}\n+\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = {\n \t[TASK_PACK_REFS] = {\n \t\t\"pack-refs\",\n \t\tmaintenance_task_pack_refs,\n-\t\tNULL,\n+\t\tpack_refs_condition,\n \t},\n };\n \ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex 219a495451..7d4ab0b91a 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -370,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'maintenance --auto unconditionally packs loose refs' '\n+\tgit update-ref refs/heads/something HEAD &&\n+\ttest_path_is_file .git/refs/heads/something &&\n+\tgit rev-parse refs/heads/something >expect &&\n+\tgit maintenance run --task=pack-refs --auto &&\n+\ttest_path_is_missing .git/refs/heads/something &&\n+\tgit rev-parse refs/heads/something >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex b37d8bf3b1..931d888bbb 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' '\n \ttest_cmp expect err\n '\n \n-for command in pack-refs gc\n+for command in pack-refs gc \"maintenance run --task=pack-refs\"\n do\n test_expect_success \"$command: auto compaction\" '\n \ttest_when_finished \"rm -rf repo\" &&\n-- \n2.44.GIT\n\n"},{"id":"491444","messageId":"ZgFecAnKzLgCbRcj@tanuki","threadId":"61142","inReplyTo":"50a3c37f92a28876c0db24e515826485c863ecc3.1711360631.git.ps@pks.im","subject":"Re: [PATCH v2 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-25T11:22:24Z","receivedAt":"2024-03-25T11:22:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 25, 2024 at 11:02:51AM +0100, Patrick Steinhardt wrote:\n[snip]\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index 2c3540d9e6..822e681028 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -343,6 +343,48 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n> \tclear_dir(dir);\n> }\n> \n> +static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n> +{\n> +\tstruct reftable_ref_record ref = {\n> +\t\t.refname = \"refs/heads/master\",\n> +\t\t.update_index = 1,\n> +\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t.value.val1 = {0x01},\n> +\t};\n> +\tstruct reftable_write_options cfg = {0};\n> +\tstruct reftable_stack *st;\n> +\tstruct strbuf table_path = STRBUF_INIT;\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tint err;\n> +\n> +\terr = reftable_new_stack(&st, dir, cfg);\n> +\tEXPECT_ERR(err);\n> +\n> +\terr = reftable_stack_add(st, write_test_ref, &ref);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(st->merged->stack_len == 1);\n> +\tEXPECT(st->stats.attempts == 0);\n> +\tEXPECT(st->stats.failures == 0);\n> +\n> +\t/*\n> +\t * Lock the newly written table such that it cannot be compacted.\n> +\t * Adding a new table to the stack should not be impacted by this, even\n> +\t * though auto-compaction will now fail.\n> +\t */\n> +\tstrbuf_addf(&table_path, \"%s/%s.lock\", dir, st->readers[0]->name);\n> +\twrite_file_buf(table_path.buf, \"\", 0);\n> +\n> +\tref.update_index = 2;\n> +\terr = reftable_stack_add(st, write_test_ref, &ref);\n> +\tEXPECT_ERR(err);\n> +\tEXPECT(st->merged->stack_len == 2);\n> +\tEXPECT(st->stats.attempts == 1);\n> +\tEXPECT(st->stats.failures == 1);\n> +\n> +\treftable_stack_destroy(st);\n> +\tclear_dir(dir);\n> +}\n> +\n\nI forgot to free the `table_path` buffer here. So this needs the\nfollowing patch on top:\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 822e681028..7b2a8b1afd 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -382,6 +382,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)\n EXPECT(st->stats.failures == 1);\n\n reftable_stack_destroy(st);\n+ strbuf_release(&table_path);\n clear_dir(dir);\n }\n\nPatrick\n"},{"id":"491446","messageId":"CAOLa=ZRAEA7rSUoYL0h-2qfEELdbPHbeGpgBJRqesyhHi9Q6WQ@mail.gmail.com","threadId":"61142","inReplyTo":"cover.1711360631.git.ps@pks.im","subject":"Re: [PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-25T11:23:42Z","receivedAt":"2024-03-25T11:23:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this is the second version of my patch series that introduces the\n> `--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)\n> and git-maintenance(1).\n>\n> Changes compared to v1:\n>\n> - Clarified some of the commit messages.\n>\n> - Adjusted the stale comment of `stack_compact_range()`.\n>\n> - Added a unit test for failing auto-compaction.\n>\n> - Clarified a comment to explain why it is fine for auto-compaction\n> to fail.\n>\n> Thanks!\n>\n> Patrick\n>\n\nThe range diff from this version looks good.\nThanks!\n"},{"id":"491464","messageId":"xmqqplvimerg.fsf@gitster.g","threadId":"61142","inReplyTo":"CAOLa=ZRAEA7rSUoYL0h-2qfEELdbPHbeGpgBJRqesyhHi9Q6WQ@mail.gmail.com","subject":"Re: [PATCH v2 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-25T16:56:35Z","receivedAt":"2024-03-25T16:56:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Patrick Steinhardt writes:\n>\n>> Hi,\n>>\n>> this is the second version of my patch series that introduces the\n>> `--auto` flag to repack refs as neeeded to git-pack-refs(1), git-gc(1)\n>> and git-maintenance(1).\n>>\n>> Changes compared to v1:\n>>\n>> - Clarified some of the commit messages.\n>>\n>> - Adjusted the stale comment of `stack_compact_range()`.\n>>\n>> - Added a unit test for failing auto-compaction.\n>>\n>> - Clarified a comment to explain why it is fine for auto-compaction\n>> to fail.\n>>\n>> Thanks!\n>>\n>> Patrick\n>>\n>\n> The range diff from this version looks good.\n> Thanks!\n\nLet's mark the topic for 'next', then.\n\nThanks, both. Will queue.\n\n"}]}