Re: [PATCH v2 8/8] upload-pack.c: convert to new serve.c "startup" config cb

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jul 5, 2021, 14:00 UTC
Message-ID: <CAFQ2z_MmpisHcYqXd0=V8NCNxxsXn=MijX747V_oxnMV1bBFaA@mail.gmail.com>
In-Reply-To: <patch-8.8-9fd6138aa62-20210628T191634Z-avarab@gmail.com>

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

> Convert the config reading code in upload-pack.c to use the new
> "startup_config" callback when we're using the v2 protocol, and
> lightly fake up the same when we're using v0 and v1.

I'm not a fan creating more global variables rather, but I'll it leave to someone else to decide what's best. This change looks consistent with its predecessor, and OK modulo a few nits below.

Show 5 quoted lines

> Thus it's clear that the "allow_uor" passed to functions like
> "is_our_ref()" is constant after startup (usually it'll never change
> for a given server's configuration, or change once).
>
> This requires a very light compatibly layer with the serve.c callback

typo.

Show 7 quoted lines

> +/*
> + * "Global" configuration that won't be affected by the type of
> + * request we're doing, or by other request data in "struct
> + * upload_pack_data" below.
> + */
> +static int v1_have_startup_config;
> +static int config_keepalive = 5;

could this include a unit? config_keepalive_secs , I think?

Show 7 quoted lines

> +       int ret;
> +       ret = upload_pack_startup_config(var, value, data);
> +       if (ret)
> +               return ret;
> +       ret = upload_pack_startup_config_v2_only(var, value, data);
> +       if (ret)
> +               return ret;

Neither of these config readers ever return -1. Do you foresee this ever happening? Or could this return void?

> +       if (!v1_have_startup_config++)
> +               git_config(upload_pack_startup_config, NULL);

See comment in previous patch about ++ for boolean values.

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