Re: [PATCH v4 6/7] fetch: after refetch, encourage auto gc repacking
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 31, 2022, 15:22 UTC
- Message-ID
- <220331.86bkxmp25k.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <28c07219fd830196af1171320b86bc2a58ba3d79.1648476132.git.gitgitgadget@gmail.com>
On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:
Show 48 quoted lines
> From: Robert Coup <robert@coup.net.nz>
>
> After invoking `fetch --refetch`, the object db will likely contain many
> duplicate objects. If auto-maintenance is enabled, invoke it with
> appropriate settings to encourage repacking/consolidation.
>
> * gc.autoPackLimit: unless this is set to 0 (disabled), override the
> value to 1 to force pack consolidation.
> * maintenance.incremental-repack.auto: unless this is set to 0, override
> the value to -1 to force incremental repacking.
>
> Signed-off-by: Robert Coup <robert@coup.net.nz>
> ---
> Documentation/fetch-options.txt | 3 ++-
> builtin/fetch.c | 19 ++++++++++++++++++-
> t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++
> 3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index d03fce5aae0..622bd84768b 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -169,7 +169,8 @@ ifndef::git-pull[]
> associated objects that are already present locally, this option fetches
> all objects as a fresh clone would. Use this to reapply a partial clone
> filter from configuration or using `--filter=` when the filter
> - definition has changed.
> + definition has changed. Automatic post-fetch maintenance will perform
> + object database pack consolidation to remove any duplicate objects.
> endif::git-pull[]
>
> --refmap=<refspec>::
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e391a5dbc55..e3791f09ed5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2306,8 +2306,25 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> NULL);
> }
>
> - if (enable_auto_gc)
> + if (enable_auto_gc) {
> + if (refetch) {
> + /*
> + * Hint auto-maintenance strongly to encourage repacking,
> + * but respect config settings disabling it.
> + */
> + int opt_val;nit: add a \n after this.
> + if (git_config_get_int("gc.autopacklimit", &opt_val))
> + opt_val = -1;
> + if (opt_val != 0)nit: don't compare against 0 or null, just !opt_val
Isn't this whole thing also clearer as:
int &forget;
if (git_conf...(..., &forget))
git_config_push_parameter("gc.autoPackLimit=1");Maybe I haven't eyeballed this enough, but aren't you ignoring explicit gc.autoPackLimit=0 configuration? Whereas what you seem to want is "set this config unlress the user has it set", for which we only need to check the git_config...(...) return value, no?
Show 6 quoted lines
> + git_config_push_parameter("gc.autoPackLimit=1");
> +
> + if (git_config_get_int("maintenance.incremental-repack.auto", &opt_val))
> + opt_val = -1;
> + if (opt_val != 0)
> + git_config_push_parameter("maintenance.incremental-repack.auto=-1");hrm, do we really need to set both of these these days (not saying we don't, just surprised). I.e. both gc.* an maintenance.* config.
*skims the code*
Urgh, yes? too_many_packs() seems to check gc.* only, but incremental_repack_auto_condition() check this variable... :(
> +test_expect_success 'fetch --refetch triggers repacking' ' > + GIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&
Nit: Can we use GIT_CONFIG_KEY_* et al for this these days, or do we still need this trace2 thingy?
> + export GIT_TRACE2_CONFIG_PARAMS && > +