Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 10, 2025, 11:22 UTC
- Message-ID
- <aOjsjpE1vuFUXXbh@pks.im>
- In-Reply-To
- <20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-9-c7962be584fa@gmail.com>
On Fri, Oct 10, 2025 at 12:27:13PM +0200, Karthik Nayak wrote:
Show 7 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 12 quoted lines
> 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.Show 30 quoted lines
> 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.
Show 15 quoted lines
> 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