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

From: Jeff King <peff@peff.net>
Date: Jul 1, 2021, 16:43 UTC
Message-ID: <YN3wvy6fhD4V+FA3@coredump.intra.peff.net>
In-Reply-To: <patch-7.8-0a4fb01ae38-20210628T191634Z-avarab@gmail.com>

On Mon, Jun 28, 2021 at 09:19:24PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

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

Show 8 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.

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

-Peff