Re: [PATCH 11/26] serve: introduce git-serve

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 9, 2018, 20:24 UTC
Message-ID: <20180109122455.5845c4f12ccdd4cada1528f2@google.com>
In-Reply-To: <20180103001828.205012-12-bmwill@google.com>

On Tue, 2 Jan 2018 16:18:13 -0800 Brandon Williams <bmwill@google.com> wrote:

Show 5 quoted lines

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

Show 12 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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