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

From: Jeff King <peff@peff.net>
Date: Jul 2, 2021, 21:13 UTC
Message-ID: <YN+BdikYt+lH+L3I@coredump.intra.peff.net>
In-Reply-To: <87o8bkamxb.fsf@evledraar.gmail.com>

On Fri, Jul 02, 2021 at 02:55:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> > Sadly I don't think this addresses my "v2 receive-pack" concern. The
> > ls_refs_startup_config() function will get called after we've received a
> > request for "ls-refs", which is good. But:
> 
> I think it does in that you rightly objected to us moving all config to
> such a callback, because for some of it we don't have the information
> needed to look it up yet, we do that in the request handler.
> 
> But for a lot of our config it's fine to do it early, hence "startup"
> config.

So I think your patch is OK, but just to lay out my thinking, it was this. The v2 serve code basically does:

  1. Somebody calls serve().

  2. It reads a capability/operation name, then dispatches to the
     appropriate function.

  3. That function reads operation-specific data, like options.

Your first patch read config at step 1. This one reads at step 2.

But from past discussions, the hide-refs config could only be read (or interpreted) after step 3, because that's when we'd see an option saying "this is for pushing".

So anything except step 3 is "too late". But that doesn't stop us from having some early config and some operation-specific late config.

Show 11 quoted lines

> Yes I've moved the ls-refs handling into the "startup" because /right
> now/ it's only handling fetches, it'll need to be moved out if and when
> we start handling pushes.
> 
> But isn't it going to be obvious that we'll need to do that then? Since
> we'll have the example of upload-pack.c doing that exact thing?
>
> I.e. do you not want to have the "startup config" concept at all, or
> would just prefer to have the ls-refs part of it pre-emotively moved out
> of it in anticipation of handling pushes some day, even if we can do
> that on "startup" now?

Sorry to be so confusing. About halfway through writing my previous email I had realized "well, I guess we can have two phases of config-reading, and that's not so bad". So I'm OK with this direction.

I don't think anything else needs to happen on top of your patch here. The "late" phase for ls-refs config reading already happens totally separately (it's in the git_config(ls_refs_config) call, which is already separate from the ensure_config_read() thing. And your patch leaves that line untouched.

Show 7 quoted lines

> > And then it is a separate thing entirely from your "serve will read
> > config" code. It's arguably more confusing, because now config is read
> > in two places, but that is already true because of this
> > ensure_config_read() thing.
> 
> This suggests to me you'd like to preemptively move it out of "startup",
> correct?

It's already out of the startup, so I think we are OK.

My big question here was: is the concept of "startup config" and "other config we read after seeing the options" too confusing? But probably not.

> I just thought preemptively doing it for "ls-refs" wouldn't be needed,
> since we'd notice in testing such a feature that "do it once" would
> break in obvious ways for multi-requests, especially with the comment
> for the "startup_config" callback explicitly calling out that case.

Yeah, we'd definitely notice. I was just wondering if we'd end up needing to back out these changes to accommodate it. But the two-phase thing solves that.

-Peff