[PATCH 0/5] serve: add "configure" callback

This is a refactoring of what a callback in serve.c needs to do to
aquire config. Currently two of them want that, and grab it in ad-hoc
ways themselves, now they can insted configure a "configure" callback
along with the existing "advertise" and "command", by the time they're
called their config will already be read with their callback.

I split this prep work off from an upcoming series where I wanted to
add a new capability, but I think this stands nicely on its own, and
simplifies the existing code.

The line count increase is mostly converting things to designated
initializers.

Ævar Arnfjörð Bjarmason (5):
  serve: mark has_capability() as static
  transport: rename "fetch" in transport_vtable to "fetch_refs"
  transport: use designated initializers
  serve: use designated initializers
  serve: add support for a git_config() callback

 ls-refs.c            | 55 +++++++++++---------------------
 ls-refs.h            |  1 +
 serve.c              | 76 ++++++++++++++++++++++++++++++++++++++------
 serve.h              |  3 --
 transport-helper.c   | 18 +++++------
 transport-internal.h |  2 +-
 transport.c          | 32 ++++++++-----------
 7 files changed, 108 insertions(+), 79 deletions(-)

-- 
2.32.0.576.g59759b6ca7d

The has_capability() function introduced in ed10cb952d3 (serve:
introduce git-serve, 2018-03-15) has never been used anywhere except
serve.c, so let's mark it as static.

It was later changed from "extern" in 554544276a6 (*.[ch]: remove
extern from function declarations using spatch, 2019-04-29), but we
could have simply marked it as "static" instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c | 4 ++--
 serve.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/serve.c b/serve.c
index aa8209f147e..6748c590b74 100644
--- a/serve.c
+++ b/serve.c

@@ -156,8 +156,8 @@ static int is_command(const char *key, struct protocol_capability **command)
 	return 0;
 }
 
-int has_capability(const struct strvec *keys, const char *capability,
-		   const char **value)
+static int has_capability(const struct strvec *keys, const char *capability,
+			  const char **value)
 {
 	int i;
 	for (i = 0; i < keys->nr; i++) {

diff --git a/serve.h b/serve.h
index fc2683e24d3..b31dd69434b 100644
--- a/serve.h
+++ b/serve.h

@@ -2,9 +2,6 @@
 #define SERVE_H
 
 struct strvec;
-int has_capability(const struct strvec *keys, const char *capability,
-		   const char **value);
-
 struct serve_options {
 	unsigned advertise_capabilities;
 	unsigned stateless_rpc;

-- 
2.32.0.576.g59759b6ca7d

On Wed, Jun 16, 2021 at 10:16 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

`strvec` isn't used anywhere in this header following removal of
has_capability(), so the forward declaration could be dropped, as
well.

Rename the "fetch" member of the transport_vtable to "fetch_refs" for
consistency with the existing "push_refs". Neither of them just push
"refs" but refs and objects, but having the two match makes the code
more readable than having it be inconsistent, especially since
"fetch_refs" is a lot easier to grep for than "fetch".

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 transport-helper.c   | 8 ++++----
 transport-internal.h | 2 +-
 transport.c          | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 4be035edb8b..8d445a8f3ee 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -671,8 +671,8 @@ static int connect_helper(struct transport *transport, const char *name,
 static struct ref *get_refs_list_using_list(struct transport *transport,
 					    int for_push);
 
-static int fetch(struct transport *transport,
-		 int nr_heads, struct ref **to_fetch)
+static int fetch_refs(struct transport *transport,
+		      int nr_heads, struct ref **to_fetch)
 {
 	struct helper_data *data = transport->data;
 	int i, count;

@@ -681,7 +681,7 @@ static int fetch(struct transport *transport,
 
 	if (process_connect(transport, 0)) {
 		do_take_over(transport);
-		return transport->vtable->fetch(transport, nr_heads, to_fetch);
+		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
 	}
 
 	/*

@@ -1263,7 +1263,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 static struct transport_vtable vtable = {
 	set_helper_option,
 	get_refs_list,
-	fetch,
+	fetch_refs,
 	push_refs,
 	connect_helper,
 	release_helper

diff --git a/transport-internal.h b/transport-internal.h
index b60f1ba9077..c4ca0b733ac 100644
--- a/transport-internal.h
+++ b/transport-internal.h

@@ -34,7 +34,7 @@ struct transport_vtable {
 	 * get_refs_list(), it should set the old_sha1 fields in the
 	 * provided refs now.
 	 **/
-	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
+	int (*fetch_refs)(struct transport *transport, int refs_nr, struct ref **refs);
 
 	/**
 	 * Push the objects and refs. Send the necessary objects, and

diff --git a/transport.c b/transport.c
index 50f5830eb6b..f60985e2dbd 100644
--- a/transport.c
+++ b/transport.c

@@ -1449,7 +1449,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 			heads[nr_heads++] = rm;
 	}
 
-	rc = transport->vtable->fetch(transport, nr_heads, heads);
+	rc = transport->vtable->fetch_refs(transport, nr_heads, heads);
 
 	free(heads);
 	return rc;

-- 
2.32.0.576.g59759b6ca7d

Change the assignments to the various transport_vtables to use
designated initializers, this makes the code easier to read and
maintain.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 transport-helper.c | 12 ++++++------
 transport.c        | 30 ++++++++++++------------------
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 8d445a8f3ee..e8dbdd11530 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -1261,12 +1261,12 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 }
 
 static struct transport_vtable vtable = {
-	set_helper_option,
-	get_refs_list,
-	fetch_refs,
-	push_refs,
-	connect_helper,
-	release_helper
+	.set_option	= set_helper_option,
+	.get_refs_list	= get_refs_list,
+	.fetch_refs	= fetch_refs,
+	.push_refs	= push_refs,
+	.connect	= connect_helper,
+	.disconnect	= release_helper
 };
 
 int transport_helper_init(struct transport *transport, const char *name)

diff --git a/transport.c b/transport.c
index f60985e2dbd..bdd9b5a93cf 100644
--- a/transport.c
+++ b/transport.c

@@ -880,12 +880,10 @@ static int disconnect_git(struct transport *transport)
 }
 
 static struct transport_vtable taken_over_vtable = {
-	NULL,
-	get_refs_via_connect,
-	fetch_refs_via_pack,
-	git_transport_push,
-	NULL,
-	disconnect_git
+	.get_refs_list	= get_refs_via_connect,
+	.fetch_refs	= fetch_refs_via_pack,
+	.push_refs	= git_transport_push,
+	.disconnect	= disconnect_git
 };
 
 void transport_take_over(struct transport *transport,

@@ -1029,21 +1027,17 @@ void transport_check_allowed(const char *type)
 }
 
 static struct transport_vtable bundle_vtable = {
-	NULL,
-	get_refs_from_bundle,
-	fetch_refs_from_bundle,
-	NULL,
-	NULL,
-	close_bundle
+	.get_refs_list	= get_refs_from_bundle,
+	.fetch_refs	= fetch_refs_from_bundle,
+	.disconnect	= close_bundle
 };
 
 static struct transport_vtable builtin_smart_vtable = {
-	NULL,
-	get_refs_via_connect,
-	fetch_refs_via_pack,
-	git_transport_push,
-	connect_git,
-	disconnect_git
+	.get_refs_list	= get_refs_via_connect,
+	.fetch_refs	= fetch_refs_via_pack,
+	.push_refs	= git_transport_push,
+	.connect	= connect_git,
+	.disconnect	= disconnect_git
 };
 
 struct transport *transport_get(struct remote *remote, const char *url)

-- 
2.32.0.576.g59759b6ca7d

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 <redacted>
---
 ls-refs.c | 55 ++++++++++++++++++-------------------------------------
 ls-refs.h |  1 +
 serve.c   | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 52 insertions(+), 38 deletions(-)

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 */

-- 
2.32.0.576.g59759b6ca7d

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

Change the declaration of the protocol_capability struct to use
designated initializers, this makes this more verbose now, but a
follow-up commit will add a new field. At that point these lines would
be too dense to be on one line comfortably.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/serve.c b/serve.c
index 6748c590b74..49ea9fc8fd5 100644
--- a/serve.c
+++ b/serve.c

@@ -73,13 +73,37 @@ struct protocol_capability {
 };
 
 static struct protocol_capability capabilities[] = {
-	{ "agent", agent_advertise, NULL },
-	{ "ls-refs", ls_refs_advertise, ls_refs },
-	{ "fetch", upload_pack_advertise, upload_pack_v2 },
-	{ "server-option", always_advertise, NULL },
-	{ "object-format", object_format_advertise, NULL },
-	{ "session-id", session_id_advertise, NULL },
-	{ "object-info", always_advertise, cap_object_info },
+	{
+		.name = "agent",
+		.advertise = agent_advertise,
+	},
+	{
+		.name = "ls-refs",
+		.advertise = ls_refs_advertise,
+		.command = ls_refs,
+	},
+	{
+		.name = "fetch",
+		.advertise = upload_pack_advertise,
+		.command = upload_pack_v2,
+	},
+	{
+		.name = "server-option",
+		.advertise = always_advertise,
+	},
+	{
+		.name = "object-format",
+		.advertise = object_format_advertise,
+	},
+	{
+		.name = "session-id",
+		.advertise = session_id_advertise,
+	},
+	{
+		.name = "object-info",
+		.advertise = always_advertise,
+		.command = cap_object_info,
+	},
 };
 
 static void advertise_capabilities(void)

-- 
2.32.0.576.g59759b6ca7d

On Wed, Jun 16, 2021 at 04:16:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

This is a refactoring of what a callback in serve.c needs to do to
aquire config. Currently two of them want that, and grab it in ad-hoc
ways themselves, now they can insted configure a "configure" callback
along with the existing "advertise" and "command", by the time they're
called their config will already be read with their callback.

I split this prep work off from an upcoming series where I wanted to
add a new capability, but I think this stands nicely on its own, and
simplifies the existing code.

The first four seem like obviously-good cleanups. I'm not so sure
about the last one, though. I left some comments there.

-Peff

As Jeff pointed out on v1 the control flow around serve.c was more
subtle than I expected, i.e. we might process N requests:
http://lore.kernel.org/git/YMolS8iJAMgbbg9D@coredump.intra.peff.net

This v2 has a 1-4/8 that's a trivially improved version of what Junio
picked up for "seen", the only change is addressing Eric's feedback
that the "struct strvec;" could also be removed.

Then 5-6/8 is a new trivial refactoring of the various callback
invocations into a rapper function, allowing us to add a trace2 region
for the advertisements & command we run.

After that 7-8/8 is a new approach of doing this "configure" callback,
I was on the fence about whether it should be another series on top,
but decided to submit it as one thing.

Now instead of there being a "configure" we explicitly have a
"startup_config" where we expect to stick all our config that doesn't
change, won't be different depending on what "mode" or "command" we
operate as, and in the case of upload-pack doesn't depend on anything
in "upload_pack_data".

As the diffstat shows it doesn't make the code shorter, but I think it
makes it easier to read and maintain, as we now clearly separate the
types of config we're reading.

Ævar Arnfjörð Bjarmason (8):
  serve: mark has_capability() as static
  transport: rename "fetch" in transport_vtable to "fetch_refs"
  transport: use designated initializers
  serve: use designated initializers
  serve.c: add call_{advertise,command}() indirection
  serve.c: add trace2 regions for advertise & command
  serve: add support for a "startup" git_config() callback
  upload-pack.c: convert to new serve.c "startup" config cb

 ls-refs.c                             |  42 +++-----
 ls-refs.h                             |   1 +
 serve.c                               | 138 +++++++++++++++++++++----
 serve.h                               |   4 -
 t/t5705-session-id-in-capabilities.sh |  16 ++-
 transport-helper.c                    |  18 ++--
 transport-internal.h                  |   2 +-
 transport.c                           |  32 +++---
 upload-pack.c                         | 139 +++++++++++++++-----------
 upload-pack.h                         |   2 +
 10 files changed, 255 insertions(+), 139 deletions(-)

Range-diff against v1:
1:  4f74d7d34c4 ! 1:  fdb0c5f4df1 serve: mark has_capability() as static
    @@ serve.c: static int is_command(const char *key, struct protocol_capability **com
     
      ## serve.h ##
     @@
    + #ifndef SERVE_H
      #define SERVE_H
      
    - struct strvec;
    +-struct strvec;
     -int has_capability(const struct strvec *keys, const char *capability,
     -		   const char **value);
     -
2:  c6720d1bf33 = 2:  7b8101dca71 transport: rename "fetch" in transport_vtable to "fetch_refs"
3:  369efe0eed5 = 3:  766045e5f1d transport: use designated initializers
4:  8e97412d584 = 4:  bd920de1629 serve: use designated initializers
-:  ----------- > 5:  d0b02aa0c7d serve.c: add call_{advertise,command}() indirection
-:  ----------- > 6:  baeee6539ad serve.c: add trace2 regions for advertise & command
5:  c8625025125 ! 7:  0a4fb01ae38 serve: add support for a git_config() callback
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## Commit message ##
    -    serve: add support for a git_config() callback
    +    serve: add support for a "startup" git_config() callback
     
         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)
    +    - 08450ef7918 (upload-pack: clear filter_options for each v2 fetch command, 2020-05-08)
         - 6b5b6e422ee (serve: advertise session ID in v2 capabilities, 2020-11-11)
    +    - 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05)
     
    -    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.
    +    Of these 08450ef7918 fixed code that needed to read config on a
    +    per-request basis, whereas most of the config reading just wants to
    +    check if we've enabled one semi-static config variable or other. We'd
    +    like to re-read that value eventually, but from request-to-request
    +    it's OK if we retain the old one, and it isn't impacted by other
    +    request data.
     
    -    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.
    +    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 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.
    +    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.
    +
    +    I'm not migrating over the code in the upload_pack_v2 function in
    +    upload-pack.c yet, that case is more complex since it deals with both
    +    v1 and v2. It will be dealt with in a code a subsequent commit.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
    @@ ls-refs.c
     -static int config_read;
     -static int advertise_unborn;
     -static int allow_unborn;
    --
    ++/* "unborn" is on by default if there's no lsrefs.unborn config */
    ++static int advertise_unborn = 1;
    ++static int allow_unborn = 1;
    + 
     -static void ensure_config_read(void)
    --{
    ++int ls_refs_startup_config(const char *var, const char *value, void *data)
    + {
     -	const char *str = NULL;
     -
     -	if (config_read)
    @@ ls-refs.c
     -	} 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.
    -@@ ls-refs.c: 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;
    + 			allow_unborn = 1;
    +-		} else if (!strcmp(str, "allow")) {
    +-			allow_unborn = 1;
    +-		} else if (!strcmp(str, "ignore")) {
    +-			/* do nothing */
     +		} else if (!strcmp(value, "ignore")) {
     +			advertise_unborn = 0;
     +			allow_unborn = 0;
    -+		} else {
    + 		} else {
    +-			die(_("invalid value '%s' for lsrefs.unborn"), str);
     +			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
    + 		}
    + 	}
    +-	config_read = 1;
    ++	return 0;
    + }
    + 
    + /*
     @@ ls-refs.c: 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);
     -
    +-	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;
     @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys,
      int ls_refs_advertise(struct repository *r, struct strbuf *value)
      {
    @@ ls-refs.h: 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_startup_config(const char *var, const char *value, void *data);
      int ls_refs_advertise(struct repository *r, struct strbuf *value);
      
      #endif /* LS_REFS_H */
    @@ serve.c: static int object_format_advertise(struct repository *r,
      	return 1;
      }
      
    -+static int session_id_configure(const char *var, const char *value, void *data)
    ++static int session_id_startup_config(const char *var, const char *value, void *data)
     +{
     +	if (!strcmp(var, "transfer.advertisesid"))
     +		advertise_sid = git_config_bool(var, value);
    @@ serve.c: 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.
    ++	 * A git_config() callback that'll be called only once for the
    ++	 * lifetime of the process, possibly over many different
    ++	 * requests. Used for reading config that's expected to be
    ++	 * static.
    ++	 *
    ++	 * The "command" or "advertise" callbacks themselves are
    ++	 * expected to read config that needs to be more current than
    ++	 * that, or which is dependent on request data.
     +	 */
    -+	int (*configure)(const char *var, const char *value, void *data);
    ++	int (*startup_config)(const char *var, const char *value, void *data);
    ++
    ++	/*
    ++	 * A boolean to check if we've called our "startup_config"
    ++	 * callback.
    ++	 */
    ++	int have_startup_config;
     +
      	/*
      	 * Function queried to see if a capability should be advertised.
    @@ serve.c: static struct protocol_capability capabilities[] = {
      	},
      	{
      		.name = "ls-refs",
    -+		.configure = ls_refs_configure,
    ++		.startup_config = ls_refs_startup_config,
      		.advertise = ls_refs_advertise,
      		.command = ls_refs,
      	},
    @@ serve.c: static struct protocol_capability capabilities[] = {
      	},
      	{
      		.name = "session-id",
    -+		.configure = session_id_configure,
    ++		.startup_config = session_id_startup_config,
      		.advertise = session_id_advertise,
      	},
      	{
    @@ serve.c: static struct protocol_capability capabilities[] = {
      	},
      };
      
    -+static int git_serve_config(const char *var, const char *value, void *data)
    ++static void read_startup_config(struct protocol_capability *command)
     +{
    -+	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;
    ++	if (!command->startup_config)
    ++		return;
    ++	if (command->have_startup_config++)
    ++		return;
    ++	git_config(command->startup_config, NULL);
     +}
     +
    - static void advertise_capabilities(void)
    + static int call_advertise(struct protocol_capability *command,
    + 			  struct repository *r, struct strbuf *value)
      {
    - 	struct strbuf capability = STRBUF_INIT;
    +@@ serve.c: static int call_advertise(struct protocol_capability *command,
    + 	struct strbuf sb = STRBUF_INIT;
    + 	const char *msg;
    + 
    ++	read_startup_config(command);
    ++
    + 	strbuf_addf(&sb, "advertise/%s", command->name);
    + 	trace2_region_enter("serve", sb.buf, r);
    + 	ret = command->advertise(r, value);
    +@@ serve.c: static int call_command(struct protocol_capability *command,
    + 	int ret;
    + 	struct strbuf sb = STRBUF_INIT;
    + 
    ++	read_startup_config(command);
    ++
    + 	strbuf_addf(&sb, "command/%s", command->name);
    + 	trace2_region_enter("serve", sb.buf, r);
    + 	ret = command->command(r, keys, request);
     @@ serve.c: 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 */
    + 		packet_write_fmt(1, "version 2\n");
-:  ----------- > 8:  9fd6138aa62 upload-pack.c: convert to new serve.c "startup" config cb
-- 
2.32.0.611.gd4a17395dfa

The has_capability() function introduced in ed10cb952d3 (serve:
introduce git-serve, 2018-03-15) has never been used anywhere except
serve.c, so let's mark it as static.

It was later changed from "extern" in 554544276a6 (*.[ch]: remove
extern from function declarations using spatch, 2019-04-29), but we
could have simply marked it as "static" instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c | 4 ++--
 serve.h | 4 ----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/serve.c b/serve.c
index aa8209f147e..6748c590b74 100644
--- a/serve.c
+++ b/serve.c

@@ -156,8 +156,8 @@ static int is_command(const char *key, struct protocol_capability **command)
 	return 0;
 }
 
-int has_capability(const struct strvec *keys, const char *capability,
-		   const char **value)
+static int has_capability(const struct strvec *keys, const char *capability,
+			  const char **value)
 {
 	int i;
 	for (i = 0; i < keys->nr; i++) {

diff --git a/serve.h b/serve.h
index fc2683e24d3..56da77a87af 100644
--- a/serve.h
+++ b/serve.h

@@ -1,10 +1,6 @@
 #ifndef SERVE_H
 #define SERVE_H
 
-struct strvec;
-int has_capability(const struct strvec *keys, const char *capability,
-		   const char **value);
-
 struct serve_options {
 	unsigned advertise_capabilities;
 	unsigned stateless_rpc;

-- 
2.32.0.611.gd4a17395dfa

Change the assignments to the various transport_vtables to use
designated initializers, this makes the code easier to read and
maintain.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 transport-helper.c | 12 ++++++------
 transport.c        | 30 ++++++++++++------------------
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 8d445a8f3ee..e8dbdd11530 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -1261,12 +1261,12 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 }
 
 static struct transport_vtable vtable = {
-	set_helper_option,
-	get_refs_list,
-	fetch_refs,
-	push_refs,
-	connect_helper,
-	release_helper
+	.set_option	= set_helper_option,
+	.get_refs_list	= get_refs_list,
+	.fetch_refs	= fetch_refs,
+	.push_refs	= push_refs,
+	.connect	= connect_helper,
+	.disconnect	= release_helper
 };
 
 int transport_helper_init(struct transport *transport, const char *name)

diff --git a/transport.c b/transport.c
index f60985e2dbd..bdd9b5a93cf 100644
--- a/transport.c
+++ b/transport.c

@@ -880,12 +880,10 @@ static int disconnect_git(struct transport *transport)
 }
 
 static struct transport_vtable taken_over_vtable = {
-	NULL,
-	get_refs_via_connect,
-	fetch_refs_via_pack,
-	git_transport_push,
-	NULL,
-	disconnect_git
+	.get_refs_list	= get_refs_via_connect,
+	.fetch_refs	= fetch_refs_via_pack,
+	.push_refs	= git_transport_push,
+	.disconnect	= disconnect_git
 };
 
 void transport_take_over(struct transport *transport,

@@ -1029,21 +1027,17 @@ void transport_check_allowed(const char *type)
 }
 
 static struct transport_vtable bundle_vtable = {
-	NULL,
-	get_refs_from_bundle,
-	fetch_refs_from_bundle,
-	NULL,
-	NULL,
-	close_bundle
+	.get_refs_list	= get_refs_from_bundle,
+	.fetch_refs	= fetch_refs_from_bundle,
+	.disconnect	= close_bundle
 };
 
 static struct transport_vtable builtin_smart_vtable = {
-	NULL,
-	get_refs_via_connect,
-	fetch_refs_via_pack,
-	git_transport_push,
-	connect_git,
-	disconnect_git
+	.get_refs_list	= get_refs_via_connect,
+	.fetch_refs	= fetch_refs_via_pack,
+	.push_refs	= git_transport_push,
+	.connect	= connect_git,
+	.disconnect	= disconnect_git
 };
 
 struct transport *transport_get(struct remote *remote, const char *url)

-- 
2.32.0.611.gd4a17395dfa

Rename the "fetch" member of the transport_vtable to "fetch_refs" for
consistency with the existing "push_refs". Neither of them just push
"refs" but refs and objects, but having the two match makes the code
more readable than having it be inconsistent, especially since
"fetch_refs" is a lot easier to grep for than "fetch".

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 transport-helper.c   | 8 ++++----
 transport-internal.h | 2 +-
 transport.c          | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 4be035edb8b..8d445a8f3ee 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -671,8 +671,8 @@ static int connect_helper(struct transport *transport, const char *name,
 static struct ref *get_refs_list_using_list(struct transport *transport,
 					    int for_push);
 
-static int fetch(struct transport *transport,
-		 int nr_heads, struct ref **to_fetch)
+static int fetch_refs(struct transport *transport,
+		      int nr_heads, struct ref **to_fetch)
 {
 	struct helper_data *data = transport->data;
 	int i, count;

@@ -681,7 +681,7 @@ static int fetch(struct transport *transport,
 
 	if (process_connect(transport, 0)) {
 		do_take_over(transport);
-		return transport->vtable->fetch(transport, nr_heads, to_fetch);
+		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
 	}
 
 	/*

@@ -1263,7 +1263,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
 static struct transport_vtable vtable = {
 	set_helper_option,
 	get_refs_list,
-	fetch,
+	fetch_refs,
 	push_refs,
 	connect_helper,
 	release_helper

diff --git a/transport-internal.h b/transport-internal.h
index b60f1ba9077..c4ca0b733ac 100644
--- a/transport-internal.h
+++ b/transport-internal.h

@@ -34,7 +34,7 @@ struct transport_vtable {
 	 * get_refs_list(), it should set the old_sha1 fields in the
 	 * provided refs now.
 	 **/
-	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
+	int (*fetch_refs)(struct transport *transport, int refs_nr, struct ref **refs);
 
 	/**
 	 * Push the objects and refs. Send the necessary objects, and

diff --git a/transport.c b/transport.c
index 50f5830eb6b..f60985e2dbd 100644
--- a/transport.c
+++ b/transport.c

@@ -1449,7 +1449,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
 			heads[nr_heads++] = rm;
 	}
 
-	rc = transport->vtable->fetch(transport, nr_heads, heads);
+	rc = transport->vtable->fetch_refs(transport, nr_heads, heads);
 
 	free(heads);
 	return rc;

-- 
2.32.0.611.gd4a17395dfa

Instead of directly calling the callbacks defined in the "struct
protocol_capability", let's call them via new
"call_{advertise,command}()" functions. There's no use in this
indirection now, but it'll be used in a subsequent commit to ensure
that config is read before they're called.

See ed10cb952d3 (serve: introduce git-serve, 2018-03-15) for the
introduction of the code being changed here.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/serve.c b/serve.c
index 49ea9fc8fd5..85cd3eab26e 100644
--- a/serve.c
+++ b/serve.c

@@ -42,6 +42,10 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)
 	return 1;
 }
 
+typedef int (*advertise_fn_t)(struct repository *r, struct strbuf *value);
+typedef int (*command_fn_t)(struct repository *r, struct strvec *keys,
+			    struct packet_reader *request);
+
 struct protocol_capability {
 	/*
 	 * The name of the capability.  The server uses this name when

@@ -56,7 +60,7 @@ struct protocol_capability {
 	 * If a value is added to 'value', the server will advertise this
 	 * capability as "<name>=<value>" instead of "<name>".
 	 */
-	int (*advertise)(struct repository *r, struct strbuf *value);
+	advertise_fn_t advertise;
 
 	/*
 	 * Function called when a client requests the capability as a command.

@@ -67,9 +71,7 @@ struct protocol_capability {
 	 *
 	 * This field should be NULL for capabilities which are not commands.
 	 */
-	int (*command)(struct repository *r,
-		       struct strvec *keys,
-		       struct packet_reader *request);
+	command_fn_t command;
 };
 
 static struct protocol_capability capabilities[] = {

@@ -106,6 +108,19 @@ static struct protocol_capability capabilities[] = {
 	},
 };
 
+static int call_advertise(struct protocol_capability *command,
+			  struct repository *r, struct strbuf *value)
+{
+	return command->advertise(r, value);
+}
+
+static int call_command(struct protocol_capability *command,
+			struct repository *r, struct strvec *keys,
+			struct packet_reader *request)
+{
+	return command->command(r, keys, request);
+}
+
 static void advertise_capabilities(void)
 {
 	struct strbuf capability = STRBUF_INIT;

@@ -115,7 +130,7 @@ static void advertise_capabilities(void)
 	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
 		struct protocol_capability *c = &capabilities[i];
 
-		if (c->advertise(the_repository, &value)) {
+		if (call_advertise(c, the_repository, &value)) {
 			strbuf_addstr(&capability, c->name);
 
 			if (value.len) {

@@ -155,9 +170,9 @@ static struct protocol_capability *get_capability(const char *key)
 
 static int is_valid_capability(const char *key)
 {
-	const struct protocol_capability *c = get_capability(key);
+	struct protocol_capability *c = get_capability(key);
 
-	return c && c->advertise(the_repository, NULL);
+	return c && call_advertise(c, the_repository, NULL);
 }
 
 static int is_command(const char *key, struct protocol_capability **command)

@@ -170,7 +185,7 @@ static int is_command(const char *key, struct protocol_capability **command)
 		if (*command)
 			die("command '%s' requested after already requesting command '%s'",
 			    out, (*command)->name);
-		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)
+		if (!cmd || !call_advertise(cmd, the_repository, NULL) || !cmd->command)
 			die("invalid command '%s'", out);
 
 		*command = cmd;

@@ -294,7 +309,7 @@ static int process_request(void)
 	if (has_capability(&keys, "session-id", &client_sid))
 		trace2_data_string("transfer", NULL, "client-sid", client_sid);
 
-	command->command(the_repository, &keys, &reader);
+	call_command(command, the_repository, &keys, &reader);
 
 	strvec_clear(&keys);
 	return 0;

-- 
2.32.0.611.gd4a17395dfa

Change the declaration of the protocol_capability struct to use
designated initializers, this makes this more verbose now, but a
follow-up commit will add a new field. At that point these lines would
be too dense to be on one line comfortably.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/serve.c b/serve.c
index 6748c590b74..49ea9fc8fd5 100644
--- a/serve.c
+++ b/serve.c

@@ -73,13 +73,37 @@ struct protocol_capability {
 };
 
 static struct protocol_capability capabilities[] = {
-	{ "agent", agent_advertise, NULL },
-	{ "ls-refs", ls_refs_advertise, ls_refs },
-	{ "fetch", upload_pack_advertise, upload_pack_v2 },
-	{ "server-option", always_advertise, NULL },
-	{ "object-format", object_format_advertise, NULL },
-	{ "session-id", session_id_advertise, NULL },
-	{ "object-info", always_advertise, cap_object_info },
+	{
+		.name = "agent",
+		.advertise = agent_advertise,
+	},
+	{
+		.name = "ls-refs",
+		.advertise = ls_refs_advertise,
+		.command = ls_refs,
+	},
+	{
+		.name = "fetch",
+		.advertise = upload_pack_advertise,
+		.command = upload_pack_v2,
+	},
+	{
+		.name = "server-option",
+		.advertise = always_advertise,
+	},
+	{
+		.name = "object-format",
+		.advertise = object_format_advertise,
+	},
+	{
+		.name = "session-id",
+		.advertise = session_id_advertise,
+	},
+	{
+		.name = "object-info",
+		.advertise = always_advertise,
+		.command = cap_object_info,
+	},
 };
 
 static void advertise_capabilities(void)

-- 
2.32.0.611.gd4a17395dfa

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)
- 08450ef7918 (upload-pack: clear filter_options for each v2 fetch command, 2020-05-08)
- 6b5b6e422ee (serve: advertise session ID in v2 capabilities, 2020-11-11)
- 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05)

Of these 08450ef7918 fixed code that needed to read config on a
per-request basis, whereas most of the config reading just wants to
check if we've enabled one semi-static config variable or other. We'd
like to re-read that value eventually, but from request-to-request
it's OK if we retain the old one, and it isn't impacted by other
request data.

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.

I'm not migrating over the code in the upload_pack_v2 function in
upload-pack.c yet, that case is more complex since it deals with both
v1 and v2. It will be dealt with in a code a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 ls-refs.c | 42 ++++++++++++++----------------------------
 ls-refs.h |  1 +
 serve.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/ls-refs.c b/ls-refs.c
index 88f6c3f60d8..02fbcfd9bde 100644
--- a/ls-refs.c
+++ b/ls-refs.c

@@ -7,37 +7,26 @@
 #include "pkt-line.h"
 #include "config.h"
 
-static int config_read;
-static int advertise_unborn;
-static int allow_unborn;
+/* "unborn" is on by default if there's no lsrefs.unborn config */
+static int advertise_unborn = 1;
+static int allow_unborn = 1;
 
-static void ensure_config_read(void)
+int ls_refs_startup_config(const char *var, const char *value, void *data)
 {
-	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;
+	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(str, "allow")) {
-			allow_unborn = 1;
-		} else if (!strcmp(str, "ignore")) {
-			/* do nothing */
+		} else if (!strcmp(value, "ignore")) {
+			advertise_unborn = 0;
+			allow_unborn = 0;
 		} else {
-			die(_("invalid value '%s' for lsrefs.unborn"), str);
+			die(_("invalid value '%s' for lsrefs.unborn"), value);
 		}
 	}
-	config_read = 1;
+	return 0;
 }
 
 /*

@@ -145,8 +134,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) {

@@ -179,7 +166,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..5fcab0d1dca 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_startup_config(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 6dbd05248b9..cce96dd5a81 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_startup_config(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)

@@ -54,6 +61,24 @@ struct protocol_capability {
 	 */
 	const char *name;
 
+	/*
+	 * A git_config() callback that'll be called only once for the
+	 * lifetime of the process, possibly over many different
+	 * requests. Used for reading config that's expected to be
+	 * static.
+	 *
+	 * The "command" or "advertise" callbacks themselves are
+	 * expected to read config that needs to be more current than
+	 * that, or which is dependent on request data.
+	 */
+	int (*startup_config)(const char *var, const char *value, void *data);
+
+	/*
+	 * A boolean to check if we've called our "startup_config"
+	 * callback.
+	 */
+	int have_startup_config;
+
 	/*
 	 * Function queried to see if a capability should be advertised.
 	 * Optionally a value can be specified by adding it to 'value'.

@@ -81,6 +106,7 @@ static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "ls-refs",
+		.startup_config = ls_refs_startup_config,
 		.advertise = ls_refs_advertise,
 		.command = ls_refs,
 	},

@@ -99,6 +125,7 @@ static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "session-id",
+		.startup_config = session_id_startup_config,
 		.advertise = session_id_advertise,
 	},
 	{

@@ -108,6 +135,15 @@ static struct protocol_capability capabilities[] = {
 	},
 };
 
+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);
+}
+
 static int call_advertise(struct protocol_capability *command,
 			  struct repository *r, struct strbuf *value)
 {

@@ -115,6 +151,8 @@ static int call_advertise(struct protocol_capability *command,
 	struct strbuf sb = STRBUF_INIT;
 	const char *msg;
 
+	read_startup_config(command);
+
 	strbuf_addf(&sb, "advertise/%s", command->name);
 	trace2_region_enter("serve", sb.buf, r);
 	ret = command->advertise(r, value);

@@ -132,6 +170,8 @@ static int call_command(struct protocol_capability *command,
 	int ret;
 	struct strbuf sb = STRBUF_INIT;
 
+	read_startup_config(command);
+
 	strbuf_addf(&sb, "command/%s", command->name);
 	trace2_region_enter("serve", sb.buf, r);
 	ret = command->command(r, keys, request);

@@ -338,8 +378,6 @@ 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);
-
 	if (options->advertise_capabilities || !options->stateless_rpc) {
 		/* serve by default supports v2 */
 		packet_write_fmt(1, "version 2\n");

-- 
2.32.0.611.gd4a17395dfa

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:

+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

On Thu, Jul 01, 2021 at 12:43:43PM -0400, Jeff King wrote:

I dunno. Maybe the solution is for ls_refs() to just do a separate
config call to pick up the operation-specific bits, like:

By the way, I think both currently and after the patch I showed,
ls_refs() has the same "bug" that we fixed for upload_pack_v2() a while
ago: in a v2 world, a client could request "ls-refs" over and over, and
each time we'd load the hiderefs config, appending duplicate config to
the list each time.

In practice this doesn't happen because unlike "fetch", which clients
must do many rounds of, clients usually issue only a single ls-refs. So
it may not be worth worrying too much about. I guess a malicious client
could convince us to very slowly allocate an arbitrary amount of memory.

-Peff

On Thu, Jul 01 2021, Jeff King wrote:

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

quoted

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)

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

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

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

So let's support this common pattern as a "startup_config" callback,
making use of our recently added "call_{advertise,command}()"

I think this is an improvement over ensure_config_read(), but I don't
understand the whole picture. If you want to control configuration and
be sure it is set early, isn't it more natural to put this in struct
serve_options? With your patch, there are still two routes to
injecting configuration: 'struct serve_options' and the git config.

IMO the former is more principled, and the control flow (serve_options
is an argument to serve()) ensures that the options are read only
once.

+       /*
+        * A git_config() callback that'll be called only once for the
+        * lifetime of the process, possibly over many different

Putting it in a struct supplied by the caller provides more
flexibility for testing: you could test multiple behaviors from a
single process, by calling serve multiple times supplying different
arguments.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

 /* Main serve loop for protocol version 2 */
 void serve(struct serve_options *options)

side note: this is a very short name for a globally visible symbol.
Would be nice if this were something like protocol_v2_serve_loop() .
Then one could drop the comment.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Now that we've factored out "call_{advertise,command}()" in a
preceding commit it becomes easy to trace all these callbacks with
trace2. Let's do that. As the tests demonstrate there's no v2 push
protocol, which the tests assert.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 serve.c                               | 24 ++++++++++++++++++++++--
 t/t5705-session-id-in-capabilities.sh | 16 ++++++++++++++--
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/serve.c b/serve.c
index 85cd3eab26e..6dbd05248b9 100644
--- a/serve.c
+++ b/serve.c

@@ -111,14 +111,34 @@ static struct protocol_capability capabilities[] = {
 static int call_advertise(struct protocol_capability *command,
 			  struct repository *r, struct strbuf *value)
 {
-	return command->advertise(r, value);
+	int ret;
+	struct strbuf sb = STRBUF_INIT;
+	const char *msg;
+
+	strbuf_addf(&sb, "advertise/%s", command->name);
+	trace2_region_enter("serve", sb.buf, r);
+	ret = command->advertise(r, value);
+	msg = ret ? "advertised" : "hidden";
+	trace2_region_leave_printf("serve", sb.buf, r, "%s", msg);
+	strbuf_release(&sb);
+
+	return ret;
 }
 
 static int call_command(struct protocol_capability *command,
 			struct repository *r, struct strvec *keys,
 			struct packet_reader *request)
 {
-	return command->command(r, keys, request);
+	int ret;
+	struct strbuf sb = STRBUF_INIT;
+
+	strbuf_addf(&sb, "command/%s", command->name);
+	trace2_region_enter("serve", sb.buf, r);
+	ret = command->command(r, keys, request);
+	trace2_region_leave("serve", sb.buf, r);
+	strbuf_release(&sb);
+
+	return ret;
 }
 
 static void advertise_capabilities(void)