diff mbox series

[5/5] serve: add support for a git_config() callback

Message ID patch-5.5-c8625025125-20210616T141332Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series serve: add "configure" callback | expand

Commit Message

Ævar Arnfjörð Bjarmason June 16, 2021, 2:16 p.m. UTC
Since the introduction of serve.c we've added git_config() callbacks
and other config reading for capabilities in the following commits:

- e20b4192a37 (upload-pack: support hidden refs with protocol v2, 2018-12-18)
- 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05)
- 6b5b6e422ee (serve: advertise session ID in v2 capabilities, 2020-11-11)

This code can be simplified by simply adding support to serve.c itself
for reading config at the right time before the capability is
called. In particular the ensure_config_read() function being gone
makes this whole thing much simpler.

A behavior change here is that we're eagerly doing the configuration
for all our capabilities regardless of which one we end up serving,
whereas before we'd defer it until the point where we were processing
the specific command.

We could try to be more lazy here and only read the config if we find
out that we're serving "ls-refs" or "session-id", but it's just a fast
git_config() callback, I think in this case simplicity trumps a
premature optimization.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c | 55 ++++++++++++++++++-------------------------------------
 ls-refs.h |  1 +
 serve.c   | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 52 insertions(+), 38 deletions(-)

Comments

Jeff King June 16, 2021, 4:22 p.m. UTC | #1
On Wed, Jun 16, 2021 at 04:16:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Since the introduction of serve.c we've added git_config() callbacks
> and other config reading for capabilities in the following commits:
> 
> - e20b4192a37 (upload-pack: support hidden refs with protocol v2, 2018-12-18)
> - 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05)
> - 6b5b6e422ee (serve: advertise session ID in v2 capabilities, 2020-11-11)
> 
> This code can be simplified by simply adding support to serve.c itself
> for reading config at the right time before the capability is
> called. In particular the ensure_config_read() function being gone
> makes this whole thing much simpler.
> 
> A behavior change here is that we're eagerly doing the configuration
> for all our capabilities regardless of which one we end up serving,
> whereas before we'd defer it until the point where we were processing
> the specific command.

Hmm. I like simplifying things, but what's the plan for when we have a
v2 receive-pack? We'll need different config based on whether we're
serving a fetch or a push.

While working on e20b4192a37, I had originally proposed patches that
would pass that context into the serve mechanism (based on whether we
were called by upload-pack or receive-pack). But the review there
indicated that v2 receive-pack would probably involve an extra
capability argument passed by the client to ls-refs.

So we'd eventually need to parse config _after_ we've received the
actual ls-refs command. To be clear, this patch isn't breaking anything
now. But I think it's painting us into a corner that we'll regret later.

Of course there are ways around this. E.g., we could read config for
both operations ahead of time, and then decide which to use later. But
that definitely isn't how it works now, and we'd have to refactor the
whole ref_is_hidden() system to fix that. Definitely do-able, but given
the relatively limited upside of this patch, I'm not sure if it's worth
it.

I'd also note that this patch doesn't touch upload_pack_v2() at all,
which also reads config. That one is extra-weird because it reads the
config fresh for every round of interaction with the client, and then
cleans it up. Which led to fixes like the one in 08450ef791
(upload-pack: clear filter_options for each v2 fetch command,
2020-05-08).

Doing a single config read would probably make that simpler. But the
config setup is shared between the v0 and v2 functions, so extracting it
into serve() would potentially be awkward.

> In particular the ensure_config_read() function being gone
> makes this whole thing much simpler.

I do agree this is awkward. The two calls for that function seem to come
from the "should we advertise ls-refs" and "actually do ls-refs"
functions. Should we be able to assume that the former ran if we are in
the latter? I'm not sure if that's true or not (i.e., because it's
rare but possible for a client to make two separate HTTP requests, one
to discover our v2 capabilities and another to actually issue ls-refs).

-Peff
diff mbox series

Patch

diff --git a/ls-refs.c b/ls-refs.c
index 88f6c3f60d8..a8a3e90bd71 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -7,38 +7,9 @@ 
 #include "pkt-line.h"
 #include "config.h"
 
-static int config_read;
-static int advertise_unborn;
-static int allow_unborn;
-
-static void ensure_config_read(void)
-{
-	const char *str = NULL;
-
-	if (config_read)
-		return;
-
-	if (repo_config_get_string_tmp(the_repository, "lsrefs.unborn", &str)) {
-		/*
-		 * If there is no such config, advertise and allow it by
-		 * default.
-		 */
-		advertise_unborn = 1;
-		allow_unborn = 1;
-	} else {
-		if (!strcmp(str, "advertise")) {
-			advertise_unborn = 1;
-			allow_unborn = 1;
-		} else if (!strcmp(str, "allow")) {
-			allow_unborn = 1;
-		} else if (!strcmp(str, "ignore")) {
-			/* do nothing */
-		} else {
-			die(_("invalid value '%s' for lsrefs.unborn"), str);
-		}
-	}
-	config_read = 1;
-}
+/* "unborn" is on by default if there's no lsrefs.unborn config */
+static int advertise_unborn = 1;
+static int allow_unborn = 1;
 
 /*
  * Check if one of the prefixes is a prefix of the ref.
@@ -128,8 +99,22 @@  static void send_possibly_unborn_head(struct ls_refs_data *data)
 	strbuf_release(&namespaced);
 }
 
-static int ls_refs_config(const char *var, const char *value, void *data)
+int ls_refs_configure(const char *var, const char *value, void *data)
 {
+	if (!strcmp(var, "lsrefs.unborn")) {
+		if (!strcmp(value, "advertise")) {
+			/* Allowed and advertised by default */
+		} else if (!strcmp(value, "allow")) {
+			advertise_unborn = 0;
+			allow_unborn = 1;
+		} else if (!strcmp(value, "ignore")) {
+			advertise_unborn = 0;
+			allow_unborn = 0;
+		} else {
+			die(_("invalid value '%s' for lsrefs.unborn"), value);
+		}
+	}
+
 	/*
 	 * We only serve fetches over v2 for now, so respect only "uploadpack"
 	 * config. This may need to eventually be expanded to "receive", but we
@@ -146,9 +131,6 @@  int ls_refs(struct repository *r, struct strvec *keys,
 	memset(&data, 0, sizeof(data));
 	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;
 		const char *out;
@@ -179,7 +161,6 @@  int ls_refs(struct repository *r, struct strvec *keys,
 int ls_refs_advertise(struct repository *r, struct strbuf *value)
 {
 	if (value) {
-		ensure_config_read();
 		if (advertise_unborn)
 			strbuf_addstr(value, "unborn");
 	}
diff --git a/ls-refs.h b/ls-refs.h
index a99e4be0bdd..afa6dfeb259 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -6,6 +6,7 @@  struct strvec;
 struct packet_reader;
 int ls_refs(struct repository *r, struct strvec *keys,
 	    struct packet_reader *request);
+int ls_refs_configure(const char *var, const char *value, void *data);
 int ls_refs_advertise(struct repository *r, struct strbuf *value);
 
 #endif /* LS_REFS_H */
diff --git a/serve.c b/serve.c
index 49ea9fc8fd5..62cb8aeacdb 100644
--- a/serve.c
+++ b/serve.c
@@ -33,6 +33,13 @@  static int object_format_advertise(struct repository *r,
 	return 1;
 }
 
+static int session_id_configure(const char *var, const char *value, void *data)
+{
+	if (!strcmp(var, "transfer.advertisesid"))
+		advertise_sid = git_config_bool(var, value);
+	return 0;
+}
+
 static int session_id_advertise(struct repository *r, struct strbuf *value)
 {
 	if (!advertise_sid)
@@ -50,6 +57,13 @@  struct protocol_capability {
 	 */
 	const char *name;
 
+	/*
+	 * A git_config() callback. If defined it'll be dispatched too
+	 * sometime before the other functions are called, giving the
+	 * capability a chance to configure itself.
+	 */
+	int (*configure)(const char *var, const char *value, void *data);
+
 	/*
 	 * Function queried to see if a capability should be advertised.
 	 * Optionally a value can be specified by adding it to 'value'.
@@ -79,6 +93,7 @@  static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "ls-refs",
+		.configure = ls_refs_configure,
 		.advertise = ls_refs_advertise,
 		.command = ls_refs,
 	},
@@ -97,6 +112,7 @@  static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "session-id",
+		.configure = session_id_configure,
 		.advertise = session_id_advertise,
 	},
 	{
@@ -106,6 +122,22 @@  static struct protocol_capability capabilities[] = {
 	},
 };
 
+static int git_serve_config(const char *var, const char *value, void *data)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
+		struct protocol_capability *c = &capabilities[i];
+		config_fn_t fn = c->configure;
+		int ret;
+		if (!fn)
+			continue;
+		ret = fn(var, value, data);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static void advertise_capabilities(void)
 {
 	struct strbuf capability = STRBUF_INIT;
@@ -303,7 +335,7 @@  static int process_request(void)
 /* Main serve loop for protocol version 2 */
 void serve(struct serve_options *options)
 {
-	git_config_get_bool("transfer.advertisesid", &advertise_sid);
+	git_config(git_serve_config, NULL);
 
 	if (options->advertise_capabilities || !options->stateless_rpc) {
 		/* serve by default supports v2 */