On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:
> From: Robert Coup <robert@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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?
> + 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 &&
> +
