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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 2, 2021, 12:55 UTC
Message-ID: <87o8bkamxb.fsf@evledraar.gmail.com>
In-Reply-To: <YN3wvy6fhD4V+FA3@coredump.intra.peff.net>

On Thu, Jul 01 2021, Jeff King wrote:

Show 16 quoted lines

> On Mon, Jun 28, 2021 at 09:19:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> So let's support this common pattern as a "startup_config" callback,
>> making use of our recently added "call_{advertise,command}()"
>> functions. This allows us to simplify e.g. the "ensure_config_read()"
>> function added in 59e1205d167 (ls-refs: report unborn targets of
>> symrefs, 2021-02-05).
>> 
>> We could read all the config for all the protocol capabilities, but
>> let's do it one callback at a time in anticipation that some won't be
>> called at all, and that some might be more expensive than others in
>> the future.
>
> 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.

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?

(More below)

Show 65 quoted lines

>> +static void read_startup_config(struct protocol_capability *command)
>> +{
>> +	if (!command->startup_config)
>> +		return;
>> +	if (command->have_startup_config++)
>> +		return;
>> +	git_config(command->startup_config, NULL);
>> +}
>
> ...we don't pass any context to the config callback here. I thought
> passing "command" might work, but looking at the ls_refs() function, it
> is the one who actually reads the pkt-lines that will tell us "hey, I'm
> doing an ls-refs for a push".
>
> So none of the serve() infrastructure can help us there; we need to read
> pkt-lines and _then_ read config.
>
> I dunno. Maybe the solution is for ls_refs() to just do a separate
> config call to pick up the operation-specific bits, like:
>
> diff --git a/ls-refs.c b/ls-refs.c
> index 88f6c3f60d..6ee70126aa 100644
> --- a/ls-refs.c
> +++ b/ls-refs.c
> @@ -130,12 +130,13 @@ static void send_possibly_unborn_head(struct ls_refs_data *data)
>  
>  static int ls_refs_config(const char *var, const char *value, void *data)
>  {
> +	struct ls_refs_data *d = data;
>  	/*
>  	 * We only serve fetches over v2 for now, so respect only "uploadpack"
>  	 * config. This may need to eventually be expanded to "receive", but we
>  	 * don't yet know how that information will be passed to ls-refs.
>  	 */
> -	return parse_hide_refs_config(var, value, "uploadpack");
> +	return parse_hide_refs_config(var, value, d->for_push ? "receive" : "uploadpack");
>  }
>  
>  int ls_refs(struct repository *r, struct strvec *keys,
> @@ -147,7 +148,6 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  	strvec_init(&data.prefixes);
>  
>  	ensure_config_read();
> -	git_config(ls_refs_config, NULL);
>  
>  	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
>  		const char *arg = request->line;
> @@ -161,8 +161,12 @@ int ls_refs(struct repository *r, struct strvec *keys,
>  			strvec_push(&data.prefixes, out);
>  		else if (!strcmp("unborn", arg))
>  			data.unborn = allow_unborn;
> +		else if (!strcmp("for-push", arg)) /* imagine this exists */
> +			data.for_push = 1;
>  	}
>  
> +	git_config(ls_refs_config, &data);
> +
>  	if (request->status != PACKET_READ_FLUSH)
>  		die(_("expected flush after ls-refs arguments"));
>  
>
> 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?

Anyway, I can do that if that addresses your concern. I thought the v1 objection was mainly "the config flow won't work that way in all cases", which you're right that I incorrectly assumed.

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.

> The patch above obviously makes no sense until we're working on v2
> receive-pack. But I think it illustrates that your patch here is not
> getting in the way (though technically I think that would also be true
> of your v1, I do like this version better).