Re: [PATCH v3 1/1] protocol: advertise multiple supported versions

[Stefan Beller <sbeller@google.com>]

On Mon, Nov 12, 2018 at 1:49 PM <steadmon@google.com> wrote:
>
> Currently the client advertises that it supports the wire protocol
> version set in the protocol.version config. However, not all services
> support the same set of protocol versions. When connecting to
> git-receive-pack, the client automatically downgrades to v0 if
> config.protocol is set to v2, but this check is not performed for other
> services.
>
> This patch creates a protocol version registry. Individual operations
> register all the protocol versions they support prior to communicating
> with a server. Versions should be listed in preference order; the
> version specified in protocol.version will automatically be moved to the
> front of the registry.
>
> The protocol version registry is passed to remote helpers via the
> GIT_PROTOCOL environment variable.
>
> Clients now advertise the full list of registered versions. Servers
> select the first recognized version from this advertisement.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>

Thanks for resending this patch!

+cc Jonathan Tan, who works a lot on the protocol side of things.

> +void register_allowed_protocol_version(enum protocol_version version)
> +{
> +       if (have_advertised_versions_already)
> +               BUG(_("attempting to register an allowed protocol version after advertisement"));

If this is a real BUG (due to wrong program flow) instead of bad user input,
we would not want to burden the translators with this message.

If it is a message that the user is intended to see upon bad input
we'd rather go with die(_("translatable text here"));


> diff --git a/protocol.h b/protocol.h
> index 2ad35e433c..b67b2259de 100644
> --- a/protocol.h
> +++ b/protocol.h
> @@ -16,6 +16,23 @@ enum protocol_version {
>   */
>  extern enum protocol_version get_protocol_version_config(void);
>
> +/*
> + * Register an allowable protocol version for a given operation. Registration
> + * must occur before attempting to advertise a version to a server process.
> + */
> +extern void register_allowed_protocol_version(enum protocol_version version);

We keep extern here as to imitate the file-local style, although
Documentation/CodingGuidelines would prefer to not have the extern keywords.
Okay.

Bonus points for converting the file to omit all extern keywords in a
resend. :-)
(I think there is no other series currently in flight touching this header file,
so it is safe to convert it "while at it", `git log --grep "while at
it"` is shorter
than expected.)

All that said, it's only nits that I contributed to this code review;
the code/design
looks good to me, and if I were maintainer I'd include it as-is, as it
fixes a long(ish)
standing protocol error.

Thanks,
Stefan