Re: [PATCH v2 7/8] serve: add support for a "startup" git_config() callback

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jul 5, 2021, 12:23 UTC
Message-ID: <CAFQ2z_Pbn2=0NuNNC9TnuLar91Nn2=CBZ7cs8ErM4guEi9BsEg@mail.gmail.com>
In-Reply-To: <patch-7.8-0a4fb01ae38-20210628T191634Z-avarab@gmail.com>

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> So let's support this common pattern as a "startup_config" callback,
> making use of our recently added "call_{advertise,command}()"

I think this is an improvement over ensure_config_read(), but I don't understand the whole picture. If you want to control configuration and be sure it is set early, isn't it more natural to put this in struct serve_options? With your patch, there are still two routes to injecting configuration: 'struct serve_options' and the git config.

IMO the former is more principled, and the control flow (serve_options is an argument to serve()) ensures that the options are read only once.

> +       /*
> +        * A git_config() callback that'll be called only once for the
> +        * lifetime of the process, possibly over many different

Putting it in a struct supplied by the caller provides more flexibility for testing: you could test multiple behaviors from a single process, by calling serve multiple times supplying different arguments.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado