Re: [PATCH v4 6/7] fetch: after refetch, encourage auto gc repacking

From: Robert Coup <robert@coup.net.nz>
Date: Apr 1, 2022, 10:51 UTC
Message-ID: <CACf-nVeTMx8y+8HzQbaTjHq06Ldon+JV+rp0yzhP_RC=6La2gg@mail.gmail.com>
In-Reply-To: <220331.86bkxmp25k.gmgdl@evledraar.gmail.com>

Hi Ævar,

On Thu, 31 Mar 2022 at 16:33, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 5 quoted lines

> > +                     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

I did this since 0 has a specific meaning ("Setting this to 0 disables"), it's not just false-y in this context. Tomayto, tomahto?

Show 12 quoted lines

>
> 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?

What I'm trying to achieve: if the user has not disabled auto-packing (autoPackLimit=0), then pass autoPackLimit=1 to the subprocess to encourage repacking. Context/why: so we don't 2x the object store size and not even attempt to repack it now, rather than at some unspecified point in the future. Maybe.

How the code achieves it:
  load autoPackLimit into opt_val
  if autoPackLimit is not specified in config: set opt_val to -1
  if opt_val is not 0: pass autoPackLimit=1 to the subprocess

AFAICT if we just if(git_config_get_int()) then if they haven't set it at all in config, we wouldn't encourage repacking in the subprocess. Which isn't what I'm trying to achieve.

Show 7 quoted lines

> 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... :(

Yes.

Show 6 quoted lines

>
> > +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?

I copied a pattern existing tests are using.

Thanks, Rob.