On Fri, Jul 02, 2021 at 02:55:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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.
> > 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
