On Tue,  2 Jan 2018 16:18:13 -0800
Brandon Williams [off-list ref] wrote:

I'll review the documentation later, once there is some consensus that
the overall design is OK. (Or maybe there already is consensus?)

Should be serve_usage.

[snip]

+struct protocol_capability {
+	const char *name; /* capability name */

Maybe document as:

  The name of the capability. The server uses this name when advertising
  this capability, and the client uses this name to invoke the command
  corresponding to this capability.

+	/*
+	 * Function queried to see if a capability should be advertised.
+	 * Optionally a value can be specified by adding it to 'value'.
+	 */
+	int (*advertise)(struct repository *r, struct strbuf *value);

Document what happens when value is appended to. For example:

  ... If value is appended to, the server will advertise this capability
  as <name>=<value> instead of <name>.

+	/*
+	 * Function called when a client requests the capability as a command.
+	 * The command request will be provided to the function via 'keys', the
+	 * capabilities requested, and 'args', the command specific parameters.
+	 *
+	 * This field should be NULL for capabilities which are not commands.
+	 */
+	int (*command)(struct repository *r,
+		       struct argv_array *keys,
+		       struct argv_array *args);

Looking at the code below, I see that the command is not executed unless
advertise returns true - this means that a command cannot be both
supported and unadvertised. Would this be too restrictive? For example,
this would disallow a gradual across-multiple-servers rollout in which
we allow but not advertise a capability, and then after some time,
advertise the capability.

If we change this, then the value parameter of advertise can be
mandatory instead of optional.