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

From: Stefan Beller <sbeller@google.com>
Date: Nov 12, 2018, 22:33 UTC
Message-ID: <CAGZ79kaPjw3UbYY=XL0gyKp2VxghwaUOJJoDhiO5sQpT8OWerQ@mail.gmail.com>
In-Reply-To: <b9968e3fb005a00e9f78627467b4fc86a6259977.1542059029.git.steadmon@google.com>

On Mon, Nov 12, 2018 at 1:49 PM <steadmon@google.com> wrote:

Show 21 quoted lines

>
> 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"));

Show 13 quoted lines

> 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