On Tue, 9 Jan 2018 14:16:42 -0800 Brandon Williams <bmw...@google.com> wrote:
> All good documentation changes. Thanks! > > > + /* > > > + * 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. My rationale for allowing a command but not advertising it is in the paragraph above (that you quoted), but if that is insufficient rationale, then I agree that we don't need to do this. > > 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. This is a consequence, not a fix - if we were to do as I suggested, then we no longer need to invoke advertise to check whether something is advertised except when we are advertising them, in which case "value" never needs to be NULL.
