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

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

[Ævar Arnfjörð Bjarmason]

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

[PATCH 1/5] serve: mark has_capability() as static

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

Re: [PATCH 1/5] serve: mark has_capability() as static

[Eric Sunshine]

On Wed, Jun 16, 2021 at 10:16 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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 <avarab@gmail.com>
> ---
> diff --git a/serve.h b/serve.h
> @@ -2,9 +2,6 @@
>  struct strvec;
> -int has_capability(const struct strvec *keys, const char *capability,
> -                  const char **value);
> -

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

Re: [PATCH 1/5] serve: mark has_capability() as static

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jun 16, 2021 at 10:16 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> 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 <avarab@gmail.com>
>> ---
>> diff --git a/serve.h b/serve.h
>> @@ -2,9 +2,6 @@
>>  struct strvec;
>> -int has_capability(const struct strvec *keys, const char *capability,
>> -                  const char **value);
>> -
>
> `strvec` isn't used anywhere in this header following removal of
> has_capability(), so the forward declaration could be dropped, as
> well.

Good eyes.

[PATCH 2/5] transport: rename "fetch" in transport_vtable to "fetch_refs"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH 3/5] transport: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH 4/5] serve: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I had the same impression.  Thanks, both.

[PATCH v2 0/8] serve: add "startup_config" callback

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
    @@ 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

[PATCH v2 1/8] serve: mark has_capability() as static

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH v2 2/8] transport: rename "fetch" in transport_vtable to "fetch_refs"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH v2 3/8] transport: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH v2 4/8] serve: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH v2 5/8] serve.c: add call_{advertise,command}() indirection

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

[PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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)
diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
index f1d189d5bcc..cda78fa7a1d 100755
--- a/t/t5705-session-id-in-capabilities.sh
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -57,7 +57,13 @@ do
 			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
 			origin &&
 		grep \"key\":\"server-sid\" tr2-client-events &&
-		grep \"key\":\"client-sid\" tr2-server-events
+		grep \"key\":\"client-sid\" tr2-server-events &&
+
+		if test "$PROTO" = 2
+		then
+			grep \"event\":\"region_enter\".*\"category\":\"serve\" tr2-server-events &&
+			grep \"event\":\"region_leave\".*\"category\":\"serve\" tr2-server-events
+		fi
 	'
 
 	test_expect_success "session IDs advertised (push v${PROTO})" '
@@ -71,7 +77,13 @@ do
 			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
 			origin HEAD:new-branch &&
 		grep \"key\":\"server-sid\" tr2-client-events &&
-		grep \"key\":\"client-sid\" tr2-server-events
+		grep \"key\":\"client-sid\" tr2-server-events &&
+
+		if test "$PROTO" = 2
+		then
+			! grep \"event\":\"region_enter\".*\"category\":\"serve\" tr2-server-events &&
+			! grep \"event\":\"region_leave\".*\"category\":\"serve\" tr2-server-events
+		fi
 	'
 done
 
-- 
2.32.0.611.gd4a17395dfa

Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

[Jeff King]

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

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

Seems reasonable. I haven't ever wanted these myself, but it seems like
a natural spot to mention when debugging server-side actions (especially
because we may get multiple rounds of "fetch" for a single upload-pack
invocation).

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

We'll do these allocations even if trace2 isn't enabled. I guess that's
probably not that big a deal in practice. I think:

  if (trace2_is_enabled())
	strbuf_addf(&sb, "advertise/%s", command->name);

would work (everything else is cheap and handles the unallocated state
fine), but it might not be worth the readability hit (and it's probably
premature optimization anyway).

-Peff

Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 01 2021, Jeff King wrote:

> On Mon, Jun 28, 2021 at 09:19:23PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
> Seems reasonable. I haven't ever wanted these myself, but it seems like
> a natural spot to mention when debugging server-side actions (especially
> because we may get multiple rounds of "fetch" for a single upload-pack
> invocation).
>
>> 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);
>
> We'll do these allocations even if trace2 isn't enabled. I guess that's
> probably not that big a deal in practice. I think:
>
>   if (trace2_is_enabled())
> 	strbuf_addf(&sb, "advertise/%s", command->name);
>
> would work (everything else is cheap and handles the unallocated state
> fine), but it might not be worth the readability hit (and it's probably
> premature optimization anyway).

Will change it to be conditional, pending further discussion...

Re: [PATCH v2 6/8] serve.c: add trace2 regions for advertise & command

[Han-Wen Nienhuys]

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> 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.

code looks good, but confused about the objective. This isn't
necessary for the rest of the series, is it?

cleanup changes leading up to here LGTM.

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

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

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 01 2021, Jeff King wrote:

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

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

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

[Jeff King]

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

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

[Han-Wen Nienhuys]

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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

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

[Han-Wen Nienhuys]

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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

[PATCH v2 8/8] upload-pack.c: convert to new serve.c "startup" config cb

[Ævar Arnfjörð Bjarmason]

Convert the config reading code in upload-pack.c to use the new
"startup_config" callback when we're using the v2 protocol, and
lightly fake up the same when we're using v0 and v1.

Before the series that fixed 08450ef7918 (upload-pack: clear
filter_options for each v2 fetch command, 2020-05-08) landed, most of
what's now "struct upload_pack_data" used to be static variables at
the top of this file.

This moves some of them back. See f203a88cf14 (upload-pack: move
keepalive to upload_pack_data, 2020-06-04), f1514c6aad0 (upload-pack:
move allow_unadvertised_object_request to upload_pack_data,
2020-06-11) etc.

I think this makes it easier to understand the this code, as we're now
clearly separating data and config that changes on a
request-to-request basis (see 08450ef7918), and the sort of config
that serve.c's "startup_config" is aimed at.

Thus it's clear that the "allow_uor" passed to functions like
"is_our_ref()" is constant after startup (usually it'll never change
for a given server's configuration, or change once).

This requires a very light compatibly layer with the serve.c callback
mechanism in the form of "upload_pack" for the v0 and v1 protocols.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c       |   1 +
 upload-pack.c | 139 ++++++++++++++++++++++++++++++--------------------
 upload-pack.h |   2 +
 3 files changed, 86 insertions(+), 56 deletions(-)

diff --git a/serve.c b/serve.c
index cce96dd5a81..1054d1459c9 100644
--- a/serve.c
+++ b/serve.c
@@ -112,6 +112,7 @@ static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "fetch",
+		.startup_config = serve_upload_pack_startup_config,
 		.advertise = upload_pack_advertise,
 		.command = upload_pack_v2,
 	},
diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb43..6c0d566e9e0 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -52,6 +52,19 @@ enum allow_uor {
 	ALLOW_ANY_SHA1 = 0x07
 };
 
+/*
+ * "Global" configuration that won't be affected by the type of
+ * request we're doing, or by other request data in "struct
+ * upload_pack_data" below.
+ */
+static int v1_have_startup_config;
+static int config_keepalive = 5;
+static enum allow_uor config_allow_uor;
+static unsigned config_advertise_sid;
+static const char *config_pack_objects_hook;
+static unsigned config_v2_allow_ref_in_want;
+static unsigned config_v2_allow_sideband_all;
+
 /*
  * Please annotate, and if possible group together, fields used only
  * for protocol v0 or only for protocol v2.
@@ -70,7 +83,6 @@ struct upload_pack_data {
 	timestamp_t deepen_since;
 	int deepen_rev_list;
 	int deepen_relative;
-	int keepalive;
 	int shallow_nr;
 	timestamp_t oldest_have;
 
@@ -85,15 +97,12 @@ struct upload_pack_data {
 	int use_sideband;
 
 	struct string_list uri_protocols;
-	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
 	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
-	const char *pack_objects_hook;
-
 	unsigned stateless_rpc : 1;				/* v0 only */
 	unsigned no_done : 1;					/* v0 only */
 	unsigned daemon_mode : 1;				/* v0 only */
@@ -109,9 +118,6 @@ struct upload_pack_data {
 	unsigned long tree_filter_max_depth;
 
 	unsigned done : 1;					/* v2 only */
-	unsigned allow_ref_in_want : 1;				/* v2 only */
-	unsigned allow_sideband_all : 1;			/* v2 only */
-	unsigned advertise_sid : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)
@@ -141,9 +147,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->allow_filter_fallback = 1;
 	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
-
-	data->keepalive = 5;
-	data->advertise_sid = 0;
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -158,8 +161,6 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
 	string_list_clear(&data->allowed_filters, 0);
-
-	free((char *)data->pack_objects_hook);
 }
 
 static void reset_timeout(unsigned int timeout)
@@ -277,10 +278,10 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 	int i;
 	FILE *pipe_fd;
 
-	if (!pack_data->pack_objects_hook)
+	if (!config_pack_objects_hook)
 		pack_objects.git_cmd = 1;
 	else {
-		strvec_push(&pack_objects.args, pack_data->pack_objects_hook);
+		strvec_push(&pack_objects.args, config_pack_objects_hook);
 		strvec_push(&pack_objects.args, "git");
 		pack_objects.use_shell = 1;
 	}
@@ -371,9 +372,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		if (!pollsize)
 			break;
 
-		polltimeout = pack_data->keepalive < 0
+		polltimeout = config_keepalive < 0
 			? -1
-			: 1000 * pack_data->keepalive;
+			: 1000 * config_keepalive;
 
 		ret = poll(pfd, pollsize, polltimeout);
 
@@ -581,9 +582,9 @@ static int get_common_commits(struct upload_pack_data *data,
 	}
 }
 
-static int is_our_ref(struct object *o, enum allow_uor allow_uor)
+static int is_our_ref(struct object *o)
 {
-	int allow_hidden_ref = (allow_uor &
+	int allow_hidden_ref = (config_allow_uor &
 				(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
@@ -593,8 +594,7 @@ static int is_our_ref(struct object *o, enum allow_uor allow_uor)
  */
 static int do_reachable_revlist(struct child_process *cmd,
 				struct object_array *src,
-				struct object_array *reachable,
-				enum allow_uor allow_uor)
+				struct object_array *reachable)
 {
 	static const char *argv[] = {
 		"rev-list", "--stdin", NULL,
@@ -627,14 +627,14 @@ static int do_reachable_revlist(struct child_process *cmd,
 			continue;
 		if (reachable && o->type == OBJ_COMMIT)
 			o->flags &= ~TMP_MARK;
-		if (!is_our_ref(o, allow_uor))
+		if (!is_our_ref(o))
 			continue;
 		if (fprintf(cmd_in, "^%s\n", oid_to_hex(&o->oid)) < 0)
 			goto error;
 	}
 	for (i = 0; i < src->nr; i++) {
 		o = src->objects[i].item;
-		if (is_our_ref(o, allow_uor)) {
+		if (is_our_ref(o)) {
 			if (reachable)
 				add_object_array(o, NULL, reachable);
 			continue;
@@ -671,8 +671,7 @@ static int get_reachable_list(struct upload_pack_data *data,
 	char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
 	const unsigned hexsz = the_hash_algo->hexsz;
 
-	if (do_reachable_revlist(&cmd, &data->shallows, reachable,
-				 data->allow_uor) < 0)
+	if (do_reachable_revlist(&cmd, &data->shallows, reachable) < 0)
 		return -1;
 
 	while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {
@@ -703,13 +702,13 @@ static int get_reachable_list(struct upload_pack_data *data,
 	return 0;
 }
 
-static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
+static int has_unreachable(struct object_array *src)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	char buf[1];
 	int i;
 
-	if (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)
+	if (do_reachable_revlist(&cmd, src, NULL) < 0)
 		return 1;
 
 	/*
@@ -748,9 +747,9 @@ static void check_non_tip(struct upload_pack_data *data)
 	 * uploadpack.allowReachableSHA1InWant,
 	 * non-tip requests can never happen.
 	 */
-	if (!data->stateless_rpc && !(data->allow_uor & ALLOW_REACHABLE_SHA1))
+	if (!data->stateless_rpc && !(config_allow_uor & ALLOW_REACHABLE_SHA1))
 		goto error;
-	if (!has_unreachable(&data->want_obj, data->allow_uor))
+	if (!has_unreachable(&data->want_obj))
 		/* All the non-tip ones are ancestors of what we advertised */
 		return;
 
@@ -758,7 +757,7 @@ static void check_non_tip(struct upload_pack_data *data)
 	/* Pick one of them (we know there at least is one) */
 	for (i = 0; i < data->want_obj.nr; i++) {
 		struct object *o = data->want_obj.objects[i].item;
-		if (!is_our_ref(o, data->allow_uor)) {
+		if (!is_our_ref(o)) {
 			packet_writer_error(&data->writer,
 					    "upload-pack: not our ref %s",
 					    oid_to_hex(&o->oid));
@@ -1123,8 +1122,8 @@ static void receive_needs(struct upload_pack_data *data,
 		}
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
-			if (!((data->allow_uor & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
-			      || is_our_ref(o, data->allow_uor)))
+			if (!((config_allow_uor & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
+			      || is_our_ref(o)))
 				has_non_tip = 1;
 			add_object_array(o, NULL, &data->want_obj);
 		}
@@ -1184,7 +1183,7 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 }
 
 static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {
-	if (d->advertise_sid)
+	if (config_advertise_sid)
 		strbuf_addf(buf, " session-id=%s", trace2_session_id());
 }
 
@@ -1210,9 +1209,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
-			     (data->allow_uor & ALLOW_TIP_SHA1) ?
+			     (config_allow_uor & ALLOW_TIP_SHA1) ?
 				     " allow-tip-sha1-in-want" : "",
-			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
+			     (config_allow_uor & ALLOW_REACHABLE_SHA1) ?
 				     " allow-reachable-sha1-in-want" : "",
 			     data->stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
@@ -1282,45 +1281,71 @@ static int parse_object_filter_config(const char *var, const char *value,
 	return 0;
 }
 
-static int upload_pack_config(const char *var, const char *value, void *cb_data)
+static int upload_pack_startup_config(const char *var, const char *value,
+				      void *cb_data)
 {
-	struct upload_pack_data *data = cb_data;
-
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_TIP_SHA1;
+			config_allow_uor |= ALLOW_TIP_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_TIP_SHA1;
+			config_allow_uor &= ~ALLOW_TIP_SHA1;
 	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_REACHABLE_SHA1;
+			config_allow_uor |= ALLOW_REACHABLE_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_REACHABLE_SHA1;
+			config_allow_uor &= ~ALLOW_REACHABLE_SHA1;
 	} else if (!strcmp("uploadpack.allowanysha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_ANY_SHA1;
+			config_allow_uor |= ALLOW_ANY_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_ANY_SHA1;
+			config_allow_uor &= ~ALLOW_ANY_SHA1;
 	} else if (!strcmp("uploadpack.keepalive", var)) {
-		data->keepalive = git_config_int(var, value);
-		if (!data->keepalive)
-			data->keepalive = -1;
-	} else if (!strcmp("uploadpack.allowfilter", var)) {
-		data->allow_filter = git_config_bool(var, value);
-	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
-		data->allow_ref_in_want = git_config_bool(var, value);
-	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
-		data->allow_sideband_all = git_config_bool(var, value);
+		config_keepalive = git_config_int(var, value);
+		if (!config_keepalive)
+			config_keepalive = -1;
 	} else if (!strcmp("core.precomposeunicode", var)) {
 		precomposed_unicode = git_config_bool(var, value);
 	} else if (!strcmp("transfer.advertisesid", var)) {
-		data->advertise_sid = git_config_bool(var, value);
+		config_advertise_sid = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
 	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
 		if (!strcmp("uploadpack.packobjectshook", var))
-			return git_config_string(&data->pack_objects_hook, var, value);
+			return git_config_string(&config_pack_objects_hook, var, value);
+	}
+	return 0;
+}
+
+static int upload_pack_startup_config_v2_only(const char *var,
+					      const char *value, void *cb_data)
+{
+	if (!strcmp("uploadpack.allowrefinwant", var))
+		config_v2_allow_ref_in_want = git_config_bool(var, value);
+	else if (!strcmp("uploadpack.allowsidebandall", var))
+		config_v2_allow_sideband_all = git_config_bool(var, value);
+	return 0;
+}
+
+int serve_upload_pack_startup_config(const char *var, const char *value, void *data)
+{
+	int ret;
+	ret = upload_pack_startup_config(var, value, data);
+	if (ret)
+		return ret;
+	ret = upload_pack_startup_config_v2_only(var, value, data);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+static int upload_pack_config(const char *var, const char *value,
+			      void *cb_data)
+{
+	struct upload_pack_data *data = cb_data;
+
+	if (!strcmp("uploadpack.allowfilter", var)) {
+		data->allow_filter = git_config_bool(var, value);
 	}
 
 	if (parse_object_filter_config(var, value, data) < 0)
@@ -1336,6 +1361,8 @@ void upload_pack(struct upload_pack_options *options)
 
 	upload_pack_data_init(&data);
 
+	if (!v1_have_startup_config++)
+		git_config(upload_pack_startup_config, NULL);
 	git_config(upload_pack_config, &data);
 
 	data.stateless_rpc = options->stateless_rpc;
@@ -1468,7 +1495,7 @@ static void process_args(struct packet_reader *request,
 		/* process want */
 		if (parse_want(&data->writer, arg, &data->want_obj))
 			continue;
-		if (data->allow_ref_in_want &&
+		if (config_v2_allow_ref_in_want &&
 		    parse_want_ref(&data->writer, arg, &data->wanted_refs,
 				   &data->want_obj))
 			continue;
@@ -1526,7 +1553,7 @@ static void process_args(struct packet_reader *request,
 		}
 
 		if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
-		     data->allow_sideband_all) &&
+		     config_v2_allow_sideband_all) &&
 		    !strcmp(arg, "sideband-all")) {
 			data->writer.use_sideband = 1;
 			continue;
diff --git a/upload-pack.h b/upload-pack.h
index 27ddcdc6cb0..bd8b1ec5b3e 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -19,5 +19,7 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,
 struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
+int serve_upload_pack_startup_config(const char *var, const char *value,
+				     void *data);
 
 #endif /* UPLOAD_PACK_H */
-- 
2.32.0.611.gd4a17395dfa

Re: [PATCH v2 8/8] upload-pack.c: convert to new serve.c "startup" config cb

[Han-Wen Nienhuys]

On Mon, Jun 28, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Convert the config reading code in upload-pack.c to use the new
> "startup_config" callback when we're using the v2 protocol, and
> lightly fake up the same when we're using v0 and v1.


I'm not a fan creating more global variables rather, but I'll it leave
to someone else to decide what's best. This change looks consistent
with its predecessor, and OK modulo a few nits below.

> Thus it's clear that the "allow_uor" passed to functions like
> "is_our_ref()" is constant after startup (usually it'll never change
> for a given server's configuration, or change once).
>
> This requires a very light compatibly layer with the serve.c callback

typo.

> +/*
> + * "Global" configuration that won't be affected by the type of
> + * request we're doing, or by other request data in "struct
> + * upload_pack_data" below.
> + */
> +static int v1_have_startup_config;
> +static int config_keepalive = 5;

could this include a unit? config_keepalive_secs , I think?

> +       int ret;
> +       ret = upload_pack_startup_config(var, value, data);
> +       if (ret)
> +               return ret;
> +       ret = upload_pack_startup_config_v2_only(var, value, data);
> +       if (ret)
> +               return ret;

Neither of these config readers ever return -1. Do you foresee this
ever happening? Or could this return void?

> +       if (!v1_have_startup_config++)
> +               git_config(upload_pack_startup_config, NULL);

See comment in previous patch about ++ for boolean values.

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

[PATCH v3 00/12] serve.[ch]: general API cleanup

[Ævar Arnfjörð Bjarmason]

For v2, see:
https://lore.kernel.org/git/cover-0.8-00000000000-20210628T191634Z-avarab@gmail.com/

Junio: Sorry about the late update, re:
http://lore.kernel.org/git/xmqqsg07cz4g.fsf@gitster.g

I think most people were on the fence about the need for this
"startup_config" callback in v2. I think we could still do without it,
but as the new 08/12 shows a big part of the API complexity was due to
serve() supporting the --advertise-refs mode directly. 08-10/12 is a
new late-series cleanup of that, which makes serve.[ch] a lot simpler.

Ævar Arnfjörð Bjarmason (12):
  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: add support for a "startup" git_config() callback
  serve.c: move version line to advertise_capabilities()
  serve.[ch]: remove "serve_options", split up --advertise-refs code
  {upload,receive}-pack tests: add --advertise-refs tests
  upload-pack: document and rename --advertise-refs
  upload-pack.c: convert to new serve.c "startup" config cb
  serve.[ch]: don't pass "struct strvec *keys" to commands

 Documentation/git-receive-pack.txt        |   5 +
 Documentation/git-upload-pack.txt         |  12 +-
 Documentation/technical/http-protocol.txt |   3 +
 Documentation/technical/protocol-v2.txt   |   3 +
 builtin/receive-pack.c                    |   3 +-
 builtin/upload-pack.c                     |  28 ++--
 http-backend.c                            |   2 +-
 ls-refs.c                                 |  45 ++----
 ls-refs.h                                 |   5 +-
 protocol-caps.c                           |   3 +-
 protocol-caps.h                           |   4 +-
 serve.c                                   | 146 ++++++++++++++------
 serve.h                                   |  12 +-
 t/helper/test-serve-v2.c                  |  14 +-
 t/t5555-http-smart-common.sh              | 159 ++++++++++++++++++++++
 transport-helper.c                        |  18 +--
 transport-internal.h                      |   2 +-
 transport.c                               |  32 ++---
 upload-pack.c                             | 154 ++++++++++++---------
 upload-pack.h                             |  16 +--
 20 files changed, 450 insertions(+), 216 deletions(-)
 create mode 100755 t/t5555-http-smart-common.sh

Range-diff against v2:
 1:  fdb0c5f4df1 =  1:  192fb64ef82 serve: mark has_capability() as static
 2:  7b8101dca71 =  2:  d716bd3c537 transport: rename "fetch" in transport_vtable to "fetch_refs"
 3:  766045e5f1d =  3:  d31690614af transport: use designated initializers
 4:  bd920de1629 =  4:  13f1a8d8325 serve: use designated initializers
 5:  d0b02aa0c7d =  5:  99eeff6f890 serve.c: add call_{advertise,command}() indirection
 7:  0a4fb01ae38 !  6:  be719dc3dc1 serve: add support for a "startup" git_config() callback
    @@ Commit message
         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.
     
    +    As we'll see in subsequent commits, by moving the
    +    transfer.advertisesid config reading out of serve() we can simplify
    +    the codepath around advertising-only requests. See 6b5b6e422ee (serve:
    +    advertise session ID in v2 capabilities, 2020-11-11)) for the commit
    +    that added transfer.advertisesid.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## ls-refs.c ##
    @@ serve.c: static struct protocol_capability capabilities[] = {
      static int call_advertise(struct protocol_capability *command,
      			  struct repository *r, struct strbuf *value)
      {
    -@@ 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;
    + 	return command->advertise(r, value);
    + }
      
    +@@ serve.c: static int call_command(struct protocol_capability *command,
    + 			struct repository *r, struct strvec *keys,
    + 			struct packet_reader *request)
    + {
    ++
     +	read_startup_config(command);
     +
    - 	strbuf_addf(&sb, "command/%s", command->name);
    - 	trace2_region_enter("serve", sb.buf, r);
    - 	ret = command->command(r, keys, request);
    + 	return 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)
 -:  ----------- >  7:  b7928ddbe9b serve.c: move version line to advertise_capabilities()
 6:  baeee6539ad !  8:  fb80f152713 serve.c: add trace2 regions for advertise & command
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    serve.c: add trace2 regions for advertise & command
    +    serve.[ch]: remove "serve_options", split up --advertise-refs code
     
    -    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.
    +    The "advertise capabilities" mode of serve.c added in
    +    ed10cb952d3 (serve: introduce git-serve, 2018-03-15) is only used by
    +    the http-backend.c to call {upload,receive}-pack with the
    +    --advertise-refs parameter. See 42526b478e3 (Add stateless RPC options
    +    to upload-pack, receive-pack, 2009-10-30).
    +
    +    Let's just make cmd_upload_pack() take the two (v2) or three (v2)
    +    parameters the the v2/v1 servicing functions need directly, and pass
    +    those in via the function signature. The logic of whether daemon mode
    +    is implied by the timeout belongs in the v1 function (only used
    +    there).
    +
    +    Once we split up the "advertise v2 refs" from "serve v2 request" it
    +    becomes clear that v2 never cared about those in combination. The only
    +    time it mattered was for v1 to emit its ref advertisement, in that
    +    case we wanted to emit the smart-http-only "no-done" capability.
    +
    +    Since we only do that in the --advertise-refs codepath let's just have
    +    it set "do_done" itself in v1's upload_pack() just before send_ref(),
    +    at that point --advertise-refs and --stateless-rpc in combination are
    +    redundant (the only user is get_info_refs() in http-backend.c), so we
    +    can just pass in --advertise-refs only.
    +
    +    Since we need to touch all the serve() and advertise_capabilities()
    +    codepaths let's rename them to less clever and obvious names, it's
    +    been suggested numerous times, the latest of which is [1]'s suggestion
    +    for protocol_v2_serve_loop(). Let's go with that.
    +
    +    1. https://lore.kernel.org/git/CAFQ2z_NyGb8rju5CKzmo6KhZXD0Dp21u-BbyCb2aNxLEoSPRJw@mail.gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    + ## builtin/upload-pack.c ##
    +@@ builtin/upload-pack.c: int cmd_upload_pack(int argc, const char **argv, const char *prefix)
    + {
    + 	const char *dir;
    + 	int strict = 0;
    +-	struct upload_pack_options opts = { 0 };
    +-	struct serve_options serve_opts = SERVE_OPTIONS_INIT;
    ++	int advertise_refs = 0;
    ++	int stateless_rpc = 0;
    ++	int timeout = 0;
    + 	struct option options[] = {
    +-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
    ++		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
    + 			 N_("quit after a single request/response exchange")),
    +-		OPT_BOOL(0, "advertise-refs", &opts.advertise_refs,
    ++		OPT_BOOL(0, "advertise-refs", &advertise_refs,
    + 			 N_("exit immediately after initial ref advertisement")),
    + 		OPT_BOOL(0, "strict", &strict,
    + 			 N_("do not try <directory>/.git/ if <directory> is no Git directory")),
    +-		OPT_INTEGER(0, "timeout", &opts.timeout,
    ++		OPT_INTEGER(0, "timeout", &timeout,
    + 			    N_("interrupt transfer after <n> seconds of inactivity")),
    + 		OPT_END()
    + 	};
    +@@ builtin/upload-pack.c: int cmd_upload_pack(int argc, const char **argv, const char *prefix)
    + 	if (argc != 1)
    + 		usage_with_options(upload_pack_usage, options);
    + 
    +-	if (opts.timeout)
    +-		opts.daemon_mode = 1;
    +-
    + 	setup_path();
    + 
    + 	dir = argv[0];
    +@@ builtin/upload-pack.c: int cmd_upload_pack(int argc, const char **argv, const char *prefix)
    + 
    + 	switch (determine_protocol_version_server()) {
    + 	case protocol_v2:
    +-		serve_opts.advertise_capabilities = opts.advertise_refs;
    +-		serve_opts.stateless_rpc = opts.stateless_rpc;
    +-		serve(&serve_opts);
    ++		if (advertise_refs)
    ++			protocol_v2_advertise_capabilities();
    ++		else
    ++			protocol_v2_serve_loop(stateless_rpc);
    + 		break;
    + 	case protocol_v1:
    + 		/*
    + 		 * v1 is just the original protocol with a version string,
    + 		 * so just fall through after writing the version string.
    + 		 */
    +-		if (opts.advertise_refs || !opts.stateless_rpc)
    ++		if (advertise_refs || !stateless_rpc)
    + 			packet_write_fmt(1, "version 1\n");
    + 
    + 		/* fallthrough */
    + 	case protocol_v0:
    +-		upload_pack(&opts);
    ++		upload_pack(advertise_refs, stateless_rpc, timeout);
    + 		break;
    + 	case protocol_unknown_version:
    + 		BUG("unknown protocol version");
    +
    + ## http-backend.c ##
    +@@ http-backend.c: static void get_info_refs(struct strbuf *hdr, char *arg)
    + 
    + 	if (service_name) {
    + 		const char *argv[] = {NULL /* service name */,
    +-			"--stateless-rpc", "--advertise-refs",
    ++			"--advertise-refs",
    + 			".", NULL};
    + 		struct rpc_service *svc = select_service(hdr, service_name);
    + 
    +
      ## serve.c ##
    -@@ serve.c: static struct protocol_capability capabilities[] = {
    - static int call_advertise(struct protocol_capability *command,
    - 			  struct repository *r, struct strbuf *value)
    +@@ serve.c: static int call_command(struct protocol_capability *command,
    + 	return command->command(r, keys, request);
    + }
    + 
    +-static void advertise_capabilities(void)
    ++void protocol_v2_advertise_capabilities(void)
      {
    --	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;
    + 	struct strbuf capability = STRBUF_INIT;
    + 	struct strbuf value = STRBUF_INIT;
    +@@ serve.c: static int process_request(void)
    + 	return 0;
      }
      
    - static int call_command(struct protocol_capability *command,
    - 			struct repository *r, struct strvec *keys,
    - 			struct packet_reader *request)
    +-/* Main serve loop for protocol version 2 */
    +-void serve(struct serve_options *options)
    ++void protocol_v2_serve_loop(int stateless_rpc)
      {
    --	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);
    +-	if (options->advertise_capabilities || !options->stateless_rpc) {
    +-		advertise_capabilities();
    +-		/*
    +-		 * If only the list of capabilities was requested exit
    +-		 * immediately after advertising capabilities
    +-		 */
    +-		if (options->advertise_capabilities)
    +-			return;
    +-	}
    ++	if (!stateless_rpc)
    ++		protocol_v2_advertise_capabilities();
    + 
    + 	/*
    + 	 * If stateless-rpc was requested then exit after
    + 	 * a single request/response exchange
    + 	 */
    +-	if (options->stateless_rpc) {
    ++	if (stateless_rpc) {
    + 		process_request();
    + 	} else {
    + 		for (;;)
    +
    + ## serve.h ##
    +@@
    + #ifndef SERVE_H
    + #define SERVE_H
    + 
    +-struct serve_options {
    +-	unsigned advertise_capabilities;
    +-	unsigned stateless_rpc;
    +-};
    +-#define SERVE_OPTIONS_INIT { 0 }
    +-void serve(struct serve_options *options);
    ++void protocol_v2_advertise_capabilities(void);
    ++void protocol_v2_serve_loop(int stateless_rpc);
    + 
    + #endif /* SERVE_H */
    +
    + ## t/helper/test-serve-v2.c ##
    +@@ t/helper/test-serve-v2.c: static char const * const serve_usage[] = {
    + 
    + int cmd__serve_v2(int argc, const char **argv)
    + {
    +-	struct serve_options opts = SERVE_OPTIONS_INIT;
    +-
    ++	int stateless_rpc = 0;
    ++	int advertise_capabilities = 0;
    + 	struct option options[] = {
    +-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
    ++		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
    + 			 N_("quit after a single request/response exchange")),
    +-		OPT_BOOL(0, "advertise-capabilities", &opts.advertise_capabilities,
    ++		OPT_BOOL(0, "advertise-capabilities", &advertise_capabilities,
    + 			 N_("exit immediately after advertising capabilities")),
    + 		OPT_END()
    + 	};
    +@@ t/helper/test-serve-v2.c: int cmd__serve_v2(int argc, const char **argv)
    + 	argc = parse_options(argc, argv, prefix, options, serve_usage,
    + 			     PARSE_OPT_KEEP_DASHDASH |
    + 			     PARSE_OPT_KEEP_UNKNOWN);
    +-	serve(&opts);
     +
    -+	return ret;
    ++	if (advertise_capabilities)
    ++		protocol_v2_advertise_capabilities();
    ++	else
    ++		protocol_v2_serve_loop(stateless_rpc);
    + 
    + 	return 0;
    + }
    +
    + ## upload-pack.c ##
    +@@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *oid,
    + 				     " allow-tip-sha1-in-want" : "",
    + 			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
    + 				     " allow-reachable-sha1-in-want" : "",
    +-			     data->stateless_rpc ? " no-done" : "",
    ++			     data->no_done ? " no-done" : "",
    + 			     symref_info.buf,
    + 			     data->allow_filter ? " filter" : "",
    + 			     session_id.buf,
    +@@ upload-pack.c: static int upload_pack_config(const char *var, const char *value, void *cb_data)
    + 	return parse_hide_refs_config(var, value, "uploadpack");
      }
      
    - static void advertise_capabilities(void)
    +-void upload_pack(struct upload_pack_options *options)
    ++void upload_pack(const int advertise_refs, const int stateless_rpc,
    ++		 const int timeout)
    + {
    + 	struct packet_reader reader;
    + 	struct upload_pack_data data;
    +@@ upload-pack.c: void upload_pack(struct upload_pack_options *options)
    + 
    + 	git_config(upload_pack_config, &data);
    + 
    +-	data.stateless_rpc = options->stateless_rpc;
    +-	data.daemon_mode = options->daemon_mode;
    +-	data.timeout = options->timeout;
    ++	data.stateless_rpc = stateless_rpc;
    ++	data.timeout = timeout;
    ++	if (data.timeout)
    ++		data.daemon_mode = 1;
    + 
    + 	head_ref_namespaced(find_symref, &data.symref);
    + 
    +-	if (options->advertise_refs || !data.stateless_rpc) {
    ++	if (advertise_refs || !data.stateless_rpc) {
    + 		reset_timeout(data.timeout);
    ++		if (advertise_refs)
    ++			data.no_done = 1;
    + 		head_ref_namespaced(send_ref, &data);
    + 		for_each_namespaced_ref(send_ref, &data);
    + 		advertise_shallow_grafts(1);
    +@@ upload-pack.c: void upload_pack(struct upload_pack_options *options)
    + 		for_each_namespaced_ref(check_ref, NULL);
    + 	}
    + 
    +-	if (!options->advertise_refs) {
    ++	if (!advertise_refs) {
    + 		packet_reader_init(&reader, 0, NULL, 0,
    + 				   PACKET_READ_CHOMP_NEWLINE |
    + 				   PACKET_READ_DIE_ON_ERR_PACKET);
     
    - ## t/t5705-session-id-in-capabilities.sh ##
    -@@ t/t5705-session-id-in-capabilities.sh: do
    - 			--upload-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-upload-pack" \
    - 			origin &&
    - 		grep \"key\":\"server-sid\" tr2-client-events &&
    --		grep \"key\":\"client-sid\" tr2-server-events
    -+		grep \"key\":\"client-sid\" tr2-server-events &&
    -+
    -+		if test "$PROTO" = 2
    -+		then
    -+			grep \"event\":\"region_enter\".*\"category\":\"serve\" tr2-server-events &&
    -+			grep \"event\":\"region_leave\".*\"category\":\"serve\" tr2-server-events
    -+		fi
    - 	'
    - 
    - 	test_expect_success "session IDs advertised (push v${PROTO})" '
    -@@ t/t5705-session-id-in-capabilities.sh: do
    - 			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
    - 			origin HEAD:new-branch &&
    - 		grep \"key\":\"server-sid\" tr2-client-events &&
    --		grep \"key\":\"client-sid\" tr2-server-events
    -+		grep \"key\":\"client-sid\" tr2-server-events &&
    -+
    -+		if test "$PROTO" = 2
    -+		then
    -+			! grep \"event\":\"region_enter\".*\"category\":\"serve\" tr2-server-events &&
    -+			! grep \"event\":\"region_leave\".*\"category\":\"serve\" tr2-server-events
    -+		fi
    - 	'
    - done
    + ## upload-pack.h ##
    +@@
    + #ifndef UPLOAD_PACK_H
    + #define UPLOAD_PACK_H
    + 
    +-struct upload_pack_options {
    +-	int stateless_rpc;
    +-	int advertise_refs;
    +-	unsigned int timeout;
    +-	int daemon_mode;
    +-};
    +-
    +-void upload_pack(struct upload_pack_options *options);
    ++void upload_pack(const int advertise_refs, const int stateless_rpc,
    ++		 const int timeout);
      
    + struct repository;
    + struct strvec;
 -:  ----------- >  9:  beafe9811c1 {upload,receive}-pack tests: add --advertise-refs tests
 -:  ----------- > 10:  c6870b5f18a upload-pack: document and rename --advertise-refs
 8:  9fd6138aa62 ! 11:  2d4c3d0d463 upload-pack.c: convert to new serve.c "startup" config cb
    @@ Commit message
         "is_our_ref()" is constant after startup (usually it'll never change
         for a given server's configuration, or change once).
     
    -    This requires a very light compatibly layer with the serve.c callback
    -    mechanism in the form of "upload_pack" for the v0 and v1 protocols.
    +    This requires a very light compatibility layer with the serve.c
    +    callback mechanism in the form of "upload_pack" for the v0 and v1
    +    protocols.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ upload-pack.c: enum allow_uor {
     + * upload_pack_data" below.
     + */
     +static int v1_have_startup_config;
    -+static int config_keepalive = 5;
    ++static int config_keepalive_secs = 5;
     +static enum allow_uor config_allow_uor;
     +static unsigned config_advertise_sid;
     +static const char *config_pack_objects_hook;
    @@ upload-pack.c: static void create_pack_file(struct upload_pack_data *pack_data,
      			break;
      
     -		polltimeout = pack_data->keepalive < 0
    -+		polltimeout = config_keepalive < 0
    ++		polltimeout = config_keepalive_secs < 0
      			? -1
     -			: 1000 * pack_data->keepalive;
    -+			: 1000 * config_keepalive;
    ++			: 1000 * config_keepalive_secs;
      
      		ret = poll(pfd, pollsize, polltimeout);
      
    @@ upload-pack.c: static int send_ref(const char *refname, const struct object_id *
     -			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
     +			     (config_allow_uor & ALLOW_REACHABLE_SHA1) ?
      				     " allow-reachable-sha1-in-want" : "",
    - 			     data->stateless_rpc ? " no-done" : "",
    + 			     data->no_done ? " no-done" : "",
      			     symref_info.buf,
     @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char *value,
      	return 0;
    @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char
     -		data->allow_ref_in_want = git_config_bool(var, value);
     -	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
     -		data->allow_sideband_all = git_config_bool(var, value);
    -+		config_keepalive = git_config_int(var, value);
    -+		if (!config_keepalive)
    -+			config_keepalive = -1;
    ++		config_keepalive_secs = git_config_int(var, value);
    ++		if (!config_keepalive_secs)
    ++			config_keepalive_secs = -1;
      	} else if (!strcmp("core.precomposeunicode", var)) {
      		precomposed_unicode = git_config_bool(var, value);
      	} else if (!strcmp("transfer.advertisesid", var)) {
    @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char
      		if (!strcmp("uploadpack.packobjectshook", var))
     -			return git_config_string(&data->pack_objects_hook, var, value);
     +			return git_config_string(&config_pack_objects_hook, var, value);
    -+	}
    + 	}
     +	return 0;
     +}
     +
    @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char
     +
     +int serve_upload_pack_startup_config(const char *var, const char *value, void *data)
     +{
    -+	int ret;
    -+	ret = upload_pack_startup_config(var, value, data);
    -+	if (ret)
    -+		return ret;
    -+	ret = upload_pack_startup_config_v2_only(var, value, data);
    -+	if (ret)
    -+		return ret;
    ++	upload_pack_startup_config(var, value, data);
    ++	upload_pack_startup_config_v2_only(var, value, data);
     +	return 0;
     +}
     +
    @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char
     +{
     +	struct upload_pack_data *data = cb_data;
     +
    -+	if (!strcmp("uploadpack.allowfilter", var)) {
    ++	if (!strcmp("uploadpack.allowfilter", var))
     +		data->allow_filter = git_config_bool(var, value);
    - 	}
      
      	if (parse_object_filter_config(var, value, data) < 0)
    -@@ upload-pack.c: void upload_pack(struct upload_pack_options *options)
    + 		return -1;
    +@@ upload-pack.c: void upload_pack(const int advertise_refs, const int stateless_rpc,
      
      	upload_pack_data_init(&data);
      
    @@ upload-pack.c: void upload_pack(struct upload_pack_options *options)
     +		git_config(upload_pack_startup_config, NULL);
      	git_config(upload_pack_config, &data);
      
    - 	data.stateless_rpc = options->stateless_rpc;
    + 	data.stateless_rpc = stateless_rpc;
     @@ upload-pack.c: static void process_args(struct packet_reader *request,
      		/* process want */
      		if (parse_want(&data->writer, arg, &data->want_obj))
 -:  ----------- > 12:  e4eb31b5b8e serve.[ch]: don't pass "struct strvec *keys" to commands
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v3 01/12] serve: mark has_capability() as static

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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.955.ge7c5360f7e7

[PATCH v3 02/12] transport: rename "fetch" in transport_vtable to "fetch_refs"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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 17e9629710a..3e8a27b0321 100644
--- a/transport.c
+++ b/transport.c
@@ -1453,7 +1453,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.955.ge7c5360f7e7

[PATCH v3 03/12] transport: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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 3e8a27b0321..f9400b9b0bd 100644
--- a/transport.c
+++ b/transport.c
@@ -883,12 +883,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,
@@ -1032,21 +1030,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.955.ge7c5360f7e7

[PATCH v3 04/12] serve: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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.955.ge7c5360f7e7

[PATCH v3 05/12] serve.c: add call_{advertise,command}() indirection

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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.955.ge7c5360f7e7

[PATCH v3 06/12] serve: add support for a "startup" git_config() callback

[Ævar Arnfjörð Bjarmason]

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.

As we'll see in subsequent commits, by moving the
transfer.advertisesid config reading out of serve() we can simplify
the codepath around advertising-only requests. See 6b5b6e422ee (serve:
advertise session ID in v2 capabilities, 2020-11-11)) for the commit
that added transfer.advertisesid.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c | 42 ++++++++++++++----------------------------
 ls-refs.h |  1 +
 serve.c   | 43 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 56 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 85cd3eab26e..3c7c9329a14 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,9 +135,20 @@ 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)
 {
+	read_startup_config(command);
+
 	return command->advertise(r, value);
 }
 
@@ -118,6 +156,9 @@ static int call_command(struct protocol_capability *command,
 			struct repository *r, struct strvec *keys,
 			struct packet_reader *request)
 {
+
+	read_startup_config(command);
+
 	return command->command(r, keys, request);
 }
 
@@ -318,8 +359,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.955.ge7c5360f7e7

[PATCH v3 07/12] serve.c: move version line to advertise_capabilities()

[Ævar Arnfjörð Bjarmason]

The advertise_capabilities() is only called from serve() and we always
emit this version line before it, it makes more sense to consider the
capabilities part of a "header" that has the version, so let's move
the writing of the version there.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/serve.c b/serve.c
index 3c7c9329a14..10209ab237d 100644
--- a/serve.c
+++ b/serve.c
@@ -168,6 +168,9 @@ static void advertise_capabilities(void)
 	struct strbuf value = STRBUF_INIT;
 	int i;
 
+	/* serve by default supports v2 */
+	packet_write_fmt(1, "version 2\n");
+
 	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
 		struct protocol_capability *c = &capabilities[i];
 
@@ -360,9 +363,6 @@ static int process_request(void)
 void serve(struct serve_options *options)
 {
 	if (options->advertise_capabilities || !options->stateless_rpc) {
-		/* serve by default supports v2 */
-		packet_write_fmt(1, "version 2\n");
-
 		advertise_capabilities();
 		/*
 		 * If only the list of capabilities was requested exit
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v3 08/12] serve.[ch]: remove "serve_options", split up --advertise-refs code

[Ævar Arnfjörð Bjarmason]

The "advertise capabilities" mode of serve.c added in
ed10cb952d3 (serve: introduce git-serve, 2018-03-15) is only used by
the http-backend.c to call {upload,receive}-pack with the
--advertise-refs parameter. See 42526b478e3 (Add stateless RPC options
to upload-pack, receive-pack, 2009-10-30).

Let's just make cmd_upload_pack() take the two (v2) or three (v2)
parameters the the v2/v1 servicing functions need directly, and pass
those in via the function signature. The logic of whether daemon mode
is implied by the timeout belongs in the v1 function (only used
there).

Once we split up the "advertise v2 refs" from "serve v2 request" it
becomes clear that v2 never cared about those in combination. The only
time it mattered was for v1 to emit its ref advertisement, in that
case we wanted to emit the smart-http-only "no-done" capability.

Since we only do that in the --advertise-refs codepath let's just have
it set "do_done" itself in v1's upload_pack() just before send_ref(),
at that point --advertise-refs and --stateless-rpc in combination are
redundant (the only user is get_info_refs() in http-backend.c), so we
can just pass in --advertise-refs only.

Since we need to touch all the serve() and advertise_capabilities()
codepaths let's rename them to less clever and obvious names, it's
been suggested numerous times, the latest of which is [1]'s suggestion
for protocol_v2_serve_loop(). Let's go with that.

1. https://lore.kernel.org/git/CAFQ2z_NyGb8rju5CKzmo6KhZXD0Dp21u-BbyCb2aNxLEoSPRJw@mail.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/upload-pack.c    | 25 ++++++++++++-------------
 http-backend.c           |  2 +-
 serve.c                  | 18 +++++-------------
 serve.h                  |  8 ++------
 t/helper/test-serve-v2.c | 14 +++++++++-----
 upload-pack.c            | 18 +++++++++++-------
 upload-pack.h            | 10 ++--------
 7 files changed, 42 insertions(+), 53 deletions(-)

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 6da8fa2607c..8506030a648 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -16,16 +16,17 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 {
 	const char *dir;
 	int strict = 0;
-	struct upload_pack_options opts = { 0 };
-	struct serve_options serve_opts = SERVE_OPTIONS_INIT;
+	int advertise_refs = 0;
+	int stateless_rpc = 0;
+	int timeout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
+		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-refs", &opts.advertise_refs,
+		OPT_BOOL(0, "advertise-refs", &advertise_refs,
 			 N_("exit immediately after initial ref advertisement")),
 		OPT_BOOL(0, "strict", &strict,
 			 N_("do not try <directory>/.git/ if <directory> is no Git directory")),
-		OPT_INTEGER(0, "timeout", &opts.timeout,
+		OPT_INTEGER(0, "timeout", &timeout,
 			    N_("interrupt transfer after <n> seconds of inactivity")),
 		OPT_END()
 	};
@@ -38,9 +39,6 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	if (argc != 1)
 		usage_with_options(upload_pack_usage, options);
 
-	if (opts.timeout)
-		opts.daemon_mode = 1;
-
 	setup_path();
 
 	dir = argv[0];
@@ -50,21 +48,22 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
-		serve_opts.advertise_capabilities = opts.advertise_refs;
-		serve_opts.stateless_rpc = opts.stateless_rpc;
-		serve(&serve_opts);
+		if (advertise_refs)
+			protocol_v2_advertise_capabilities();
+		else
+			protocol_v2_serve_loop(stateless_rpc);
 		break;
 	case protocol_v1:
 		/*
 		 * v1 is just the original protocol with a version string,
 		 * so just fall through after writing the version string.
 		 */
-		if (opts.advertise_refs || !opts.stateless_rpc)
+		if (advertise_refs || !stateless_rpc)
 			packet_write_fmt(1, "version 1\n");
 
 		/* fallthrough */
 	case protocol_v0:
-		upload_pack(&opts);
+		upload_pack(advertise_refs, stateless_rpc, timeout);
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
diff --git a/http-backend.c b/http-backend.c
index b329bf63f09..d37463cec8b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -534,7 +534,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
 
 	if (service_name) {
 		const char *argv[] = {NULL /* service name */,
-			"--stateless-rpc", "--advertise-refs",
+			"--advertise-refs",
 			".", NULL};
 		struct rpc_service *svc = select_service(hdr, service_name);
 
diff --git a/serve.c b/serve.c
index 10209ab237d..cacb0bc4e0e 100644
--- a/serve.c
+++ b/serve.c
@@ -162,7 +162,7 @@ static int call_command(struct protocol_capability *command,
 	return command->command(r, keys, request);
 }
 
-static void advertise_capabilities(void)
+void protocol_v2_advertise_capabilities(void)
 {
 	struct strbuf capability = STRBUF_INIT;
 	struct strbuf value = STRBUF_INIT;
@@ -359,24 +359,16 @@ static int process_request(void)
 	return 0;
 }
 
-/* Main serve loop for protocol version 2 */
-void serve(struct serve_options *options)
+void protocol_v2_serve_loop(int stateless_rpc)
 {
-	if (options->advertise_capabilities || !options->stateless_rpc) {
-		advertise_capabilities();
-		/*
-		 * If only the list of capabilities was requested exit
-		 * immediately after advertising capabilities
-		 */
-		if (options->advertise_capabilities)
-			return;
-	}
+	if (!stateless_rpc)
+		protocol_v2_advertise_capabilities();
 
 	/*
 	 * If stateless-rpc was requested then exit after
 	 * a single request/response exchange
 	 */
-	if (options->stateless_rpc) {
+	if (stateless_rpc) {
 		process_request();
 	} else {
 		for (;;)
diff --git a/serve.h b/serve.h
index 56da77a87af..f946cf904a2 100644
--- a/serve.h
+++ b/serve.h
@@ -1,11 +1,7 @@
 #ifndef SERVE_H
 #define SERVE_H
 
-struct serve_options {
-	unsigned advertise_capabilities;
-	unsigned stateless_rpc;
-};
-#define SERVE_OPTIONS_INIT { 0 }
-void serve(struct serve_options *options);
+void protocol_v2_advertise_capabilities(void);
+void protocol_v2_serve_loop(int stateless_rpc);
 
 #endif /* SERVE_H */
diff --git a/t/helper/test-serve-v2.c b/t/helper/test-serve-v2.c
index aee35e5aef4..28e905afc36 100644
--- a/t/helper/test-serve-v2.c
+++ b/t/helper/test-serve-v2.c
@@ -10,12 +10,12 @@ static char const * const serve_usage[] = {
 
 int cmd__serve_v2(int argc, const char **argv)
 {
-	struct serve_options opts = SERVE_OPTIONS_INIT;
-
+	int stateless_rpc = 0;
+	int advertise_capabilities = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
+		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-capabilities", &opts.advertise_capabilities,
+		OPT_BOOL(0, "advertise-capabilities", &advertise_capabilities,
 			 N_("exit immediately after advertising capabilities")),
 		OPT_END()
 	};
@@ -25,7 +25,11 @@ int cmd__serve_v2(int argc, const char **argv)
 	argc = parse_options(argc, argv, prefix, options, serve_usage,
 			     PARSE_OPT_KEEP_DASHDASH |
 			     PARSE_OPT_KEEP_UNKNOWN);
-	serve(&opts);
+
+	if (advertise_capabilities)
+		protocol_v2_advertise_capabilities();
+	else
+		protocol_v2_serve_loop(stateless_rpc);
 
 	return 0;
 }
diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb43..48e8bf5694c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1214,7 +1214,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 				     " allow-tip-sha1-in-want" : "",
 			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
 				     " allow-reachable-sha1-in-want" : "",
-			     data->stateless_rpc ? " no-done" : "",
+			     data->no_done ? " no-done" : "",
 			     symref_info.buf,
 			     data->allow_filter ? " filter" : "",
 			     session_id.buf,
@@ -1329,7 +1329,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-void upload_pack(struct upload_pack_options *options)
+void upload_pack(const int advertise_refs, const int stateless_rpc,
+		 const int timeout)
 {
 	struct packet_reader reader;
 	struct upload_pack_data data;
@@ -1338,14 +1339,17 @@ void upload_pack(struct upload_pack_options *options)
 
 	git_config(upload_pack_config, &data);
 
-	data.stateless_rpc = options->stateless_rpc;
-	data.daemon_mode = options->daemon_mode;
-	data.timeout = options->timeout;
+	data.stateless_rpc = stateless_rpc;
+	data.timeout = timeout;
+	if (data.timeout)
+		data.daemon_mode = 1;
 
 	head_ref_namespaced(find_symref, &data.symref);
 
-	if (options->advertise_refs || !data.stateless_rpc) {
+	if (advertise_refs || !data.stateless_rpc) {
 		reset_timeout(data.timeout);
+		if (advertise_refs)
+			data.no_done = 1;
 		head_ref_namespaced(send_ref, &data);
 		for_each_namespaced_ref(send_ref, &data);
 		advertise_shallow_grafts(1);
@@ -1355,7 +1359,7 @@ void upload_pack(struct upload_pack_options *options)
 		for_each_namespaced_ref(check_ref, NULL);
 	}
 
-	if (!options->advertise_refs) {
+	if (!advertise_refs) {
 		packet_reader_init(&reader, 0, NULL, 0,
 				   PACKET_READ_CHOMP_NEWLINE |
 				   PACKET_READ_DIE_ON_ERR_PACKET);
diff --git a/upload-pack.h b/upload-pack.h
index 27ddcdc6cb0..a06498bbea7 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -1,14 +1,8 @@
 #ifndef UPLOAD_PACK_H
 #define UPLOAD_PACK_H
 
-struct upload_pack_options {
-	int stateless_rpc;
-	int advertise_refs;
-	unsigned int timeout;
-	int daemon_mode;
-};
-
-void upload_pack(struct upload_pack_options *options);
+void upload_pack(const int advertise_refs, const int stateless_rpc,
+		 const int timeout);
 
 struct repository;
 struct strvec;
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v3 09/12] {upload,receive}-pack tests: add --advertise-refs tests

[Ævar Arnfjörð Bjarmason]

The --advertise-refs option had no explicit tests of its own, only
other http tests that would fail at a distance if it it was
broken. Let's test its behavior explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5555-http-smart-common.sh | 145 +++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)
 create mode 100755 t/t5555-http-smart-common.sh

diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
new file mode 100755
index 00000000000..389ee96987b
--- /dev/null
+++ b/t/t5555-http-smart-common.sh
@@ -0,0 +1,145 @@
+test_description='test functionality common to smart fetch & push'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit --no-tag initial
+'
+
+test_expect_success 'git upload-pack --advertise-refs' '
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	# We only care about GIT_PROTOCOL, not GIT_TEST_PROTOCOL_VERSION
+	sane_unset GIT_PROTOCOL &&
+	GIT_TEST_PROTOCOL_VERSION=2 \
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# The --advertise-refs alias works
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v0' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	git upload-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# With explicit v0
+	GIT_PROTOCOL=version=0 \
+	git upload-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v0' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	git receive-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# With explicit v0
+	GIT_PROTOCOL=version=0 \
+	git receive-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v1' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	version 1
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=1 \
+	git upload-pack --advertise-refs . >out &&
+
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v1' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	version 1
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=1 \
+	git receive-pack --advertise-refs . >out &&
+
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v2' '
+	cat >expect <<-EOF &&
+	version 2
+	agent=FAKE
+	ls-refs=unborn
+	fetch=shallow wait-for-done
+	server-option
+	object-format=$(test_oid algo)
+	object-info
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=2 \
+	GIT_USER_AGENT=FAKE \
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v2' '
+	# There is no v2 yet for receive-pack, implicit v0
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=2 \
+	git receive-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_done
-- 
2.32.0.955.ge7c5360f7e7

[PATCH] fixup! {upload,receive}-pack tests: add --advertise-refs tests

[Carlo Marcelo Arenas Belón]

avoids CI windows run failures showing:

  non-executable tests: t5555-http-smart-common.sh
  make: *** [Makefile:90: test-lint-executable] Error 1
  make: *** Waiting for unfinished jobs....
  Error: Process completed with exit code 2.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t5555-http-smart-common.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
index 8d9d6a556f..49faf5e283 100755
--- a/t/t5555-http-smart-common.sh
+++ b/t/t5555-http-smart-common.sh
@@ -1,3 +1,5 @@
+#!/bin/sh
+
 test_description='test functionality common to smart fetch & push'
 
 . ./test-lib.sh
-- 
2.33.0.rc0.433.g9a510e7e11

[PATCH v3 10/12] upload-pack: document and rename --advertise-refs

[Ævar Arnfjörð Bjarmason]

The --advertise-refs documentation in git-upload-pack added in
9812f2136b3 (upload-pack.c: use parse-options API, 2016-05-31) hasn't
been entirely true ever since v2 support was implemented in
e52449b6722 (connect: request remote refs using v2, 2018-03-15). Under
v2 we don't advertise the refs at all, but rather dump the
capabilities header.

This option has always been an obscure internal implementation detail,
it wasn't even documented for git-receive-pack. Since it has exactly
one user let's rename it to --http-backend-info-refs, which is more
accurate and points the reader in the right direction. Let's also
cross-link this from the protocol v1 and v2 documentation.

I'm retaining a hidden --advertise-refs alias in case there's any
external users of this, and making both options hidden to the bash
completion (as with most other internal-only options).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-receive-pack.txt        |  5 +++++
 Documentation/git-upload-pack.txt         | 12 ++++++++----
 Documentation/technical/http-protocol.txt |  3 +++
 Documentation/technical/protocol-v2.txt   |  3 +++
 builtin/receive-pack.c                    |  3 ++-
 builtin/upload-pack.c                     |  5 +++--
 http-backend.c                            |  2 +-
 t/t5555-http-smart-common.sh              | 14 ++++++++++++++
 8 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 25702ed7307..014a78409b9 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -41,6 +41,11 @@ OPTIONS
 <directory>::
 	The repository to sync into.
 
+--http-backend-info-refs::
+	Used by linkgit:git-http-backend[1] to serve up
+	`$GIT_URL/info/refs?service=git-receive-pack` requests. See
+	`--http-backend-info-refs` in linkgit:git-upload-pack[1].
+
 PRE-RECEIVE HOOK
 ----------------
 Before any ref is updated, if $GIT_DIR/hooks/pre-receive file exists
diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index 9822c1eb1ad..739416ec83d 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -36,10 +36,14 @@ OPTIONS
 	This fits with the HTTP POST request processing model where
 	a program may read the request, write a response, and must exit.
 
---advertise-refs::
-	Only the initial ref advertisement is output, and the program exits
-	immediately. This fits with the HTTP GET request model, where
-	no request content is received but a response must be produced.
+--http-backend-info-refs::
+	Used by linkgit:git-http-backend[1] to serve up
+	`$GIT_URL/info/refs?service=git-upload-pack` requests. See
+	"Smart Clients" in link:technical/http-protocol.html[the HTTP
+	transfer protocols] documentation and "HTTP Transport" in
+	link:technical/protocol-v2.html[the Git Wire Protocol, Version
+	2] documentation. Also understood by
+	linkgit:git-receive-pack[1].
 
 <directory>::
 	The repository to sync from.
diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index 96d89ea9b22..cc5126cfeda 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -225,6 +225,9 @@ The client may send Extra Parameters (see
 Documentation/technical/pack-protocol.txt) as a colon-separated string
 in the Git-Protocol HTTP header.
 
+Uses the `--http-backend-info-refs` option to
+linkgit:git-upload-pack[1].
+
 Dumb Server Response
 ^^^^^^^^^^^^^^^^^^^^
 Dumb servers MUST respond with the dumb server reply format.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1040d853198..213538f1d0e 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -81,6 +81,9 @@ A v2 server would reply:
 Subsequent requests are then made directly to the service
 `$GIT_URL/git-upload-pack`. (This works the same for git-receive-pack).
 
+Uses the `--http-backend-info-refs` option to
+linkgit:git-upload-pack[1].
+
 Capability Advertisement
 ------------------------
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513ac..b12820dbd6c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2478,7 +2478,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("quiet")),
 		OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
-		OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
+		OPT_HIDDEN_BOOL(0, "http-backend-info-refs", &advertise_refs, NULL),
+		OPT_ALIAS(0, "advertise-refs", "http-backend-info-refs"),
 		OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL),
 		OPT_END()
 	};
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 8506030a648..125af53885f 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -22,8 +22,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-refs", &advertise_refs,
-			 N_("exit immediately after initial ref advertisement")),
+		OPT_HIDDEN_BOOL(0, "http-backend-info-refs", &advertise_refs,
+				N_("serve up the info/refs for git-http-backend")),
+		OPT_ALIAS(0, "advertise-refs", "http-backend-info-refs"),
 		OPT_BOOL(0, "strict", &strict,
 			 N_("do not try <directory>/.git/ if <directory> is no Git directory")),
 		OPT_INTEGER(0, "timeout", &timeout,
diff --git a/http-backend.c b/http-backend.c
index d37463cec8b..838374edb91 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -534,7 +534,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
 
 	if (service_name) {
 		const char *argv[] = {NULL /* service name */,
-			"--advertise-refs",
+			"--http-backend-info-refs",
 			".", NULL};
 		struct rpc_service *svc = select_service(hdr, service_name);
 
diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
index 389ee96987b..8d9d6a556f0 100755
--- a/t/t5555-http-smart-common.sh
+++ b/t/t5555-http-smart-common.sh
@@ -6,6 +6,20 @@ test_expect_success 'setup' '
 	test_commit --no-tag initial
 '
 
+test_expect_success 'git upload-pack --http-backend-info-refs and --advertise-refs are aliased' '
+	git upload-pack --http-backend-info-refs . >expected 2>err.expected &&
+	git upload-pack --advertise-refs . >actual 2>err.actual &&
+	test_cmp err.expected err.actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git receive-pack --http-backend-info-refs and --advertise-refs are aliased' '
+	git receive-pack --http-backend-info-refs . >expected 2>err.expected &&
+	git receive-pack --advertise-refs . >actual 2>err.actual &&
+	test_cmp err.expected err.actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'git upload-pack --advertise-refs' '
 	cat >expect <<-EOF &&
 	$(git rev-parse HEAD) HEAD
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v3 11/12] upload-pack.c: convert to new serve.c "startup" config cb

[Ævar Arnfjörð Bjarmason]

Convert the config reading code in upload-pack.c to use the new
"startup_config" callback when we're using the v2 protocol, and
lightly fake up the same when we're using v0 and v1.

Before the series that fixed 08450ef7918 (upload-pack: clear
filter_options for each v2 fetch command, 2020-05-08) landed, most of
what's now "struct upload_pack_data" used to be static variables at
the top of this file.

This moves some of them back. See f203a88cf14 (upload-pack: move
keepalive to upload_pack_data, 2020-06-04), f1514c6aad0 (upload-pack:
move allow_unadvertised_object_request to upload_pack_data,
2020-06-11) etc.

I think this makes it easier to understand the this code, as we're now
clearly separating data and config that changes on a
request-to-request basis (see 08450ef7918), and the sort of config
that serve.c's "startup_config" is aimed at.

Thus it's clear that the "allow_uor" passed to functions like
"is_our_ref()" is constant after startup (usually it'll never change
for a given server's configuration, or change once).

This requires a very light compatibility layer with the serve.c
callback mechanism in the form of "upload_pack" for the v0 and v1
protocols.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c       |   1 +
 upload-pack.c | 133 +++++++++++++++++++++++++++++---------------------
 upload-pack.h |   2 +
 3 files changed, 80 insertions(+), 56 deletions(-)

diff --git a/serve.c b/serve.c
index cacb0bc4e0e..162e8cea550 100644
--- a/serve.c
+++ b/serve.c
@@ -112,6 +112,7 @@ static struct protocol_capability capabilities[] = {
 	},
 	{
 		.name = "fetch",
+		.startup_config = serve_upload_pack_startup_config,
 		.advertise = upload_pack_advertise,
 		.command = upload_pack_v2,
 	},
diff --git a/upload-pack.c b/upload-pack.c
index 48e8bf5694c..b9cfc09795b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -52,6 +52,19 @@ enum allow_uor {
 	ALLOW_ANY_SHA1 = 0x07
 };
 
+/*
+ * "Global" configuration that won't be affected by the type of
+ * request we're doing, or by other request data in "struct
+ * upload_pack_data" below.
+ */
+static int v1_have_startup_config;
+static int config_keepalive_secs = 5;
+static enum allow_uor config_allow_uor;
+static unsigned config_advertise_sid;
+static const char *config_pack_objects_hook;
+static unsigned config_v2_allow_ref_in_want;
+static unsigned config_v2_allow_sideband_all;
+
 /*
  * Please annotate, and if possible group together, fields used only
  * for protocol v0 or only for protocol v2.
@@ -70,7 +83,6 @@ struct upload_pack_data {
 	timestamp_t deepen_since;
 	int deepen_rev_list;
 	int deepen_relative;
-	int keepalive;
 	int shallow_nr;
 	timestamp_t oldest_have;
 
@@ -85,15 +97,12 @@ struct upload_pack_data {
 	int use_sideband;
 
 	struct string_list uri_protocols;
-	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
 	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
-	const char *pack_objects_hook;
-
 	unsigned stateless_rpc : 1;				/* v0 only */
 	unsigned no_done : 1;					/* v0 only */
 	unsigned daemon_mode : 1;				/* v0 only */
@@ -109,9 +118,6 @@ struct upload_pack_data {
 	unsigned long tree_filter_max_depth;
 
 	unsigned done : 1;					/* v2 only */
-	unsigned allow_ref_in_want : 1;				/* v2 only */
-	unsigned allow_sideband_all : 1;			/* v2 only */
-	unsigned advertise_sid : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)
@@ -141,9 +147,6 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->allow_filter_fallback = 1;
 	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
-
-	data->keepalive = 5;
-	data->advertise_sid = 0;
 }
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
@@ -158,8 +161,6 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
 	string_list_clear(&data->allowed_filters, 0);
-
-	free((char *)data->pack_objects_hook);
 }
 
 static void reset_timeout(unsigned int timeout)
@@ -277,10 +278,10 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 	int i;
 	FILE *pipe_fd;
 
-	if (!pack_data->pack_objects_hook)
+	if (!config_pack_objects_hook)
 		pack_objects.git_cmd = 1;
 	else {
-		strvec_push(&pack_objects.args, pack_data->pack_objects_hook);
+		strvec_push(&pack_objects.args, config_pack_objects_hook);
 		strvec_push(&pack_objects.args, "git");
 		pack_objects.use_shell = 1;
 	}
@@ -371,9 +372,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		if (!pollsize)
 			break;
 
-		polltimeout = pack_data->keepalive < 0
+		polltimeout = config_keepalive_secs < 0
 			? -1
-			: 1000 * pack_data->keepalive;
+			: 1000 * config_keepalive_secs;
 
 		ret = poll(pfd, pollsize, polltimeout);
 
@@ -581,9 +582,9 @@ static int get_common_commits(struct upload_pack_data *data,
 	}
 }
 
-static int is_our_ref(struct object *o, enum allow_uor allow_uor)
+static int is_our_ref(struct object *o)
 {
-	int allow_hidden_ref = (allow_uor &
+	int allow_hidden_ref = (config_allow_uor &
 				(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
@@ -593,8 +594,7 @@ static int is_our_ref(struct object *o, enum allow_uor allow_uor)
  */
 static int do_reachable_revlist(struct child_process *cmd,
 				struct object_array *src,
-				struct object_array *reachable,
-				enum allow_uor allow_uor)
+				struct object_array *reachable)
 {
 	static const char *argv[] = {
 		"rev-list", "--stdin", NULL,
@@ -627,14 +627,14 @@ static int do_reachable_revlist(struct child_process *cmd,
 			continue;
 		if (reachable && o->type == OBJ_COMMIT)
 			o->flags &= ~TMP_MARK;
-		if (!is_our_ref(o, allow_uor))
+		if (!is_our_ref(o))
 			continue;
 		if (fprintf(cmd_in, "^%s\n", oid_to_hex(&o->oid)) < 0)
 			goto error;
 	}
 	for (i = 0; i < src->nr; i++) {
 		o = src->objects[i].item;
-		if (is_our_ref(o, allow_uor)) {
+		if (is_our_ref(o)) {
 			if (reachable)
 				add_object_array(o, NULL, reachable);
 			continue;
@@ -671,8 +671,7 @@ static int get_reachable_list(struct upload_pack_data *data,
 	char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
 	const unsigned hexsz = the_hash_algo->hexsz;
 
-	if (do_reachable_revlist(&cmd, &data->shallows, reachable,
-				 data->allow_uor) < 0)
+	if (do_reachable_revlist(&cmd, &data->shallows, reachable) < 0)
 		return -1;
 
 	while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) {
@@ -703,13 +702,13 @@ static int get_reachable_list(struct upload_pack_data *data,
 	return 0;
 }
 
-static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
+static int has_unreachable(struct object_array *src)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	char buf[1];
 	int i;
 
-	if (do_reachable_revlist(&cmd, src, NULL, allow_uor) < 0)
+	if (do_reachable_revlist(&cmd, src, NULL) < 0)
 		return 1;
 
 	/*
@@ -748,9 +747,9 @@ static void check_non_tip(struct upload_pack_data *data)
 	 * uploadpack.allowReachableSHA1InWant,
 	 * non-tip requests can never happen.
 	 */
-	if (!data->stateless_rpc && !(data->allow_uor & ALLOW_REACHABLE_SHA1))
+	if (!data->stateless_rpc && !(config_allow_uor & ALLOW_REACHABLE_SHA1))
 		goto error;
-	if (!has_unreachable(&data->want_obj, data->allow_uor))
+	if (!has_unreachable(&data->want_obj))
 		/* All the non-tip ones are ancestors of what we advertised */
 		return;
 
@@ -758,7 +757,7 @@ static void check_non_tip(struct upload_pack_data *data)
 	/* Pick one of them (we know there at least is one) */
 	for (i = 0; i < data->want_obj.nr; i++) {
 		struct object *o = data->want_obj.objects[i].item;
-		if (!is_our_ref(o, data->allow_uor)) {
+		if (!is_our_ref(o)) {
 			packet_writer_error(&data->writer,
 					    "upload-pack: not our ref %s",
 					    oid_to_hex(&o->oid));
@@ -1123,8 +1122,8 @@ static void receive_needs(struct upload_pack_data *data,
 		}
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
-			if (!((data->allow_uor & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
-			      || is_our_ref(o, data->allow_uor)))
+			if (!((config_allow_uor & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
+			      || is_our_ref(o)))
 				has_non_tip = 1;
 			add_object_array(o, NULL, &data->want_obj);
 		}
@@ -1184,7 +1183,7 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
 }
 
 static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {
-	if (d->advertise_sid)
+	if (config_advertise_sid)
 		strbuf_addf(buf, " session-id=%s", trace2_session_id());
 }
 
@@ -1210,9 +1209,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 		packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n",
 			     oid_to_hex(oid), refname_nons,
 			     0, capabilities,
-			     (data->allow_uor & ALLOW_TIP_SHA1) ?
+			     (config_allow_uor & ALLOW_TIP_SHA1) ?
 				     " allow-tip-sha1-in-want" : "",
-			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
+			     (config_allow_uor & ALLOW_REACHABLE_SHA1) ?
 				     " allow-reachable-sha1-in-want" : "",
 			     data->no_done ? " no-done" : "",
 			     symref_info.buf,
@@ -1282,46 +1281,66 @@ static int parse_object_filter_config(const char *var, const char *value,
 	return 0;
 }
 
-static int upload_pack_config(const char *var, const char *value, void *cb_data)
+static int upload_pack_startup_config(const char *var, const char *value,
+				      void *cb_data)
 {
-	struct upload_pack_data *data = cb_data;
-
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_TIP_SHA1;
+			config_allow_uor |= ALLOW_TIP_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_TIP_SHA1;
+			config_allow_uor &= ~ALLOW_TIP_SHA1;
 	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_REACHABLE_SHA1;
+			config_allow_uor |= ALLOW_REACHABLE_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_REACHABLE_SHA1;
+			config_allow_uor &= ~ALLOW_REACHABLE_SHA1;
 	} else if (!strcmp("uploadpack.allowanysha1inwant", var)) {
 		if (git_config_bool(var, value))
-			data->allow_uor |= ALLOW_ANY_SHA1;
+			config_allow_uor |= ALLOW_ANY_SHA1;
 		else
-			data->allow_uor &= ~ALLOW_ANY_SHA1;
+			config_allow_uor &= ~ALLOW_ANY_SHA1;
 	} else if (!strcmp("uploadpack.keepalive", var)) {
-		data->keepalive = git_config_int(var, value);
-		if (!data->keepalive)
-			data->keepalive = -1;
-	} else if (!strcmp("uploadpack.allowfilter", var)) {
-		data->allow_filter = git_config_bool(var, value);
-	} else if (!strcmp("uploadpack.allowrefinwant", var)) {
-		data->allow_ref_in_want = git_config_bool(var, value);
-	} else if (!strcmp("uploadpack.allowsidebandall", var)) {
-		data->allow_sideband_all = git_config_bool(var, value);
+		config_keepalive_secs = git_config_int(var, value);
+		if (!config_keepalive_secs)
+			config_keepalive_secs = -1;
 	} else if (!strcmp("core.precomposeunicode", var)) {
 		precomposed_unicode = git_config_bool(var, value);
 	} else if (!strcmp("transfer.advertisesid", var)) {
-		data->advertise_sid = git_config_bool(var, value);
+		config_advertise_sid = git_config_bool(var, value);
 	}
 
 	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
 	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
 		if (!strcmp("uploadpack.packobjectshook", var))
-			return git_config_string(&data->pack_objects_hook, var, value);
+			return git_config_string(&config_pack_objects_hook, var, value);
 	}
+	return 0;
+}
+
+static int upload_pack_startup_config_v2_only(const char *var,
+					      const char *value, void *cb_data)
+{
+	if (!strcmp("uploadpack.allowrefinwant", var))
+		config_v2_allow_ref_in_want = git_config_bool(var, value);
+	else if (!strcmp("uploadpack.allowsidebandall", var))
+		config_v2_allow_sideband_all = git_config_bool(var, value);
+	return 0;
+}
+
+int serve_upload_pack_startup_config(const char *var, const char *value, void *data)
+{
+	upload_pack_startup_config(var, value, data);
+	upload_pack_startup_config_v2_only(var, value, data);
+	return 0;
+}
+
+static int upload_pack_config(const char *var, const char *value,
+			      void *cb_data)
+{
+	struct upload_pack_data *data = cb_data;
+
+	if (!strcmp("uploadpack.allowfilter", var))
+		data->allow_filter = git_config_bool(var, value);
 
 	if (parse_object_filter_config(var, value, data) < 0)
 		return -1;
@@ -1337,6 +1356,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
 
 	upload_pack_data_init(&data);
 
+	if (!v1_have_startup_config++)
+		git_config(upload_pack_startup_config, NULL);
 	git_config(upload_pack_config, &data);
 
 	data.stateless_rpc = stateless_rpc;
@@ -1472,7 +1493,7 @@ static void process_args(struct packet_reader *request,
 		/* process want */
 		if (parse_want(&data->writer, arg, &data->want_obj))
 			continue;
-		if (data->allow_ref_in_want &&
+		if (config_v2_allow_ref_in_want &&
 		    parse_want_ref(&data->writer, arg, &data->wanted_refs,
 				   &data->want_obj))
 			continue;
@@ -1530,7 +1551,7 @@ static void process_args(struct packet_reader *request,
 		}
 
 		if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
-		     data->allow_sideband_all) &&
+		     config_v2_allow_sideband_all) &&
 		    !strcmp(arg, "sideband-all")) {
 			data->writer.use_sideband = 1;
 			continue;
diff --git a/upload-pack.h b/upload-pack.h
index a06498bbea7..52011d9ecff 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -13,5 +13,7 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,
 struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
+int serve_upload_pack_startup_config(const char *var, const char *value,
+				     void *data);
 
 #endif /* UPLOAD_PACK_H */
-- 
2.32.0.955.ge7c5360f7e7

[PATCH v3 12/12] serve.[ch]: don't pass "struct strvec *keys" to commands

[Ævar Arnfjörð Bjarmason]

The serve.c API added in ed10cb952d3 (serve: introduce git-serve,
2018-03-15) was passing in the raw capabilities "keys", but nothing
downstream of it ever used them.

Let's remove that code because it's not needed, and because if and
when we need to pass data about the advertisement (I have some WIP
patches for that), it makes much more sense to have the serve.c parse
the capabilities, and then pass specific information we need down than
expecting its API users to re-do their own parsing of the raw data.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c       | 3 +--
 ls-refs.h       | 4 +---
 protocol-caps.c | 3 +--
 protocol-caps.h | 4 +---
 serve.c         | 7 +++----
 upload-pack.c   | 3 +--
 upload-pack.h   | 4 +---
 7 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/ls-refs.c b/ls-refs.c
index 02fbcfd9bde..787e3849d97 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -127,8 +127,7 @@ static int ls_refs_config(const char *var, const char *value, void *data)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int ls_refs(struct repository *r, struct strvec *keys,
-	    struct packet_reader *request)
+int ls_refs(struct repository *r, struct packet_reader *request)
 {
 	struct ls_refs_data data;
 
diff --git a/ls-refs.h b/ls-refs.h
index 5fcab0d1dca..32f9e0eddd0 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -2,10 +2,8 @@
 #define LS_REFS_H
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int ls_refs(struct repository *r, struct strvec *keys,
-	    struct packet_reader *request);
+int ls_refs(struct repository *r, 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);
 
diff --git a/protocol-caps.c b/protocol-caps.c
index 901b6795e42..bbde91810ac 100644
--- a/protocol-caps.c
+++ b/protocol-caps.c
@@ -75,8 +75,7 @@ static void send_info(struct repository *r, struct packet_writer *writer,
 	strbuf_release(&send_buffer);
 }
 
-int cap_object_info(struct repository *r, struct strvec *keys,
-		    struct packet_reader *request)
+int cap_object_info(struct repository *r, struct packet_reader *request)
 {
 	struct requested_info info;
 	struct packet_writer writer;
diff --git a/protocol-caps.h b/protocol-caps.h
index 0a9f49df115..15c4550360c 100644
--- a/protocol-caps.h
+++ b/protocol-caps.h
@@ -2,9 +2,7 @@
 #define PROTOCOL_CAPS_H
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int cap_object_info(struct repository *r, struct strvec *keys,
-		    struct packet_reader *request);
+int cap_object_info(struct repository *r, struct packet_reader *request);
 
 #endif /* PROTOCOL_CAPS_H */
diff --git a/serve.c b/serve.c
index 162e8cea550..3e82178c0f7 100644
--- a/serve.c
+++ b/serve.c
@@ -50,7 +50,7 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)
 }
 
 typedef int (*advertise_fn_t)(struct repository *r, struct strbuf *value);
-typedef int (*command_fn_t)(struct repository *r, struct strvec *keys,
+typedef int (*command_fn_t)(struct repository *r,
 			    struct packet_reader *request);
 
 struct protocol_capability {
@@ -89,8 +89,7 @@ struct protocol_capability {
 
 	/*
 	 * Function called when a client requests the capability as a command.
-	 * The function will be provided the capabilities requested via 'keys'
-	 * as well as a struct packet_reader 'request' which the command should
+	 * Will be provided a struct packet_reader 'request' which it should
 	 * use to read the command specific part of the request.  Every command
 	 * MUST read until a flush packet is seen before sending a response.
 	 *
@@ -160,7 +159,7 @@ static int call_command(struct protocol_capability *command,
 
 	read_startup_config(command);
 
-	return command->command(r, keys, request);
+	return command->command(r, request);
 }
 
 void protocol_v2_advertise_capabilities(void)
diff --git a/upload-pack.c b/upload-pack.c
index b9cfc09795b..097b982c010 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1680,8 +1680,7 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
-int upload_pack_v2(struct repository *r, struct strvec *keys,
-		   struct packet_reader *request)
+int upload_pack_v2(struct repository *r, struct packet_reader *request)
 {
 	enum fetch_state state = FETCH_PROCESS_ARGS;
 	struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index 52011d9ecff..0ae817c1d0d 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -5,10 +5,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
 		 const int timeout);
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int upload_pack_v2(struct repository *r, struct strvec *keys,
-		   struct packet_reader *request);
+int upload_pack_v2(struct repository *r, struct packet_reader *request);
 
 struct strbuf;
 int upload_pack_advertise(struct repository *r,
-- 
2.32.0.955.ge7c5360f7e7

Re: [PATCH v3 00/12] serve.[ch]: general API cleanup

[Josh Steadmon]

On 2021.07.22 01:40, Ævar Arnfjörð Bjarmason wrote:
> For v2, see:
> https://lore.kernel.org/git/cover-0.8-00000000000-20210628T191634Z-avarab@gmail.com/
> 
> Junio: Sorry about the late update, re:
> http://lore.kernel.org/git/xmqqsg07cz4g.fsf@gitster.g
> 
> I think most people were on the fence about the need for this
> "startup_config" callback in v2. I think we could still do without it,
> but as the new 08/12 shows a big part of the API complexity was due to
> serve() supporting the --advertise-refs mode directly. 08-10/12 is a
> new late-series cleanup of that, which makes serve.[ch] a lot simpler.
> 
> Ævar Arnfjörð Bjarmason (12):
>   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: add support for a "startup" git_config() callback
>   serve.c: move version line to advertise_capabilities()
>   serve.[ch]: remove "serve_options", split up --advertise-refs code
>   {upload,receive}-pack tests: add --advertise-refs tests
>   upload-pack: document and rename --advertise-refs
>   upload-pack.c: convert to new serve.c "startup" config cb
>   serve.[ch]: don't pass "struct strvec *keys" to commands

Sorry for the delayed response on this series; I've been away from work
for the past several weeks.

I'm in favor of the cleanups in patches 1-4. I'm afraid I am not
familiar enough with the server code to have useful feedback on the rest
of the patches, at least not at first glance.

[PATCH v4 00/10] serve.[ch]: general API cleanup + --advertise-refs cleanup

[Ævar Arnfjörð Bjarmason]

This is a general API cleanup and simplifiction of of serve.[ch] and
its interaction with "upload-pack --advertise-refs".

Previous rounds of this series had a way to define "startup config"
for commands hanging off the v2 server. That's now entirely gone,
along with prep work required for it.

For v3, see :
https://lore.kernel.org/git/cover-00.12-00000000000-20210721T233307Z-avarab@gmail.com/

The fixup for the missing test shebang has also been squashed in,
thanks Carlo!:
https://lore.kernel.org/git/20210803060005.18151-1-carenas@gmail.com/

Despite the overall diffstat this is a net deletion of code in any
meaningful sense of reducing complexity, i.e. it goes into the plus on
the diffstat due to the addition of previously missing tests, and the
use of designated initializers.

Ævar Arnfjörð Bjarmason (10):
  serve: mark has_capability() as static
  transport: rename "fetch" in transport_vtable to "fetch_refs"
  transport: use designated initializers
  serve: use designated initializers
  serve.[ch]: don't pass "struct strvec *keys" to commands
  serve: move transfer.advertiseSID check into session_id_advertise()
  serve.c: move version line to advertise_capabilities()
  {upload,receive}-pack tests: add --advertise-refs tests
  serve.[ch]: remove "serve_options", split up --advertise-refs code
  upload-pack: document and rename --advertise-refs

 Documentation/git-receive-pack.txt        |   5 +
 Documentation/git-upload-pack.txt         |  12 +-
 Documentation/technical/http-protocol.txt |   3 +
 Documentation/technical/protocol-v2.txt   |   3 +
 builtin/receive-pack.c                    |   3 +-
 builtin/upload-pack.c                     |  28 ++--
 http-backend.c                            |   2 +-
 ls-refs.c                                 |   3 +-
 ls-refs.h                                 |   4 +-
 protocol-caps.c                           |   3 +-
 protocol-caps.h                           |   4 +-
 serve.c                                   |  82 ++++++-----
 serve.h                                   |  12 +-
 t/helper/test-serve-v2.c                  |  14 +-
 t/t5555-http-smart-common.sh              | 161 ++++++++++++++++++++++
 transport-helper.c                        |  18 +--
 transport-internal.h                      |   2 +-
 transport.c                               |  32 ++---
 upload-pack.c                             |  21 +--
 upload-pack.h                             |  14 +-
 20 files changed, 298 insertions(+), 128 deletions(-)
 create mode 100755 t/t5555-http-smart-common.sh

Range-diff against v3:
 1:  192fb64ef82 =  1:  13bb1a06078 serve: mark has_capability() as static
 2:  d716bd3c537 =  2:  162f717436f transport: rename "fetch" in transport_vtable to "fetch_refs"
 3:  d31690614af =  3:  d3d5818adda transport: use designated initializers
 4:  13f1a8d8325 =  4:  8da15c373cc serve: use designated initializers
 5:  99eeff6f890 <  -:  ----------- serve.c: add call_{advertise,command}() indirection
 6:  be719dc3dc1 <  -:  ----------- serve: add support for a "startup" git_config() callback
12:  e4eb31b5b8e !  5:  7714f81d62c serve.[ch]: don't pass "struct strvec *keys" to commands
    @@ Commit message
         2018-03-15) was passing in the raw capabilities "keys", but nothing
         downstream of it ever used them.
     
    -    Let's remove that code because it's not needed, and because if and
    -    when we need to pass data about the advertisement (I have some WIP
    -    patches for that), it makes much more sense to have the serve.c parse
    -    the capabilities, and then pass specific information we need down than
    -    expecting its API users to re-do their own parsing of the raw data.
    +    Let's remove that code because it's not needed. If we do end up
    +    needing to pass information about the advertisement in the future
    +    it'll make more sense to have serve.c parse the capabilities keys and
    +    pass the result of its parsing, rather than expecting expecting its
    +    API users to parse the same keys again.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ ls-refs.h
     -int ls_refs(struct repository *r, struct strvec *keys,
     -	    struct packet_reader *request);
     +int ls_refs(struct repository *r, 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 */
     
      ## protocol-caps.c ##
     @@ protocol-caps.c: static void send_info(struct repository *r, struct packet_writer *writer,
    - 	strbuf_release(&send_buffer);
    + 	}
      }
      
     -int cap_object_info(struct repository *r, struct strvec *keys,
    @@ protocol-caps.h
      #endif /* PROTOCOL_CAPS_H */
     
      ## serve.c ##
    -@@ serve.c: static int session_id_advertise(struct repository *r, struct strbuf *value)
    - }
    - 
    - typedef int (*advertise_fn_t)(struct repository *r, struct strbuf *value);
    --typedef int (*command_fn_t)(struct repository *r, struct strvec *keys,
    -+typedef int (*command_fn_t)(struct repository *r,
    - 			    struct packet_reader *request);
    - 
    - struct protocol_capability {
     @@ serve.c: struct protocol_capability {
      
      	/*
    @@ serve.c: struct protocol_capability {
      	 * use to read the command specific part of the request.  Every command
      	 * MUST read until a flush packet is seen before sending a response.
      	 *
    -@@ serve.c: static int call_command(struct protocol_capability *command,
    + 	 * This field should be NULL for capabilities which are not commands.
    + 	 */
    +-	int (*command)(struct repository *r,
    +-		       struct strvec *keys,
    +-		       struct packet_reader *request);
    ++	int (*command)(struct repository *r, struct packet_reader *request);
    + };
      
    - 	read_startup_config(command);
    + static struct protocol_capability capabilities[] = {
    +@@ serve.c: static int process_request(void)
    + 	if (has_capability(&keys, "session-id", &client_sid))
    + 		trace2_data_string("transfer", NULL, "client-sid", client_sid);
      
    --	return command->command(r, keys, request);
    -+	return command->command(r, request);
    - }
    +-	command->command(the_repository, &keys, &reader);
    ++	command->command(the_repository, &reader);
      
    - void protocol_v2_advertise_capabilities(void)
    + 	strvec_clear(&keys);
    + 	return 0;
     
      ## upload-pack.c ##
     @@ upload-pack.c: enum fetch_state {
    @@ upload-pack.c: enum fetch_state {
      	struct upload_pack_data data;
     
      ## upload-pack.h ##
    -@@ upload-pack.h: void upload_pack(const int advertise_refs, const int stateless_rpc,
    - 		 const int timeout);
    +@@ upload-pack.h: struct upload_pack_options {
    + void upload_pack(struct upload_pack_options *options);
      
      struct repository;
     -struct strvec;
 -:  ----------- >  6:  33db6b7cf53 serve: move transfer.advertiseSID check into session_id_advertise()
 7:  b7928ddbe9b !  7:  9828e08cbad serve.c: move version line to advertise_capabilities()
    @@ Commit message
         serve.c: move version line to advertise_capabilities()
     
         The advertise_capabilities() is only called from serve() and we always
    -    emit this version line before it, it makes more sense to consider the
    -    capabilities part of a "header" that has the version, so let's move
    -    the writing of the version there.
    +    emit this version line before it. In a subsequent commit I'll make
    +    builtin/upload-pack.c sometimes call advertise_capabilities()
    +    directly, so it'll make sense to have this line emitted by
    +    advertise_capabilities(), not serve() itself.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 9:  beafe9811c1 !  8:  fd58abb7420 {upload,receive}-pack tests: add --advertise-refs tests
    @@ Commit message
     
      ## t/t5555-http-smart-common.sh (new) ##
     @@
    ++#!/bin/sh
    ++
     +test_description='test functionality common to smart fetch & push'
     +
     +. ./test-lib.sh
 8:  fb80f152713 !  9:  c9a35868933 serve.[ch]: remove "serve_options", split up --advertise-refs code
    @@ http-backend.c: static void get_info_refs(struct strbuf *hdr, char *arg)
      
     
      ## serve.c ##
    -@@ serve.c: static int call_command(struct protocol_capability *command,
    - 	return command->command(r, keys, request);
    - }
    +@@ serve.c: static struct protocol_capability capabilities[] = {
    + 	},
    + };
      
     -static void advertise_capabilities(void)
     +void protocol_v2_advertise_capabilities(void)
    @@ upload-pack.h
     +		 const int timeout);
      
      struct repository;
    - struct strvec;
    + struct packet_reader;
10:  c6870b5f18a = 10:  2006cb234b6 upload-pack: document and rename --advertise-refs
11:  2d4c3d0d463 <  -:  ----------- upload-pack.c: convert to new serve.c "startup" config cb
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v4 01/10] serve: mark has_capability() as static

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 serve.c | 4 ++--
 serve.h | 4 ----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/serve.c b/serve.c
index f11c0e07c45..7bf5f23ea6a 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.33.0.rc0.597.gc569a812f0a

[PATCH v4 02/10] transport: rename "fetch" in transport_vtable to "fetch_refs"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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 17e9629710a..3e8a27b0321 100644
--- a/transport.c
+++ b/transport.c
@@ -1453,7 +1453,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.33.0.rc0.597.gc569a812f0a

[PATCH v4 03/10] transport: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 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 3e8a27b0321..f9400b9b0bd 100644
--- a/transport.c
+++ b/transport.c
@@ -883,12 +883,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,
@@ -1032,21 +1030,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.33.0.rc0.597.gc569a812f0a

[PATCH v4 04/10] serve: use designated initializers

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 serve.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/serve.c b/serve.c
index 7bf5f23ea6a..33d00c8c634 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.33.0.rc0.597.gc569a812f0a

[PATCH v4 05/10] serve.[ch]: don't pass "struct strvec *keys" to commands

[Ævar Arnfjörð Bjarmason]

The serve.c API added in ed10cb952d3 (serve: introduce git-serve,
2018-03-15) was passing in the raw capabilities "keys", but nothing
downstream of it ever used them.

Let's remove that code because it's not needed. If we do end up
needing to pass information about the advertisement in the future
it'll make more sense to have serve.c parse the capabilities keys and
pass the result of its parsing, rather than expecting expecting its
API users to parse the same keys again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c       | 3 +--
 ls-refs.h       | 4 +---
 protocol-caps.c | 3 +--
 protocol-caps.h | 4 +---
 serve.c         | 9 +++------
 upload-pack.c   | 3 +--
 upload-pack.h   | 4 +---
 7 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/ls-refs.c b/ls-refs.c
index 88f6c3f60d8..a1a0250607f 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -138,8 +138,7 @@ static int ls_refs_config(const char *var, const char *value, void *data)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int ls_refs(struct repository *r, struct strvec *keys,
-	    struct packet_reader *request)
+int ls_refs(struct repository *r, struct packet_reader *request)
 {
 	struct ls_refs_data data;
 
diff --git a/ls-refs.h b/ls-refs.h
index a99e4be0bdd..e2243a547bb 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -2,10 +2,8 @@
 #define LS_REFS_H
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int ls_refs(struct repository *r, struct strvec *keys,
-	    struct packet_reader *request);
+int ls_refs(struct repository *r, struct packet_reader *request);
 int ls_refs_advertise(struct repository *r, struct strbuf *value);
 
 #endif /* LS_REFS_H */
diff --git a/protocol-caps.c b/protocol-caps.c
index 13a9e63a04a..4b345c55046 100644
--- a/protocol-caps.c
+++ b/protocol-caps.c
@@ -74,8 +74,7 @@ static void send_info(struct repository *r, struct packet_writer *writer,
 	}
 }
 
-int cap_object_info(struct repository *r, struct strvec *keys,
-		    struct packet_reader *request)
+int cap_object_info(struct repository *r, struct packet_reader *request)
 {
 	struct requested_info info;
 	struct packet_writer writer;
diff --git a/protocol-caps.h b/protocol-caps.h
index 0a9f49df115..15c4550360c 100644
--- a/protocol-caps.h
+++ b/protocol-caps.h
@@ -2,9 +2,7 @@
 #define PROTOCOL_CAPS_H
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int cap_object_info(struct repository *r, struct strvec *keys,
-		    struct packet_reader *request);
+int cap_object_info(struct repository *r, struct packet_reader *request);
 
 #endif /* PROTOCOL_CAPS_H */
diff --git a/serve.c b/serve.c
index 33d00c8c634..967bf17d623 100644
--- a/serve.c
+++ b/serve.c
@@ -60,16 +60,13 @@ struct protocol_capability {
 
 	/*
 	 * Function called when a client requests the capability as a command.
-	 * The function will be provided the capabilities requested via 'keys'
-	 * as well as a struct packet_reader 'request' which the command should
+	 * Will be provided a struct packet_reader 'request' which it should
 	 * use to read the command specific part of the request.  Every command
 	 * MUST read until a flush packet is seen before sending a response.
 	 *
 	 * This field should be NULL for capabilities which are not commands.
 	 */
-	int (*command)(struct repository *r,
-		       struct strvec *keys,
-		       struct packet_reader *request);
+	int (*command)(struct repository *r, struct packet_reader *request);
 };
 
 static struct protocol_capability capabilities[] = {
@@ -294,7 +291,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);
+	command->command(the_repository, &reader);
 
 	strvec_clear(&keys);
 	return 0;
diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb43..ed60a9abd60 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1655,8 +1655,7 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
-int upload_pack_v2(struct repository *r, struct strvec *keys,
-		   struct packet_reader *request)
+int upload_pack_v2(struct repository *r, struct packet_reader *request)
 {
 	enum fetch_state state = FETCH_PROCESS_ARGS;
 	struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index 27ddcdc6cb0..63e3252c98d 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -11,10 +11,8 @@ struct upload_pack_options {
 void upload_pack(struct upload_pack_options *options);
 
 struct repository;
-struct strvec;
 struct packet_reader;
-int upload_pack_v2(struct repository *r, struct strvec *keys,
-		   struct packet_reader *request);
+int upload_pack_v2(struct repository *r, struct packet_reader *request);
 
 struct strbuf;
 int upload_pack_advertise(struct repository *r,
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v4 06/10] serve: move transfer.advertiseSID check into session_id_advertise()

[Ævar Arnfjörð Bjarmason]

In 6b5b6e422ee (serve: advertise session ID in v2 capabilities,
2020-11-11) the check for transfer.advertiseSID was added to the
beginning of the main serve() loop. Thus on startup of the server we'd
populate it.

Let's instead use an explicit lazy initialization pattern in
session_id_advertise() itself, we'll still look the config up only
once per-process, but by moving it out of serve() itself the further
changing of that routine becomes easier.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/serve.c b/serve.c
index 967bf17d623..26829991333 100644
--- a/serve.c
+++ b/serve.c
@@ -9,7 +9,7 @@
 #include "serve.h"
 #include "upload-pack.h"
 
-static int advertise_sid;
+static int advertise_sid = -1;
 
 static int always_advertise(struct repository *r,
 			    struct strbuf *value)
@@ -35,6 +35,9 @@ static int object_format_advertise(struct repository *r,
 
 static int session_id_advertise(struct repository *r, struct strbuf *value)
 {
+	if (advertise_sid == -1 &&
+	    git_config_get_bool("transfer.advertisesid", &advertise_sid))
+		advertise_sid = 0;
 	if (!advertise_sid)
 		return 0;
 	if (value)
@@ -300,8 +303,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.33.0.rc0.597.gc569a812f0a

[PATCH v4 07/10] serve.c: move version line to advertise_capabilities()

[Ævar Arnfjörð Bjarmason]

The advertise_capabilities() is only called from serve() and we always
emit this version line before it. In a subsequent commit I'll make
builtin/upload-pack.c sometimes call advertise_capabilities()
directly, so it'll make sense to have this line emitted by
advertise_capabilities(), not serve() itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 serve.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/serve.c b/serve.c
index 26829991333..412c2cd1317 100644
--- a/serve.c
+++ b/serve.c
@@ -112,6 +112,9 @@ static void advertise_capabilities(void)
 	struct strbuf value = STRBUF_INIT;
 	int i;
 
+	/* serve by default supports v2 */
+	packet_write_fmt(1, "version 2\n");
+
 	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
 		struct protocol_capability *c = &capabilities[i];
 
@@ -304,9 +307,6 @@ static int process_request(void)
 void serve(struct serve_options *options)
 {
 	if (options->advertise_capabilities || !options->stateless_rpc) {
-		/* serve by default supports v2 */
-		packet_write_fmt(1, "version 2\n");
-
 		advertise_capabilities();
 		/*
 		 * If only the list of capabilities was requested exit
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v4 08/10] {upload,receive}-pack tests: add --advertise-refs tests

[Ævar Arnfjörð Bjarmason]

The --advertise-refs option had no explicit tests of its own, only
other http tests that would fail at a distance if it it was
broken. Let's test its behavior explicitly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5555-http-smart-common.sh | 147 +++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100755 t/t5555-http-smart-common.sh

diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
new file mode 100755
index 00000000000..b813099ed2c
--- /dev/null
+++ b/t/t5555-http-smart-common.sh
@@ -0,0 +1,147 @@
+#!/bin/sh
+
+test_description='test functionality common to smart fetch & push'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit --no-tag initial
+'
+
+test_expect_success 'git upload-pack --advertise-refs' '
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	# We only care about GIT_PROTOCOL, not GIT_TEST_PROTOCOL_VERSION
+	sane_unset GIT_PROTOCOL &&
+	GIT_TEST_PROTOCOL_VERSION=2 \
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# The --advertise-refs alias works
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v0' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	git upload-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# With explicit v0
+	GIT_PROTOCOL=version=0 \
+	git upload-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v0' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	git receive-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect &&
+
+	# With explicit v0
+	GIT_PROTOCOL=version=0 \
+	git receive-pack --advertise-refs . >out 2>err &&
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v1' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	version 1
+	$(git rev-parse HEAD) HEAD
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=1 \
+	git upload-pack --advertise-refs . >out &&
+
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v1' '
+	# With no specified protocol
+	cat >expect <<-EOF &&
+	version 1
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=1 \
+	git receive-pack --advertise-refs . >out &&
+
+	test-tool pkt-line unpack <out >actual 2>err &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git upload-pack --advertise-refs: v2' '
+	cat >expect <<-EOF &&
+	version 2
+	agent=FAKE
+	ls-refs=unborn
+	fetch=shallow wait-for-done
+	server-option
+	object-format=$(test_oid algo)
+	object-info
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=2 \
+	GIT_USER_AGENT=FAKE \
+	git upload-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_expect_success 'git receive-pack --advertise-refs: v2' '
+	# There is no v2 yet for receive-pack, implicit v0
+	cat >expect <<-EOF &&
+	$(git rev-parse HEAD) $(git symbolic-ref HEAD)
+	0000
+	EOF
+
+	GIT_PROTOCOL=version=2 \
+	git receive-pack --advertise-refs . >out 2>err &&
+
+	test-tool pkt-line unpack <out >actual &&
+	test_must_be_empty err &&
+	test_cmp actual expect
+'
+
+test_done
-- 
2.33.0.rc0.597.gc569a812f0a

[PATCH v4 09/10] serve.[ch]: remove "serve_options", split up --advertise-refs code

[Ævar Arnfjörð Bjarmason]

The "advertise capabilities" mode of serve.c added in
ed10cb952d3 (serve: introduce git-serve, 2018-03-15) is only used by
the http-backend.c to call {upload,receive}-pack with the
--advertise-refs parameter. See 42526b478e3 (Add stateless RPC options
to upload-pack, receive-pack, 2009-10-30).

Let's just make cmd_upload_pack() take the two (v2) or three (v2)
parameters the the v2/v1 servicing functions need directly, and pass
those in via the function signature. The logic of whether daemon mode
is implied by the timeout belongs in the v1 function (only used
there).

Once we split up the "advertise v2 refs" from "serve v2 request" it
becomes clear that v2 never cared about those in combination. The only
time it mattered was for v1 to emit its ref advertisement, in that
case we wanted to emit the smart-http-only "no-done" capability.

Since we only do that in the --advertise-refs codepath let's just have
it set "do_done" itself in v1's upload_pack() just before send_ref(),
at that point --advertise-refs and --stateless-rpc in combination are
redundant (the only user is get_info_refs() in http-backend.c), so we
can just pass in --advertise-refs only.

Since we need to touch all the serve() and advertise_capabilities()
codepaths let's rename them to less clever and obvious names, it's
been suggested numerous times, the latest of which is [1]'s suggestion
for protocol_v2_serve_loop(). Let's go with that.

1. https://lore.kernel.org/git/CAFQ2z_NyGb8rju5CKzmo6KhZXD0Dp21u-BbyCb2aNxLEoSPRJw@mail.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/upload-pack.c    | 25 ++++++++++++-------------
 http-backend.c           |  2 +-
 serve.c                  | 18 +++++-------------
 serve.h                  |  8 ++------
 t/helper/test-serve-v2.c | 14 +++++++++-----
 upload-pack.c            | 18 +++++++++++-------
 upload-pack.h            | 10 ++--------
 7 files changed, 42 insertions(+), 53 deletions(-)

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 6da8fa2607c..8506030a648 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -16,16 +16,17 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 {
 	const char *dir;
 	int strict = 0;
-	struct upload_pack_options opts = { 0 };
-	struct serve_options serve_opts = SERVE_OPTIONS_INIT;
+	int advertise_refs = 0;
+	int stateless_rpc = 0;
+	int timeout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
+		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-refs", &opts.advertise_refs,
+		OPT_BOOL(0, "advertise-refs", &advertise_refs,
 			 N_("exit immediately after initial ref advertisement")),
 		OPT_BOOL(0, "strict", &strict,
 			 N_("do not try <directory>/.git/ if <directory> is no Git directory")),
-		OPT_INTEGER(0, "timeout", &opts.timeout,
+		OPT_INTEGER(0, "timeout", &timeout,
 			    N_("interrupt transfer after <n> seconds of inactivity")),
 		OPT_END()
 	};
@@ -38,9 +39,6 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	if (argc != 1)
 		usage_with_options(upload_pack_usage, options);
 
-	if (opts.timeout)
-		opts.daemon_mode = 1;
-
 	setup_path();
 
 	dir = argv[0];
@@ -50,21 +48,22 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
-		serve_opts.advertise_capabilities = opts.advertise_refs;
-		serve_opts.stateless_rpc = opts.stateless_rpc;
-		serve(&serve_opts);
+		if (advertise_refs)
+			protocol_v2_advertise_capabilities();
+		else
+			protocol_v2_serve_loop(stateless_rpc);
 		break;
 	case protocol_v1:
 		/*
 		 * v1 is just the original protocol with a version string,
 		 * so just fall through after writing the version string.
 		 */
-		if (opts.advertise_refs || !opts.stateless_rpc)
+		if (advertise_refs || !stateless_rpc)
 			packet_write_fmt(1, "version 1\n");
 
 		/* fallthrough */
 	case protocol_v0:
-		upload_pack(&opts);
+		upload_pack(advertise_refs, stateless_rpc, timeout);
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
diff --git a/http-backend.c b/http-backend.c
index b329bf63f09..d37463cec8b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -534,7 +534,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
 
 	if (service_name) {
 		const char *argv[] = {NULL /* service name */,
-			"--stateless-rpc", "--advertise-refs",
+			"--advertise-refs",
 			".", NULL};
 		struct rpc_service *svc = select_service(hdr, service_name);
 
diff --git a/serve.c b/serve.c
index 412c2cd1317..1817edc7f57 100644
--- a/serve.c
+++ b/serve.c
@@ -106,7 +106,7 @@ static struct protocol_capability capabilities[] = {
 	},
 };
 
-static void advertise_capabilities(void)
+void protocol_v2_advertise_capabilities(void)
 {
 	struct strbuf capability = STRBUF_INIT;
 	struct strbuf value = STRBUF_INIT;
@@ -303,24 +303,16 @@ static int process_request(void)
 	return 0;
 }
 
-/* Main serve loop for protocol version 2 */
-void serve(struct serve_options *options)
+void protocol_v2_serve_loop(int stateless_rpc)
 {
-	if (options->advertise_capabilities || !options->stateless_rpc) {
-		advertise_capabilities();
-		/*
-		 * If only the list of capabilities was requested exit
-		 * immediately after advertising capabilities
-		 */
-		if (options->advertise_capabilities)
-			return;
-	}
+	if (!stateless_rpc)
+		protocol_v2_advertise_capabilities();
 
 	/*
 	 * If stateless-rpc was requested then exit after
 	 * a single request/response exchange
 	 */
-	if (options->stateless_rpc) {
+	if (stateless_rpc) {
 		process_request();
 	} else {
 		for (;;)
diff --git a/serve.h b/serve.h
index 56da77a87af..f946cf904a2 100644
--- a/serve.h
+++ b/serve.h
@@ -1,11 +1,7 @@
 #ifndef SERVE_H
 #define SERVE_H
 
-struct serve_options {
-	unsigned advertise_capabilities;
-	unsigned stateless_rpc;
-};
-#define SERVE_OPTIONS_INIT { 0 }
-void serve(struct serve_options *options);
+void protocol_v2_advertise_capabilities(void);
+void protocol_v2_serve_loop(int stateless_rpc);
 
 #endif /* SERVE_H */
diff --git a/t/helper/test-serve-v2.c b/t/helper/test-serve-v2.c
index aee35e5aef4..28e905afc36 100644
--- a/t/helper/test-serve-v2.c
+++ b/t/helper/test-serve-v2.c
@@ -10,12 +10,12 @@ static char const * const serve_usage[] = {
 
 int cmd__serve_v2(int argc, const char **argv)
 {
-	struct serve_options opts = SERVE_OPTIONS_INIT;
-
+	int stateless_rpc = 0;
+	int advertise_capabilities = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
+		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-capabilities", &opts.advertise_capabilities,
+		OPT_BOOL(0, "advertise-capabilities", &advertise_capabilities,
 			 N_("exit immediately after advertising capabilities")),
 		OPT_END()
 	};
@@ -25,7 +25,11 @@ int cmd__serve_v2(int argc, const char **argv)
 	argc = parse_options(argc, argv, prefix, options, serve_usage,
 			     PARSE_OPT_KEEP_DASHDASH |
 			     PARSE_OPT_KEEP_UNKNOWN);
-	serve(&opts);
+
+	if (advertise_capabilities)
+		protocol_v2_advertise_capabilities();
+	else
+		protocol_v2_serve_loop(stateless_rpc);
 
 	return 0;
 }
diff --git a/upload-pack.c b/upload-pack.c
index ed60a9abd60..fc38f04d983 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1214,7 +1214,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 				     " allow-tip-sha1-in-want" : "",
 			     (data->allow_uor & ALLOW_REACHABLE_SHA1) ?
 				     " allow-reachable-sha1-in-want" : "",
-			     data->stateless_rpc ? " no-done" : "",
+			     data->no_done ? " no-done" : "",
 			     symref_info.buf,
 			     data->allow_filter ? " filter" : "",
 			     session_id.buf,
@@ -1329,7 +1329,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-void upload_pack(struct upload_pack_options *options)
+void upload_pack(const int advertise_refs, const int stateless_rpc,
+		 const int timeout)
 {
 	struct packet_reader reader;
 	struct upload_pack_data data;
@@ -1338,14 +1339,17 @@ void upload_pack(struct upload_pack_options *options)
 
 	git_config(upload_pack_config, &data);
 
-	data.stateless_rpc = options->stateless_rpc;
-	data.daemon_mode = options->daemon_mode;
-	data.timeout = options->timeout;
+	data.stateless_rpc = stateless_rpc;
+	data.timeout = timeout;
+	if (data.timeout)
+		data.daemon_mode = 1;
 
 	head_ref_namespaced(find_symref, &data.symref);
 
-	if (options->advertise_refs || !data.stateless_rpc) {
+	if (advertise_refs || !data.stateless_rpc) {
 		reset_timeout(data.timeout);
+		if (advertise_refs)
+			data.no_done = 1;
 		head_ref_namespaced(send_ref, &data);
 		for_each_namespaced_ref(send_ref, &data);
 		advertise_shallow_grafts(1);
@@ -1355,7 +1359,7 @@ void upload_pack(struct upload_pack_options *options)
 		for_each_namespaced_ref(check_ref, NULL);
 	}
 
-	if (!options->advertise_refs) {
+	if (!advertise_refs) {
 		packet_reader_init(&reader, 0, NULL, 0,
 				   PACKET_READ_CHOMP_NEWLINE |
 				   PACKET_READ_DIE_ON_ERR_PACKET);
diff --git a/upload-pack.h b/upload-pack.h
index 63e3252c98d..d6ee25ea98e 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -1,14 +1,8 @@
 #ifndef UPLOAD_PACK_H
 #define UPLOAD_PACK_H
 
-struct upload_pack_options {
-	int stateless_rpc;
-	int advertise_refs;
-	unsigned int timeout;
-	int daemon_mode;
-};
-
-void upload_pack(struct upload_pack_options *options);
+void upload_pack(const int advertise_refs, const int stateless_rpc,
+		 const int timeout);
 
 struct repository;
 struct packet_reader;
-- 
2.33.0.rc0.597.gc569a812f0a

Re: [PATCH v4 09/10] serve.[ch]: remove "serve_options", split up --advertise-refs code

[Derrick Stolee]

On 8/4/2021 9:25 PM, Ævar Arnfjörð Bjarmason wrote:
...  
> -struct upload_pack_options {
> -	int stateless_rpc;
> -	int advertise_refs;
> -	unsigned int timeout;
> -	int daemon_mode;
> -};
> -
> -void upload_pack(struct upload_pack_options *options);
> +void upload_pack(const int advertise_refs, const int stateless_rpc,
> +		 const int timeout);

Normally, I would err on keeping a struct that presents the different
parameters of a complicated method, since it is easier to add options
without modifying all callers. However, in this case we already had
some confusion because the 'daemon_mode' option is dependent on the
other values.

The fact that these parameters are translated into a 'struct
upload_pack_data' immediately within upload_pack() shows that these
can be grouped into a "context" object for internal use without
needing this pattern for the method signature.

Works for me.

Thanks,
-Stolee

[PATCH v4 10/10] upload-pack: document and rename --advertise-refs

[Ævar Arnfjörð Bjarmason]

The --advertise-refs documentation in git-upload-pack added in
9812f2136b3 (upload-pack.c: use parse-options API, 2016-05-31) hasn't
been entirely true ever since v2 support was implemented in
e52449b6722 (connect: request remote refs using v2, 2018-03-15). Under
v2 we don't advertise the refs at all, but rather dump the
capabilities header.

This option has always been an obscure internal implementation detail,
it wasn't even documented for git-receive-pack. Since it has exactly
one user let's rename it to --http-backend-info-refs, which is more
accurate and points the reader in the right direction. Let's also
cross-link this from the protocol v1 and v2 documentation.

I'm retaining a hidden --advertise-refs alias in case there's any
external users of this, and making both options hidden to the bash
completion (as with most other internal-only options).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-receive-pack.txt        |  5 +++++
 Documentation/git-upload-pack.txt         | 12 ++++++++----
 Documentation/technical/http-protocol.txt |  3 +++
 Documentation/technical/protocol-v2.txt   |  3 +++
 builtin/receive-pack.c                    |  3 ++-
 builtin/upload-pack.c                     |  5 +++--
 http-backend.c                            |  2 +-
 t/t5555-http-smart-common.sh              | 14 ++++++++++++++
 8 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 25702ed7307..014a78409b9 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -41,6 +41,11 @@ OPTIONS
 <directory>::
 	The repository to sync into.
 
+--http-backend-info-refs::
+	Used by linkgit:git-http-backend[1] to serve up
+	`$GIT_URL/info/refs?service=git-receive-pack` requests. See
+	`--http-backend-info-refs` in linkgit:git-upload-pack[1].
+
 PRE-RECEIVE HOOK
 ----------------
 Before any ref is updated, if $GIT_DIR/hooks/pre-receive file exists
diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index 9822c1eb1ad..739416ec83d 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -36,10 +36,14 @@ OPTIONS
 	This fits with the HTTP POST request processing model where
 	a program may read the request, write a response, and must exit.
 
---advertise-refs::
-	Only the initial ref advertisement is output, and the program exits
-	immediately. This fits with the HTTP GET request model, where
-	no request content is received but a response must be produced.
+--http-backend-info-refs::
+	Used by linkgit:git-http-backend[1] to serve up
+	`$GIT_URL/info/refs?service=git-upload-pack` requests. See
+	"Smart Clients" in link:technical/http-protocol.html[the HTTP
+	transfer protocols] documentation and "HTTP Transport" in
+	link:technical/protocol-v2.html[the Git Wire Protocol, Version
+	2] documentation. Also understood by
+	linkgit:git-receive-pack[1].
 
 <directory>::
 	The repository to sync from.
diff --git a/Documentation/technical/http-protocol.txt b/Documentation/technical/http-protocol.txt
index 96d89ea9b22..cc5126cfeda 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -225,6 +225,9 @@ The client may send Extra Parameters (see
 Documentation/technical/pack-protocol.txt) as a colon-separated string
 in the Git-Protocol HTTP header.
 
+Uses the `--http-backend-info-refs` option to
+linkgit:git-upload-pack[1].
+
 Dumb Server Response
 ^^^^^^^^^^^^^^^^^^^^
 Dumb servers MUST respond with the dumb server reply format.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 1040d853198..213538f1d0e 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -81,6 +81,9 @@ A v2 server would reply:
 Subsequent requests are then made directly to the service
 `$GIT_URL/git-upload-pack`. (This works the same for git-receive-pack).
 
+Uses the `--http-backend-info-refs` option to
+linkgit:git-upload-pack[1].
+
 Capability Advertisement
 ------------------------
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2d1f97e1ca7..7ffb45d7062 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2477,7 +2477,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("quiet")),
 		OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
-		OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
+		OPT_HIDDEN_BOOL(0, "http-backend-info-refs", &advertise_refs, NULL),
+		OPT_ALIAS(0, "advertise-refs", "http-backend-info-refs"),
 		OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL),
 		OPT_END()
 	};
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 8506030a648..125af53885f 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -22,8 +22,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
 			 N_("quit after a single request/response exchange")),
-		OPT_BOOL(0, "advertise-refs", &advertise_refs,
-			 N_("exit immediately after initial ref advertisement")),
+		OPT_HIDDEN_BOOL(0, "http-backend-info-refs", &advertise_refs,
+				N_("serve up the info/refs for git-http-backend")),
+		OPT_ALIAS(0, "advertise-refs", "http-backend-info-refs"),
 		OPT_BOOL(0, "strict", &strict,
 			 N_("do not try <directory>/.git/ if <directory> is no Git directory")),
 		OPT_INTEGER(0, "timeout", &timeout,
diff --git a/http-backend.c b/http-backend.c
index d37463cec8b..838374edb91 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -534,7 +534,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
 
 	if (service_name) {
 		const char *argv[] = {NULL /* service name */,
-			"--advertise-refs",
+			"--http-backend-info-refs",
 			".", NULL};
 		struct rpc_service *svc = select_service(hdr, service_name);
 
diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
index b813099ed2c..49faf5e283b 100755
--- a/t/t5555-http-smart-common.sh
+++ b/t/t5555-http-smart-common.sh
@@ -8,6 +8,20 @@ test_expect_success 'setup' '
 	test_commit --no-tag initial
 '
 
+test_expect_success 'git upload-pack --http-backend-info-refs and --advertise-refs are aliased' '
+	git upload-pack --http-backend-info-refs . >expected 2>err.expected &&
+	git upload-pack --advertise-refs . >actual 2>err.actual &&
+	test_cmp err.expected err.actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git receive-pack --http-backend-info-refs and --advertise-refs are aliased' '
+	git receive-pack --http-backend-info-refs . >expected 2>err.expected &&
+	git receive-pack --advertise-refs . >actual 2>err.actual &&
+	test_cmp err.expected err.actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'git upload-pack --advertise-refs' '
 	cat >expect <<-EOF &&
 	$(git rev-parse HEAD) HEAD
-- 
2.33.0.rc0.597.gc569a812f0a

Re: [PATCH v4 00/10] serve.[ch]: general API cleanup + --advertise-refs cleanup

[Derrick Stolee]

On 8/4/2021 9:25 PM, Ævar Arnfjörð Bjarmason wrote:
> This is a general API cleanup and simplifiction of of serve.[ch] and
> its interaction with "upload-pack --advertise-refs".

s/simplifiction/simplification/ (not that it matters much)

The changes included in this series seem innocent enough. The added
tests are nice. Organizing struct initializers is good to do before
adding options, which I see are coming later.

This version LGTM.

Thanks,
-Stolee