| Message ID | 59357266bd86e8e0ace9217a97717129a6f76188.1538516853.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Limit client version advertisements | expand |
On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote: > > For services other than git-receive-pack, clients currently advertise > that they support the version set in the protocol.version config, > regardless of whether or not there is actually an implementation of that > service for the given protocol version. This causes backwards- > compatibility problems when a new implementation for the given > protocol version is added. > > This patch sets maximum allowed protocol versions for git-receive-pack, > git-upload-archive, and git-upload-pack. > > Previously, git-receive-pack would downgrade from v2 to v0, but would > allow v1 if set in protocol.version. Now, it will downgrade from v2 to > v1. But does git-receive-pack understand v1? As to my understanding we have not even defined v1 for push (receive-pack) and archive --remote (upload-archive). v1 is only known to fetch (upload-pack). > +enum protocol_version determine_maximum_protocol_version( > + const char *service, enum protocol_version default_version) > +{ > + if (!strcmp(service, "git-receive-pack")) > + return protocol_v1; > + else if (!strcmp(service, "git-upload-archive")) > + return protocol_v1; so I would think these two would be _v0. ... goes and checks ... aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1, 2017-10-16) seems to actually teach v1 to receive-pack as well, but upload-archive was completely off radar, so I think returning (v1, v0, v2 in the order as in the code) would make sense? Asides from this, I thought there was a deliberate decision that we'd want to avoid a strict order on the protocol versions, but I could not find prior discussion on list to back up this claim. :/ For example we'd go with e.g. enums instead of integers for version numbers, as then some internal setup could also have things like protocol_v2018-10-02 or protocol_vWhatever; some protocol version may be advantageous to the client, some to the server, and we'd need to negotiate the best version that both are happy with. (e.g. the server may like version 0, 2 and 3, and the client may like 0,2,4 as 3 is bad security wise for the client, so both would negotiate to 2 as their best case) From a maintenance perspective, do we want to keep this part of the code central, as it ties protocol (as proxied by service name) to the max version number? I would think that we'd rather have the decision local to the code, i.e. builtin/fetch would need to tell protocol.c that it can do (0,1,2) and builtin/push can do (0,1), and then the networking layers of code would figure out by the input from the caller and the input from the user (configured protocol.version) what is the best to go forward from then on. But I guess having the central place here is not to bad either. How will it cope with the desire of protocol v2 to have only one end point (c.f. serve.{c,h} via builtin/serve as "git serve") ? Stefan
On 2018.10.02 15:28, Stefan Beller wrote: > On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote: > > > > For services other than git-receive-pack, clients currently advertise > > that they support the version set in the protocol.version config, > > regardless of whether or not there is actually an implementation of that > > service for the given protocol version. This causes backwards- > > compatibility problems when a new implementation for the given > > protocol version is added. > > > > This patch sets maximum allowed protocol versions for git-receive-pack, > > git-upload-archive, and git-upload-pack. > > > > Previously, git-receive-pack would downgrade from v2 to v0, but would > > allow v1 if set in protocol.version. Now, it will downgrade from v2 to > > v1. > > But does git-receive-pack understand v1? > As to my understanding we have not even defined v1 > for push (receive-pack) and archive --remote (upload-archive). > v1 is only known to fetch (upload-pack). > > > +enum protocol_version determine_maximum_protocol_version( > > + const char *service, enum protocol_version default_version) > > +{ > > + if (!strcmp(service, "git-receive-pack")) > > + return protocol_v1; > > + else if (!strcmp(service, "git-upload-archive")) > > + return protocol_v1; > > so I would think these two would be _v0. > ... goes and checks ... > aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1, > 2017-10-16) seems to actually teach v1 to receive-pack as well, > but upload-archive was completely off radar, so I think returning > (v1, v0, v2 in the order as in the code) would make sense? I believe that git-upload-archive can still speak version 1 without any trouble, but it at least doesn't break anything in the test suite to limit this to v0 either. > Asides from this, I thought there was a deliberate decision > that we'd want to avoid a strict order on the protocol versions, > but I could not find prior discussion on list to back up this claim. :/ > > For example we'd go with e.g. enums instead of integers > for version numbers, as then some internal setup could > also have things like protocol_v2018-10-02 or protocol_vWhatever; > some protocol version may be advantageous to the client, some to > the server, and we'd need to negotiate the best version that both > are happy with. (e.g. the server may like version 0, 2 and 3, and > the client may like 0,2,4 as 3 is bad security wise for the client, > so both would negotiate to 2 as their best case) Is there a method or design for advertising multiple acceptable versions from the client? From my understanding, we can only add a single version=X field in the advertisement, but IIUC we can extend this fairly easily? Perhaps we can have "version=X" to mean the preferred version, and then a repeatable "acceptable_version=Y" field or similar? > From a maintenance perspective, do we want to keep > this part of the code central, as it ties protocol (as proxied > by service name) to the max version number? > I would think that we'd rather have the decision local to the > code, i.e. builtin/fetch would need to tell protocol.c that it > can do (0,1,2) and builtin/push can do (0,1), and then the > networking layers of code would figure out by the input > from the caller and the input from the user (configured > protocol.version) what is the best to go forward from > then on. I like having it centralized, because enforcing this in git_connect() and discover_refs() catches all the outgoing version advertisements, but there's lots of code paths that lead to those two functions that would all have to have the acceptable version numbers plumbed through. I suppose we could also have a registry of services to version numbers, but I tend to dislike non-local sources of data. But if the list likes that approach better, I'll be happy to implement it. > But I guess having the central place here is not to > bad either. How will it cope with the desire of protocol v2 > to have only one end point (c.f. serve.{c,h} via builtin/serve > as "git serve") ? I'm not sure about this. In my series to add a v2 archive command, I added support for a new endpoint for proto v2 and I don't recall seeing any complaints, but that is still open for review. I suppose if we are strict about serving from a single endpoint, the version registry makes more sense, and individual operations can declare acceptable version numbers before calling any network code? Thanks for the review, Josh
On Wed, Oct 3, 2018 at 2:34 PM Josh Steadmon <steadmon@google.com> wrote: > > I believe that git-upload-archive can still speak version 1 without any > trouble, but it at least doesn't break anything in the test suite to > limit this to v0 either. ok, let me just play around with archive to follow along: Running the excerpt that I found in one of the tests in t5000 GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar (whoooa ... that spews a lot of text into my terminal, which makes sense as transported tar files unlike packets starting with PACK are not cut short; so onwards:) $ git init test && cd test $ GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar ... git< \2fatal: no such ref: HEAD ... fatal: sent error to the client: git upload-archive: archiver died with error remote: git upload-archive: archiver died with error This sounds similar to a bug that I have on my todo list for clone recursing into submodules. Maybe we need to talk about HEAD-less repositories and how to solve handling them in general. Usually it doesn't happen except for corner cases like now, so: $ git commit --allow-empty -m "commit" [master (root-commit) 24d7967] commit $ GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar 15:28:00.762615 pkt-line.c:80 packet: git> argument HEAD 15:28:00.762704 pkt-line.c:80 packet: git> 0000 15:28:00.766336 pkt-line.c:80 packet: git> ACK 15:28:00.766428 pkt-line.c:80 packet: git> 0000 15:28:00.766483 pkt-line.c:80 packet: git< ACK 15:28:00.766508 pkt-line.c:80 packet: git< 0000 15:28:00.767694 pkt-line.c:80 packet: git< \2 15:28:00.767524 pkt-line.c:80 packet: git< argument HEAD 15:28:00.767583 pkt-line.c:80 packet: git< 0000 remote: 15:28:00.767524 pkt-line.c:80 packet: git< argument HEAD remote: 15:28:00.767583 pkt-line.c:80 packet: git< 0000 15:28:00.768758 pkt-line.c:80 packet: git> 0000 15:28:00.770475 pkt-line.c:80 packet: git< \1pax_global_header ... \0\0\0\0\0\0\0\0\0\0\0\0\ ... # not too bad but the tar file contains a lot of zeros $ Ah I forgot the crucial part, so $ GIT_TRACE_PACKET=1 git -c protocol.version=1 archive --remote=. HEAD >b5.tar 15:33:10.030508 pkt-line.c:80 packet: git> argument HEAD ... This pretty much looks like v0 as v1 would send a "version 1", c.f. $ GIT_TRACE_PACKET=1 git -c protocol.version=1 fetch . 15:34:26.111013 pkt-line.c:80 packet: upload-pack> version 1 15:34:26.111140 pkt-line.c:80 packet: fetch< version 1 .... > > Is there a method or design for advertising multiple acceptable versions > from the client? I think the client can send multiple versions, looking through protocol.c (and not the documentation as I should for this:) /* * Determine which protocol version the client has requested. Since * multiple 'version' keys can be sent by the client, indicating that * the client is okay to speak any of them, select the greatest version * that the client has requested. This is due to the assumption that * the most recent protocol version will be the most state-of-the-art. */ ... const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT); string_list_split(&list, git_protocol, ':', -1); ... for_each_string_list_item(item, &list) { ... if (skip_prefix(item->string, "version=", &value)) ... in determine_protocol_version_server which already had the client speak to it, so I think at least the server can deal with multiple versions. But given that we transport the version in env variables, we'd need to be extra careful if we just did not see the version in the --remote=. above? > From my understanding, we can only add a single > version=X field in the advertisement, but IIUC we can extend this fairly > easily? Perhaps we can have "version=X" to mean the preferred version, > and then a repeatable "acceptable_version=Y" field or similar? Just re-use "version X", separated by colons as above. > > From a maintenance perspective, do we want to keep > > this part of the code central, as it ties protocol (as proxied > > by service name) to the max version number? > > I would think that we'd rather have the decision local to the > > code, i.e. builtin/fetch would need to tell protocol.c that it > > can do (0,1,2) and builtin/push can do (0,1), and then the > > networking layers of code would figure out by the input > > from the caller and the input from the user (configured > > protocol.version) what is the best to go forward from > > then on. > > I like having it centralized, because enforcing this in git_connect() > and discover_refs() catches all the outgoing version advertisements, but > there's lots of code paths that lead to those two functions that would > all have to have the acceptable version numbers plumbed through. Makes sense. > I suppose we could also have a registry of services to version numbers, > but I tend to dislike non-local sources of data. But if the list likes > that approach better, I'll be happy to implement it. > > But I guess having the central place here is not to > > bad either. How will it cope with the desire of protocol v2 > > to have only one end point (c.f. serve.{c,h} via builtin/serve > > as "git serve") ? > > I'm not sure about this. In my series to add a v2 archive command, I > added support for a new endpoint for proto v2 and I don't recall seeing > any complaints, but that is still open for review. Ah I guess new end points would imply that you can speak at least a given version N. > I suppose if we are strict about serving from a single endpoint, the > version registry makes more sense, and individual operations can declare > acceptable version numbers before calling any network code? Ah yeah, that makes sense! Thanks for your explanations and prodding, Stefan
On 2018.10.03 15:47, Stefan Beller wrote: > On Wed, Oct 3, 2018 at 2:34 PM Josh Steadmon <steadmon@google.com> wrote: > > > > Is there a method or design for advertising multiple acceptable versions > > from the client? > > I think the client can send multiple versions, looking through protocol.c > (and not the documentation as I should for this:) > > /* > * Determine which protocol version the client has requested. Since > * multiple 'version' keys can be sent by the client, indicating that > * the client is okay to speak any of them, select the greatest version > * that the client has requested. This is due to the assumption that > * the most recent protocol version will be the most state-of-the-art. > */ > ... > const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT); > string_list_split(&list, git_protocol, ':', -1); > ... > for_each_string_list_item(item, &list) { > ... > if (skip_prefix(item->string, "version=", &value)) > ... > > in determine_protocol_version_server which already had the client > speak to it, so I think at least the server can deal with multiple versions. > > But given that we transport the version in env variables, we'd > need to be extra careful if we just did not see the version > in the --remote=. above? Sorry, I'm not sure I understand this. What about env variables requires caution? > > From my understanding, we can only add a single > > version=X field in the advertisement, but IIUC we can extend this fairly > > easily? Perhaps we can have "version=X" to mean the preferred version, > > and then a repeatable "acceptable_version=Y" field or similar? > > Just re-use "version X", separated by colons as above. > > > > From a maintenance perspective, do we want to keep > > > this part of the code central, as it ties protocol (as proxied > > > by service name) to the max version number? > > > I would think that we'd rather have the decision local to the > > > code, i.e. builtin/fetch would need to tell protocol.c that it > > > can do (0,1,2) and builtin/push can do (0,1), and then the > > > networking layers of code would figure out by the input > > > from the caller and the input from the user (configured > > > protocol.version) what is the best to go forward from > > > then on. > > > > I like having it centralized, because enforcing this in git_connect() > > and discover_refs() catches all the outgoing version advertisements, but > > there's lots of code paths that lead to those two functions that would > > all have to have the acceptable version numbers plumbed through. > > Makes sense. > > > I suppose we could also have a registry of services to version numbers, > > but I tend to dislike non-local sources of data. But if the list likes > > that approach better, I'll be happy to implement it. > > > > But I guess having the central place here is not to > > > bad either. How will it cope with the desire of protocol v2 > > > to have only one end point (c.f. serve.{c,h} via builtin/serve > > > as "git serve") ? > > > > I'm not sure about this. In my series to add a v2 archive command, I > > added support for a new endpoint for proto v2 and I don't recall seeing > > any complaints, but that is still open for review. > > Ah I guess new end points would imply that you can speak at least > a given version N. > > > I suppose if we are strict about serving from a single endpoint, the > > version registry makes more sense, and individual operations can declare > > acceptable version numbers before calling any network code? > > Ah yeah, that makes sense! Thinking out loud here. Please let me know if I say something stupid :) So we'll have (up to) three pieces of version information we'll care about for version negotiation: 1. (maybe) a client-side protocol.version config entry 2. a list of acceptable proto versions for the currently running operation on the client 3. a list of acceptable proto versions for the server endpoint that handles the request According to the doc on protocol.version: "If set, clients will attempt to communicate with a server using the specified protocol version. If unset, no attempt will be made by the client to communicate using a particular protocol version, this results in protocol version 0 being used." So, if protocol.version is not set, or set to 0, the client should not attempt any sort of version negotiation. Otherwise, the client prefers a particular version, but we don't guarantee that they will actually use that version after the (unspecified) version negotiation procedure. If protocol.version is set to something other than 0, we construct a list of acceptable versions for the given operation. If the protocol.version entry is present in that list, we move it to the front of the list to note that it is the preferred version. We send all of these, in order, in the request. When the server endpoint begins to handle a request, it first constructs a list of acceptable versions. If the client specifies a list of versions, we check them one-by-one to see if they are acceptable. If we find a match, we use that version. If we don't find any matches or if the client did not send a version list, we default to v0. Seem reasonable? Thanks, Josh
> > But given that we transport the version in env variables, we'd > > need to be extra careful if we just did not see the version > > in the --remote=. above? > > Sorry, I'm not sure I understand this. What about env variables requires > caution? By locally transporting the version via env variables means the absence of "version X" lines in the GIT_TRACE output is not a clear indication of version 0, I would think. It is a strong indicator, but now we'd have to dig and see if the env variables were set outside? > > > > > I suppose if we are strict about serving from a single endpoint, the > > > version registry makes more sense, and individual operations can declare > > > acceptable version numbers before calling any network code? > > > > Ah yeah, that makes sense! > > Thinking out loud here. Please let me know if I say something stupid :) > > So we'll have (up to) three pieces of version information we'll care > about for version negotiation: > > 1. (maybe) a client-side protocol.version config entry (and in case we don't, we have it implicit ly hardcoded, as we have to choose the default for users that don't care themselves about this setting) > 2. a list of acceptable proto versions for the currently running > operation on the client > 3. a list of acceptable proto versions for the server endpoint that > handles the request Yes that matches my understanding. The issue is between (1) and (2) as (1) is in a generic config, whereas (2) is specific to the command, such that it may differ. And as a user you may want to express things like: "use the highest version", which is done by setting (1) to "version 2" despite (2) not having support of all commands for v2. > According to the doc on protocol.version: "If set, clients will attempt > to communicate with a server using the specified protocol version. If > unset, no attempt will be made by the client to communicate using a > particular protocol version, this results in protocol version 0 being > used." > > So, if protocol.version is not set, or set to 0, the client should not > attempt any sort of version negotiation. Yes, as version 0 is unaware of versions, i.e. there are old installations out there where all the versioning code is not there, so in case of an old client the new server *must* speak v0 to be able to communicate (and vice versa). > Otherwise, the client prefers a > particular version, but we don't guarantee that they will actually use > that version after the (unspecified) version negotiation procedure. > > If protocol.version is set to something other than 0, we construct a > list of acceptable versions for the given operation. If the > protocol.version entry is present in that list, we move it to the front > of the list to note that it is the preferred version. We send all of > these, in order, in the request. > > When the server endpoint begins to handle a request, it first constructs > a list of acceptable versions. If the client specifies a list of > versions, we check them one-by-one to see if they are acceptable. If we > find a match, we use that version. If we don't find any matches or if > the client did not send a version list, we default to v0. > > Seem reasonable? This sounds super reasonable! Thanks Stefan
On 2018.10.05 12:25, Stefan Beller wrote: > > > > I suppose if we are strict about serving from a single endpoint, the > > > > version registry makes more sense, and individual operations can declare > > > > acceptable version numbers before calling any network code? > > > > > > Ah yeah, that makes sense! > > > > Thinking out loud here. Please let me know if I say something stupid :) > > > > So we'll have (up to) three pieces of version information we'll care > > about for version negotiation: > > > > 1. (maybe) a client-side protocol.version config entry > > (and in case we don't, we have it implicit ly hardcoded, as > we have to choose the default for users that don't care themselves about > this setting) > > > 2. a list of acceptable proto versions for the currently running > > operation on the client > > 3. a list of acceptable proto versions for the server endpoint that > > handles the request > > Yes that matches my understanding. The issue is between (1) and (2) > as (1) is in a generic config, whereas (2) is specific to the command, > such that it may differ. And as a user you may want to express things > like: "use the highest version", which is done by setting (1) to "version 2" > despite (2) not having support of all commands for v2. > > > According to the doc on protocol.version: "If set, clients will attempt > > to communicate with a server using the specified protocol version. If > > unset, no attempt will be made by the client to communicate using a > > particular protocol version, this results in protocol version 0 being > > used." > > > > So, if protocol.version is not set, or set to 0, the client should not > > attempt any sort of version negotiation. > > Yes, as version 0 is unaware of versions, i.e. there are old installations > out there where all the versioning code is not there, so in case of an > old client the new server *must* speak v0 to be able to communicate > (and vice versa). > > > > Otherwise, the client prefers a > > particular version, but we don't guarantee that they will actually use > > that version after the (unspecified) version negotiation procedure. > > > > If protocol.version is set to something other than 0, we construct a > > list of acceptable versions for the given operation. If the > > protocol.version entry is present in that list, we move it to the front > > of the list to note that it is the preferred version. We send all of > > these, in order, in the request. > > > > When the server endpoint begins to handle a request, it first constructs > > a list of acceptable versions. If the client specifies a list of > > versions, we check them one-by-one to see if they are acceptable. If we > > find a match, we use that version. If we don't find any matches or if > > the client did not send a version list, we default to v0. > > > > Seem reasonable? > > This sounds super reasonable! So this runs into problems with remote-curl (and possibly other remote helpers): builtin/push.c can declare whatever allowed versions it wants, but those are not carried over when remote-curl is started to actually talk to the remote. What's worse, remote-curl starts its HTTP connection before it knows what command it's actually acting as a helper for. For now, I'm going to try adding an --allowed_versions flag for the remote helpers, but if anyone has a better idea, let me know. Thanks, Josh
Hi, Josh Steadmon wrote: > So this runs into problems with remote-curl (and possibly other remote > helpers): > > builtin/push.c can declare whatever allowed versions it wants, but those > are not carried over when remote-curl is started to actually talk to the > remote. What's worse, remote-curl starts its HTTP connection before it > knows what command it's actually acting as a helper for. > > For now, I'm going to try adding an --allowed_versions flag for the > remote helpers, but if anyone has a better idea, let me know. There are some external remote helpers (see "git help remote-helpers" for the documented interface), so alas, they can't take new flags. That said, you can add a new remote helper capability and then communicate on stdin, or in a pinch you can use the existing "option" capability. Remote helpers are also allowed to query "git config" if they want to (either using the config machinery in config.h or by running "git config"). Thanks, Jonathan
Josh Steadmon wrote: > For now, I'm going to try adding an --allowed_versions flag for the > remote helpers, but if anyone has a better idea, let me know. I forgot to mention: the stateless-connect remote helper capability is still experimental so you're free to change its interface (e.g. to change the syntax of the stateless-connect command it provides). Thanks again, Jonathan
On 2018.10.12 16:32, Jonathan Nieder wrote: > Josh Steadmon wrote: > > > For now, I'm going to try adding an --allowed_versions flag for the > > remote helpers, but if anyone has a better idea, let me know. > > I forgot to mention: the stateless-connect remote helper capability is > still experimental so you're free to change its interface (e.g. to > change the syntax of the stateless-connect command it provides). For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version string to the remote helpers.
Josh Steadmon wrote: > On 2018.10.12 16:32, Jonathan Nieder wrote: >> Josh Steadmon wrote: >>> For now, I'm going to try adding an --allowed_versions flag for the >>> remote helpers, but if anyone has a better idea, let me know. >> >> I forgot to mention: the stateless-connect remote helper capability is >> still experimental so you're free to change its interface (e.g. to >> change the syntax of the stateless-connect command it provides). > > For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version > string to the remote helpers. Makes sense. Thanks. :)
Jonathan Nieder <jrnieder@gmail.com> writes: > Josh Steadmon wrote: >> On 2018.10.12 16:32, Jonathan Nieder wrote: >>> Josh Steadmon wrote: > >>>> For now, I'm going to try adding an --allowed_versions flag for the >>>> remote helpers, but if anyone has a better idea, let me know. >>> >>> I forgot to mention: the stateless-connect remote helper capability is >>> still experimental so you're free to change its interface (e.g. to >>> change the syntax of the stateless-connect command it provides). >> >> For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version >> string to the remote helpers. > > Makes sense. Thanks. :) Yeah, it does.
diff --git a/connect.c b/connect.c index 94547e5056..4a8cd78239 100644 --- a/connect.c +++ b/connect.c @@ -1228,14 +1228,11 @@ struct child_process *git_connect(int fd[2], const char *url, struct child_process *conn; enum protocol protocol; enum protocol_version version = get_protocol_version_config(); + enum protocol_version max_version; - /* - * NEEDSWORK: If we are trying to use protocol v2 and we are planning - * to perform a push, then fallback to v0 since the client doesn't know - * how to push yet using v2. - */ - if (version == protocol_v2 && !strcmp("git-receive-pack", prog)) - version = protocol_v0; + max_version = determine_maximum_protocol_version(prog, version); + if (version > max_version) + version = max_version; /* Without this we cannot rely on waitpid() to tell * what happened to our children. diff --git a/protocol.c b/protocol.c index 5e636785d1..1c553d8b99 100644 --- a/protocol.c +++ b/protocol.c @@ -79,3 +79,16 @@ enum protocol_version determine_protocol_version_client(const char *server_respo return version; } + +enum protocol_version determine_maximum_protocol_version( + const char *service, enum protocol_version default_version) +{ + if (!strcmp(service, "git-receive-pack")) + return protocol_v1; + else if (!strcmp(service, "git-upload-archive")) + return protocol_v1; + else if (!strcmp(service, "git-upload-pack")) + return protocol_v2; + + return default_version; +} diff --git a/protocol.h b/protocol.h index 2ad35e433c..eabc0c5fab 100644 --- a/protocol.h +++ b/protocol.h @@ -31,4 +31,11 @@ extern enum protocol_version determine_protocol_version_server(void); */ extern enum protocol_version determine_protocol_version_client(const char *server_response); +/* + * Used by a client to determine the maximum protocol version to advertise for a + * particular service. If the service is unrecognized, return default_version. + */ +extern enum protocol_version determine_maximum_protocol_version( + const char *service, enum protocol_version default_version); + #endif /* PROTOCOL_H */ diff --git a/remote-curl.c b/remote-curl.c index fb28309e85..028adb76ae 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -344,6 +344,7 @@ static struct discovery *discover_refs(const char *service, int for_push) int http_ret, maybe_smart = 0; struct http_get_options http_options; enum protocol_version version = get_protocol_version_config(); + enum protocol_version max_version; if (last && !strcmp(service, last->service)) return last; @@ -360,13 +361,9 @@ static struct discovery *discover_refs(const char *service, int for_push) strbuf_addf(&refs_url, "service=%s", service); } - /* - * NEEDSWORK: If we are trying to use protocol v2 and we are planning - * to perform a push, then fallback to v0 since the client doesn't know - * how to push yet using v2. - */ - if (version == protocol_v2 && !strcmp("git-receive-pack", service)) - version = protocol_v0; + max_version = determine_maximum_protocol_version(service, version); + if (version > max_version) + version = max_version; /* Add the extra Git-Protocol header */ if (get_protocol_http_header(version, &protocol_header))
For services other than git-receive-pack, clients currently advertise that they support the version set in the protocol.version config, regardless of whether or not there is actually an implementation of that service for the given protocol version. This causes backwards- compatibility problems when a new implementation for the given protocol version is added. This patch sets maximum allowed protocol versions for git-receive-pack, git-upload-archive, and git-upload-pack. Previously, git-receive-pack would downgrade from v2 to v0, but would allow v1 if set in protocol.version. Now, it will downgrade from v2 to v1. Signed-off-by: Josh Steadmon <steadmon@google.com> --- connect.c | 11 ++++------- protocol.c | 13 +++++++++++++ protocol.h | 7 +++++++ remote-curl.c | 11 ++++------- 4 files changed, 28 insertions(+), 14 deletions(-)