On 01/09, Jonathan Tan wrote:
> On Tue, 2 Jan 2018 16:18:13 -0800
> Brandon Williams <bmwill@xxxxxxxxxx> 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>.
>
All good documentation changes.
> > + /*
> > + * 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.
One way to change this would be to just add another function to the
struct which is called to check if the command is allowed, instead of
relying on the same function to do that for both advertise and
allow...though I don't see a big win for allowing a command but not
advertising it.
>
> If we change this, then the value parameter of advertise can be
> mandatory instead of optional.
I don't see how this fixes the issue you bring up.
--
Brandon Williams
