On Tue, 2 Jan 2018 16:18:13 -0800
Brandon Williams <bmw...@google.com> wrote:
> diff --git a/Documentation/technical/protocol-v2.txt
> b/Documentation/technical/protocol-v2.txt
> new file mode 100644
> index 000000000..b87ba3816
> --- /dev/null
> +++ b/Documentation/technical/protocol-v2.txt
I'll review the documentation later, once there is some consensus that
the overall design is OK. (Or maybe there already is consensus?)
> diff --git a/builtin/serve.c b/builtin/serve.c
> new file mode 100644
> index 000000000..bb726786a
> --- /dev/null
> +++ b/builtin/serve.c
> @@ -0,0 +1,30 @@
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "serve.h"
> +
> +static char const * const grep_usage[] = {
Should be serve_usage.
> diff --git a/serve.c b/serve.c
> new file mode 100644
> index 000000000..da8127775
> --- /dev/null
> +++ b/serve.c
[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.
