From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 10 Oct 2025 11:22:54 GMT
Subject: Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize'
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:
> 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