Re: [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table

On Tue, Sep 14 2021, Jeff King wrote:

-static int is_valid_capability(const char *key)
+static int receive_client_capability(const char *key)
 {
 	const char *value;
 	const struct protocol_capability *c = get_capability(key, &value);
 
-	return c && c->advertise(the_repository, NULL);
+	if (!c || !c->advertise(the_repository, NULL))
+		return 0;
+
+	if (c->receive)
+		c->receive(the_repository, value);
+	return 1;
 }
 

I haven't actually run this yet (and need to zZzZ soon), but AFAICT at
the end of this series you leave the existing advertise semantics of
advertise() be (which is fine). I have this unsubmitted patch locally
which you may or may not want to work into this.

I considered splitting up the advertise() method as well, i.e. we could
have a new "is_advertised" boolean callback, and then a
"capability_string" or whatever. "server-option" and "object-info" never
add anything, so they could leave that as NULL.

But it's probably not worth it, just food for thought...

-- >8 -- serve: document that "advertise" is called in two modes

If we're being called with a non-NULL argument we're sending out the
advertisement line, but if it's NULL we're actually in the middle of a
request.

So you can use the check for NULL to emit your own "die" on "return
0", like "wtf, I said I don't support command xyz, why are you calling
it?", as opposed to the default "invalid command '%s'".

Maybe nobody cares, and we can't assume that we're going from an
advertisement to a command for the same request anyway (can
we?). I.e. are we serving multiple clients?

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

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

@@ -55,6 +55,12 @@ struct protocol_capability {
 	 * Optionally a value can be specified by adding it to 'value'.
 	 * If a value is added to 'value', the server will advertise this
 	 * capability as "<name>=<value>" instead of "<name>".
+	 *
+	 * When called with a NULL value we're past the advertisement
+	 * itself, and are checking during a request whether we
+	 * support this command. This can be checked and used used to
+	 * e.g. emit a die("we support this, but don't have it
+	 * enabled!") error.
 	 */
 	int (*advertise)(struct repository *r, struct strbuf *value);
 

-- 
2.33.0.1013.ge8323766266