| Message ID | cover.1542162201.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Advertise multiple supported proto versions | expand |
Josh Steadmon <steadmon@google.com> writes: > Fix several bugs identified in v3, clarify commit message, and clean up > extern keyword in protocol.h. It is good to descirbe the change relative to v3 here, which would help those who are interested and reviewed v3. To help those who missed the boat and v4 is their first encounter with this series, having the description relative to v3 alone and nothing else is not very friendly, though. > + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c > + --- a/builtin/upload-archive.c > + +++ b/builtin/upload-archive.c > +@@ > + #include "builtin.h" > + #include "archive.h" > + #include "pkt-line.h" > ++#include "protocol.h" > + #include "sideband.h" > + #include "run-command.h" > + #include "argv-array.h" > +@@ > + if (argc == 2 && !strcmp(argv[1], "-h")) > + usage(upload_archive_usage); > + > ++ register_allowed_protocol_version(protocol_v0); > ++ > + /* > + * Set up sideband subprocess. > + * This one unfortunately seems to interact rather badly with your js/remote-archive-v2 topic which is already in 'next', when this topic is merged to 'pu', and my attempt to mechanically resolve conflicts breaks t5000.
On 2018.11.14 19:22, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > Fix several bugs identified in v3, clarify commit message, and clean up > > extern keyword in protocol.h. > > It is good to descirbe the change relative to v3 here, which would > help those who are interested and reviewed v3. > > To help those who missed the boat and v4 is their first encounter > with this series, having the description relative to v3 alone and > nothing else is not very friendly, though. Ack. Will keep this in mind for future patches. > > + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c > > + --- a/builtin/upload-archive.c > > + +++ b/builtin/upload-archive.c > > +@@ > > + #include "builtin.h" > > + #include "archive.h" > > + #include "pkt-line.h" > > ++#include "protocol.h" > > + #include "sideband.h" > > + #include "run-command.h" > > + #include "argv-array.h" > > +@@ > > + if (argc == 2 && !strcmp(argv[1], "-h")) > > + usage(upload_archive_usage); > > + > > ++ register_allowed_protocol_version(protocol_v0); > > ++ > > + /* > > + * Set up sideband subprocess. > > + * > > This one unfortunately seems to interact rather badly with your > js/remote-archive-v2 topic which is already in 'next', when this > topic is merged to 'pu', and my attempt to mechanically resolve > conflicts breaks t5000. Hmm, should we drop js/remote-archive-v2 then? Any solution that unblocks js/remote-archive-v2 will almost certainly depend on this patch.
Josh Steadmon <steadmon@google.com> writes: >> This one unfortunately seems to interact rather badly with your >> js/remote-archive-v2 topic which is already in 'next', when this >> topic is merged to 'pu', and my attempt to mechanically resolve >> conflicts breaks t5000. > > Hmm, should we drop js/remote-archive-v2 then? Any solution that > unblocks js/remote-archive-v2 will almost certainly depend on this > patch. That one is already in 'next' for quite some time. If you are retrating it, that's OK. Let me revert the topic out of 'next' and discard it, and then queue this one in 'pu'. Thanks.
Fix several bugs identified in v3, clarify commit message, and clean up extern keyword in protocol.h. Josh Steadmon (1): protocol: advertise multiple supported versions builtin/archive.c | 3 + builtin/clone.c | 4 ++ builtin/fetch-pack.c | 4 ++ builtin/fetch.c | 5 ++ builtin/ls-remote.c | 5 ++ builtin/pull.c | 5 ++ builtin/push.c | 4 ++ builtin/receive-pack.c | 3 + builtin/send-pack.c | 3 + builtin/upload-archive.c | 3 + builtin/upload-pack.c | 4 ++ connect.c | 47 +++++++-------- protocol.c | 122 ++++++++++++++++++++++++++++++++++++--- protocol.h | 23 +++++++- remote-curl.c | 28 ++++++--- t/t5570-git-daemon.sh | 2 +- t/t5700-protocol-v1.sh | 8 +-- t/t5702-protocol-v2.sh | 16 +++-- transport-helper.c | 6 ++ 19 files changed, 237 insertions(+), 58 deletions(-) Range-diff against v3: 1: b9968e3fb0 ! 1: 3c023991c0 protocol: advertise multiple supported versions @@ -4,22 +4,25 @@ 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. + support the same set of protocol versions. For example, git-receive-pack + supports v1 and v0, but not v2. If a client connects to git-receive-pack + and requests v2, it will instead be downgraded to v0. Other services, + such as git-upload-archive, do not do any version negotiation checks. - 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. + This patch creates a protocol version registry. Individual client and + server programs register all the protocol versions they support prior to + communicating with a remote instance. 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. + select the first allowed version from this advertisement. + + While we're at it, remove unnecessary externs from function declarations + in protocol.h. Signed-off-by: Josh Steadmon <steadmon@google.com> @@ -165,6 +168,20 @@ git_config(git_push_config, &flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); + diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c + --- a/builtin/receive-pack.c + +++ b/builtin/receive-pack.c +@@ + + packet_trace_identity("receive-pack"); + ++ register_allowed_protocol_version(protocol_v1); ++ register_allowed_protocol_version(protocol_v0); ++ + argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0); + + if (argc > 1) + diff --git a/builtin/send-pack.c b/builtin/send-pack.c --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -179,6 +196,42 @@ argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0); if (argc > 0) { + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c + --- a/builtin/upload-archive.c + +++ b/builtin/upload-archive.c +@@ + #include "builtin.h" + #include "archive.h" + #include "pkt-line.h" ++#include "protocol.h" + #include "sideband.h" + #include "run-command.h" + #include "argv-array.h" +@@ + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(upload_archive_usage); + ++ register_allowed_protocol_version(protocol_v0); ++ + /* + * Set up sideband subprocess. + * + + diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c + --- a/builtin/upload-pack.c + +++ b/builtin/upload-pack.c +@@ + packet_trace_identity("upload-pack"); + read_replace_refs = 0; + ++ register_allowed_protocol_version(protocol_v2); ++ register_allowed_protocol_version(protocol_v1); ++ register_allowed_protocol_version(protocol_v0); ++ + argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0); + + if (argc != 1) + diff --git a/connect.c b/connect.c --- a/connect.c +++ b/connect.c @@ -311,7 +364,7 @@ +static enum protocol_version *allowed_versions; +static int nr_allowed_versions; +static int alloc_allowed_versions; -+static int have_advertised_versions_already = 0; ++static int version_registration_locked = 0; + +static const char protocol_v0_string[] = "0"; +static const char protocol_v1_string[] = "1"; @@ -357,8 +410,8 @@ +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 (version_registration_locked) ++ BUG("late attempt to register an allowed protocol version"); + + ALLOC_GROW(allowed_versions, nr_allowed_versions + 1, + alloc_allowed_versions); @@ -384,13 +437,23 @@ + string_list_clear(&version_list, 0); +} + ++static int is_allowed_protocol_version(enum protocol_version version) ++{ ++ int i; ++ version_registration_locked = 1; ++ for (i = 0; i < nr_allowed_versions; i++) ++ if (version == allowed_versions[i]) ++ return 1; ++ return 0; ++} ++ +void get_client_protocol_version_advertisement(struct strbuf *advert) +{ -+ int tmp_nr = nr_allowed_versions; ++ int i, tmp_nr = nr_allowed_versions; + enum protocol_version *tmp_allowed_versions, config_version; + strbuf_reset(advert); + -+ have_advertised_versions_already = 1; ++ version_registration_locked = 1; + + config_version = get_protocol_version_config(); + if (config_version == protocol_v0) { @@ -409,20 +472,19 @@ + } + + if (tmp_allowed_versions[0] != config_version) -+ for (int i = 1; i < nr_allowed_versions; i++) ++ for (i = 1; i < nr_allowed_versions; i++) + if (tmp_allowed_versions[i] == config_version) { -+ enum protocol_version swap = -+ tmp_allowed_versions[0]; -+ tmp_allowed_versions[0] = -+ tmp_allowed_versions[i]; -+ tmp_allowed_versions[i] = swap; ++ SWAP(tmp_allowed_versions[0], ++ tmp_allowed_versions[i]); + } + + strbuf_addf(advert, "version=%s", + format_protocol_version(tmp_allowed_versions[0])); -+ for (int i = 1; i < tmp_nr; i++) ++ for (i = 1; i < tmp_nr; i++) + strbuf_addf(advert, ":version=%s", + format_protocol_version(tmp_allowed_versions[i])); ++ ++ free(tmp_allowed_versions); +} + enum protocol_version determine_protocol_version_server(void) @@ -447,7 +509,8 @@ if (skip_prefix(item->string, "version=", &value)) { v = parse_protocol_version(value); - if (v > version) -+ if (v != protocol_unknown_version) { ++ if (v != protocol_unknown_version && ++ is_allowed_protocol_version(v)) { version = v; + break; + } @@ -459,29 +522,46 @@ --- a/protocol.h +++ b/protocol.h @@ + * 'protocol.version' config. If unconfigured, a value of 'protocol_v0' is + * returned. */ - extern enum protocol_version get_protocol_version_config(void); - +-extern enum protocol_version get_protocol_version_config(void); ++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); ++void register_allowed_protocol_version(enum protocol_version version); + +/* + * Register allowable protocol versions from the GIT_PROTOCOL environment var. + */ -+extern void register_allowed_protocol_versions_from_env(void); ++void register_allowed_protocol_versions_from_env(void); + +/* + * Fill a strbuf with a version advertisement string suitable for use in the + * GIT_PROTOCOL environment variable or similar version negotiation field. + */ -+extern void get_client_protocol_version_advertisement(struct strbuf *advert); -+ ++void get_client_protocol_version_advertisement(struct strbuf *advert); + /* * Used by a server to determine which protocol version should be used based on - * a client's request, communicated via the 'GIT_PROTOCOL' environment variable +@@ + * request a particular protocol version, a default of 'protocol_v0' will be + * used. + */ +-extern enum protocol_version determine_protocol_version_server(void); ++enum protocol_version determine_protocol_version_server(void); + + /* + * Used by a client to determine which protocol version the server is speaking + * based on the server's initial response. + */ +-extern enum protocol_version determine_protocol_version_client(const char *server_response); ++enum protocol_version determine_protocol_version_client(const char *server_response); + + #endif /* PROTOCOL_H */ diff --git a/remote-curl.c b/remote-curl.c --- a/remote-curl.c