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

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

quoted

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.

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.

quoted

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