Hi Ævar,
On Thu, 31 Mar 2022 at 16:33, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> > + 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?
>
> 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.
> 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.
>
> > +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.
