* [PATCH 1/9] refs: move to using the '.optimize' functions
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 2/9] refs: cleanup code around optimization Karthik Nayak
` (7 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The `struct ref_store` variable, exposes two ways to optimize a reftable
backend:
1. pack_refs
2. optimize
The former was specific to the 'files' + 'packed' refs backend. The
latter is more generic and covers all backends. While the naming is
different, both these tend to perform the same functionality.
In the following commit, we will consolidate this code to only maintain
the 'optimize' functions. In preparation, modify the backends to also do
the same, by moving to supporting the 'optimize' function. All users of
the refs subsystem already use the 'optimize' function so there is no
changes needed on the callee side.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/debug.c | 8 ++++----
refs/files-backend.c | 14 ++------------
refs/packed-backend.c | 6 +++---
refs/reftable-backend.c | 13 +++----------
4 files changed, 12 insertions(+), 29 deletions(-)
diff --git a/refs/debug.c b/refs/debug.c
index 162c24e5cc..67a0bcc57b 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -117,11 +117,11 @@ static int debug_transaction_abort(struct ref_store *refs,
return res;
}
-static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)
+static int debug_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
- int res = drefs->refs->be->pack_refs(drefs->refs, opts);
- trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
+ int res = drefs->refs->be->optimize(drefs->refs, opts);
+ trace_printf_key(&trace_refs, "optimize: %d\n", res);
return res;
}
@@ -431,7 +431,7 @@ struct ref_storage_be refs_be_debug = {
.transaction_finish = debug_transaction_finish,
.transaction_abort = debug_transaction_abort,
- .pack_refs = debug_pack_refs,
+ .optimize = debug_optimize,
.rename_ref = debug_rename_ref,
.copy_ref = debug_copy_ref,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4cda57981..0b81bd7f74 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,
return 0;
}
-static int files_pack_refs(struct ref_store *ref_store,
- struct pack_refs_opts *opts)
+static int files_optimize(struct ref_store *ref_store,
+ struct pack_refs_opts *opts)
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
@@ -1513,15 +1513,6 @@ static int files_pack_refs(struct ref_store *ref_store,
return 0;
}
-static int files_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)
-{
- /*
- * For the "files" backend, "optimizing" is the same as "packing".
- * So, we just call the existing worker function for packing.
- */
- return files_pack_refs(ref_store, opts);
-}
-
/*
* People using contrib's git-new-workdir have .git/logs/refs ->
* /some/other/path/.git/logs/refs, and that may live on another device.
@@ -3972,7 +3963,6 @@ struct ref_storage_be refs_be_files = {
.transaction_finish = files_transaction_finish,
.transaction_abort = files_transaction_abort,
- .pack_refs = files_pack_refs,
.optimize = files_optimize,
.rename_ref = files_rename_ref,
.copy_ref = files_copy_ref,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 1ab0c50393..20cf9fab18 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1773,8 +1773,8 @@ static int packed_transaction_finish(struct ref_store *ref_store,
return ret;
}
-static int packed_pack_refs(struct ref_store *ref_store UNUSED,
- struct pack_refs_opts *pack_opts UNUSED)
+static int packed_optimize(struct ref_store *ref_store UNUSED,
+ struct pack_refs_opts *pack_opts UNUSED)
{
/*
* Packed refs are already packed. It might be that loose refs
@@ -2129,7 +2129,7 @@ struct ref_storage_be refs_be_packed = {
.transaction_finish = packed_transaction_finish,
.transaction_abort = packed_transaction_abort,
- .pack_refs = packed_pack_refs,
+ .optimize = packed_optimize,
.rename_ref = NULL,
.copy_ref = NULL,
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 0b7ec3ae15..59018b93d1 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1699,11 +1699,11 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,
return ret;
}
-static int reftable_be_pack_refs(struct ref_store *ref_store,
- struct pack_refs_opts *opts)
+static int reftable_be_optimize(struct ref_store *ref_store,
+ struct pack_refs_opts *opts)
{
struct reftable_ref_store *refs =
- reftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, "pack_refs");
+ reftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, "optimize_refs");
struct reftable_stack *stack;
int ret;
@@ -1732,12 +1732,6 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,
return ret;
}
-static int reftable_be_optimize(struct ref_store *ref_store,
- struct pack_refs_opts *opts)
-{
- return reftable_be_pack_refs(ref_store, opts);
-}
-
struct write_create_symref_arg {
struct reftable_ref_store *refs;
struct reftable_stack *stack;
@@ -2715,7 +2709,6 @@ struct ref_storage_be refs_be_reftable = {
.transaction_finish = reftable_be_transaction_finish,
.transaction_abort = reftable_be_transaction_abort,
- .pack_refs = reftable_be_pack_refs,
.optimize = reftable_be_optimize,
.rename_ref = reftable_be_rename_ref,
.copy_ref = reftable_be_copy_ref,
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 1/9] refs: move to using the '.optimize' functions
2025-10-10 10:27 ` [PATCH 1/9] refs: move to using the '.optimize' functions Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 8:18 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:05PM +0200, Karthik Nayak wrote:
> The `struct ref_store` variable, exposes two ways to optimize a reftable
> backend:
>
> 1. pack_refs
> 2. optimize
>
> The former was specific to the 'files' + 'packed' refs backend. The
> latter is more generic and covers all backends. While the naming is
> different, both these tend to perform the same functionality.
"tend to perform" is a bit of a curious thing to say, as it raises the
question when it doesn't.
> In the following commit, we will consolidate this code to only maintain
> the 'optimize' functions. In preparation, modify the backends to also do
> the same, by moving to supporting the 'optimize' function.
Maybe: "In preparation, modify the backends so that they exclusively
implement the `optimize` callback, only."
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a4cda57981..0b81bd7f74 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,
> return 0;
> }
>
> -static int files_pack_refs(struct ref_store *ref_store,
> - struct pack_refs_opts *opts)
> +static int files_optimize(struct ref_store *ref_store,
> + struct pack_refs_opts *opts)
`struct pack_refs_opts` really should be renamed to
`refs_optimize_opts`. Let's read on, maybe you do this in subsequent
patches.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/9] refs: move to using the '.optimize' functions
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 8:18 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 8:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:05PM +0200, Karthik Nayak wrote:
>> The `struct ref_store` variable, exposes two ways to optimize a reftable
>> backend:
>>
>> 1. pack_refs
>> 2. optimize
>>
>> The former was specific to the 'files' + 'packed' refs backend. The
>> latter is more generic and covers all backends. While the naming is
>> different, both these tend to perform the same functionality.
>
> "tend to perform" is a bit of a curious thing to say, as it raises the
> question when it doesn't.
>
Indeed, will rephrase.
>> In the following commit, we will consolidate this code to only maintain
>> the 'optimize' functions. In preparation, modify the backends to also do
>> the same, by moving to supporting the 'optimize' function.
>
> Maybe: "In preparation, modify the backends so that they exclusively
> implement the `optimize` callback, only."
>
This reads better, thanks!
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index a4cda57981..0b81bd7f74 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1445,8 +1445,8 @@ static int should_pack_refs(struct files_ref_store *refs,
>> return 0;
>> }
>>
>> -static int files_pack_refs(struct ref_store *ref_store,
>> - struct pack_refs_opts *opts)
>> +static int files_optimize(struct ref_store *ref_store,
>> + struct pack_refs_opts *opts)
>
> `struct pack_refs_opts` really should be renamed to
> `refs_optimize_opts`. Let's read on, maybe you do this in subsequent
> patches.
>
Yup, that's handled in a follow up patch!
> Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/9] refs: cleanup code around optimization
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
2025-10-10 10:27 ` [PATCH 1/9] refs: move to using the '.optimize' functions Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts' Karthik Nayak
` (6 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The previous commit, moved all backends to only use/support the
'optimize' function within the `ref_store` structure. With this, cleanup
all references to the 'pack_refs' field of the structure and code around
it.
Modify existing documentation in this regard.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 6 ------
refs.h | 10 +++-------
refs/refs-internal.h | 3 ---
3 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 40acaa3f42..77dc1ab501 100644
--- a/refs.c
+++ b/refs.c
@@ -2312,12 +2312,6 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
refs->gitdir = xstrdup(path);
}
-/* backend functions */
-int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)
-{
- return refs->be->pack_refs(refs, opts);
-}
-
int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)
{
return refs->be->optimize(refs, opts);
diff --git a/refs.h b/refs.h
index 2dd7ac1a16..c6c955d78d 100644
--- a/refs.h
+++ b/refs.h
@@ -499,7 +499,7 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,
const struct string_list *refnames);
/*
- * Flags for controlling behaviour of pack_refs()
+ * Flags for controlling behaviour of refs_optimize()
* PACK_REFS_PRUNE: Prune loose refs after packing
* PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
* result are decided by the ref backend. Backends may ignore
@@ -514,15 +514,11 @@ struct pack_refs_opts {
struct string_list *includes;
};
-/*
- * Write a packed-refs file for the current repository.
- * flags: Combination of the above PACK_REFS_* flags.
- */
-int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);
-
/*
* Optimize the ref store. The exact behavior is up to the backend.
* For the files backend, this is equivalent to packing refs.
+ *
+ * flags: Combination of the above PACK_REFS_* flags.
*/
int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 4671517dad..fc5149df5b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -422,8 +422,6 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,
struct ref_transaction *transaction,
struct strbuf *err);
-typedef int pack_refs_fn(struct ref_store *ref_store,
- struct pack_refs_opts *opts);
typedef int optimize_fn(struct ref_store *ref_store,
struct pack_refs_opts *opts);
typedef int rename_ref_fn(struct ref_store *ref_store,
@@ -550,7 +548,6 @@ struct ref_storage_be {
ref_transaction_finish_fn *transaction_finish;
ref_transaction_abort_fn *transaction_abort;
- pack_refs_fn *pack_refs;
optimize_fn *optimize;
rename_ref_fn *rename_ref;
copy_ref_fn *copy_ref;
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] refs: cleanup code around optimization
2025-10-10 10:27 ` [PATCH 2/9] refs: cleanup code around optimization Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 8:22 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:06PM +0200, Karthik Nayak wrote:
> The previous commit, moved all backends to only use/support the
> 'optimize' function within the `ref_store` structure. With this, cleanup
> all references to the 'pack_refs' field of the structure and code around
> it.
>
> Modify existing documentation in this regard.
Makes sense.
> diff --git a/refs.h b/refs.h
> index 2dd7ac1a16..c6c955d78d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -514,15 +514,11 @@ struct pack_refs_opts {
> struct string_list *includes;
> };
>
> -/*
> - * Write a packed-refs file for the current repository.
> - * flags: Combination of the above PACK_REFS_* flags.
> - */
> -int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);
> -
> /*
> * Optimize the ref store. The exact behavior is up to the backend.
> * For the files backend, this is equivalent to packing refs.
> + *
> + * flags: Combination of the above PACK_REFS_* flags.
> */
> int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);
>
I don't think it makes sense to carry over this documentation here. If
not already the case, we should document the `struct opts::flags` field
in the structure itself.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] refs: cleanup code around optimization
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 8:22 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 8:22 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:06PM +0200, Karthik Nayak wrote:
>> The previous commit, moved all backends to only use/support the
>> 'optimize' function within the `ref_store` structure. With this, cleanup
>> all references to the 'pack_refs' field of the structure and code around
>> it.
>>
>> Modify existing documentation in this regard.
>
> Makes sense.
>
>> diff --git a/refs.h b/refs.h
>> index 2dd7ac1a16..c6c955d78d 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -514,15 +514,11 @@ struct pack_refs_opts {
>> struct string_list *includes;
>> };
>>
>> -/*
>> - * Write a packed-refs file for the current repository.
>> - * flags: Combination of the above PACK_REFS_* flags.
>> - */
>> -int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);
>> -
>> /*
>> * Optimize the ref store. The exact behavior is up to the backend.
>> * For the files backend, this is equivalent to packing refs.
>> + *
>> + * flags: Combination of the above PACK_REFS_* flags.
>> */
>> int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);
>>
>
> I don't think it makes sense to carry over this documentation here. If
> not already the case, we should document the `struct opts::flags` field
> in the structure itself.
>
> Patrick
Fair enough, let me remove this extra line added.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
2025-10-10 10:27 ` [PATCH 1/9] refs: move to using the '.optimize' functions Karthik Nayak
2025-10-10 10:27 ` [PATCH 2/9] refs: cleanup code around optimization Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees Karthik Nayak
` (5 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The previous commit removed all references to 'pack_refs()' within
the refs subsystem. Continue this cleanup by also renaming
'pack_refs_opts' to 'optimize_refs_opts' and the respective flags
accordingly. Keeping the naming consistent will make the code easier to
maintain.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
pack-refs.c | 8 ++++----
refs.c | 2 +-
refs.h | 18 +++++++++---------
refs/debug.c | 2 +-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 2 +-
refs/reftable-backend.c | 4 ++--
8 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/pack-refs.c b/pack-refs.c
index 1a5e07d8b8..fee77fbf9f 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -14,10 +14,10 @@ int pack_refs_core(int argc,
{
struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
struct string_list included_refs = STRING_LIST_INIT_NODUP;
- struct pack_refs_opts pack_refs_opts = {
+ struct optimize_refs_opts pack_refs_opts = {
.exclusions = &excludes,
.includes = &included_refs,
- .flags = PACK_REFS_PRUNE,
+ .flags = OPTIMIZE_REFS_PRUNE,
};
struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
struct string_list_item *item;
@@ -26,8 +26,8 @@ int pack_refs_core(int argc,
struct option opts[] = {
OPT_BOOL(0, "all", &pack_all, N_("pack everything")),
- OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
- OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), PACK_REFS_AUTO),
+ OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), OPTIMIZE_REFS_PRUNE),
+ OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), OPTIMIZE_REFS_AUTO),
OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
N_("references to include")),
OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
diff --git a/refs.c b/refs.c
index 77dc1ab501..514fb85af2 100644
--- a/refs.c
+++ b/refs.c
@@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
refs->gitdir = xstrdup(path);
}
-int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)
+int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
{
return refs->be->optimize(refs, opts);
}
diff --git a/refs.h b/refs.h
index c6c955d78d..58b222ac02 100644
--- a/refs.h
+++ b/refs.h
@@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,
/*
* Flags for controlling behaviour of refs_optimize()
- * PACK_REFS_PRUNE: Prune loose refs after packing
- * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
- * result are decided by the ref backend. Backends may ignore
- * this flag and fall back to a normal repack.
+ * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing
+ * OPTIMIZE_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
+ * result are decided by the ref backend. Backends may ignore
+ * this flag and fall back to a normal repack.
*/
-#define PACK_REFS_PRUNE (1 << 0)
-#define PACK_REFS_AUTO (1 << 1)
+#define OPTIMIZE_REFS_PRUNE (1 << 0)
+#define OPTIMIZE_REFS_AUTO (1 << 1)
-struct pack_refs_opts {
+struct optimize_refs_opts {
unsigned int flags;
struct ref_exclusions *exclusions;
struct string_list *includes;
@@ -518,9 +518,9 @@ struct pack_refs_opts {
* Optimize the ref store. The exact behavior is up to the backend.
* For the files backend, this is equivalent to packing refs.
*
- * flags: Combination of the above PACK_REFS_* flags.
+ * flags: Combination of the above OPTIMIZE_REFS_* flags.
*/
-int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts);
+int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts);
/*
* Setup reflog before using. Fill in err and return -1 on failure.
diff --git a/refs/debug.c b/refs/debug.c
index 67a0bcc57b..260d7457db 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -117,7 +117,7 @@ static int debug_transaction_abort(struct ref_store *refs,
return res;
}
-static int debug_optimize(struct ref_store *ref_store, struct pack_refs_opts *opts)
+static int debug_optimize(struct ref_store *ref_store, struct optimize_refs_opts *opts)
{
struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
int res = drefs->refs->be->optimize(drefs->refs, opts);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0b81bd7f74..1c37899006 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1356,7 +1356,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_
*/
static int should_pack_ref(struct files_ref_store *refs,
const struct reference *ref,
- struct pack_refs_opts *opts)
+ struct optimize_refs_opts *opts)
{
struct string_list_item *item;
@@ -1384,7 +1384,7 @@ static int should_pack_ref(struct files_ref_store *refs,
}
static int should_pack_refs(struct files_ref_store *refs,
- struct pack_refs_opts *opts)
+ struct optimize_refs_opts *opts)
{
struct ref_iterator *iter;
size_t packed_size;
@@ -1392,7 +1392,7 @@ static int should_pack_refs(struct files_ref_store *refs,
size_t limit;
int ret;
- if (!(opts->flags & PACK_REFS_AUTO))
+ if (!(opts->flags & OPTIMIZE_REFS_AUTO))
return 1;
ret = packed_refs_size(refs->packed_ref_store, &packed_size);
@@ -1446,7 +1446,7 @@ static int should_pack_refs(struct files_ref_store *refs,
}
static int files_optimize(struct ref_store *ref_store,
- struct pack_refs_opts *opts)
+ struct optimize_refs_opts *opts)
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
@@ -1489,7 +1489,7 @@ static int files_optimize(struct ref_store *ref_store,
iter->ref.name, err.buf);
/* Schedule the loose reference for pruning if requested. */
- if ((opts->flags & PACK_REFS_PRUNE)) {
+ if ((opts->flags & OPTIMIZE_REFS_PRUNE)) {
struct ref_to_prune *n;
FLEX_ALLOC_STR(n, name, iter->ref.name);
oidcpy(&n->oid, iter->ref.oid);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 20cf9fab18..acaa5a6e57 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1774,7 +1774,7 @@ static int packed_transaction_finish(struct ref_store *ref_store,
}
static int packed_optimize(struct ref_store *ref_store UNUSED,
- struct pack_refs_opts *pack_opts UNUSED)
+ struct optimize_refs_opts *pack_optsa UNUSED)
{
/*
* Packed refs are already packed. It might be that loose refs
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index fc5149df5b..4ef2422729 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -423,7 +423,7 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,
struct strbuf *err);
typedef int optimize_fn(struct ref_store *ref_store,
- struct pack_refs_opts *opts);
+ struct optimize_refs_opts *opts);
typedef int rename_ref_fn(struct ref_store *ref_store,
const char *oldref, const char *newref,
const char *logmsg);
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 59018b93d1..d77714366a 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1700,7 +1700,7 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED,
}
static int reftable_be_optimize(struct ref_store *ref_store,
- struct pack_refs_opts *opts)
+ struct optimize_refs_opts *opts)
{
struct reftable_ref_store *refs =
reftable_be_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB, "optimize_refs");
@@ -1714,7 +1714,7 @@ static int reftable_be_optimize(struct ref_store *ref_store,
if (!stack)
stack = refs->main_backend.stack;
- if (opts->flags & PACK_REFS_AUTO)
+ if (opts->flags & OPTIMIZE_REFS_AUTO)
ret = reftable_stack_auto_compact(stack);
else
ret = reftable_stack_compact_all(stack, NULL);
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'
2025-10-10 10:27 ` [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts' Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 8:52 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:07PM +0200, Karthik Nayak wrote:
> The previous commit removed all references to 'pack_refs()' within
> the refs subsystem. Continue this cleanup by also renaming
> 'pack_refs_opts' to 'optimize_refs_opts' and the respective flags
> accordingly. Keeping the naming consistent will make the code easier to
> maintain.
Good, this is what I was hoping to see :)
> diff --git a/refs.c b/refs.c
> index 77dc1ab501..514fb85af2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
> refs->gitdir = xstrdup(path);
> }
>
> -int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)
> +int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
An options struct for a function `do_something()` should typically be
called `struct do_something_opts`. So we should rename the struct to
`refs_optimize_opts`.
> diff --git a/refs.h b/refs.h
> index c6c955d78d..58b222ac02 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,
>
> /*
> * Flags for controlling behaviour of refs_optimize()
> - * PACK_REFS_PRUNE: Prune loose refs after packing
> - * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
> - * result are decided by the ref backend. Backends may ignore
> - * this flag and fall back to a normal repack.
> + * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing
> + * OPTIMIZE_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
> + * result are decided by the ref backend. Backends may ignore
> + * this flag and fall back to a normal repack.
> */
> -#define PACK_REFS_PRUNE (1 << 0)
> -#define PACK_REFS_AUTO (1 << 1)
> +#define OPTIMIZE_REFS_PRUNE (1 << 0)
> +#define OPTIMIZE_REFS_AUTO (1 << 1)
And these would then be called `REFS_OPTIMIZE_PRUNE` etc.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts'
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 8:52 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 8:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:07PM +0200, Karthik Nayak wrote:
>> The previous commit removed all references to 'pack_refs()' within
>> the refs subsystem. Continue this cleanup by also renaming
>> 'pack_refs_opts' to 'optimize_refs_opts' and the respective flags
>> accordingly. Keeping the naming consistent will make the code easier to
>> maintain.
>
> Good, this is what I was hoping to see :)
>
>> diff --git a/refs.c b/refs.c
>> index 77dc1ab501..514fb85af2 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2312,7 +2312,7 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
>> refs->gitdir = xstrdup(path);
>> }
>>
>> -int refs_optimize(struct ref_store *refs, struct pack_refs_opts *opts)
>> +int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
>
> An options struct for a function `do_something()` should typically be
> called `struct do_something_opts`. So we should rename the struct to
> `refs_optimize_opts`.
>
Will do that.
>> diff --git a/refs.h b/refs.h
>> index c6c955d78d..58b222ac02 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -500,15 +500,15 @@ void refs_warn_dangling_symrefs(struct ref_store *refs, FILE *fp,
>>
>> /*
>> * Flags for controlling behaviour of refs_optimize()
>> - * PACK_REFS_PRUNE: Prune loose refs after packing
>> - * PACK_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
>> - * result are decided by the ref backend. Backends may ignore
>> - * this flag and fall back to a normal repack.
>> + * OPTIMIZE_REFS_PRUNE: Prune loose refs after packing
>> + * OPTIMIZE_REFS_AUTO: Pack refs on a best effort basis. The heuristics and end
>> + * result are decided by the ref backend. Backends may ignore
>> + * this flag and fall back to a normal repack.
>> */
>> -#define PACK_REFS_PRUNE (1 << 0)
>> -#define PACK_REFS_AUTO (1 << 1)
>> +#define OPTIMIZE_REFS_PRUNE (1 << 0)
>> +#define OPTIMIZE_REFS_AUTO (1 << 1)
>
> And these would then be called `REFS_OPTIMIZE_PRUNE` etc.
>
> Patrick
Likewise, thanks for pointing it out.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (2 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 3/9] refs: rename 'pack_refs_opts' to 'optimize_refs_opts' Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 5/9] t/t0450: split whitespace consistency check per subcommand Karthik Nayak
` (4 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In ac0bad0af4 (t0601: refactor tests to be shareable, 2025-09-19), we
refactored 't/t0601-reffiles-pack-refs.sh' to move all of the tests to
't/pack-refs-tests.sh', which became a common test suite which was also
used by 't/t1463-refs-optimize.sh'.
This also moved the 'test_done' directive to 't/pack-refs-tests.sh'.
Which inhibits additional tests from being added to either of the tests.
Let's move the directive out to both the tests, so that we can add
additional specific tests to them. Also the test flow logic shouldn't be
part of tests which can be embedded in other test scripts.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
t/pack-refs-tests.sh | 1 -
t/t0601-reffiles-pack-refs.sh | 2 ++
t/t1463-refs-optimize.sh | 2 ++
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 095823d915..6a71838ffa 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -460,4 +460,3 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '
)
'
-test_done
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 12cf5d1dcb..3c706978ef 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -18,3 +18,5 @@ export GIT_TEST_DEFAULT_REF_FORMAT
. ./test-lib.sh
. "$TEST_DIRECTORY"/pack-refs-tests.sh
+
+test_done
diff --git a/t/t1463-refs-optimize.sh b/t/t1463-refs-optimize.sh
index c11c905d79..9afe3c1ed7 100755
--- a/t/t1463-refs-optimize.sh
+++ b/t/t1463-refs-optimize.sh
@@ -15,3 +15,5 @@ export GIT_TEST_DEFAULT_REF_FORMAT
pack_refs='refs optimize'
. "$TEST_DIRECTORY"/pack-refs-tests.sh
+
+test_done
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 5/9] t/t0450: split whitespace consistency check per subcommand
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (3 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 4/9] t/pack-refs-tests: move the 'test_done' to callees Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 10:27 ` [PATCH 6/9] reftable/stack: return stack segments directly Karthik Nayak
` (3 subsequent siblings)
8 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The test t0450 contains a test to ensure that leading whitespaces within
a commands help text is consistent. This would catch issues such as:
usage: git show-ref [--head] [-d | --dereference]
[-s | --hash[=<n>]] [--abbrev[=<n>]] [--branches] [--tags]
[--] [<pattern>...]
where the second line has inconsistent leading whitespaces. However this
considers that all lines within a command will be aligned similarly.
This works for most commands, however when dealing commands which
include subcommands, this assumption doesn't hold. Consider the help
text for 'git-refs(1)':
usage: git refs migrate --ref-format=<format> [--no-reflog] [--dry-run]
or: git refs verify [--strict] [--verbose]
or: git refs list [--count=<count>] [--shell|--perl|--python|--tcl]
[(--sort=<key>)...] [--format=<format>]
[--include-root-refs] [--points-at=<object>]
[--merged[=<object>]] [--no-merged[=<object>]]
[--contains[=<object>]] [--no-contains[=<object>]]
[(--exclude=<pattern>)...] [--start-after=<marker>]
[ --stdin | (<pattern>...)]
or: git refs exists <ref>
or: git refs optimize [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
With the current implementation of this test, any flags added to 'git
refs optimize' in a newline would require it to be aligned with the
flags of 'git refs list'. Which is incorrect, since we'd want it to be
aligned with the flags already added to 'git refs optimize'.
Let's modify the test to work with subcommands. Do this by swapping out
the old logic. The old logic simply counts the number of spaces for all
lines with leading spaces and checks to make sure they're equal.
Instead, now we create a list of (subcommand number, leading space) and
then ensure that there are only unique values per subcommand.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
t/t0450-txt-doc-vs-help.sh | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index e12e18f97f..150655f9f0 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -94,19 +94,23 @@ do
check_dashed_labels "$(help_to_synopsis "$builtin")"
'
- test_expect_success "$builtin -h output has consistent spacing" '
+ test_expect_success "$builtin -h output has consistent spacing for each subcommand" '
h2s="$(help_to_synopsis "$builtin")" &&
- sed -n \
- -e "/^ / {
- s/[^ ].*//;
- p;
- }" \
- <"$h2s" >help &&
- sort -u help >help.ws &&
- if test -s help.ws
- then
- test_line_count = 1 help.ws
- fi
+
+ # For each subcommand, capture the number of whitespaces
+ # specific to that subcommand.
+ awk "
+ /^[^ ]/ { subcommand++ } # Count the number of subcommands
+ /^ / {
+ match(\$0, /^ */);
+ print subcommand, RLENGTH;
+ }
+ " <"$h2s" \
+ | sort -u \
+ | cut -d" " -f1 \
+ | uniq -d > help.inconsistent &&
+
+ test_must_be_empty help.inconsistent
'
adoc="$(builtin_to_adoc "$builtin")" &&
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 6/9] reftable/stack: return stack segments directly
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (4 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 5/9] t/t0450: split whitespace consistency check per subcommand Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 7/9] reftable/stack: add function to check if optimization is required Karthik Nayak
` (2 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The `stack_table_sizes_for_compaction()` function returns individual
sizes of each reftable table. This function is only called by
`reftable_stack_auto_compact()` to process the table sizes.
Modify the function to directly return the segments, this avoids the
extra step of receiving the sizes only to pass it to
`suggest_compaction_segment()`.
A future commit will also add functionality for checking if
auto-compaction is necessary without performing it. This change allows
code re-usability in that context.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
reftable/stack.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index f91ce50bcd..9d9326ce0e 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1631,7 +1631,8 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
return seg;
}
-static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
+static int stack_segments_for_compaction(struct reftable_stack *st,
+ struct segment *seg)
{
int version = (st->opts.hash_id == REFTABLE_HASH_SHA1) ? 1 : 2;
int overhead = header_size(version) - 1;
@@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
if (!sizes)
- return NULL;
+ return REFTABLE_OUT_OF_MEMORY_ERROR;
for (size_t i = 0; i < st->merged->tables_len; i++)
sizes[i] = st->tables[i]->size - overhead;
- return sizes;
+ *seg = suggest_compaction_segment(sizes, st->merged->tables_len,
+ st->opts.auto_compaction_factor);
+ reftable_free(sizes);
+
+ return 0;
}
int reftable_stack_auto_compact(struct reftable_stack *st)
{
struct segment seg;
- uint64_t *sizes;
+ int err = 0;
if (st->merged->tables_len < 2)
return 0;
- sizes = stack_table_sizes_for_compaction(st);
- if (!sizes)
- return REFTABLE_OUT_OF_MEMORY_ERROR;
-
- seg = suggest_compaction_segment(sizes, st->merged->tables_len,
- st->opts.auto_compaction_factor);
- reftable_free(sizes);
+ err = stack_segments_for_compaction(st, &seg);
+ if (err)
+ return err;
if (segment_size(&seg) > 0)
return stack_compact_range(st, seg.start, seg.end - 1,
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] reftable/stack: return stack segments directly
2025-10-10 10:27 ` [PATCH 6/9] reftable/stack: return stack segments directly Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 9:01 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote:
> The `stack_table_sizes_for_compaction()` function returns individual
> sizes of each reftable table. This function is only called by
> `reftable_stack_auto_compact()` to process the table sizes.
Maybe: "to decide which tables need to be compacted, if any."
> Modify the function to directly return the segments, this avoids the
s/this/which/
> extra step of receiving the sizes only to pass it to
> `suggest_compaction_segment()`.
>
> A future commit will also add functionality for checking if
s/if/whether/
> auto-compaction is necessary without performing it. This change allows
> code re-usability in that context.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> reftable/stack.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index f91ce50bcd..9d9326ce0e 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
>
> REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
> if (!sizes)
> - return NULL;
> + return REFTABLE_OUT_OF_MEMORY_ERROR;
>
> for (size_t i = 0; i < st->merged->tables_len; i++)
> sizes[i] = st->tables[i]->size - overhead;
>
> - return sizes;
> + *seg = suggest_compaction_segment(sizes, st->merged->tables_len,
> + st->opts.auto_compaction_factor);
> + reftable_free(sizes);
> +
> + return 0;
> }
>
> int reftable_stack_auto_compact(struct reftable_stack *st)
> {
> struct segment seg;
> - uint64_t *sizes;
> + int err = 0;
The initialization is unnecessary.
Other than that this patch looks good to me.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] reftable/stack: return stack segments directly
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 9:01 ` Karthik Nayak
2025-10-13 11:10 ` Patrick Steinhardt
0 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 9:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote:
>> The `stack_table_sizes_for_compaction()` function returns individual
>> sizes of each reftable table. This function is only called by
>> `reftable_stack_auto_compact()` to process the table sizes.
>
> Maybe: "to decide which tables need to be compacted, if any."
>
Reads better, will update.
>> Modify the function to directly return the segments, this avoids the
>
> s/this/which/
>
will do.
>> extra step of receiving the sizes only to pass it to
>> `suggest_compaction_segment()`.
>>
>> A future commit will also add functionality for checking if
>
> s/if/whether/
>
This should be okay either ways I believe, but shouldn't matter much,
will change.
>
>> auto-compaction is necessary without performing it. This change allows
>> code re-usability in that context.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> reftable/stack.c | 23 ++++++++++++-----------
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/reftable/stack.c b/reftable/stack.c
>> index f91ce50bcd..9d9326ce0e 100644
>> --- a/reftable/stack.c
>> +++ b/reftable/stack.c
>> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
>>
>> REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
>> if (!sizes)
>> - return NULL;
>> + return REFTABLE_OUT_OF_MEMORY_ERROR;
>>
>> for (size_t i = 0; i < st->merged->tables_len; i++)
>> sizes[i] = st->tables[i]->size - overhead;
>>
>> - return sizes;
>> + *seg = suggest_compaction_segment(sizes, st->merged->tables_len,
>> + st->opts.auto_compaction_factor);
>> + reftable_free(sizes);
>> +
>> + return 0;
>> }
>>
>> int reftable_stack_auto_compact(struct reftable_stack *st)
>> {
>> struct segment seg;
>> - uint64_t *sizes;
>> + int err = 0;
>
> The initialization is unnecessary.
>
True, but I always think its nicer to define the base state like this.
This makes it less error prone when/if the code is modified in the
future. But doesn't really matter, so let me remove it.
> Other than that this patch looks good to me.
>
> Patrick
Thanks for the review.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] reftable/stack: return stack segments directly
2025-10-13 9:01 ` Karthik Nayak
@ 2025-10-13 11:10 ` Patrick Steinhardt
0 siblings, 0 replies; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-13 11:10 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Mon, Oct 13, 2025 at 02:01:15AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> diff --git a/reftable/stack.c b/reftable/stack.c
> >> index f91ce50bcd..9d9326ce0e 100644
> >> --- a/reftable/stack.c
> >> +++ b/reftable/stack.c
> >> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
> >>
> >> REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
> >> if (!sizes)
> >> - return NULL;
> >> + return REFTABLE_OUT_OF_MEMORY_ERROR;
> >>
> >> for (size_t i = 0; i < st->merged->tables_len; i++)
> >> sizes[i] = st->tables[i]->size - overhead;
> >>
> >> - return sizes;
> >> + *seg = suggest_compaction_segment(sizes, st->merged->tables_len,
> >> + st->opts.auto_compaction_factor);
> >> + reftable_free(sizes);
> >> +
> >> + return 0;
> >> }
> >>
> >> int reftable_stack_auto_compact(struct reftable_stack *st)
> >> {
> >> struct segment seg;
> >> - uint64_t *sizes;
> >> + int err = 0;
> >
> > The initialization is unnecessary.
> >
>
> True, but I always think its nicer to define the base state like this.
> This makes it less error prone when/if the code is modified in the
> future. But doesn't really matter, so let me remove it.
The counterargument to this is that it doesn't give us any nice compiler
warnings if a specific return forgets to set the variable, so we may
return successfully by accident. If you leave this uninitialized then
the compiler warns you for every early return where you don't initialize
the variable. For all I can tell this warning is also quite reliable.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 7/9] reftable/stack: add function to check if optimization is required
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (5 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 6/9] reftable/stack: return stack segments directly Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be` Karthik Nayak
2025-10-10 10:27 ` [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The reftable backend for references, performs auto-compaction as part of
its regular flow. This allows it to stay optimized.
Compaction can also be triggered voluntarily by the user via the 'git
pack-refs' or the 'git refs optimize' command. However, currently there
is no way for the user to check if optimization is required without
actually performing it. Add and expose
`reftable_stack_compaction_required()` which will allow users to check
if the reftable backend can be optimized.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
reftable/reftable-stack.h | 3 +++
reftable/stack.c | 18 ++++++++++++++++++
t/unit-tests/u-reftable-stack.c | 12 ++++++++++--
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index d70fcb705d..754e955206 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -123,6 +123,9 @@ struct reftable_log_expiry_config {
int reftable_stack_compact_all(struct reftable_stack *st,
struct reftable_log_expiry_config *config);
+/* check if heuristic based compaction is required */
+int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);
+
/* heuristically compact unbalanced table stack. */
int reftable_stack_auto_compact(struct reftable_stack *st);
diff --git a/reftable/stack.c b/reftable/stack.c
index 9d9326ce0e..732141d46c 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1652,6 +1652,24 @@ static int stack_segments_for_compaction(struct reftable_stack *st,
return 0;
}
+int reftable_stack_compaction_required(struct reftable_stack *st, bool *required)
+{
+ struct segment seg;
+ int err = 0;
+
+ if (st->merged->tables_len < 2) {
+ *required = false;
+ return 0;
+ }
+
+ err = stack_segments_for_compaction(st, &seg);
+ if (err)
+ return err;
+
+ *required = segment_size(&seg) > 0;
+ return 0;
+}
+
int reftable_stack_auto_compact(struct reftable_stack *st)
{
struct segment seg;
diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c
index a8b91812e8..80a87a15df 100644
--- a/t/unit-tests/u-reftable-stack.c
+++ b/t/unit-tests/u-reftable-stack.c
@@ -1067,6 +1067,7 @@ void test_reftable_stack__add_performs_auto_compaction(void)
.value_type = REFTABLE_REF_SYMREF,
.value.symref = (char *) "master",
};
+ bool required = false;
char buf[128];
/*
@@ -1087,10 +1088,17 @@ void test_reftable_stack__add_performs_auto_compaction(void)
* auto compaction is disabled. When enabled, we should merge
* all tables in the stack.
*/
- if (i != n)
+ cl_assert_equal_i(reftable_stack_compaction_required(st, &required), 0);
+ if (i != n) {
cl_assert_equal_i(st->merged->tables_len, i + 1);
- else
+ if (i < 1)
+ cl_assert_equal_b(required, false);
+ else
+ cl_assert_equal_b(required, true);
+ } else {
cl_assert_equal_i(st->merged->tables_len, 1);
+ cl_assert_equal_b(required, false);
+ }
}
reftable_stack_destroy(st);
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 7/9] reftable/stack: add function to check if optimization is required
2025-10-10 10:27 ` [PATCH 7/9] reftable/stack: add function to check if optimization is required Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 9:04 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:11PM +0200, Karthik Nayak wrote:
> The reftable backend for references, performs auto-compaction as part of
s/for references, //
> its regular flow. This allows it to stay optimized.
Maybe: "flow, which is required to keep the number of tables part of a
stack at bay."
> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
> index d70fcb705d..754e955206 100644
> --- a/reftable/reftable-stack.h
> +++ b/reftable/reftable-stack.h
> @@ -123,6 +123,9 @@ struct reftable_log_expiry_config {
> int reftable_stack_compact_all(struct reftable_stack *st,
> struct reftable_log_expiry_config *config);
>
> +/* check if heuristic based compaction is required */
> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);
Let's use a full sentence here, starting with an upper-case letter and
ending with punctuation.
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 9d9326ce0e..732141d46c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1652,6 +1652,24 @@ static int stack_segments_for_compaction(struct reftable_stack *st,
> return 0;
> }
>
> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required)
> +{
> + struct segment seg;
> + int err = 0;
> +
> + if (st->merged->tables_len < 2) {
> + *required = false;
> + return 0;
> + }
> +
> + err = stack_segments_for_compaction(st, &seg);
> + if (err)
> + return err;
> +
> + *required = segment_size(&seg) > 0;
> + return 0;
> +}
Okay, makes sense. If we have at least one segment that we'd perform
compaction for we return true, otherwise false.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 7/9] reftable/stack: add function to check if optimization is required
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 9:04 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 9:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:11PM +0200, Karthik Nayak wrote:
>> The reftable backend for references, performs auto-compaction as part of
>
> s/for references, //
>
>> its regular flow. This allows it to stay optimized.
>
> Maybe: "flow, which is required to keep the number of tables part of a
> stack at bay."
>
Will change.
>> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
>> index d70fcb705d..754e955206 100644
>> --- a/reftable/reftable-stack.h
>> +++ b/reftable/reftable-stack.h
>> @@ -123,6 +123,9 @@ struct reftable_log_expiry_config {
>> int reftable_stack_compact_all(struct reftable_stack *st,
>> struct reftable_log_expiry_config *config);
>>
>> +/* check if heuristic based compaction is required */
>> +int reftable_stack_compaction_required(struct reftable_stack *st, bool *required);
>
> Let's use a full sentence here, starting with an upper-case letter and
> ending with punctuation.
>
Will do.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (6 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 7/9] reftable/stack: add function to check if optimization is required Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-10 10:27 ` [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
To allow users of the refs namespace to check if the reference backend
requires optimization, add a new field `optimize_required` field to
`struct ref_storage_be`. This field is of type `optimize_required_fn`
which is also introduced in this commit.
Modify the debug, files, packed and reftable backend to implement this
field. A following commit will expose this via 'git pack-refs' and 'git
refs optimize'.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/debug.c | 13 +++++++++++++
refs/files-backend.c | 11 +++++++++++
refs/packed-backend.c | 13 +++++++++++++
refs/refs-internal.h | 6 ++++++
refs/reftable-backend.c | 25 +++++++++++++++++++++++++
5 files changed, 68 insertions(+)
diff --git a/refs/debug.c b/refs/debug.c
index 260d7457db..71031c8326 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -125,6 +125,17 @@ static int debug_optimize(struct ref_store *ref_store, struct optimize_refs_opts
return res;
}
+static int debug_optimize_required(struct ref_store *ref_store,
+ struct optimize_refs_opts *opts,
+ bool *required)
+{
+ struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+ int res = drefs->refs->be->optimize_required(drefs->refs, opts, required);
+ trace_printf_key(&trace_refs, "optimize_required: %s, res: %d\n",
+ required ? "yes" : "no", res);
+ return res;
+}
+
static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
const char *newref, const char *logmsg)
{
@@ -432,6 +443,8 @@ struct ref_storage_be refs_be_debug = {
.transaction_abort = debug_transaction_abort,
.optimize = debug_optimize,
+ .optimize_required = debug_optimize_required,
+
.rename_ref = debug_rename_ref,
.copy_ref = debug_copy_ref,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1c37899006..c262ae1a7b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,
return 0;
}
+static int files_optimize_required(struct ref_store *ref_store,
+ struct optimize_refs_opts *opts,
+ bool *required)
+{
+ struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
+ "optimize_required");
+ *required = should_pack_refs(refs, opts);
+ return 0;
+}
+
/*
* People using contrib's git-new-workdir have .git/logs/refs ->
* /some/other/path/.git/logs/refs, and that may live on another device.
@@ -3964,6 +3974,7 @@ struct ref_storage_be refs_be_files = {
.transaction_abort = files_transaction_abort,
.optimize = files_optimize,
+ .optimize_required = files_optimize_required,
.rename_ref = files_rename_ref,
.copy_ref = files_copy_ref,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index acaa5a6e57..c94948f618 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,
return 0;
}
+static int packed_optimize_required(struct ref_store *ref_store UNUSED,
+ struct optimize_refs_opts *opts UNUSED,
+ bool *required)
+{
+ /*
+ * Packed refs are already optimized.
+ */
+ *required = false;
+ return 0;
+}
+
static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store UNUSED)
{
return empty_ref_iterator_begin();
@@ -2130,6 +2141,8 @@ struct ref_storage_be refs_be_packed = {
.transaction_abort = packed_transaction_abort,
.optimize = packed_optimize,
+ .optimize_required = packed_optimize_required,
+
.rename_ref = NULL,
.copy_ref = NULL,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 4ef2422729..5c49567560 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -424,6 +424,11 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,
typedef int optimize_fn(struct ref_store *ref_store,
struct optimize_refs_opts *opts);
+
+typedef int optimize_required_fn(struct ref_store *ref_store,
+ struct optimize_refs_opts *opts,
+ bool *required);
+
typedef int rename_ref_fn(struct ref_store *ref_store,
const char *oldref, const char *newref,
const char *logmsg);
@@ -549,6 +554,7 @@ struct ref_storage_be {
ref_transaction_abort_fn *transaction_abort;
optimize_fn *optimize;
+ optimize_required_fn *optimize_required;
rename_ref_fn *rename_ref;
copy_ref_fn *copy_ref;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index d77714366a..df39fe9b38 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,
return ret;
}
+static int reftable_be_optimize_required(struct ref_store *ref_store,
+ struct optimize_refs_opts *opts,
+ bool *required)
+{
+ struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,
+ "optimize_refs_required");
+ struct reftable_stack *stack;
+
+ if (refs->err)
+ return refs->err;
+
+ stack = refs->worktree_backend.stack;
+ if (!stack)
+ stack = refs->main_backend.stack;
+
+ if (opts->flags & OPTIMIZE_REFS_AUTO)
+ return reftable_stack_compaction_required(stack, required);
+ else
+ *required = true;
+
+ return 0;
+}
+
struct write_create_symref_arg {
struct reftable_ref_store *refs;
struct reftable_stack *stack;
@@ -2710,6 +2733,8 @@ struct ref_storage_be refs_be_reftable = {
.transaction_abort = reftable_be_transaction_abort,
.optimize = reftable_be_optimize,
+ .optimize_required = reftable_be_optimize_required,
+
.rename_ref = reftable_be_rename_ref,
.copy_ref = reftable_be_copy_ref,
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`
2025-10-10 10:27 ` [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be` Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 9:46 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:12PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 1c37899006..c262ae1a7b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,
> return 0;
> }
>
> +static int files_optimize_required(struct ref_store *ref_store,
> + struct optimize_refs_opts *opts,
> + bool *required)
> +{
> + struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
> + "optimize_required");
> + *required = should_pack_refs(refs, opts);
> + return 0;
> +}
Okay, this is nice and straight-forward. One might argue that we could
also have the following (uncompiled, so pseudo-code-y) implementation:
static int files_optimize_required(struct ref_store *ref_store,
struct optimize_refs_opts *opts,
bool *required)
{
struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
"optimize_required");
*required = should_pack_refs(refs, opts) ||
refs_optimize_required(refs->packed_refs, opts);
return 0;
}
But on the other hand we know that they aren't ever repacked, so it's
probably dumb.
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index acaa5a6e57..c94948f618 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,
> return 0;
> }
>
> +static int packed_optimize_required(struct ref_store *ref_store UNUSED,
> + struct optimize_refs_opts *opts UNUSED,
> + bool *required)
> +{
> + /*
> + * Packed refs are already optimized.
> + */
> + *required = false;
> + return 0;
> +}
Yup.
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index d77714366a..df39fe9b38 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,
> return ret;
> }
>
> +static int reftable_be_optimize_required(struct ref_store *ref_store,
> + struct optimize_refs_opts *opts,
> + bool *required)
> +{
> + struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,
> + "optimize_refs_required");
> + struct reftable_stack *stack;
> +
> + if (refs->err)
> + return refs->err;
> +
> + stack = refs->worktree_backend.stack;
> + if (!stack)
> + stack = refs->main_backend.stack;
> +
> + if (opts->flags & OPTIMIZE_REFS_AUTO)
> + return reftable_stack_compaction_required(stack, required);
> + else
> + *required = true;
This doesn't make much sense. We should only indicate that we require
repacking when there are at least two tables in the stack.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 9:46 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 9:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:12PM +0200, Karthik Nayak wrote:
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 1c37899006..c262ae1a7b 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,
>> return 0;
>> }
>>
>> +static int files_optimize_required(struct ref_store *ref_store,
>> + struct optimize_refs_opts *opts,
>> + bool *required)
>> +{
>> + struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
>> + "optimize_required");
>> + *required = should_pack_refs(refs, opts);
>> + return 0;
>> +}
>
> Okay, this is nice and straight-forward. One might argue that we could
> also have the following (uncompiled, so pseudo-code-y) implementation:
>
> static int files_optimize_required(struct ref_store *ref_store,
> struct optimize_refs_opts *opts,
> bool *required)
> {
> struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
> "optimize_required");
> *required = should_pack_refs(refs, opts) ||
> refs_optimize_required(refs->packed_refs, opts);
> return 0;
> }
>
> But on the other hand we know that they aren't ever repacked, so it's
> probably dumb.
>
I also think that individually the packed-backend shouldn't care about
the files backend. So it shouldn't be calling `should_pack_refs()`. In
short, the packed backend knows that it is always optimized and it
should simply state that.
>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
>> index acaa5a6e57..c94948f618 100644
>> --- a/refs/packed-backend.c
>> +++ b/refs/packed-backend.c
>> @@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,
>> return 0;
>> }
>>
>> +static int packed_optimize_required(struct ref_store *ref_store UNUSED,
>> + struct optimize_refs_opts *opts UNUSED,
>> + bool *required)
>> +{
>> + /*
>> + * Packed refs are already optimized.
>> + */
>> + *required = false;
>> + return 0;
>> +}
>
> Yup.
>
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index d77714366a..df39fe9b38 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,
>> return ret;
>> }
>>
>> +static int reftable_be_optimize_required(struct ref_store *ref_store,
>> + struct optimize_refs_opts *opts,
>> + bool *required)
>> +{
>> + struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,
>> + "optimize_refs_required");
>> + struct reftable_stack *stack;
>> +
>> + if (refs->err)
>> + return refs->err;
>> +
>> + stack = refs->worktree_backend.stack;
>> + if (!stack)
>> + stack = refs->main_backend.stack;
>> +
>> + if (opts->flags & OPTIMIZE_REFS_AUTO)
>> + return reftable_stack_compaction_required(stack, required);
>> + else
>> + *required = true;
>
> This doesn't make much sense. We should only indicate that we require
> repacking when there are at least two tables in the stack.
>
> Patrick
Fair enough, I was thinking that when users say 'git refs optimize'
without any options. We always call `reftable_stack_compact_all()`. So
it would always make sense to say required = true.
But `reftable_stack_compact_all()` does make that check. So I'll add it
to `reftable_stack_compaction_required()` and call that with a
`use_heuristics` parameter.
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-10 10:27 [PATCH 0/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
` (7 preceding siblings ...)
2025-10-10 10:27 ` [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be` Karthik Nayak
@ 2025-10-10 10:27 ` Karthik Nayak
2025-10-10 11:22 ` Patrick Steinhardt
8 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-10 10:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The 'git pack-refs' and 'git refs optimize' commands allow users to
optimize their reference backends. However they provide no functionality
to check if optimization is required without performing it.
Add a '--required' flag to these commands to do that. This is useful
on the server side where this information can be utilized to perform
more targetted maintenance runs of the repository.
Add a corresponding test for the files backend. For the reftable
backend, this cannot be tested easily as it performs auto-compaction.
However, an earlier commit ensured the functionality was covered by
unit test.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-pack-refs.adoc | 1 +
Documentation/git-refs.adoc | 1 +
Documentation/pack-refs-options.adoc | 5 +++++
builtin/pack-refs.c | 2 +-
builtin/refs.c | 2 +-
pack-refs.c | 11 ++++++++++-
pack-refs.h | 10 ++++++++--
refs.c | 7 +++++++
refs.h | 7 +++++++
t/pack-refs-tests.sh | 20 ++++++++++++++++++++
10 files changed, 61 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc
index fde9f2f294..62bc01b29b 100644
--- a/Documentation/git-pack-refs.adoc
+++ b/Documentation/git-pack-refs.adoc
@@ -9,6 +9,7 @@ SYNOPSIS
--------
[verse]
'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
+ [--required]
DESCRIPTION
-----------
diff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc
index fa33680cc7..4df28b7d92 100644
--- a/Documentation/git-refs.adoc
+++ b/Documentation/git-refs.adoc
@@ -20,6 +20,7 @@ git refs list [--count=<count>] [--shell|--perl|--python|--tcl]
[ --stdin | (<pattern>...)]
git refs exists <ref>
git refs optimize [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
+ [--required]
DESCRIPTION
-----------
diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc
index 0b11282941..66d69530b9 100644
--- a/Documentation/pack-refs-options.adoc
+++ b/Documentation/pack-refs-options.adoc
@@ -50,3 +50,8 @@ the provided `--exclude` patterns.
+
When used with `--include`, refs provided to `--include`, minus refs that are
provided to `--exclude` will be packed.
+
+--required::
+
+Check if pack-refs is required to run, without actually performing the changes.
+Returns an exit code of 0 if optimization is required.
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 3446b84cda..bd2df4d0b7 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -8,7 +8,7 @@ int cmd_pack_refs(int argc,
struct repository *repo)
{
static char const * const pack_refs_usage[] = {
- N_("git pack-refs " PACK_REFS_OPTS),
+ N_("git pack-refs " PACK_REFS_OPTS(PACK_REFS_OPTS_SPACES_14)),
NULL
};
diff --git a/builtin/refs.c b/builtin/refs.c
index 3064f888b2..fca55997be 100644
--- a/builtin/refs.c
+++ b/builtin/refs.c
@@ -20,7 +20,7 @@
N_("git refs exists <ref>")
#define REFS_OPTIMIZE_USAGE \
- N_("git refs optimize " PACK_REFS_OPTS)
+ N_("git refs optimize " PACK_REFS_OPTS(PACK_REFS_OPTS_SPACES_18))
static int cmd_refs_migrate(int argc, const char **argv, const char *prefix,
struct repository *repo UNUSED)
diff --git a/pack-refs.c b/pack-refs.c
index fee77fbf9f..5d4d4266de 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -21,6 +21,7 @@ int pack_refs_core(int argc,
};
struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
struct string_list_item *item;
+ bool check_required = false;
int pack_all = 0;
int ret;
@@ -28,6 +29,7 @@ int pack_refs_core(int argc,
OPT_BOOL(0, "all", &pack_all, N_("pack everything")),
OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), OPTIMIZE_REFS_PRUNE),
OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), OPTIMIZE_REFS_AUTO),
+ OPT_BOOL(0, "required", &check_required, N_("check if optimization is required")),
OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
N_("references to include")),
OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
@@ -47,7 +49,14 @@ int pack_refs_core(int argc,
if (!pack_refs_opts.includes->nr)
string_list_append(pack_refs_opts.includes, "refs/tags/*");
- ret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);
+ if (check_required) {
+ bool required = false;
+ ret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,
+ &required);
+ ret |= !required;
+ } else {
+ ret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);
+ }
clear_ref_exclusions(&excludes);
string_list_clear(&included_refs, 0);
diff --git a/pack-refs.h b/pack-refs.h
index 5de27e7da8..59951ff692 100644
--- a/pack-refs.h
+++ b/pack-refs.h
@@ -7,9 +7,15 @@ struct repository;
* Shared usage string for options common to git-pack-refs(1)
* and git-refs-optimize(1). The command-specific part (e.g., "git refs optimize ")
* must be prepended by the caller.
+ *
+ * Since multiple commands use the same opts, they need to provide the appropriate
+ * spaces for correct alignment.
*/
-#define PACK_REFS_OPTS \
- "[--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]"
+#define PACK_REFS_OPTS_SPACES_14 " "
+#define PACK_REFS_OPTS_SPACES_18 " "
+#define PACK_REFS_OPTS(spaces) \
+ "[--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]\n" \
+ spaces "[--required]"
/*
* The core logic for pack-refs and its clones.
diff --git a/refs.c b/refs.c
index 514fb85af2..59a48b36b7 100644
--- a/refs.c
+++ b/refs.c
@@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
return refs->be->optimize(refs, opts);
}
+int refs_optimize_required(struct ref_store *refs,
+ struct optimize_refs_opts *opts,
+ bool *required)
+{
+ return refs->be->optimize_required(refs, opts, required);
+}
+
int reference_get_peeled_oid(struct repository *repo,
const struct reference *ref,
struct object_id *peeled_oid)
diff --git a/refs.h b/refs.h
index 58b222ac02..59f0e519c0 100644
--- a/refs.h
+++ b/refs.h
@@ -522,6 +522,13 @@ struct optimize_refs_opts {
*/
int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts);
+/*
+ * Check if refs backend can be optimized by calling 'refs_optimize'.
+ */
+int refs_optimize_required(struct ref_store *ref_store,
+ struct optimize_refs_opts *opts,
+ bool *required);
+
/*
* Setup reflog before using. Fill in err and return -1 on failure.
*/
diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 6a71838ffa..d3e58572e2 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -460,3 +460,23 @@ test_expect_success 'pack-refs does not store invalid peeled tag value' '
)
'
+test_expect_success "refs optimize --required works as expected" '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git commit --allow-empty --message "initial" &&
+ test_must_fail git ${pack_refs} --all --auto --required &&
+
+ # Create 14 additional references, which brings us to
+ # 15 together with the default branch.
+ printf "create refs/heads/loose-%d HEAD\n" $(test_seq 14) >stdin &&
+ git update-ref --stdin <stdin &&
+ test_path_is_missing .git/packed-refs &&
+ test_must_fail git ${pack_refs} --all --auto --required &&
+
+ # Create the 16th reference, which should cause us to repack.
+ git update-ref refs/heads/loose-15 HEAD &&
+ git ${pack_refs} --all --auto --required
+ )
+'
--
2.51.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-10 10:27 ` [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize' Karthik Nayak
@ 2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 12:37 ` Karthik Nayak
2025-10-13 13:40 ` Junio C Hamano
0 siblings, 2 replies; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-10 11:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:
> The 'git pack-refs' and 'git refs optimize' commands allow users to
> optimize their reference backends. However they provide no functionality
> to check if optimization is required without performing it.
>
> Add a '--required' flag to these commands to do that. This is useful
> on the server side where this information can be utilized to perform
> more targetted maintenance runs of the repository.
s/targetted/targeted/
> Add a corresponding test for the files backend. For the reftable
> backend, this cannot be tested easily as it performs auto-compaction.
> However, an earlier commit ensured the functionality was covered by
> unit test.
You can disable auto-compaction via the
"GIT_TEST_REFTABLE_AUTOCOMPACTION" environment variable, so it should be
rather easy to add a test.
> diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc
> index fde9f2f294..62bc01b29b 100644
> --- a/Documentation/git-pack-refs.adoc
> +++ b/Documentation/git-pack-refs.adoc
> @@ -9,6 +9,7 @@ SYNOPSIS
> --------
> [verse]
> 'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
> + [--required]
Hm, I'm not a huge fan of that name. "--dry-run" might be a better name,
but on the other hand one might expect that such a command also yields
information about what happens or that the compaction should be
successful. So I cannot come up with a better name, either.
> diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc
> index 0b11282941..66d69530b9 100644
> --- a/Documentation/pack-refs-options.adoc
> +++ b/Documentation/pack-refs-options.adoc
> @@ -50,3 +50,8 @@ the provided `--exclude` patterns.
> +
> When used with `--include`, refs provided to `--include`, minus refs that are
> provided to `--exclude` will be packed.
> +
> +--required::
> +
> +Check if pack-refs is required to run, without actually performing the changes.
Let's not say "pack-refs" here anymore, as we're migrating away from
that name towards more generic terminology. Maybe:
Check whether the reference store needs to be optimized without
actually performing the changes.
> diff --git a/pack-refs.c b/pack-refs.c
> index fee77fbf9f..5d4d4266de 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -21,6 +21,7 @@ int pack_refs_core(int argc,
> };
> struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
> struct string_list_item *item;
> + bool check_required = false;
> int pack_all = 0;
> int ret;
>
> @@ -28,6 +29,7 @@ int pack_refs_core(int argc,
> OPT_BOOL(0, "all", &pack_all, N_("pack everything")),
> OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), OPTIMIZE_REFS_PRUNE),
> OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), OPTIMIZE_REFS_AUTO),
> + OPT_BOOL(0, "required", &check_required, N_("check if optimization is required")),
> OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
> N_("references to include")),
> OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
> @@ -47,7 +49,14 @@ int pack_refs_core(int argc,
> if (!pack_refs_opts.includes->nr)
> string_list_append(pack_refs_opts.includes, "refs/tags/*");
>
> - ret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);
> + if (check_required) {
> + bool required = false;
> + ret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,
> + &required);
> + ret |= !required;
I think we shouldn't use `|=` here, and furthermore I think we need to
define semantics more specifically. E.g.:
- Return 0 in case optimization is required.
- Return 2 in case no optimization is required. I mostly shy away
from `1` here because it might already be used? We'll have to
double check though.
- Return any other non-zero error code in case an error occurs.
This also needs to be documented.
> diff --git a/refs.c b/refs.c
> index 514fb85af2..59a48b36b7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
> return refs->be->optimize(refs, opts);
> }
>
> +int refs_optimize_required(struct ref_store *refs,
> + struct optimize_refs_opts *opts,
> + bool *required)
> +{
> + return refs->be->optimize_required(refs, opts, required);
> +}
> +
I feel like this should be introduced in a preceding commit.
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-10 11:22 ` Patrick Steinhardt
@ 2025-10-13 12:37 ` Karthik Nayak
2025-10-13 13:40 ` Junio C Hamano
1 sibling, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-10-13 12:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 5428 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:
>> The 'git pack-refs' and 'git refs optimize' commands allow users to
>> optimize their reference backends. However they provide no functionality
>> to check if optimization is required without performing it.
>>
>> Add a '--required' flag to these commands to do that. This is useful
>> on the server side where this information can be utilized to perform
>> more targetted maintenance runs of the repository.
>
> s/targetted/targeted/
>
My spell check says both are accepted. But the latter seems to be the
general spelling.
>> Add a corresponding test for the files backend. For the reftable
>> backend, this cannot be tested easily as it performs auto-compaction.
>> However, an earlier commit ensured the functionality was covered by
>> unit test.
>
> You can disable auto-compaction via the
> "GIT_TEST_REFTABLE_AUTOCOMPACTION" environment variable, so it should be
> rather easy to add a test.
>
Good point, let me add a test.
>> diff --git a/Documentation/git-pack-refs.adoc b/Documentation/git-pack-refs.adoc
>> index fde9f2f294..62bc01b29b 100644
>> --- a/Documentation/git-pack-refs.adoc
>> +++ b/Documentation/git-pack-refs.adoc
>> @@ -9,6 +9,7 @@ SYNOPSIS
>> --------
>> [verse]
>> 'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
>> + [--required]
>
> Hm, I'm not a huge fan of that name. "--dry-run" might be a better name,
> but on the other hand one might expect that such a command also yields
> information about what happens or that the compaction should be
> successful. So I cannot come up with a better name, either.
>
Yea, I too had the same thought process. Something like `--dry-run`
would create the actual compacted files but not replace the existing
files.
We could perhaps use `--check` ? But I'm okay with either.
>> diff --git a/Documentation/pack-refs-options.adoc b/Documentation/pack-refs-options.adoc
>> index 0b11282941..66d69530b9 100644
>> --- a/Documentation/pack-refs-options.adoc
>> +++ b/Documentation/pack-refs-options.adoc
>> @@ -50,3 +50,8 @@ the provided `--exclude` patterns.
>> +
>> When used with `--include`, refs provided to `--include`, minus refs that are
>> provided to `--exclude` will be packed.
>> +
>> +--required::
>> +
>> +Check if pack-refs is required to run, without actually performing the changes.
>
> Let's not say "pack-refs" here anymore, as we're migrating away from
> that name towards more generic terminology. Maybe:
>
> Check whether the reference store needs to be optimized without
> actually performing the changes.
>
This is better, will amend.
>> diff --git a/pack-refs.c b/pack-refs.c
>> index fee77fbf9f..5d4d4266de 100644
>> --- a/pack-refs.c
>> +++ b/pack-refs.c
>> @@ -21,6 +21,7 @@ int pack_refs_core(int argc,
>> };
>> struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
>> struct string_list_item *item;
>> + bool check_required = false;
>> int pack_all = 0;
>> int ret;
>>
>> @@ -28,6 +29,7 @@ int pack_refs_core(int argc,
>> OPT_BOOL(0, "all", &pack_all, N_("pack everything")),
>> OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), OPTIMIZE_REFS_PRUNE),
>> OPT_BIT(0, "auto", &pack_refs_opts.flags, N_("auto-pack refs as needed"), OPTIMIZE_REFS_AUTO),
>> + OPT_BOOL(0, "required", &check_required, N_("check if optimization is required")),
>> OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
>> N_("references to include")),
>> OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
>> @@ -47,7 +49,14 @@ int pack_refs_core(int argc,
>> if (!pack_refs_opts.includes->nr)
>> string_list_append(pack_refs_opts.includes, "refs/tags/*");
>>
>> - ret = refs_optimize(get_main_ref_store(repo), &pack_refs_opts);
>> + if (check_required) {
>> + bool required = false;
>> + ret = refs_optimize_required(get_main_ref_store(repo), &pack_refs_opts,
>> + &required);
>> + ret |= !required;
>
> I think we shouldn't use `|=` here, and furthermore I think we need to
> define semantics more specifically. E.g.:
>
> - Return 0 in case optimization is required.
>
> - Return 2 in case no optimization is required. I mostly shy away
> from `1` here because it might already be used? We'll have to
> double check though.
>
> - Return any other non-zero error code in case an error occurs.
>
> This also needs to be documented.
>
Fair. So I checked and the only backend which returns an error is the
reftable backend and this is explicitly only the
`REFTABLE_OUT_OF_MEMORY_ERROR` error, which is equal to -13.
So let's do 0 for optimization required, 1 for no optimization required
and other non-zero for error.
I'll modify accordingly.
>> diff --git a/refs.c b/refs.c
>> index 514fb85af2..59a48b36b7 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2317,6 +2317,13 @@ int refs_optimize(struct ref_store *refs, struct optimize_refs_opts *opts)
>> return refs->be->optimize(refs, opts);
>> }
>>
>> +int refs_optimize_required(struct ref_store *refs,
>> + struct optimize_refs_opts *opts,
>> + bool *required)
>> +{
>> + return refs->be->optimize_required(refs, opts, required);
>> +}
>> +
>
> I feel like this should be introduced in a preceding commit.
>
> Patrick
Yeah, It would be better there, will move it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-10 11:22 ` Patrick Steinhardt
2025-10-13 12:37 ` Karthik Nayak
@ 2025-10-13 13:40 ` Junio C Hamano
2025-10-13 14:37 ` Junio C Hamano
1 sibling, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2025-10-13 13:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
Patrick Steinhardt <ps@pks.im> writes:
>> 'git pack-refs' [--all] [--no-prune] [--auto] [--include <pattern>] [--exclude <pattern>]
>> + [--required]
>
> Hm, I'm not a huge fan of that name. "--dry-run" might be a better name,
> but on the other hand one might expect that such a command also yields
> information about what happens or that the compaction should be
> successful.
I had the same reaction. First I thought this would make the
command fail if the requested action cannot be done, as it is
required to fulfill the request, or something, but anything user
asks to do is by default required in a sane world, so it does not
make much sense.
If I understand the motivation behind this correctly, we want a
cheap check similar to what is implicitly used before auto-gc kicks
in (i.e., asking "do we have enough loose objects that makes us
worried" without actually counting all the loose objects but
estimating the number cheaply), but "--dry-run", while being much
better than "--required" to convey the intention, has an extra
connotation that it does a much more thorough job than mere
estimating---doing almost the same thing as the real job but without
leaving any lasting effect on the data---which makes the word
inappropriate, too.
Perhaps "--check-" followed by a word specific to what we are trying
to achieve (e.g., if we are trying to see if auto-compaction is
necessary, "--check-for-auto" "check for auto compaction")? I
dunno.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-13 13:40 ` Junio C Hamano
@ 2025-10-13 14:37 ` Junio C Hamano
2025-10-14 15:08 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2025-10-13 14:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
Junio C Hamano <gitster@pobox.com> writes:
> Perhaps "--check-" followed by a word specific to what we are trying
> to achieve (e.g., if we are trying to see if auto-compaction is
> necessary, "--check-for-auto" "check for auto compaction")? I
> dunno.
After reading what you did in the previou step, I am reasonably sure
"required" is a wrong word to use, with or without other words like
"check". Semantically it is similar to the should_pack_refs() check
that we use for pack-refs even before "optimize" came. We expect it
to answer this question cheaply: are we better off if we repacked,
or can we go on without repacking for now? It is not about "are we
performing so poorly that we MUST optimize now?"
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-13 14:37 ` Junio C Hamano
@ 2025-10-14 15:08 ` Karthik Nayak
2025-10-14 17:46 ` Junio C Hamano
0 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-14 15:08 UTC (permalink / raw)
To: Junio C Hamano, Patrick Steinhardt; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Perhaps "--check-" followed by a word specific to what we are trying
>> to achieve (e.g., if we are trying to see if auto-compaction is
>> necessary, "--check-for-auto" "check for auto compaction")? I
>> dunno.
>
> After reading what you did in the previou step, I am reasonably sure
> "required" is a wrong word to use, with or without other words like
> "check". Semantically it is similar to the should_pack_refs() check
> that we use for pack-refs even before "optimize" came. We expect it
> to answer this question cheaply: are we better off if we repacked,
> or can we go on without repacking for now? It is not about "are we
> performing so poorly that we MUST optimize now?"
I agree '--required' isn't the best name, and like we discussed
'--dry-run' wouldn't be either since that would imply that the work is
being done but not persisted.
I was leaning towards '--check', which is simple. But It might be nicer
to be verbose here and simply add something like '--is-worthwhile'.
Being verbose here is okay, since it will only be used sparingly and
specifically by those who require such a use case.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-14 15:08 ` Karthik Nayak
@ 2025-10-14 17:46 ` Junio C Hamano
2025-10-15 7:50 ` Srivastava, Nitin
2025-10-15 8:19 ` Karthik Nayak
0 siblings, 2 replies; 36+ messages in thread
From: Junio C Hamano @ 2025-10-14 17:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git
Karthik Nayak <karthik.188@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Perhaps "--check-" followed by a word specific to what we are trying
>>> to achieve (e.g., if we are trying to see if auto-compaction is
>>> necessary, "--check-for-auto" "check for auto compaction")? I
>>> dunno.
>>
>> After reading what you did in the previou step, I am reasonably sure
>> "required" is a wrong word to use, with or without other words like
>> "check". Semantically it is similar to the should_pack_refs() check
>> that we use for pack-refs even before "optimize" came. We expect it
>> to answer this question cheaply: are we better off if we repacked,
>> or can we go on without repacking for now? It is not about "are we
>> performing so poorly that we MUST optimize now?"
>
> I agree '--required' isn't the best name, and like we discussed
> '--dry-run' wouldn't be either since that would imply that the work is
> being done but not persisted.
>
> I was leaning towards '--check', which is simple. But It might be nicer
> to be verbose here and simply add something like '--is-worthwhile'.
>
> Being verbose here is okay, since it will only be used sparingly and
> specifically by those who require such a use case.
Nah, "worthwhile" is relative and it would be less meaningful
without expressing for what goal we are judging how it is worthwhile
to do.
Choosing a phrase around "check" is better, I would think.
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-14 17:46 ` Junio C Hamano
@ 2025-10-15 7:50 ` Srivastava, Nitin
2025-10-15 8:19 ` Karthik Nayak
1 sibling, 0 replies; 36+ messages in thread
From: Srivastava, Nitin @ 2025-10-15 7:50 UTC (permalink / raw)
To: git@vger.kernel.org
Please unsubscribe me from all email notifications.
-----Original Message-----
From: Junio C Hamano <gitster@pobox.com>
Sent: Tuesday, October 14, 2025 11:17 PM
To: Karthik Nayak <karthik.188@gmail.com>
Cc: Patrick Steinhardt <ps@pks.im>; git@vger.kernel.org
Subject: Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
Karthik Nayak <karthik.188@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Perhaps "--check-" followed by a word specific to what we are trying
>>> to achieve (e.g., if we are trying to see if auto-compaction is
>>> necessary, "--check-for-auto" "check for auto compaction")? I
>>> dunno.
>>
>> After reading what you did in the previou step, I am reasonably sure
>> "required" is a wrong word to use, with or without other words like
>> "check". Semantically it is similar to the should_pack_refs() check
>> that we use for pack-refs even before "optimize" came. We expect it
>> to answer this question cheaply: are we better off if we repacked, or
>> can we go on without repacking for now? It is not about "are we
>> performing so poorly that we MUST optimize now?"
>
> I agree '--required' isn't the best name, and like we discussed
> '--dry-run' wouldn't be either since that would imply that the work is
> being done but not persisted.
>
> I was leaning towards '--check', which is simple. But It might be
> nicer to be verbose here and simply add something like '--is-worthwhile'.
>
> Being verbose here is okay, since it will only be used sparingly and
> specifically by those who require such a use case.
Nah, "worthwhile" is relative and it would be less meaningful without expressing for what goal we are judging how it is worthwhile to do.
Choosing a phrase around "check" is better, I would think.
This transmission is intended solely for the addressee and contains confidential information.
If you are not the intended recipient, please immediately inform the sender and delete the message and any attachments from your system.
Furthermore, 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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-14 17:46 ` Junio C Hamano
2025-10-15 7:50 ` Srivastava, Nitin
@ 2025-10-15 8:19 ` Karthik Nayak
2025-10-15 9:29 ` Karthik Nayak
1 sibling, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-15 8:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Perhaps "--check-" followed by a word specific to what we are trying
>>>> to achieve (e.g., if we are trying to see if auto-compaction is
>>>> necessary, "--check-for-auto" "check for auto compaction")? I
>>>> dunno.
>>>
>>> After reading what you did in the previou step, I am reasonably sure
>>> "required" is a wrong word to use, with or without other words like
>>> "check". Semantically it is similar to the should_pack_refs() check
>>> that we use for pack-refs even before "optimize" came. We expect it
>>> to answer this question cheaply: are we better off if we repacked,
>>> or can we go on without repacking for now? It is not about "are we
>>> performing so poorly that we MUST optimize now?"
>>
>> I agree '--required' isn't the best name, and like we discussed
>> '--dry-run' wouldn't be either since that would imply that the work is
>> being done but not persisted.
>>
>> I was leaning towards '--check', which is simple. But It might be nicer
>> to be verbose here and simply add something like '--is-worthwhile'.
>>
>> Being verbose here is okay, since it will only be used sparingly and
>> specifically by those who require such a use case.
>
> Nah, "worthwhile" is relative and it would be less meaningful
> without expressing for what goal we are judging how it is worthwhile
> to do.
>
I see what you mean.
> Choosing a phrase around "check" is better, I would think.
How about simply `--check`. Since the flag can be used with other
existing flags, it would make more sense to do
git refs optimize --all --auto --check
git refs optimize --check
git refs optimize --all --check
I will send in a new version around this and we can discuss further on
top of that!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-15 8:19 ` Karthik Nayak
@ 2025-10-15 9:29 ` Karthik Nayak
2025-10-15 12:14 ` Patrick Steinhardt
0 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-10-15 9:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]
Karthik Nayak <karthik.188@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Junio C Hamano <gitster@pobox.com> writes:
>>>>
>>>>> Perhaps "--check-" followed by a word specific to what we are trying
>>>>> to achieve (e.g., if we are trying to see if auto-compaction is
>>>>> necessary, "--check-for-auto" "check for auto compaction")? I
>>>>> dunno.
>>>>
>>>> After reading what you did in the previou step, I am reasonably sure
>>>> "required" is a wrong word to use, with or without other words like
>>>> "check". Semantically it is similar to the should_pack_refs() check
>>>> that we use for pack-refs even before "optimize" came. We expect it
>>>> to answer this question cheaply: are we better off if we repacked,
>>>> or can we go on without repacking for now? It is not about "are we
>>>> performing so poorly that we MUST optimize now?"
>>>
>>> I agree '--required' isn't the best name, and like we discussed
>>> '--dry-run' wouldn't be either since that would imply that the work is
>>> being done but not persisted.
>>>
>>> I was leaning towards '--check', which is simple. But It might be nicer
>>> to be verbose here and simply add something like '--is-worthwhile'.
>>>
>>> Being verbose here is okay, since it will only be used sparingly and
>>> specifically by those who require such a use case.
>>
>> Nah, "worthwhile" is relative and it would be less meaningful
>> without expressing for what goal we are judging how it is worthwhile
>> to do.
>>
>
> I see what you mean.
>
>> Choosing a phrase around "check" is better, I would think.
>
> How about simply `--check`. Since the flag can be used with other
> existing flags, it would make more sense to do
>
> git refs optimize --all --auto --check
> git refs optimize --check
> git refs optimize --all --check
>
> I will send in a new version around this and we can discuss further on
> top of that!
After speaking to Patrick today about this and his work on 'git
maintenance', we realized we can actually broaden the scope. We could
implement something like 'git maintenance needed'. This would check on a
whole if repository maintenance is needed for refs and objects.
I do think there is some merit in some of the patches of this series, so
I'll spawn out couple of new series and send it to the list.
Junio, from your latest 'What's cooking', I notice that this wasn't
picked up, which is good. I hope its not too much churn in that sense :)
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-15 9:29 ` Karthik Nayak
@ 2025-10-15 12:14 ` Patrick Steinhardt
2025-10-15 20:17 ` Junio C Hamano
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-10-15 12:14 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, git
On Wed, Oct 15, 2025 at 02:29:35AM -0700, Karthik Nayak wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> Karthik Nayak <karthik.188@gmail.com> writes:
> >>
> >>> Junio C Hamano <gitster@pobox.com> writes:
> >>>
> >>>> Junio C Hamano <gitster@pobox.com> writes:
> >>>>
> >>>>> Perhaps "--check-" followed by a word specific to what we are trying
> >>>>> to achieve (e.g., if we are trying to see if auto-compaction is
> >>>>> necessary, "--check-for-auto" "check for auto compaction")? I
> >>>>> dunno.
> >>>>
> >>>> After reading what you did in the previou step, I am reasonably sure
> >>>> "required" is a wrong word to use, with or without other words like
> >>>> "check". Semantically it is similar to the should_pack_refs() check
> >>>> that we use for pack-refs even before "optimize" came. We expect it
> >>>> to answer this question cheaply: are we better off if we repacked,
> >>>> or can we go on without repacking for now? It is not about "are we
> >>>> performing so poorly that we MUST optimize now?"
> >>>
> >>> I agree '--required' isn't the best name, and like we discussed
> >>> '--dry-run' wouldn't be either since that would imply that the work is
> >>> being done but not persisted.
> >>>
> >>> I was leaning towards '--check', which is simple. But It might be nicer
> >>> to be verbose here and simply add something like '--is-worthwhile'.
> >>>
> >>> Being verbose here is okay, since it will only be used sparingly and
> >>> specifically by those who require such a use case.
> >>
> >> Nah, "worthwhile" is relative and it would be less meaningful
> >> without expressing for what goal we are judging how it is worthwhile
> >> to do.
> >>
> >
> > I see what you mean.
> >
> >> Choosing a phrase around "check" is better, I would think.
> >
> > How about simply `--check`. Since the flag can be used with other
> > existing flags, it would make more sense to do
> >
> > git refs optimize --all --auto --check
> > git refs optimize --check
> > git refs optimize --all --check
> >
> > I will send in a new version around this and we can discuss further on
> > top of that!
>
> After speaking to Patrick today about this and his work on 'git
> maintenance', we realized we can actually broaden the scope. We could
> implement something like 'git maintenance needed'. This would check on a
> whole if repository maintenance is needed for refs and objects.
Yup. The idea here is that git-maintenance(1) is already split up into
different tasks, one of which is the "pack-refs" task. And each of these
tasks already has a callback to verify whether or not we need to execute
the task.
So if we build on top of that mechanism we can easily broaden the scope
of this patch series to:
- Check whether maintenance is needed for any of the enabled tasks.
- Check whether maintenance is needed for a single task.
- Check whether maintenance is needed given a specific maintenance
strategy.
Another benefit is that it's a bit easier to put a verb into a proper
subcommand of git-maintenance(1) instead of having to add a verb-ish
flag to git-pack-refs(1). `git maintenance is-needed` feels way more
natural to me at least compared to `git refs optimize --check`.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
2025-10-15 12:14 ` Patrick Steinhardt
@ 2025-10-15 20:17 ` Junio C Hamano
0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2025-10-15 20:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
Patrick Steinhardt <ps@pks.im> writes:
> So if we build on top of that mechanism we can easily broaden the scope
> of this patch series to:
>
> - Check whether maintenance is needed for any of the enabled tasks.
>
> - Check whether maintenance is needed for a single task.
>
> - Check whether maintenance is needed given a specific maintenance
> strategy.
>
> Another benefit is that it's a bit easier to put a verb into a proper
> subcommand of git-maintenance(1) instead of having to add a verb-ish
> flag to git-pack-refs(1). `git maintenance is-needed` feels way more
> natural to me at least compared to `git refs optimize --check`.
Very nice.
^ permalink raw reply [flat|nested] 36+ messages in thread