Re: [PATCH v2 0/3] Use default values from settings instead of config

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 29, 2021, 06:43 UTC
Message-ID: <CAPig+cS7PF=F0XWO-EyVz_KuLnbnhQi3N01gyozSv99PmB_1RA@mail.gmail.com>
In-Reply-To: <kl6lh7e53nnn.fsf@chooglen-macbookpro.roam.corp.google.com>

On Mon, Sep 27, 2021 at 1:59 PM Glen Choo <chooglen@google.com> wrote:

Show 7 quoted lines

> I wasn't clear about this in the original message, but I think this is
> ready to merge in its current form. I'd love to hear from reviewers who
> can poke holes in that :)
>
> Cc Eric and Taylor who have given the most review on v1 (thanks again!).
> I believe I've addressed your comments and I'd be interesting in hearing
> your thoughts on v2.

I re-read the entire thread and left a few comments on the v2 patches. One or two of the comments might be actionable (i.e. deserve a re-roll), but most are probably subjective.

Oh, I forgot to mention in my review of [3/3] that in:

    In the case where core.multiPackIndex is not set in the config, the auto
    condition would fail, causing the incremental-repack task to not be
    run. Because we always want to consider the default values, we should
    just always just use the_repository->settings.

you can probably drop one or both "just"s.