{"thread":{"id":"64292","subject":"[PATCH 0/9] refs: add a '--required' flag to 'git refs optimize'","startedAt":"2025-10-10T10:27:15Z","lastAt":"2025-10-15T20:17:14Z","messageCount":36,"participants":["Karthik Nayak","Patrick Steinhardt","Junio C Hamano","Srivastava, Nitin"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"528481","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":null,"subject":"[PATCH 0/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:04Z","receivedAt":"2025-10-10T10:27:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"At GitLab, we use a transaction manager in Gitaly (our service layer on\ntop of Git) to manage incoming requests. This provides snapshotting\ncapabilities and provides ACID properties. To optimize for performance,\nread snapshots are shared. This means they are cheaper to initiate than\nwrite snapshots. However, housekeeping requires a write snapshot.\n\nCurrently, we run housekeeping in write snapshots which includes\noptimizing references. If Git exposed information regarding if\noptimization was required, then we could spawn a read snapshot to check\nif optimization was required and only spawn a write snapshot if needed.\n\nThis patch series adds a '--required' flag to 'git refs optimize' which\nwill indicate if optimization is required for the reference backend or\nnot.\n\nThe series is structured as follows:\n\nPatches 1-4 are mostly cleanup patches, cleaning up existing code post\nthe addition of 'git refs optimize'. Some of them could be potentially\ndropped.\n\nPatch 5 fixes the test which checks for inconsisten leading whitespaces\nin our documentation for builtin commands to also consider subcommands.\n\nPatches 6-8 are preliminary patches which add the required changes to\nprovide a functionality which only checks if optimization is required\nwithout running it.\n\nPatch 9 adds the '--required' flag.\n\nThe series is based on top of 60f3f52f17 (The sixteenth batch,\n2025-10-08) with 'ps/ref-peeled-tags' merged in.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-pack-refs.adoc     |  1 +\n Documentation/git-refs.adoc          |  1 +\n Documentation/pack-refs-options.adoc |  5 +++++\n builtin/pack-refs.c                  |  2 +-\n builtin/refs.c                       |  2 +-\n pack-refs.c                          | 19 ++++++++++++-----\n pack-refs.h                          | 10 +++++++--\n refs.c                               | 11 +++++-----\n refs.h                               | 31 +++++++++++++++------------\n refs/debug.c                         | 21 ++++++++++++++----\n refs/files-backend.c                 | 27 ++++++++++++------------\n refs/packed-backend.c                | 19 ++++++++++++++---\n refs/refs-internal.h                 | 11 ++++++----\n refs/reftable-backend.c              | 34 +++++++++++++++++++++++-------\n reftable/reftable-stack.h            |  3 +++\n reftable/stack.c                     | 41 ++++++++++++++++++++++++++----------\n t/pack-refs-tests.sh                 | 21 +++++++++++++++++-\n t/t0450-txt-doc-vs-help.sh           | 28 +++++++++++++-----------\n t/t0601-reffiles-pack-refs.sh        |  2 ++\n t/t1463-refs-optimize.sh             |  2 ++\n t/unit-tests/u-reftable-stack.c      | 12 +++++++++--\n 21 files changed, 217 insertions(+), 86 deletions(-)\n\nKarthik Nayak (9):\n      refs: move to using the '.optimize' functions\n      refs: cleanup code around optimization\n      refs: rename 'pack_refs_opts' to 'optimize_refs_opts'\n      t/pack-refs-tests: move the 'test_done' to callees\n      t/t0450: split whitespace consistency check per subcommand\n      reftable/stack: return stack segments directly\n      reftable/stack: add function to check if optimization is required\n      refs: add a `optimize_required` field to `struct ref_storage_be`\n      refs: add a '--required' flag to 'git refs optimize'\n\n\n\nbase-commit: 9081ca56692a28ce89a49537e6c568b3ccdef405\nchange-id: 20250926-562-add-option-to-check-if-reference-backend-needs-repacking-aa600eb3e055\n\nThanks\n- Karthik\n\n"},{"id":"528482","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-1-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 1/9] refs: move to using the '.optimize' functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:05Z","receivedAt":"2025-10-10T10:27:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `struct ref_store` variable, exposes two ways to optimize a reftable\nbackend:\n\n  1. pack_refs\n  2. optimize\n\nThe former was specific to the 'files' + 'packed' refs backend. The\nlatter is more generic and covers all backends. While the naming is\ndifferent, both these tend to perform the same functionality.\n\nIn the following commit, we will consolidate this code to only maintain\nthe 'optimize' functions. In preparation, modify the backends to also do\nthe same, by moving to supporting the 'optimize' function. All users of\nthe refs subsystem already use the 'optimize' function so there is no\nchanges needed on the callee side.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs/debug.c            |  8 ++++----\n refs/files-backend.c    | 14 ++------------\n refs/packed-backend.c   |  6 +++---\n refs/reftable-backend.c | 13 +++----------\n 4 files changed, 12 insertions(+), 29 deletions(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 162c24e5cc..67a0bcc57b 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -117,11 +117,11 @@ static int debug_transaction_abort(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)\n+static int debug_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->pack_refs(drefs->refs, opts);\n-\ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n+\tint res = drefs->refs->be->optimize(drefs->refs, opts);\n+\ttrace_printf_key(&trace_refs, \"optimize: %d\\n\", res);\n \treturn res;\n }\n \n@@ -431,7 +431,7 @@ struct ref_storage_be refs_be_debug = {\n \t.transaction_finish = debug_transaction_finish,\n \t.transaction_abort = debug_transaction_abort,\n \n-\t.pack_refs = debug_pack_refs,\n+\t.optimize = debug_optimize,\n \t.rename_ref = debug_rename_ref,\n \t.copy_ref = debug_copy_ref,\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a4cda57981..0b81bd7f74 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,\n \treturn 0;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store,\n-\t\t\t   struct pack_refs_opts *opts)\n+static int files_optimize(struct ref_store *ref_store,\n+\t\t\t  struct pack_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1513,15 +1513,6 @@ static int files_pack_refs(struct ref_store *ref_store,\n \treturn 0;\n }\n \n-static int files_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)\n-{\n-\t/*\n-\t * For the \"files\" backend, \"optimizing\" is the same as \"packing\".\n-\t * So, we just call the existing worker function for packing.\n-\t */\n-\treturn files_pack_refs(ref_store, opts);\n-}\n-\n /*\n  * People using contrib's git-new-workdir have .git/logs/refs ->\n  * /some/other/path/.git/logs/refs, and that may live on another device.\n@@ -3972,7 +3963,6 @@ struct ref_storage_be refs_be_files = {\n \t.transaction_finish = files_transaction_finish,\n \t.transaction_abort = files_transaction_abort,\n \n-\t.pack_refs = files_pack_refs,\n \t.optimize = files_optimize,\n \t.rename_ref = files_rename_ref,\n \t.copy_ref = files_copy_ref,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 1ab0c50393..20cf9fab18 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1773,8 +1773,8 @@ static int packed_transaction_finish(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n-\t\t\t    struct pack_refs_opts *pack_opts UNUSED)\n+static int packed_optimize(struct ref_store *ref_store UNUSED,\n+\t\t\t   struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\n@@ -2129,7 +2129,7 @@ struct ref_storage_be refs_be_packed = {\n \t.transaction_finish = packed_transaction_finish,\n \t.transaction_abort = packed_transaction_abort,\n \n-\t.pack_refs = packed_pack_refs,\n+\t.optimize = packed_optimize,\n \t.rename_ref = NULL,\n \t.copy_ref = NULL,\n \ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 0b7ec3ae15..59018b93d1 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1699,11 +1699,11 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n \treturn ret;\n }\n \n-static int reftable_be_pack_refs(struct ref_store *ref_store,\n-\t\t\t\t struct pack_refs_opts *opts)\n+static int reftable_be_optimize(struct ref_store *ref_store,\n+\t\t\t\tstruct pack_refs_opts *opts)\n {\n \tstruct reftable_ref_store *refs =\n-\t\treftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, \"pack_refs\");\n+\t\treftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, \"optimize_refs\");\n \tstruct reftable_stack *stack;\n \tint ret;\n \n@@ -1732,12 +1732,6 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int reftable_be_optimize(struct ref_store *ref_store,\n-\t\t\t\tstruct pack_refs_opts *opts)\n-{\n-\treturn reftable_be_pack_refs(ref_store, opts);\n-}\n-\n struct write_create_symref_arg {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_stack *stack;\n@@ -2715,7 +2709,6 @@ struct ref_storage_be refs_be_reftable = {\n \t.transaction_finish = reftable_be_transaction_finish,\n \t.transaction_abort = reftable_be_transaction_abort,\n \n-\t.pack_refs = reftable_be_pack_refs,\n \t.optimize = reftable_be_optimize,\n \t.rename_ref = reftable_be_rename_ref,\n \t.copy_ref = reftable_be_copy_ref,\n\n-- \n2.51.0\n\n"},{"id":"528483","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-2-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 2/9] refs: cleanup code around optimization","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:06Z","receivedAt":"2025-10-10T10:27:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The previous commit, moved all backends to only use/support the\n'optimize' function within the `ref_store` structure. With this, cleanup\nall references to the 'pack_refs' field of the structure and code around\nit.\n\nModify existing documentation in this regard.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs.c               |  6 ------\n refs.h               | 10 +++-------\n refs/refs-internal.h |  3 ---\n 3 files changed, 3 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 40acaa3f42..77dc1ab501 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2312,12 +2312,6 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n \trefs->gitdir = xstrdup(path);\n }\n \n-/* backend functions */\n-int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n-{\n-\treturn refs->be->pack_refs(refs, opts);\n-}\n-\n int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)\n {\n \treturn refs->be->optimize(refs, opts);\ndiff --git a/refs.h b/refs.h\nindex 2dd7ac1a16..c6c955d78d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -499,7 +499,7 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,\n \t\t\t\tconst struct string_list *refnames);\n \n /*\n- * Flags for controlling behaviour of pack_refs()\n+ * Flags for controlling behaviour of refs_optimize()\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@@ -514,15 +514,11 @@ struct pack_refs_opts {\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- */\n-int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n-\n /*\n  * Optimize the ref store. The exact behavior is up to the backend.\n  * For the files backend, this is equivalent to packing refs.\n+ *\n+ * flags: Combination of the above PACK_REFS_* flags.\n  */\n int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 4671517dad..fc5149df5b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -422,8 +422,6 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t      struct ref_transaction *transaction,\n \t\t\t\t      struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store,\n-\t\t\t struct pack_refs_opts *opts);\n typedef int optimize_fn(struct ref_store *ref_store,\n \t\t\tstruct pack_refs_opts *opts);\n typedef int rename_ref_fn(struct ref_store *ref_store,\n@@ -550,7 +548,6 @@ struct ref_storage_be {\n \tref_transaction_finish_fn *transaction_finish;\n \tref_transaction_abort_fn *transaction_abort;\n \n-\tpack_refs_fn *pack_refs;\n \toptimize_fn *optimize;\n \trename_ref_fn *rename_ref;\n \tcopy_ref_fn *copy_ref;\n\n-- \n2.51.0\n\n"},{"id":"528484","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-3-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:07Z","receivedAt":"2025-10-10T10:27:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The previous commit removed all references to 'pack_refs()' within\nthe refs subsystem. Continue this cleanup by also renaming\n'pack_refs_opts' to 'optimize_refs_opts' and the respective flags\naccordingly. Keeping the naming consistent will make the code easier to\nmaintain.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n pack-refs.c             |  8 ++++----\n refs.c                  |  2 +-\n refs.h                  | 18 +++++++++---------\n refs/debug.c            |  2 +-\n refs/files-backend.c    | 10 +++++-----\n refs/packed-backend.c   |  2 +-\n refs/refs-internal.h    |  2 +-\n refs/reftable-backend.c |  4 ++--\n 8 files changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 1a5e07d8b8..fee77fbf9f 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -14,10 +14,10 @@ int pack_refs_core(int argc,\n {\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+\tstruct optimize_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\t.flags = OPTIMIZE_REFS_PRUNE,\n \t};\n \tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n@@ -26,8 +26,8 @@ int pack_refs_core(int argc,\n \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_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), OPTIMIZE_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), OPTIMIZE_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/refs.c b/refs.c\nindex 77dc1ab501..514fb85af2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n \trefs->gitdir = xstrdup(path);\n }\n \n-int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)\n+int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n {\n \treturn refs->be->optimize(refs, opts);\n }\ndiff --git a/refs.h b/refs.h\nindex c6c955d78d..58b222ac02 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,\n \n /*\n  * Flags for controlling behaviour of refs_optimize()\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+ * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing\n+ * OPTIMIZE_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 (1 << 0)\n-#define PACK_REFS_AUTO  (1 << 1)\n+#define OPTIMIZE_REFS_PRUNE (1 << 0)\n+#define OPTIMIZE_REFS_AUTO  (1 << 1)\n \n-struct pack_refs_opts {\n+struct optimize_refs_opts {\n \tunsigned int flags;\n \tstruct ref_exclusions *exclusions;\n \tstruct string_list *includes;\n@@ -518,9 +518,9 @@ struct pack_refs_opts {\n  * Optimize the ref store. The exact behavior is up to the backend.\n  * For the files backend, this is equivalent to packing refs.\n  *\n- * flags: Combination of the above PACK_REFS_* flags.\n+ * flags: Combination of the above OPTIMIZE_REFS_* flags.\n  */\n-int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);\n+int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts);\n \n /*\n  * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 67a0bcc57b..260d7457db 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -117,7 +117,7 @@ static int debug_transaction_abort(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)\n+static int debug_optimize(struct ref_store *ref_store, struct optimize_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n \tint res = drefs->refs->be->optimize(drefs->refs, opts);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0b81bd7f74..1c37899006 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1356,7 +1356,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n  */\n static int should_pack_ref(struct files_ref_store *refs,\n \t\t\t   const struct reference *ref,\n-\t\t\t   struct pack_refs_opts *opts)\n+\t\t\t   struct optimize_refs_opts *opts)\n {\n \tstruct string_list_item *item;\n \n@@ -1384,7 +1384,7 @@ static int should_pack_ref(struct files_ref_store *refs,\n }\n \n static int should_pack_refs(struct files_ref_store *refs,\n-\t\t\t    struct pack_refs_opts *opts)\n+\t\t\t    struct optimize_refs_opts *opts)\n {\n \tstruct ref_iterator *iter;\n \tsize_t packed_size;\n@@ -1392,7 +1392,7 @@ static int should_pack_refs(struct files_ref_store *refs,\n \tsize_t limit;\n \tint ret;\n \n-\tif (!(opts->flags & PACK_REFS_AUTO))\n+\tif (!(opts->flags & OPTIMIZE_REFS_AUTO))\n \t\treturn 1;\n \n \tret = packed_refs_size(refs->packed_ref_store, &packed_size);\n@@ -1446,7 +1446,7 @@ static int should_pack_refs(struct files_ref_store *refs,\n }\n \n static int files_optimize(struct ref_store *ref_store,\n-\t\t\t  struct pack_refs_opts *opts)\n+\t\t\t  struct optimize_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1489,7 +1489,7 @@ static int files_optimize(struct ref_store *ref_store,\n \t\t\t    iter->ref.name, err.buf);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n-\t\tif ((opts->flags & PACK_REFS_PRUNE)) {\n+\t\tif ((opts->flags & OPTIMIZE_REFS_PRUNE)) {\n \t\t\tstruct ref_to_prune *n;\n \t\t\tFLEX_ALLOC_STR(n, name, iter->ref.name);\n \t\t\toidcpy(&n->oid, iter->ref.oid);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 20cf9fab18..acaa5a6e57 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1774,7 +1774,7 @@ static int packed_transaction_finish(struct ref_store *ref_store,\n }\n \n static int packed_optimize(struct ref_store *ref_store UNUSED,\n-\t\t\t   struct pack_refs_opts *pack_opts UNUSED)\n+\t\t\t   struct optimize_refs_opts *pack_optsa UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex fc5149df5b..4ef2422729 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -423,7 +423,7 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t      struct strbuf *err);\n \n typedef int optimize_fn(struct ref_store *ref_store,\n-\t\t\tstruct pack_refs_opts *opts);\n+\t\t\tstruct optimize_refs_opts *opts);\n typedef int rename_ref_fn(struct ref_store *ref_store,\n \t\t\t  const char *oldref, const char *newref,\n \t\t\t  const char *logmsg);\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 59018b93d1..d77714366a 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1700,7 +1700,7 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,\n }\n \n static int reftable_be_optimize(struct ref_store *ref_store,\n-\t\t\t\tstruct pack_refs_opts *opts)\n+\t\t\t\tstruct optimize_refs_opts *opts)\n {\n \tstruct reftable_ref_store *refs =\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, \"optimize_refs\");\n@@ -1714,7 +1714,7 @@ static int reftable_be_optimize(struct ref_store *ref_store,\n \tif (!stack)\n \t\tstack = refs->main_backend.stack;\n \n-\tif (opts->flags & PACK_REFS_AUTO)\n+\tif (opts->flags & OPTIMIZE_REFS_AUTO)\n \t\tret = reftable_stack_auto_compact(stack);\n \telse\n \t\tret = reftable_stack_compact_all(stack, NULL);\n\n-- \n2.51.0\n\n"},{"id":"528485","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-4-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:08Z","receivedAt":"2025-10-10T10:27:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In ac0bad0af4 (t0601: refactor tests to be shareable, 2025-09-19), we\nrefactored 't/t0601-reffiles-pack-refs.sh' to move all of the tests to\n't/pack-refs-tests.sh', which became a common test suite which was also\nused by 't/t1463-refs-optimize.sh'.\n\nThis also moved the 'test_done' directive to 't/pack-refs-tests.sh'.\nWhich inhibits additional tests from being added to either of the tests.\nLet's move the directive out to both the tests, so that we can add\nadditional specific tests to them. Also the test flow logic shouldn't be\npart of tests which can be embedded in other test scripts.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n t/pack-refs-tests.sh          | 1 -\n t/t0601-reffiles-pack-refs.sh | 2 ++\n t/t1463-refs-optimize.sh      | 2 ++\n 3 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 095823d915..6a71838ffa 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -460,4 +460,3 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '\n \t)\n '\n \n-test_done\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex 12cf5d1dcb..3c706978ef 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -18,3 +18,5 @@ export GIT_TEST_DEFAULT_REF_FORMAT\n . ./test-lib.sh\n \n . \"$TEST_DIRECTORY\"/pack-refs-tests.sh\n+\n+test_done\ndiff --git a/t/t1463-refs-optimize.sh b/t/t1463-refs-optimize.sh\nindex c11c905d79..9afe3c1ed7 100755\n--- a/t/t1463-refs-optimize.sh\n+++ b/t/t1463-refs-optimize.sh\n@@ -15,3 +15,5 @@ export GIT_TEST_DEFAULT_REF_FORMAT\n \n pack_refs='refs optimize'\n . \"$TEST_DIRECTORY\"/pack-refs-tests.sh\n+\n+test_done\n\n-- \n2.51.0\n\n"},{"id":"528486","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-5-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 5/9] t/t0450: split whitespace consistency check per subcommand","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:09Z","receivedAt":"2025-10-10T10:27:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The test t0450 contains a test to ensure that leading whitespaces within\na commands help text is consistent. This would catch issues such as:\n\n  usage: git show-ref [--head] [-d | --dereference]\n                         [-s | --hash[=<n>]] [--abbrev[=<n>]] [--branches] [--tags]\n                      [--] [<pattern>...]\n\nwhere the second line has inconsistent leading whitespaces. However this\nconsiders that all lines within a command will be aligned similarly.\nThis works for most commands, however when dealing commands which\ninclude subcommands, this assumption doesn't hold. Consider the help\ntext for 'git-refs(1)':\n\n  usage: git refs migrate --ref-format=<format> [--no-reflog] [--dry-run]\n     or: git refs verify [--strict] [--verbose]\n     or: git refs list [--count=<count>] [--shell|--perl|--python|--tcl]\n                                  [(--sort=<key>)...] [--format=<format>]\n                                  [--include-root-refs] [--points-at=<object>]\n                                  [--merged[=<object>]] [--no-merged[=<object>]]\n                                  [--contains[=<object>]] [--no-contains[=<object>]]\n                                  [(--exclude=<pattern>)...] [--start-after=<marker>]\n                                  [ --stdin | (<pattern>...)]\n     or: git refs exists <ref>\n     or: git refs optimize [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n\nWith the current implementation of this test, any flags added to 'git\nrefs optimize' in a newline would require it to be aligned with the\nflags of 'git refs list'. Which is incorrect, since we'd want it to be\naligned with the flags already added to 'git refs optimize'.\n\nLet's modify the test to work with subcommands. Do this by swapping out\nthe old logic. The old logic simply counts the number of spaces for all\nlines with leading spaces and checks to make sure they're equal.\nInstead, now we create a list of (subcommand number, leading space) and\nthen ensure that there are only unique values per subcommand.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n t/t0450-txt-doc-vs-help.sh | 28 ++++++++++++++++------------\n 1 file changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh\nindex e12e18f97f..150655f9f0 100755\n--- a/t/t0450-txt-doc-vs-help.sh\n+++ b/t/t0450-txt-doc-vs-help.sh\n@@ -94,19 +94,23 @@ do\n \t\tcheck_dashed_labels \"$(help_to_synopsis \"$builtin\")\"\n \t'\n \n-\ttest_expect_success \"$builtin -h output has consistent spacing\" '\n+\ttest_expect_success \"$builtin -h output has consistent spacing for each subcommand\" '\n \t\th2s=\"$(help_to_synopsis \"$builtin\")\" &&\n-\t\tsed -n \\\n-\t\t\t-e \"/^ / {\n-\t\t\t\ts/[^ ].*//;\n-\t\t\t\tp;\n-\t\t\t}\" \\\n-\t\t\t<\"$h2s\" >help &&\n-\t\tsort -u help >help.ws &&\n-\t\tif test -s help.ws\n-\t\tthen\n-\t\t\ttest_line_count = 1 help.ws\n-\t\tfi\n+\n+\t\t# For each subcommand, capture the number of whitespaces\n+\t\t# specific to that subcommand.\n+\t\tawk \"\n+\t\t    /^[^ ]/ { subcommand++ } # Count the number of subcommands\n+\t\t    /^ / {\n+\t\t       match(\\$0, /^ */);\n+\t\t       print subcommand, RLENGTH;\n+\t\t    }\n+\t\t\" <\"$h2s\" \\\n+\t\t| sort -u \\\n+\t\t| cut -d\" \" -f1 \\\n+\t\t| uniq -d > help.inconsistent &&\n+\n+\t\ttest_must_be_empty help.inconsistent\n \t'\n \n \tadoc=\"$(builtin_to_adoc \"$builtin\")\" &&\n\n-- \n2.51.0\n\n"},{"id":"528487","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-6-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 6/9] reftable/stack: return stack segments directly","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:10Z","receivedAt":"2025-10-10T10:27:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `stack_table_sizes_for_compaction()` function returns individual\nsizes of each reftable table. This function is only called by\n`reftable_stack_auto_compact()` to process the table sizes.\n\nModify the function to directly return the segments, this avoids the\nextra step of receiving the sizes only to pass it to\n`suggest_compaction_segment()`.\n\nA future commit will also add functionality for checking if\nauto-compaction is necessary without performing it. This change allows\ncode re-usability in that context.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n reftable/stack.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex f91ce50bcd..9d9326ce0e 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1631,7 +1631,8 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,\n \treturn seg;\n }\n \n-static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n+static int stack_segments_for_compaction(struct reftable_stack *st,\n+\t\t\t\t\t struct segment *seg)\n {\n \tint version = (st->opts.hash_id == REFTABLE_HASH_SHA1) ? 1 : 2;\n \tint overhead = header_size(version) - 1;\n@@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n \n \tREFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);\n \tif (!sizes)\n-\t\treturn NULL;\n+\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \n \tfor (size_t i = 0; i < st->merged->tables_len; i++)\n \t\tsizes[i] = st->tables[i]->size - overhead;\n \n-\treturn sizes;\n+\t*seg = suggest_compaction_segment(sizes, st->merged->tables_len,\n+\t\t\t\t\t  st->opts.auto_compaction_factor);\n+\treftable_free(sizes);\n+\n+\treturn 0;\n }\n \n int reftable_stack_auto_compact(struct reftable_stack *st)\n {\n \tstruct segment seg;\n-\tuint64_t *sizes;\n+\tint err = 0;\n \n \tif (st->merged->tables_len < 2)\n \t\treturn 0;\n \n-\tsizes = stack_table_sizes_for_compaction(st);\n-\tif (!sizes)\n-\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n-\n-\tseg = suggest_compaction_segment(sizes, st->merged->tables_len,\n-\t\t\t\t\t st->opts.auto_compaction_factor);\n-\treftable_free(sizes);\n+\terr = stack_segments_for_compaction(st, &seg);\n+\tif (err)\n+\t\treturn err;\n \n \tif (segment_size(&seg) > 0)\n \t\treturn stack_compact_range(st, seg.start, seg.end - 1,\n\n-- \n2.51.0\n\n"},{"id":"528488","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-7-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 7/9] reftable/stack: add function to check if optimization is required","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:11Z","receivedAt":"2025-10-10T10:27:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reftable backend for references, performs auto-compaction as part of\nits regular flow. This allows it to stay optimized.\n\nCompaction can also be triggered voluntarily by the user via the 'git\npack-refs' or the 'git refs optimize' command. However, currently there\nis no way for the user to check if optimization is required without\nactually performing it. Add and expose\n`reftable_stack_compaction_required()` which will allow users to check\nif the reftable backend can be optimized.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n reftable/reftable-stack.h       |  3 +++\n reftable/stack.c                | 18 ++++++++++++++++++\n t/unit-tests/u-reftable-stack.c | 12 ++++++++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex d70fcb705d..754e955206 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -123,6 +123,9 @@ struct reftable_log_expiry_config {\n int reftable_stack_compact_all(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *config);\n \n+/* check if heuristic based compaction is required  */\n+int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);\n+\n /* heuristically compact unbalanced table stack. */\n int reftable_stack_auto_compact(struct reftable_stack *st);\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 9d9326ce0e..732141d46c 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1652,6 +1652,24 @@ static int stack_segments_for_compaction(struct reftable_stack *st,\n \treturn 0;\n }\n \n+int reftable_stack_compaction_required(struct reftable_stack *st, bool *required)\n+{\n+\tstruct segment seg;\n+\tint err = 0;\n+\n+\tif (st->merged->tables_len < 2) {\n+\t\t*required = false;\n+\t\treturn 0;\n+\t}\n+\n+\terr = stack_segments_for_compaction(st, &seg);\n+\tif (err)\n+\t\treturn err;\n+\n+\t*required = segment_size(&seg) > 0;\n+\treturn 0;\n+}\n+\n int reftable_stack_auto_compact(struct reftable_stack *st)\n {\n \tstruct segment seg;\ndiff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c\nindex a8b91812e8..80a87a15df 100644\n--- a/t/unit-tests/u-reftable-stack.c\n+++ b/t/unit-tests/u-reftable-stack.c\n@@ -1067,6 +1067,7 @@ void test_reftable_stack__add_performs_auto_compaction(void)\n \t\t\t.value_type = REFTABLE_REF_SYMREF,\n \t\t\t.value.symref = (char *) \"master\",\n \t\t};\n+\t\tbool required = false;\n \t\tchar buf[128];\n \n \t\t/*\n@@ -1087,10 +1088,17 @@ void test_reftable_stack__add_performs_auto_compaction(void)\n \t\t * auto compaction is disabled. When enabled, we should merge\n \t\t * all tables in the stack.\n \t\t */\n-\t\tif (i != n)\n+\t\tcl_assert_equal_i(reftable_stack_compaction_required(st, &required), 0);\n+\t\tif (i != n) {\n \t\t\tcl_assert_equal_i(st->merged->tables_len, i + 1);\n-\t\telse\n+\t\t\tif (i < 1)\n+\t\t\t\tcl_assert_equal_b(required, false);\n+\t\t\telse\n+\t\t\t\tcl_assert_equal_b(required, true);\n+\t\t} else {\n \t\t\tcl_assert_equal_i(st->merged->tables_len, 1);\n+\t\t\tcl_assert_equal_b(required, false);\n+\t\t}\n \t}\n \n \treftable_stack_destroy(st);\n\n-- \n2.51.0\n\n"},{"id":"528489","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-8-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:12Z","receivedAt":"2025-10-10T10:27:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"To allow users of the refs namespace to check if the reference backend\nrequires optimization, add a new field `optimize_required` field to\n`struct ref_storage_be`. This field is of type `optimize_required_fn`\nwhich is also introduced in this commit.\n\nModify the debug, files, packed and reftable backend to implement this\nfield. A following commit will expose this via 'git pack-refs' and 'git\nrefs optimize'.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs/debug.c            | 13 +++++++++++++\n refs/files-backend.c    | 11 +++++++++++\n refs/packed-backend.c   | 13 +++++++++++++\n refs/refs-internal.h    |  6 ++++++\n refs/reftable-backend.c | 25 +++++++++++++++++++++++++\n 5 files changed, 68 insertions(+)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 260d7457db..71031c8326 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -125,6 +125,17 @@ static int debug_optimize(struct ref_store *ref_store, struct optimize_refs_opts\n \treturn res;\n }\n \n+static int debug_optimize_required(struct ref_store *ref_store,\n+\t\t\t\t   struct optimize_refs_opts *opts,\n+\t\t\t\t   bool *required)\n+{\n+\tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n+\tint res = drefs->refs->be->optimize_required(drefs->refs, opts, required);\n+\ttrace_printf_key(&trace_refs, \"optimize_required: %s, res: %d\\n\",\n+\t\t\t required ? \"yes\" : \"no\", res);\n+\treturn res;\n+}\n+\n static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,\n \t\t\t    const char *newref, const char *logmsg)\n {\n@@ -432,6 +443,8 @@ struct ref_storage_be refs_be_debug = {\n \t.transaction_abort = debug_transaction_abort,\n \n \t.optimize = debug_optimize,\n+\t.optimize_required = debug_optimize_required,\n+\n \t.rename_ref = debug_rename_ref,\n \t.copy_ref = debug_copy_ref,\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1c37899006..c262ae1a7b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,\n \treturn 0;\n }\n \n+static int files_optimize_required(struct ref_store *ref_store,\n+\t\t\t\t   struct optimize_refs_opts *opts,\n+\t\t\t\t   bool *required)\n+{\n+\tstruct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,\n+\t\t\t\t\t\t      \"optimize_required\");\n+\t*required = should_pack_refs(refs, opts);\n+\treturn 0;\n+}\n+\n /*\n  * People using contrib's git-new-workdir have .git/logs/refs ->\n  * /some/other/path/.git/logs/refs, and that may live on another device.\n@@ -3964,6 +3974,7 @@ struct ref_storage_be refs_be_files = {\n \t.transaction_abort = files_transaction_abort,\n \n \t.optimize = files_optimize,\n+\t.optimize_required = files_optimize_required,\n \t.rename_ref = files_rename_ref,\n \t.copy_ref = files_copy_ref,\n \ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex acaa5a6e57..c94948f618 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,\n \treturn 0;\n }\n \n+static int packed_optimize_required(struct ref_store *ref_store UNUSED,\n+\t\t\t\t    struct optimize_refs_opts *opts UNUSED,\n+\t\t\t\t    bool *required)\n+{\n+\t/*\n+\t * Packed refs are already optimized.\n+\t */\n+\t*required = false;\n+\treturn 0;\n+}\n+\n static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store UNUSED)\n {\n \treturn empty_ref_iterator_begin();\n@@ -2130,6 +2141,8 @@ struct ref_storage_be refs_be_packed = {\n \t.transaction_abort = packed_transaction_abort,\n \n \t.optimize = packed_optimize,\n+\t.optimize_required = packed_optimize_required,\n+\n \t.rename_ref = NULL,\n \t.copy_ref = NULL,\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 4ef2422729..5c49567560 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -424,6 +424,11 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \n typedef int optimize_fn(struct ref_store *ref_store,\n \t\t\tstruct optimize_refs_opts *opts);\n+\n+typedef int optimize_required_fn(struct ref_store *ref_store,\n+\t\t\t\t struct optimize_refs_opts *opts,\n+\t\t\t\t bool *required);\n+\n typedef int rename_ref_fn(struct ref_store *ref_store,\n \t\t\t  const char *oldref, const char *newref,\n \t\t\t  const char *logmsg);\n@@ -549,6 +554,7 @@ struct ref_storage_be {\n \tref_transaction_abort_fn *transaction_abort;\n \n \toptimize_fn *optimize;\n+\toptimize_required_fn *optimize_required;\n \trename_ref_fn *rename_ref;\n \tcopy_ref_fn *copy_ref;\n \ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex d77714366a..df39fe9b38 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,\n \treturn ret;\n }\n \n+static int reftable_be_optimize_required(struct ref_store *ref_store,\n+\t\t\t\t\t struct optimize_refs_opts *opts,\n+\t\t\t\t\t bool *required)\n+{\n+\tstruct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,\n+\t\t\t\t\t\t\t       \"optimize_refs_required\");\n+\tstruct reftable_stack *stack;\n+\n+\tif (refs->err)\n+\t\treturn refs->err;\n+\n+\tstack = refs->worktree_backend.stack;\n+\tif (!stack)\n+\t\tstack = refs->main_backend.stack;\n+\n+\tif (opts->flags & OPTIMIZE_REFS_AUTO)\n+\t\treturn reftable_stack_compaction_required(stack, required);\n+\telse\n+\t\t*required = true;\n+\n+\treturn 0;\n+}\n+\n struct write_create_symref_arg {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_stack *stack;\n@@ -2710,6 +2733,8 @@ struct ref_storage_be refs_be_reftable = {\n \t.transaction_abort = reftable_be_transaction_abort,\n \n \t.optimize = reftable_be_optimize,\n+\t.optimize_required = reftable_be_optimize_required,\n+\n \t.rename_ref = reftable_be_rename_ref,\n \t.copy_ref = reftable_be_copy_ref,\n \n\n-- \n2.51.0\n\n"},{"id":"528490","messageId":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-9-c7962be584fa@gmail.com","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-0-c7962be584fa@gmail.com","subject":"[PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-10T10:27:13Z","receivedAt":"2025-10-10T10:27:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'git pack-refs' and 'git refs optimize' commands allow users to\noptimize their reference backends. However they provide no functionality\nto check if optimization is required without performing it.\n\nAdd a '--required' flag to these commands to do that. This is useful\non the server side where this information can be utilized to perform\nmore targetted maintenance runs of the repository.\n\nAdd a corresponding test for the files backend. For the reftable\nbackend, this cannot be tested easily as it performs auto-compaction.\nHowever, an earlier commit ensured the functionality was covered by\nunit test.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n Documentation/git-pack-refs.adoc     |  1 +\n Documentation/git-refs.adoc          |  1 +\n Documentation/pack-refs-options.adoc |  5 +++++\n builtin/pack-refs.c                  |  2 +-\n builtin/refs.c                       |  2 +-\n pack-refs.c                          | 11 ++++++++++-\n pack-refs.h                          | 10 ++++++++--\n refs.c                               |  7 +++++++\n refs.h                               |  7 +++++++\n t/pack-refs-tests.sh                 | 20 ++++++++++++++++++++\n 10 files changed, 61 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc\nindex fde9f2f294..62bc01b29b 100644\n--- a/Documentation/git-pack-refs.adoc\n+++ b/Documentation/git-pack-refs.adoc\n@@ -9,6 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n+                [--required]\n \n DESCRIPTION\n -----------\ndiff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc\nindex fa33680cc7..4df28b7d92 100644\n--- a/Documentation/git-refs.adoc\n+++ b/Documentation/git-refs.adoc\n@@ -20,6 +20,7 @@ git refs list [--count=<count>] [--shell|--perl|--python|--tcl]\n \t\t   [ --stdin | (<pattern>...)]\n git refs exists <ref>\n git refs optimize [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n+\t\t  [--required]\n \n DESCRIPTION\n -----------\ndiff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc\nindex 0b11282941..66d69530b9 100644\n--- a/Documentation/pack-refs-options.adoc\n+++ b/Documentation/pack-refs-options.adoc\n@@ -50,3 +50,8 @@ the provided `--exclude` patterns.\n +\n When used with `--include`, refs provided to `--include`, minus refs that are\n provided to `--exclude` will be packed.\n+\n+--required::\n+\n+Check if pack-refs is required to run, without actually performing the changes.\n+Returns an exit code of 0 if optimization is required.\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 3446b84cda..bd2df4d0b7 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -8,7 +8,7 @@ int cmd_pack_refs(int argc,\n \t\t  struct repository *repo)\n {\n \tstatic char const * const pack_refs_usage[] = {\n-\t\tN_(\"git pack-refs \" PACK_REFS_OPTS),\n+\t\tN_(\"git pack-refs \" PACK_REFS_OPTS(PACK_REFS_OPTS_SPACES_14)),\n \t\tNULL\n \t};\n \ndiff --git a/builtin/refs.c b/builtin/refs.c\nindex 3064f888b2..fca55997be 100644\n--- a/builtin/refs.c\n+++ b/builtin/refs.c\n@@ -20,7 +20,7 @@\n \tN_(\"git refs exists <ref>\")\n \n #define REFS_OPTIMIZE_USAGE \\\n-\tN_(\"git refs optimize \" PACK_REFS_OPTS)\n+\tN_(\"git refs optimize \" PACK_REFS_OPTS(PACK_REFS_OPTS_SPACES_18))\n \n static int cmd_refs_migrate(int argc, const char **argv, const char *prefix,\n \t\t\t    struct repository *repo UNUSED)\ndiff --git a/pack-refs.c b/pack-refs.c\nindex fee77fbf9f..5d4d4266de 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -21,6 +21,7 @@ int pack_refs_core(int argc,\n \t};\n \tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n+\tbool check_required = false;\n \tint pack_all = 0;\n \tint ret;\n \n@@ -28,6 +29,7 @@ int pack_refs_core(int argc,\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)\"), OPTIMIZE_REFS_PRUNE),\n \t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), OPTIMIZE_REFS_AUTO),\n+\t\tOPT_BOOL(0, \"required\", &check_required, N_(\"check if optimization is required\")),\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\"),\n@@ -47,7 +49,14 @@ int pack_refs_core(int argc,\n \tif (!pack_refs_opts.includes->nr)\n \t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n \n-\tret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);\n+\tif (check_required) {\n+\t\tbool required = false;\n+\t\tret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,\n+\t\t\t\t\t     &required);\n+\t\tret |= !required;\n+\t} else {\n+\t\tret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);\n+\t}\n \n \tclear_ref_exclusions(&excludes);\n \tstring_list_clear(&included_refs, 0);\ndiff --git a/pack-refs.h b/pack-refs.h\nindex 5de27e7da8..59951ff692 100644\n--- a/pack-refs.h\n+++ b/pack-refs.h\n@@ -7,9 +7,15 @@ struct repository;\n  * Shared usage string for options common to git-pack-refs(1)\n  * and git-refs-optimize(1). The command-specific part (e.g., \"git refs optimize \")\n  * must be prepended by the caller.\n+ *\n+ * Since multiple commands use the same opts, they need to provide the appropriate\n+ * spaces for correct alignment.\n  */\n-#define PACK_REFS_OPTS \\\n-\t\"[--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\"\n+#define PACK_REFS_OPTS_SPACES_14 \"              \"\n+#define PACK_REFS_OPTS_SPACES_18 \"                  \"\n+#define PACK_REFS_OPTS(spaces)                                                        \\\n+\t\"[--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\\n\" \\\n+\tspaces \"[--required]\"\n \n /*\n  * The core logic for pack-refs and its clones.\ndiff --git a/refs.c b/refs.c\nindex 514fb85af2..59a48b36b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n \treturn refs->be->optimize(refs, opts);\n }\n \n+int refs_optimize_required(struct ref_store *refs,\n+\t\t\t   struct optimize_refs_opts *opts,\n+\t\t\t   bool *required)\n+{\n+\treturn refs->be->optimize_required(refs, opts, required);\n+}\n+\n int reference_get_peeled_oid(struct repository *repo,\n \t\t\t     const struct reference *ref,\n \t\t\t     struct object_id *peeled_oid)\ndiff --git a/refs.h b/refs.h\nindex 58b222ac02..59f0e519c0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -522,6 +522,13 @@ struct optimize_refs_opts {\n  */\n int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts);\n \n+/*\n+ * Check if refs backend can be optimized by calling 'refs_optimize'.\n+ */\n+int refs_optimize_required(struct ref_store *ref_store,\n+\t\t\t   struct optimize_refs_opts *opts,\n+\t\t\t   bool *required);\n+\n /*\n  * Setup reflog before using. Fill in err and return -1 on failure.\n  */\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 6a71838ffa..d3e58572e2 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -460,3 +460,23 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '\n \t)\n '\n \n+test_expect_success \"refs optimize --required works as expected\" '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty --message \"initial\" &&\n+\t\ttest_must_fail git ${pack_refs} --all --auto --required &&\n+\n+\t\t# Create 14 additional references, which brings us to\n+\t\t# 15 together with the default branch.\n+\t\tprintf \"create refs/heads/loose-%d HEAD\\n\" $(test_seq 14) >stdin &&\n+\t\tgit update-ref --stdin <stdin &&\n+\t\ttest_path_is_missing .git/packed-refs &&\n+\t\ttest_must_fail git ${pack_refs} --all --auto --required &&\n+\n+\t\t# Create the 16th reference, which should cause us to repack.\n+\t\tgit update-ref refs/heads/loose-15 HEAD &&\n+\t\tgit ${pack_refs} --all --auto --required\n+\t)\n+'\n\n-- \n2.51.0\n\n"},{"id":"528491","messageId":"aOjsYoeMkF6-w7G7@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-1-c7962be584fa@gmail.com","subject":"Re: [PATCH 1/9] refs: move to using the '.optimize' functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:10Z","receivedAt":"2025-10-10T11:22:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:05PM +0200, Karthik Nayak wrote:\n> The `struct ref_store` variable, exposes two ways to optimize a reftable\n> backend:\n> \n>   1. pack_refs\n>   2. optimize\n> \n> The former was specific to the 'files' + 'packed' refs backend. The\n> latter is more generic and covers all backends. While the naming is\n> different, both these tend to perform the same functionality.\n\n\"tend to perform\" is a bit of a curious thing to say, as it raises the\nquestion when it doesn't.\n\n> In the following commit, we will consolidate this code to only maintain\n> the 'optimize' functions. In preparation, modify the backends to also do\n> the same, by moving to supporting the 'optimize' function.\n\nMaybe: \"In preparation, modify the backends so that they exclusively\nimplement the `optimize` callback, only.\"\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index a4cda57981..0b81bd7f74 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,\n>  \treturn 0;\n>  }\n>  \n> -static int files_pack_refs(struct ref_store *ref_store,\n> -\t\t\t   struct pack_refs_opts *opts)\n> +static int files_optimize(struct ref_store *ref_store,\n> +\t\t\t  struct pack_refs_opts *opts)\n\n`struct pack_refs_opts` really should be renamed to\n`refs_optimize_opts`. Let's read on, maybe you do this in subsequent\npatches.\n\nPatrick\n"},{"id":"528492","messageId":"aOjsa3UWw3ygo8FU@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-2-c7962be584fa@gmail.com","subject":"Re: [PATCH 2/9] refs: cleanup code around optimization","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:19Z","receivedAt":"2025-10-10T11:22:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:06PM +0200, Karthik Nayak wrote:\n> The previous commit, moved all backends to only use/support the\n> 'optimize' function within the `ref_store` structure. With this, cleanup\n> all references to the 'pack_refs' field of the structure and code around\n> it.\n> \n> Modify existing documentation in this regard.\n\nMakes sense.\n\n> diff --git a/refs.h b/refs.h\n> index 2dd7ac1a16..c6c955d78d 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -514,15 +514,11 @@ struct pack_refs_opts {\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> - */\n> -int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n> -\n>  /*\n>   * Optimize the ref store. The exact behavior is up to the backend.\n>   * For the files backend, this is equivalent to packing refs.\n> + *\n> + * flags: Combination of the above PACK_REFS_* flags.\n>   */\n>  int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);\n>  \n\nI don't think it makes sense to carry over this documentation here. If\nnot already the case, we should document the `struct opts::flags` field\nin the structure itself.\n\nPatrick\n"},{"id":"528493","messageId":"aOjscGfX4ns-5qCm@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-3-c7962be584fa@gmail.com","subject":"Re: [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:24Z","receivedAt":"2025-10-10T11:22:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:07PM +0200, Karthik Nayak wrote:\n> The previous commit removed all references to 'pack_refs()' within\n> the refs subsystem. Continue this cleanup by also renaming\n> 'pack_refs_opts' to 'optimize_refs_opts' and the respective flags\n> accordingly. Keeping the naming consistent will make the code easier to\n> maintain.\n\nGood, this is what I was hoping to see :)\n\n> diff --git a/refs.c b/refs.c\n> index 77dc1ab501..514fb85af2 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n>  \trefs->gitdir = xstrdup(path);\n>  }\n>  \n> -int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)\n> +int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n\nAn options struct for a function `do_something()` should typically be\ncalled `struct do_something_opts`. So we should rename the struct to\n`refs_optimize_opts`.\n\n> diff --git a/refs.h b/refs.h\n> index c6c955d78d..58b222ac02 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,\n>  \n>  /*\n>   * Flags for controlling behaviour of refs_optimize()\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> + * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing\n> + * OPTIMIZE_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 (1 << 0)\n> -#define PACK_REFS_AUTO  (1 << 1)\n> +#define OPTIMIZE_REFS_PRUNE (1 << 0)\n> +#define OPTIMIZE_REFS_AUTO  (1 << 1)\n\nAnd these would then be called `REFS_OPTIMIZE_PRUNE` etc.\n\nPatrick\n"},{"id":"528494","messageId":"aOjsdtXFgVb8QcJF@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-4-c7962be584fa@gmail.com","subject":"Re: [PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:30Z","receivedAt":"2025-10-10T11:22:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:08PM +0200, Karthik Nayak wrote:\n> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n> index 095823d915..6a71838ffa 100644\n> --- a/t/pack-refs-tests.sh\n> +++ b/t/pack-refs-tests.sh\n> @@ -460,4 +460,3 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '\n>  \t)\n>  '\n>  \n> -test_done\n\nThere's an empty trailing line now.\n\nPatrick\n"},{"id":"528495","messageId":"aOjse9udPEoWqUaO@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-6-c7962be584fa@gmail.com","subject":"Re: [PATCH 6/9] reftable/stack: return stack segments directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:35Z","receivedAt":"2025-10-10T11:22:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote:\n> The `stack_table_sizes_for_compaction()` function returns individual\n> sizes of each reftable table. This function is only called by\n> `reftable_stack_auto_compact()` to process the table sizes.\n\nMaybe: \"to decide which tables need to be compacted, if any.\"\n\n> Modify the function to directly return the segments, this avoids the\n\ns/this/which/\n\n> extra step of receiving the sizes only to pass it to\n> `suggest_compaction_segment()`.\n> \n> A future commit will also add functionality for checking if\n\ns/if/whether/\n\n> auto-compaction is necessary without performing it. This change allows\n> code re-usability in that context.\n> \n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  reftable/stack.c | 23 ++++++++++++-----------\n>  1 file changed, 12 insertions(+), 11 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index f91ce50bcd..9d9326ce0e 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n>  \n>  \tREFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);\n>  \tif (!sizes)\n> -\t\treturn NULL;\n> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n>  \n>  \tfor (size_t i = 0; i < st->merged->tables_len; i++)\n>  \t\tsizes[i] = st->tables[i]->size - overhead;\n>  \n> -\treturn sizes;\n> +\t*seg = suggest_compaction_segment(sizes, st->merged->tables_len,\n> +\t\t\t\t\t  st->opts.auto_compaction_factor);\n> +\treftable_free(sizes);\n> +\n> +\treturn 0;\n>  }\n>  \n>  int reftable_stack_auto_compact(struct reftable_stack *st)\n>  {\n>  \tstruct segment seg;\n> -\tuint64_t *sizes;\n> +\tint err = 0;\n\nThe initialization is unnecessary.\n\nOther than that this patch looks good to me.\n\nPatrick\n"},{"id":"528496","messageId":"aOjsgQge8Wqh4odB@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-7-c7962be584fa@gmail.com","subject":"Re: [PATCH 7/9] reftable/stack: add function to check if optimization is required","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:41Z","receivedAt":"2025-10-10T11:22:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:11PM +0200, Karthik Nayak wrote:\n> The reftable backend for references, performs auto-compaction as part of\n\ns/for references, //\n\n> its regular flow. This allows it to stay optimized.\n\nMaybe: \"flow, which is required to keep the number of tables part of a\nstack at bay.\"\n\n> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n> index d70fcb705d..754e955206 100644\n> --- a/reftable/reftable-stack.h\n> +++ b/reftable/reftable-stack.h\n> @@ -123,6 +123,9 @@ struct reftable_log_expiry_config {\n>  int reftable_stack_compact_all(struct reftable_stack *st,\n>  \t\t\t       struct reftable_log_expiry_config *config);\n>  \n> +/* check if heuristic based compaction is required  */\n> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);\n\nLet's use a full sentence here, starting with an upper-case letter and\nending with punctuation.\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 9d9326ce0e..732141d46c 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -1652,6 +1652,24 @@ static int stack_segments_for_compaction(struct reftable_stack *st,\n>  \treturn 0;\n>  }\n>  \n> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required)\n> +{\n> +\tstruct segment seg;\n> +\tint err = 0;\n> +\n> +\tif (st->merged->tables_len < 2) {\n> +\t\t*required = false;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\terr = stack_segments_for_compaction(st, &seg);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\t*required = segment_size(&seg) > 0;\n> +\treturn 0;\n> +}\n\nOkay, makes sense. If we have at least one segment that we'd perform\ncompaction for we return true, otherwise false.\n\nPatrick\n"},{"id":"528497","messageId":"aOjsh4102DYctgQE@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-8-c7962be584fa@gmail.com","subject":"Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:47Z","receivedAt":"2025-10-10T11:22:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:12PM +0200, Karthik Nayak wrote:\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 1c37899006..c262ae1a7b 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,\n>  \treturn 0;\n>  }\n>  \n> +static int files_optimize_required(struct ref_store *ref_store,\n> +\t\t\t\t   struct optimize_refs_opts *opts,\n> +\t\t\t\t   bool *required)\n> +{\n> +\tstruct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,\n> +\t\t\t\t\t\t      \"optimize_required\");\n> +\t*required = should_pack_refs(refs, opts);\n> +\treturn 0;\n> +}\n\nOkay, this is nice and straight-forward. One might argue that we could\nalso have the following (uncompiled, so pseudo-code-y) implementation:\n\n\tstatic int files_optimize_required(struct ref_store *ref_store,\n\t\t\t\t\t   struct optimize_refs_opts *opts,\n\t\t\t\t\t   bool *required)\n\t{\n\t\tstruct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,\n\t\t\t\t\t\t\t      \"optimize_required\");\n\t\t*required = should_pack_refs(refs, opts) ||\n\t\t\trefs_optimize_required(refs->packed_refs, opts);\n\t\treturn 0;\n\t}\n\nBut on the other hand we know that they aren't ever repacked, so it's\nprobably dumb.\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index acaa5a6e57..c94948f618 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,\n>  \treturn 0;\n>  }\n>  \n> +static int packed_optimize_required(struct ref_store *ref_store UNUSED,\n> +\t\t\t\t    struct optimize_refs_opts *opts UNUSED,\n> +\t\t\t\t    bool *required)\n> +{\n> +\t/*\n> +\t * Packed refs are already optimized.\n> +\t */\n> +\t*required = false;\n> +\treturn 0;\n> +}\n\nYup.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index d77714366a..df39fe9b38 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,\n>  \treturn ret;\n>  }\n>  \n> +static int reftable_be_optimize_required(struct ref_store *ref_store,\n> +\t\t\t\t\t struct optimize_refs_opts *opts,\n> +\t\t\t\t\t bool *required)\n> +{\n> +\tstruct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,\n> +\t\t\t\t\t\t\t       \"optimize_refs_required\");\n> +\tstruct reftable_stack *stack;\n> +\n> +\tif (refs->err)\n> +\t\treturn refs->err;\n> +\n> +\tstack = refs->worktree_backend.stack;\n> +\tif (!stack)\n> +\t\tstack = refs->main_backend.stack;\n> +\n> +\tif (opts->flags & OPTIMIZE_REFS_AUTO)\n> +\t\treturn reftable_stack_compaction_required(stack, required);\n> +\telse\n> +\t\t*required = true;\n\nThis doesn't make much sense. We should only indicate that we require\nrepacking when there are at least two tables in the stack.\n\nPatrick\n"},{"id":"528498","messageId":"aOjsjpE1vuFUXXbh@pks.im","threadId":"64292","inReplyTo":"20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-9-c7962be584fa@gmail.com","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T11:22:54Z","receivedAt":"2025-10-10T11:23:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:\n> The 'git pack-refs' and 'git refs optimize' commands allow users to\n> optimize their reference backends. However they provide no functionality\n> to check if optimization is required without performing it.\n> \n> Add a '--required' flag to these commands to do that. This is useful\n> on the server side where this information can be utilized to perform\n> more targetted maintenance runs of the repository.\n\ns/targetted/targeted/\n\n> Add a corresponding test for the files backend. For the reftable\n> backend, this cannot be tested easily as it performs auto-compaction.\n> However, an earlier commit ensured the functionality was covered by\n> unit test.\n\nYou can disable auto-compaction via the\n\"GIT_TEST_REFTABLE_AUTOCOMPACTION\" environment variable, so it should be\nrather easy to add a test.\n\n> diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc\n> index fde9f2f294..62bc01b29b 100644\n> --- a/Documentation/git-pack-refs.adoc\n> +++ b/Documentation/git-pack-refs.adoc\n> @@ -9,6 +9,7 @@ SYNOPSIS\n>  --------\n>  [verse]\n>  'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n> +                [--required]\n\nHm, I'm not a huge fan of that name. \"--dry-run\" might be a better name,\nbut on the other hand one might expect that such a command also yields\ninformation about what happens or that the compaction should be\nsuccessful. So I cannot come up with a better name, either.\n\n> diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc\n> index 0b11282941..66d69530b9 100644\n> --- a/Documentation/pack-refs-options.adoc\n> +++ b/Documentation/pack-refs-options.adoc\n> @@ -50,3 +50,8 @@ the provided `--exclude` patterns.\n>  +\n>  When used with `--include`, refs provided to `--include`, minus refs that are\n>  provided to `--exclude` will be packed.\n> +\n> +--required::\n> +\n> +Check if pack-refs is required to run, without actually performing the changes.\n\nLet's not say \"pack-refs\" here anymore, as we're migrating away from\nthat name towards more generic terminology. Maybe:\n\n    Check whether the reference store needs to be optimized without\n    actually performing the changes.\n\n> diff --git a/pack-refs.c b/pack-refs.c\n> index fee77fbf9f..5d4d4266de 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -21,6 +21,7 @@ int pack_refs_core(int argc,\n>  \t};\n>  \tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n>  \tstruct string_list_item *item;\n> +\tbool check_required = false;\n>  \tint pack_all = 0;\n>  \tint ret;\n>  \n> @@ -28,6 +29,7 @@ int pack_refs_core(int argc,\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)\"), OPTIMIZE_REFS_PRUNE),\n>  \t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), OPTIMIZE_REFS_AUTO),\n> +\t\tOPT_BOOL(0, \"required\", &check_required, N_(\"check if optimization is required\")),\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\"),\n> @@ -47,7 +49,14 @@ int pack_refs_core(int argc,\n>  \tif (!pack_refs_opts.includes->nr)\n>  \t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n>  \n> -\tret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);\n> +\tif (check_required) {\n> +\t\tbool required = false;\n> +\t\tret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,\n> +\t\t\t\t\t     &required);\n> +\t\tret |= !required;\n\nI think we shouldn't use `|=` here, and furthermore I think we need to\ndefine semantics more specifically. E.g.:\n\n    - Return 0 in case optimization is required.\n\n    - Return 2 in case no optimization is required. I mostly shy away\n      from `1` here because it might already be used? We'll have to\n      double check though.\n\n    - Return any other non-zero error code in case an error occurs.\n\nThis also needs to be documented.\n\n> diff --git a/refs.c b/refs.c\n> index 514fb85af2..59a48b36b7 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n>  \treturn refs->be->optimize(refs, opts);\n>  }\n>  \n> +int refs_optimize_required(struct ref_store *refs,\n> +\t\t\t   struct optimize_refs_opts *opts,\n> +\t\t\t   bool *required)\n> +{\n> +\treturn refs->be->optimize_required(refs, opts, required);\n> +}\n> +\n\nI feel like this should be introduced in a preceding commit.\n\nPatrick\n"},{"id":"528592","messageId":"CAOLa=ZRuZebNnPz9punnHxBqSPNp+CbM4FJ6BG1RwC1D_Fjz0w@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsYoeMkF6-w7G7@pks.im","subject":"Re: [PATCH 1/9] refs: move to using the '.optimize' functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T08:18:24Z","receivedAt":"2025-10-13T08:18:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:05PM +0200, Karthik Nayak wrote:\n>> The `struct ref_store` variable, exposes two ways to optimize a reftable\n>> backend:\n>>\n>>   1. pack_refs\n>>   2. optimize\n>>\n>> The former was specific to the 'files' + 'packed' refs backend. The\n>> latter is more generic and covers all backends. While the naming is\n>> different, both these tend to perform the same functionality.\n>\n> \"tend to perform\" is a bit of a curious thing to say, as it raises the\n> question when it doesn't.\n>\n\nIndeed, will rephrase.\n\n>> In the following commit, we will consolidate this code to only maintain\n>> the 'optimize' functions. In preparation, modify the backends to also do\n>> the same, by moving to supporting the 'optimize' function.\n>\n> Maybe: \"In preparation, modify the backends so that they exclusively\n> implement the `optimize` callback, only.\"\n>\n\nThis reads better, thanks!\n\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index a4cda57981..0b81bd7f74 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,\n>>  \treturn 0;\n>>  }\n>>\n>> -static int files_pack_refs(struct ref_store *ref_store,\n>> -\t\t\t   struct pack_refs_opts *opts)\n>> +static int files_optimize(struct ref_store *ref_store,\n>> +\t\t\t  struct pack_refs_opts *opts)\n>\n> `struct pack_refs_opts` really should be renamed to\n> `refs_optimize_opts`. Let's read on, maybe you do this in subsequent\n> patches.\n>\n\nYup, that's handled in a follow up patch!\n\n> Patrick\n"},{"id":"528593","messageId":"CAOLa=ZQC2jd=ZJjnaVEC-obYfBLipenYDFpQjsxcBX4_ULdUcA@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsa3UWw3ygo8FU@pks.im","subject":"Re: [PATCH 2/9] refs: cleanup code around optimization","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T08:22:23Z","receivedAt":"2025-10-13T08:22:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:06PM +0200, Karthik Nayak wrote:\n>> The previous commit, moved all backends to only use/support the\n>> 'optimize' function within the `ref_store` structure. With this, cleanup\n>> all references to the 'pack_refs' field of the structure and code around\n>> it.\n>>\n>> Modify existing documentation in this regard.\n>\n> Makes sense.\n>\n>> diff --git a/refs.h b/refs.h\n>> index 2dd7ac1a16..c6c955d78d 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -514,15 +514,11 @@ struct pack_refs_opts {\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>> - */\n>> -int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n>> -\n>>  /*\n>>   * Optimize the ref store. The exact behavior is up to the backend.\n>>   * For the files backend, this is equivalent to packing refs.\n>> + *\n>> + * flags: Combination of the above PACK_REFS_* flags.\n>>   */\n>>  int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);\n>>\n>\n> I don't think it makes sense to carry over this documentation here. If\n> not already the case, we should document the `struct opts::flags` field\n> in the structure itself.\n>\n> Patrick\n\nFair enough, let me remove this extra line added.\n"},{"id":"528600","messageId":"CAOLa=ZRkKhjnqw+7J8sc=OSJ6Z1POW8Q6U65=-JwzeRRm6YR4A@mail.gmail.com","threadId":"64292","inReplyTo":"aOjscGfX4ns-5qCm@pks.im","subject":"Re: [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T08:52:55Z","receivedAt":"2025-10-13T08:52:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:07PM +0200, Karthik Nayak wrote:\n>> The previous commit removed all references to 'pack_refs()' within\n>> the refs subsystem. Continue this cleanup by also renaming\n>> 'pack_refs_opts' to 'optimize_refs_opts' and the respective flags\n>> accordingly. Keeping the naming consistent will make the code easier to\n>> maintain.\n>\n> Good, this is what I was hoping to see :)\n>\n>> diff --git a/refs.c b/refs.c\n>> index 77dc1ab501..514fb85af2 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n>>  \trefs->gitdir = xstrdup(path);\n>>  }\n>>\n>> -int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)\n>> +int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n>\n> An options struct for a function `do_something()` should typically be\n> called `struct do_something_opts`. So we should rename the struct to\n> `refs_optimize_opts`.\n>\n\nWill do that.\n\n>> diff --git a/refs.h b/refs.h\n>> index c6c955d78d..58b222ac02 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,\n>>\n>>  /*\n>>   * Flags for controlling behaviour of refs_optimize()\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>> + * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing\n>> + * OPTIMIZE_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 (1 << 0)\n>> -#define PACK_REFS_AUTO  (1 << 1)\n>> +#define OPTIMIZE_REFS_PRUNE (1 << 0)\n>> +#define OPTIMIZE_REFS_AUTO  (1 << 1)\n>\n> And these would then be called `REFS_OPTIMIZE_PRUNE` etc.\n>\n> Patrick\n\nLikewise, thanks for pointing it out.\n"},{"id":"528601","messageId":"CAOLa=ZQMHY7q1rj0j_x7KnuXoasae9kXhDGcq+gdVrnmza4Rsg@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsdtXFgVb8QcJF@pks.im","subject":"Re: [PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T08:54:38Z","receivedAt":"2025-10-13T08:54:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:08PM +0200, Karthik Nayak wrote:\n>> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n>> index 095823d915..6a71838ffa 100644\n>> --- a/t/pack-refs-tests.sh\n>> +++ b/t/pack-refs-tests.sh\n>> @@ -460,4 +460,3 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '\n>>  \t)\n>>  '\n>>\n>> -test_done\n>\n> There's an empty trailing line now.\n>\n> Patrick\n\nOops, this must have snuck in when I rebased on top of\n'ps/ref-peeled-tags'. Will fix.\n"},{"id":"528602","messageId":"CAOLa=ZSfiD4KTSMyc2WW4oC2zyDO9CHmUWwnws3GhCdbtL4rAA@mail.gmail.com","threadId":"64292","inReplyTo":"aOjse9udPEoWqUaO@pks.im","subject":"Re: [PATCH 6/9] reftable/stack: return stack segments directly","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T09:01:15Z","receivedAt":"2025-10-13T09:01:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote:\n>> The `stack_table_sizes_for_compaction()` function returns individual\n>> sizes of each reftable table. This function is only called by\n>> `reftable_stack_auto_compact()` to process the table sizes.\n>\n> Maybe: \"to decide which tables need to be compacted, if any.\"\n>\n\nReads better, will update.\n\n>> Modify the function to directly return the segments, this avoids the\n>\n> s/this/which/\n>\n\nwill do.\n\n>> extra step of receiving the sizes only to pass it to\n>> `suggest_compaction_segment()`.\n>>\n>> A future commit will also add functionality for checking if\n>\n> s/if/whether/\n>\n\nThis should be okay either ways I believe, but shouldn't matter much,\nwill change.\n\n>\n>> auto-compaction is necessary without performing it. This change allows\n>> code re-usability in that context.\n>>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>>  reftable/stack.c | 23 ++++++++++++-----------\n>>  1 file changed, 12 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/reftable/stack.c b/reftable/stack.c\n>> index f91ce50bcd..9d9326ce0e 100644\n>> --- a/reftable/stack.c\n>> +++ b/reftable/stack.c\n>> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n>>\n>>  \tREFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);\n>>  \tif (!sizes)\n>> -\t\treturn NULL;\n>> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n>>\n>>  \tfor (size_t i = 0; i < st->merged->tables_len; i++)\n>>  \t\tsizes[i] = st->tables[i]->size - overhead;\n>>\n>> -\treturn sizes;\n>> +\t*seg = suggest_compaction_segment(sizes, st->merged->tables_len,\n>> +\t\t\t\t\t  st->opts.auto_compaction_factor);\n>> +\treftable_free(sizes);\n>> +\n>> +\treturn 0;\n>>  }\n>>\n>>  int reftable_stack_auto_compact(struct reftable_stack *st)\n>>  {\n>>  \tstruct segment seg;\n>> -\tuint64_t *sizes;\n>> +\tint err = 0;\n>\n> The initialization is unnecessary.\n>\n\nTrue, but I always think its nicer to define the base state like this.\nThis makes it less error prone when/if the code is modified in the\nfuture. But doesn't really matter, so let me remove it.\n\n> Other than that this patch looks good to me.\n>\n> Patrick\n\nThanks for the review.\n"},{"id":"528603","messageId":"CAOLa=ZR2zOBkS8wQk0=dr-4wU6XR2O-GHV=8LEvzq6F5Bs+tSg@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsgQge8Wqh4odB@pks.im","subject":"Re: [PATCH 7/9] reftable/stack: add function to check if optimization is required","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T09:04:10Z","receivedAt":"2025-10-13T09:04:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:11PM +0200, Karthik Nayak wrote:\n>> The reftable backend for references, performs auto-compaction as part of\n>\n> s/for references, //\n>\n>> its regular flow. This allows it to stay optimized.\n>\n> Maybe: \"flow, which is required to keep the number of tables part of a\n> stack at bay.\"\n>\n\nWill change.\n\n>> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n>> index d70fcb705d..754e955206 100644\n>> --- a/reftable/reftable-stack.h\n>> +++ b/reftable/reftable-stack.h\n>> @@ -123,6 +123,9 @@ struct reftable_log_expiry_config {\n>>  int reftable_stack_compact_all(struct reftable_stack *st,\n>>  \t\t\t       struct reftable_log_expiry_config *config);\n>>\n>> +/* check if heuristic based compaction is required  */\n>> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);\n>\n> Let's use a full sentence here, starting with an upper-case letter and\n> ending with punctuation.\n>\n\nWill do.\n\n[snip]\n"},{"id":"528612","messageId":"CAOLa=ZTQWj7BUDNx5iuoYJ6YR5JqhFVK9O2vWiKqL5CNExpsUw@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsh4102DYctgQE@pks.im","subject":"Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T09:46:33Z","receivedAt":"2025-10-13T09:46:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:12PM +0200, Karthik Nayak wrote:\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 1c37899006..c262ae1a7b 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,\n>>  \treturn 0;\n>>  }\n>>\n>> +static int files_optimize_required(struct ref_store *ref_store,\n>> +\t\t\t\t   struct optimize_refs_opts *opts,\n>> +\t\t\t\t   bool *required)\n>> +{\n>> +\tstruct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,\n>> +\t\t\t\t\t\t      \"optimize_required\");\n>> +\t*required = should_pack_refs(refs, opts);\n>> +\treturn 0;\n>> +}\n>\n> Okay, this is nice and straight-forward. One might argue that we could\n> also have the following (uncompiled, so pseudo-code-y) implementation:\n>\n> \tstatic int files_optimize_required(struct ref_store *ref_store,\n> \t\t\t\t\t   struct optimize_refs_opts *opts,\n> \t\t\t\t\t   bool *required)\n> \t{\n> \t\tstruct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,\n> \t\t\t\t\t\t\t      \"optimize_required\");\n> \t\t*required = should_pack_refs(refs, opts) ||\n> \t\t\trefs_optimize_required(refs->packed_refs, opts);\n> \t\treturn 0;\n> \t}\n>\n> But on the other hand we know that they aren't ever repacked, so it's\n> probably dumb.\n>\n\nI also think that individually the packed-backend shouldn't care about\nthe files backend. So it shouldn't be calling `should_pack_refs()`. In\nshort, the packed backend knows that it is always optimized and it\nshould simply state that.\n\n>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n>> index acaa5a6e57..c94948f618 100644\n>> --- a/refs/packed-backend.c\n>> +++ b/refs/packed-backend.c\n>> @@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,\n>>  \treturn 0;\n>>  }\n>>\n>> +static int packed_optimize_required(struct ref_store *ref_store UNUSED,\n>> +\t\t\t\t    struct optimize_refs_opts *opts UNUSED,\n>> +\t\t\t\t    bool *required)\n>> +{\n>> +\t/*\n>> +\t * Packed refs are already optimized.\n>> +\t */\n>> +\t*required = false;\n>> +\treturn 0;\n>> +}\n>\n> Yup.\n>\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index d77714366a..df39fe9b38 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,\n>>  \treturn ret;\n>>  }\n>>\n>> +static int reftable_be_optimize_required(struct ref_store *ref_store,\n>> +\t\t\t\t\t struct optimize_refs_opts *opts,\n>> +\t\t\t\t\t bool *required)\n>> +{\n>> +\tstruct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,\n>> +\t\t\t\t\t\t\t       \"optimize_refs_required\");\n>> +\tstruct reftable_stack *stack;\n>> +\n>> +\tif (refs->err)\n>> +\t\treturn refs->err;\n>> +\n>> +\tstack = refs->worktree_backend.stack;\n>> +\tif (!stack)\n>> +\t\tstack = refs->main_backend.stack;\n>> +\n>> +\tif (opts->flags & OPTIMIZE_REFS_AUTO)\n>> +\t\treturn reftable_stack_compaction_required(stack, required);\n>> +\telse\n>> +\t\t*required = true;\n>\n> This doesn't make much sense. We should only indicate that we require\n> repacking when there are at least two tables in the stack.\n>\n> Patrick\n\nFair enough, I was thinking that when users say 'git refs optimize'\nwithout any options. We always call `reftable_stack_compact_all()`. So\nit would always make sense to say required = true.\n\nBut `reftable_stack_compact_all()` does make that check. So I'll add it\nto `reftable_stack_compaction_required()` and call that with a\n`use_heuristics` parameter.\n\nThanks\n"},{"id":"528615","messageId":"aOzeMLVx6P_AbxCQ@pks.im","threadId":"64292","inReplyTo":"CAOLa=ZSfiD4KTSMyc2WW4oC2zyDO9CHmUWwnws3GhCdbtL4rAA@mail.gmail.com","subject":"Re: [PATCH 6/9] reftable/stack: return stack segments directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-13T11:10:40Z","receivedAt":"2025-10-13T11:10:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 13, 2025 at 02:01:15AM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> >> diff --git a/reftable/stack.c b/reftable/stack.c\n> >> index f91ce50bcd..9d9326ce0e 100644\n> >> --- a/reftable/stack.c\n> >> +++ b/reftable/stack.c\n> >> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)\n> >>\n> >>  \tREFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);\n> >>  \tif (!sizes)\n> >> -\t\treturn NULL;\n> >> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> >>\n> >>  \tfor (size_t i = 0; i < st->merged->tables_len; i++)\n> >>  \t\tsizes[i] = st->tables[i]->size - overhead;\n> >>\n> >> -\treturn sizes;\n> >> +\t*seg = suggest_compaction_segment(sizes, st->merged->tables_len,\n> >> +\t\t\t\t\t  st->opts.auto_compaction_factor);\n> >> +\treftable_free(sizes);\n> >> +\n> >> +\treturn 0;\n> >>  }\n> >>\n> >>  int reftable_stack_auto_compact(struct reftable_stack *st)\n> >>  {\n> >>  \tstruct segment seg;\n> >> -\tuint64_t *sizes;\n> >> +\tint err = 0;\n> >\n> > The initialization is unnecessary.\n> >\n> \n> True, but I always think its nicer to define the base state like this.\n> This makes it less error prone when/if the code is modified in the\n> future. But doesn't really matter, so let me remove it.\n\nThe counterargument to this is that it doesn't give us any nice compiler\nwarnings if a specific return forgets to set the variable, so we may\nreturn successfully by accident. If you leave this uninitialized then\nthe compiler warns you for every early return where you don't initialize\nthe variable. For all I can tell this warning is also quite reliable.\n\nPatrick\n"},{"id":"528620","messageId":"CAOLa=ZQz3Fho2N1KBrbaj3OykXeCx1b5NTjBoPs0uL5uBm3z0g@mail.gmail.com","threadId":"64292","inReplyTo":"aOjsjpE1vuFUXXbh@pks.im","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-13T12:37:08Z","receivedAt":"2025-10-13T12:37:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:\n>> The 'git pack-refs' and 'git refs optimize' commands allow users to\n>> optimize their reference backends. However they provide no functionality\n>> to check if optimization is required without performing it.\n>>\n>> Add a '--required' flag to these commands to do that. This is useful\n>> on the server side where this information can be utilized to perform\n>> more targetted maintenance runs of the repository.\n>\n> s/targetted/targeted/\n>\n\nMy spell check says both are accepted. But the latter seems to be the\ngeneral spelling.\n\n>> Add a corresponding test for the files backend. For the reftable\n>> backend, this cannot be tested easily as it performs auto-compaction.\n>> However, an earlier commit ensured the functionality was covered by\n>> unit test.\n>\n> You can disable auto-compaction via the\n> \"GIT_TEST_REFTABLE_AUTOCOMPACTION\" environment variable, so it should be\n> rather easy to add a test.\n>\n\nGood point, let me add a test.\n\n>> diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc\n>> index fde9f2f294..62bc01b29b 100644\n>> --- a/Documentation/git-pack-refs.adoc\n>> +++ b/Documentation/git-pack-refs.adoc\n>> @@ -9,6 +9,7 @@ SYNOPSIS\n>>  --------\n>>  [verse]\n>>  'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n>> +                [--required]\n>\n> Hm, I'm not a huge fan of that name. \"--dry-run\" might be a better name,\n> but on the other hand one might expect that such a command also yields\n> information about what happens or that the compaction should be\n> successful. So I cannot come up with a better name, either.\n>\n\nYea, I too had the same thought process. Something like `--dry-run`\nwould create the actual compacted files but not replace the existing\nfiles.\n\nWe could perhaps use `--check` ? But I'm okay with either.\n\n>> diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc\n>> index 0b11282941..66d69530b9 100644\n>> --- a/Documentation/pack-refs-options.adoc\n>> +++ b/Documentation/pack-refs-options.adoc\n>> @@ -50,3 +50,8 @@ the provided `--exclude` patterns.\n>>  +\n>>  When used with `--include`, refs provided to `--include`, minus refs that are\n>>  provided to `--exclude` will be packed.\n>> +\n>> +--required::\n>> +\n>> +Check if pack-refs is required to run, without actually performing the changes.\n>\n> Let's not say \"pack-refs\" here anymore, as we're migrating away from\n> that name towards more generic terminology. Maybe:\n>\n>     Check whether the reference store needs to be optimized without\n>     actually performing the changes.\n>\n\nThis is better, will amend.\n\n>> diff --git a/pack-refs.c b/pack-refs.c\n>> index fee77fbf9f..5d4d4266de 100644\n>> --- a/pack-refs.c\n>> +++ b/pack-refs.c\n>> @@ -21,6 +21,7 @@ int pack_refs_core(int argc,\n>>  \t};\n>>  \tstruct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n>>  \tstruct string_list_item *item;\n>> +\tbool check_required = false;\n>>  \tint pack_all = 0;\n>>  \tint ret;\n>>\n>> @@ -28,6 +29,7 @@ int pack_refs_core(int argc,\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)\"), OPTIMIZE_REFS_PRUNE),\n>>  \t\tOPT_BIT(0, \"auto\", &pack_refs_opts.flags, N_(\"auto-pack refs as needed\"), OPTIMIZE_REFS_AUTO),\n>> +\t\tOPT_BOOL(0, \"required\", &check_required, N_(\"check if optimization is required\")),\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\"),\n>> @@ -47,7 +49,14 @@ int pack_refs_core(int argc,\n>>  \tif (!pack_refs_opts.includes->nr)\n>>  \t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n>>\n>> -\tret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);\n>> +\tif (check_required) {\n>> +\t\tbool required = false;\n>> +\t\tret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,\n>> +\t\t\t\t\t     &required);\n>> +\t\tret |= !required;\n>\n> I think we shouldn't use `|=` here, and furthermore I think we need to\n> define semantics more specifically. E.g.:\n>\n>     - Return 0 in case optimization is required.\n>\n>     - Return 2 in case no optimization is required. I mostly shy away\n>       from `1` here because it might already be used? We'll have to\n>       double check though.\n>\n>     - Return any other non-zero error code in case an error occurs.\n>\n> This also needs to be documented.\n>\n\nFair. So I checked and the only backend which returns an error is the\nreftable backend and this is explicitly only the\n`REFTABLE_OUT_OF_MEMORY_ERROR` error, which is equal to -13.\n\nSo let's do 0 for optimization required, 1 for no optimization required\nand other non-zero for error.\n\nI'll modify accordingly.\n\n>> diff --git a/refs.c b/refs.c\n>> index 514fb85af2..59a48b36b7 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)\n>>  \treturn refs->be->optimize(refs, opts);\n>>  }\n>>\n>> +int refs_optimize_required(struct ref_store *refs,\n>> +\t\t\t   struct optimize_refs_opts *opts,\n>> +\t\t\t   bool *required)\n>> +{\n>> +\treturn refs->be->optimize_required(refs, opts, required);\n>> +}\n>> +\n>\n> I feel like this should be introduced in a preceding commit.\n>\n> Patrick\n\nYeah, It would be better there, will move it.\n"},{"id":"528622","messageId":"xmqqwm4y538p.fsf@gitster.g","threadId":"64292","inReplyTo":"aOjsjpE1vuFUXXbh@pks.im","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-13T13:40:38Z","receivedAt":"2025-10-13T13:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>>  'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n>> +                [--required]\n>\n> Hm, I'm not a huge fan of that name. \"--dry-run\" might be a better name,\n> but on the other hand one might expect that such a command also yields\n> information about what happens or that the compaction should be\n> successful.\n\nI had the same reaction.   First I thought this would make the\ncommand fail if the requested action cannot be done, as it is\nrequired to fulfill the request, or something, but anything user\nasks to do is by default required in a sane world, so it does not\nmake much sense.\n\nIf I understand the motivation behind this correctly, we want a\ncheap check similar to what is implicitly used before auto-gc kicks\nin (i.e., asking \"do we have enough loose objects that makes us\nworried\" without actually counting all the loose objects but\nestimating the number cheaply), but \"--dry-run\", while being much\nbetter than \"--required\" to convey the intention, has an extra\nconnotation that it does a much more thorough job than mere\nestimating---doing almost the same thing as the real job but without\nleaving any lasting effect on the data---which makes the word\ninappropriate, too.\n\nPerhaps \"--check-\" followed by a word specific to what we are trying\nto achieve (e.g., if we are trying to see if auto-compaction is\nnecessary, \"--check-for-auto\" \"check for auto compaction\")?  I\ndunno.\n\n"},{"id":"528626","messageId":"xmqqh5w250lw.fsf@gitster.g","threadId":"64292","inReplyTo":"xmqqwm4y538p.fsf@gitster.g","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-13T14:37:31Z","receivedAt":"2025-10-13T14:37:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Perhaps \"--check-\" followed by a word specific to what we are trying\n> to achieve (e.g., if we are trying to see if auto-compaction is\n> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n> dunno.\n\nAfter reading what you did in the previou step, I am reasonably sure\n\"required\" is a wrong word to use, with or without other words like\n\"check\".  Semantically it is similar to the should_pack_refs() check\nthat we use for pack-refs even before \"optimize\" came.  We expect it\nto answer this question cheaply: are we better off if we repacked,\nor can we go on without repacking for now?  It is not about \"are we\nperforming so poorly that we MUST optimize now?\"\n"},{"id":"528741","messageId":"CAOLa=ZTRG=x5PofmSSfHhJ3Lig2NjhU8buVWAXU=aKom2PDsDg@mail.gmail.com","threadId":"64292","inReplyTo":"xmqqh5w250lw.fsf@gitster.g","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-14T15:08:56Z","receivedAt":"2025-10-14T15:08:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Perhaps \"--check-\" followed by a word specific to what we are trying\n>> to achieve (e.g., if we are trying to see if auto-compaction is\n>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n>> dunno.\n>\n> After reading what you did in the previou step, I am reasonably sure\n> \"required\" is a wrong word to use, with or without other words like\n> \"check\".  Semantically it is similar to the should_pack_refs() check\n> that we use for pack-refs even before \"optimize\" came.  We expect it\n> to answer this question cheaply: are we better off if we repacked,\n> or can we go on without repacking for now?  It is not about \"are we\n> performing so poorly that we MUST optimize now?\"\n\nI agree '--required' isn't the best name, and like we discussed\n'--dry-run' wouldn't be either since that would imply that the work is\nbeing done but not persisted.\n\nI was leaning towards '--check', which is simple. But It might be nicer\nto be verbose here and simply add something like '--is-worthwhile'.\n\nBeing verbose here is okay, since it will only be used sparingly and\nspecifically by those who require such a use case.\n"},{"id":"528749","messageId":"xmqqldldxtoc.fsf@gitster.g","threadId":"64292","inReplyTo":"CAOLa=ZTRG=x5PofmSSfHhJ3Lig2NjhU8buVWAXU=aKom2PDsDg@mail.gmail.com","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-14T17:46:43Z","receivedAt":"2025-10-14T17:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Perhaps \"--check-\" followed by a word specific to what we are trying\n>>> to achieve (e.g., if we are trying to see if auto-compaction is\n>>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n>>> dunno.\n>>\n>> After reading what you did in the previou step, I am reasonably sure\n>> \"required\" is a wrong word to use, with or without other words like\n>> \"check\".  Semantically it is similar to the should_pack_refs() check\n>> that we use for pack-refs even before \"optimize\" came.  We expect it\n>> to answer this question cheaply: are we better off if we repacked,\n>> or can we go on without repacking for now?  It is not about \"are we\n>> performing so poorly that we MUST optimize now?\"\n>\n> I agree '--required' isn't the best name, and like we discussed\n> '--dry-run' wouldn't be either since that would imply that the work is\n> being done but not persisted.\n>\n> I was leaning towards '--check', which is simple. But It might be nicer\n> to be verbose here and simply add something like '--is-worthwhile'.\n>\n> Being verbose here is okay, since it will only be used sparingly and\n> specifically by those who require such a use case.\n\nNah, \"worthwhile\" is relative and it would be less meaningful\nwithout expressing for what goal we are judging how it is worthwhile\nto do.\n\nChoosing a phrase around \"check\" is better, I would think.\n\n\n"},{"id":"528791","messageId":"AS8PR04MB843639DC158AE8E035D83FA4A8E8A@AS8PR04MB8436.eurprd04.prod.outlook.com","threadId":"64292","inReplyTo":"xmqqldldxtoc.fsf@gitster.g","subject":"RE: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Srivastava, Nitin","fromEmail":"nitin.srivastava@knorr-bremse.com","sentAt":"2025-10-15T07:50:11Z","receivedAt":"2025-10-15T07:50:34Z","isPatch":true,"sender":{"key":"nitin.srivastava@knorr-bremse.com","avatar":null},"body":"\nPlease unsubscribe me from all email notifications.\n\n\n-----Original Message-----\nFrom: Junio C Hamano <gitster@pobox.com>\nSent: Tuesday, October 14, 2025 11:17 PM\nTo: Karthik Nayak <karthik.188@gmail.com>\nCc: Patrick Steinhardt <ps@pks.im>; git@vger.kernel.org\nSubject: Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'\n\nKarthik Nayak <karthik.188@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Perhaps \"--check-\" followed by a word specific to what we are trying\n>>> to achieve (e.g., if we are trying to see if auto-compaction is\n>>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n>>> dunno.\n>>\n>> After reading what you did in the previou step, I am reasonably sure\n>> \"required\" is a wrong word to use, with or without other words like\n>> \"check\".  Semantically it is similar to the should_pack_refs() check\n>> that we use for pack-refs even before \"optimize\" came.  We expect it\n>> to answer this question cheaply: are we better off if we repacked, or\n>> can we go on without repacking for now?  It is not about \"are we\n>> performing so poorly that we MUST optimize now?\"\n>\n> I agree '--required' isn't the best name, and like we discussed\n> '--dry-run' wouldn't be either since that would imply that the work is\n> being done but not persisted.\n>\n> I was leaning towards '--check', which is simple. But It might be\n> nicer to be verbose here and simply add something like '--is-worthwhile'.\n>\n> Being verbose here is okay, since it will only be used sparingly and\n> specifically by those who require such a use case.\n\nNah, \"worthwhile\" is relative and it would be less meaningful without expressing for what goal we are judging how it is worthwhile to do.\n\nChoosing a phrase around \"check\" is better, I would think.\n\n\n\n\n\nThis transmission is intended solely for the addressee and contains confidential information.\nIf you are not the intended recipient, please immediately inform the sender and delete the message and any attachments from your system.\nFurthermore, please do not copy the message or disclose the contents to anyone unless agreed otherwise. To the extent permitted by law we shall in no way be liable for any damages, whatever their nature, arising out of transmission failures, viruses, external influence, delays and the like.\n"},{"id":"528792","messageId":"CAOLa=ZQj20e_p99dzWjmrQ2i-RnLjsnEN9ihuDirV4iPN0QqPg@mail.gmail.com","threadId":"64292","inReplyTo":"xmqqldldxtoc.fsf@gitster.g","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-15T08:19:32Z","receivedAt":"2025-10-15T08:19:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>\n>>>> Perhaps \"--check-\" followed by a word specific to what we are trying\n>>>> to achieve (e.g., if we are trying to see if auto-compaction is\n>>>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n>>>> dunno.\n>>>\n>>> After reading what you did in the previou step, I am reasonably sure\n>>> \"required\" is a wrong word to use, with or without other words like\n>>> \"check\".  Semantically it is similar to the should_pack_refs() check\n>>> that we use for pack-refs even before \"optimize\" came.  We expect it\n>>> to answer this question cheaply: are we better off if we repacked,\n>>> or can we go on without repacking for now?  It is not about \"are we\n>>> performing so poorly that we MUST optimize now?\"\n>>\n>> I agree '--required' isn't the best name, and like we discussed\n>> '--dry-run' wouldn't be either since that would imply that the work is\n>> being done but not persisted.\n>>\n>> I was leaning towards '--check', which is simple. But It might be nicer\n>> to be verbose here and simply add something like '--is-worthwhile'.\n>>\n>> Being verbose here is okay, since it will only be used sparingly and\n>> specifically by those who require such a use case.\n>\n> Nah, \"worthwhile\" is relative and it would be less meaningful\n> without expressing for what goal we are judging how it is worthwhile\n> to do.\n>\n\nI see what you mean.\n\n> Choosing a phrase around \"check\" is better, I would think.\n\nHow about simply `--check`. Since the flag can be used with other\nexisting flags, it would make more sense to do\n\n  git refs optimize --all --auto --check\n  git refs optimize --check\n  git refs optimize --all --check\n\nI will send in a new version around this and we can discuss further on\ntop of that!\n"},{"id":"528794","messageId":"CAOLa=ZRdxm787nE4FSr2VUHDB+hW06Ggc6yUcKmeTKAb6B7YOA@mail.gmail.com","threadId":"64292","inReplyTo":"CAOLa=ZQj20e_p99dzWjmrQ2i-RnLjsnEN9ihuDirV4iPN0QqPg@mail.gmail.com","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-15T09:29:35Z","receivedAt":"2025-10-15T09:29:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Karthik Nayak <karthik.188@gmail.com> writes:\n>>\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>\n>>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>>\n>>>>> Perhaps \"--check-\" followed by a word specific to what we are trying\n>>>>> to achieve (e.g., if we are trying to see if auto-compaction is\n>>>>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n>>>>> dunno.\n>>>>\n>>>> After reading what you did in the previou step, I am reasonably sure\n>>>> \"required\" is a wrong word to use, with or without other words like\n>>>> \"check\".  Semantically it is similar to the should_pack_refs() check\n>>>> that we use for pack-refs even before \"optimize\" came.  We expect it\n>>>> to answer this question cheaply: are we better off if we repacked,\n>>>> or can we go on without repacking for now?  It is not about \"are we\n>>>> performing so poorly that we MUST optimize now?\"\n>>>\n>>> I agree '--required' isn't the best name, and like we discussed\n>>> '--dry-run' wouldn't be either since that would imply that the work is\n>>> being done but not persisted.\n>>>\n>>> I was leaning towards '--check', which is simple. But It might be nicer\n>>> to be verbose here and simply add something like '--is-worthwhile'.\n>>>\n>>> Being verbose here is okay, since it will only be used sparingly and\n>>> specifically by those who require such a use case.\n>>\n>> Nah, \"worthwhile\" is relative and it would be less meaningful\n>> without expressing for what goal we are judging how it is worthwhile\n>> to do.\n>>\n>\n> I see what you mean.\n>\n>> Choosing a phrase around \"check\" is better, I would think.\n>\n> How about simply `--check`. Since the flag can be used with other\n> existing flags, it would make more sense to do\n>\n>   git refs optimize --all --auto --check\n>   git refs optimize --check\n>   git refs optimize --all --check\n>\n> I will send in a new version around this and we can discuss further on\n> top of that!\n\nAfter speaking to Patrick today about this and his work on 'git\nmaintenance', we realized we can actually broaden the scope. We could\nimplement something like 'git maintenance needed'. This would check on a\nwhole if repository maintenance is needed for refs and objects.\n\nI do think there is some merit in some of the patches of this series, so\nI'll spawn out couple of new series and send it to the list.\n\nJunio, from your latest 'What's cooking', I notice that this wasn't\npicked up, which is good. I hope its not too much churn in that sense :)\n\nThanks\n"},{"id":"528804","messageId":"aO-QIn7KAVlBmfRq@pks.im","threadId":"64292","inReplyTo":"CAOLa=ZRdxm787nE4FSr2VUHDB+hW06Ggc6yUcKmeTKAb6B7YOA@mail.gmail.com","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-15T12:14:26Z","receivedAt":"2025-10-15T12:14:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 15, 2025 at 02:29:35AM -0700, Karthik Nayak wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n> \n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >> Karthik Nayak <karthik.188@gmail.com> writes:\n> >>\n> >>> Junio C Hamano <gitster@pobox.com> writes:\n> >>>\n> >>>> Junio C Hamano <gitster@pobox.com> writes:\n> >>>>\n> >>>>> Perhaps \"--check-\" followed by a word specific to what we are trying\n> >>>>> to achieve (e.g., if we are trying to see if auto-compaction is\n> >>>>> necessary, \"--check-for-auto\" \"check for auto compaction\")?  I\n> >>>>> dunno.\n> >>>>\n> >>>> After reading what you did in the previou step, I am reasonably sure\n> >>>> \"required\" is a wrong word to use, with or without other words like\n> >>>> \"check\".  Semantically it is similar to the should_pack_refs() check\n> >>>> that we use for pack-refs even before \"optimize\" came.  We expect it\n> >>>> to answer this question cheaply: are we better off if we repacked,\n> >>>> or can we go on without repacking for now?  It is not about \"are we\n> >>>> performing so poorly that we MUST optimize now?\"\n> >>>\n> >>> I agree '--required' isn't the best name, and like we discussed\n> >>> '--dry-run' wouldn't be either since that would imply that the work is\n> >>> being done but not persisted.\n> >>>\n> >>> I was leaning towards '--check', which is simple. But It might be nicer\n> >>> to be verbose here and simply add something like '--is-worthwhile'.\n> >>>\n> >>> Being verbose here is okay, since it will only be used sparingly and\n> >>> specifically by those who require such a use case.\n> >>\n> >> Nah, \"worthwhile\" is relative and it would be less meaningful\n> >> without expressing for what goal we are judging how it is worthwhile\n> >> to do.\n> >>\n> >\n> > I see what you mean.\n> >\n> >> Choosing a phrase around \"check\" is better, I would think.\n> >\n> > How about simply `--check`. Since the flag can be used with other\n> > existing flags, it would make more sense to do\n> >\n> >   git refs optimize --all --auto --check\n> >   git refs optimize --check\n> >   git refs optimize --all --check\n> >\n> > I will send in a new version around this and we can discuss further on\n> > top of that!\n> \n> After speaking to Patrick today about this and his work on 'git\n> maintenance', we realized we can actually broaden the scope. We could\n> implement something like 'git maintenance needed'. This would check on a\n> whole if repository maintenance is needed for refs and objects.\n\nYup. The idea here is that git-maintenance(1) is already split up into\ndifferent tasks, one of which is the \"pack-refs\" task. And each of these\ntasks already has a callback to verify whether or not we need to execute\nthe task.\n\nSo if we build on top of that mechanism we can easily broaden the scope\nof this patch series to:\n\n  - Check whether maintenance is needed for any of the enabled tasks.\n\n  - Check whether maintenance is needed for a single task.\n\n  - Check whether maintenance is needed given a specific maintenance\n    strategy.\n\nAnother benefit is that it's a bit easier to put a verb into a proper\nsubcommand of git-maintenance(1) instead of having to add a verb-ish\nflag to git-pack-refs(1). `git maintenance is-needed` feels way more\nnatural to me at least compared to `git refs optimize --check`.\n\nThanks!\n\nPatrick\n"},{"id":"528830","messageId":"xmqqqzv3udh4.fsf@gitster.g","threadId":"64292","inReplyTo":"aO-QIn7KAVlBmfRq@pks.im","subject":"Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-15T20:17:11Z","receivedAt":"2025-10-15T20:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> So if we build on top of that mechanism we can easily broaden the scope\n> of this patch series to:\n>\n>   - Check whether maintenance is needed for any of the enabled tasks.\n>\n>   - Check whether maintenance is needed for a single task.\n>\n>   - Check whether maintenance is needed given a specific maintenance\n>     strategy.\n>\n> Another benefit is that it's a bit easier to put a verb into a proper\n> subcommand of git-maintenance(1) instead of having to add a verb-ish\n> flag to git-pack-refs(1). `git maintenance is-needed` feels way more\n> natural to me at least compared to `git refs optimize --check`.\n\nVery nice.\n"}]}