From: Karthik Nayak Date: Mon, 13 Oct 2025 12:37:08 GMT Subject: Re: [PATCH 9/9] refs: add a '--required' flag to 'git refs optimize' Message-ID: In-Reply-To: Patrick Steinhardt 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 ] [--exclude ] >> + [--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.