transitioning to protocol v2

connect.c struct child_process *git_connect(int fd[2], const char *url, · line 833

Quoted 5 lines · Brandon Williams

connect.c +5 -0

+		/* If using a new version put that stuff here after a second null byte */
+		strbuf_addch(&request, '\0');
+		strbuf_addf(&request, "version=%d%c", 2, '\0');
+		/* subsequent supported versions can also be added */
+		strbuf_addf(&request, "version=%d%c", 3, '\0');

Isn't this last one meant only as a comment?

Quoted 9 lines · Junio C Hamano

connect.c struct child_process *git_connect(int fd[2], const char *url, · line 833

Quoted 5 lines · Brandon Williams

connect.c +5 -0

+		/* If using a new version put that stuff here after a second null byte */
+		strbuf_addch(&request, '\0');
+		strbuf_addf(&request, "version=%d%c", 2, '\0');
+		/* subsequent supported versions can also be added */
+		strbuf_addf(&request, "version=%d%c", 3, '\0');

Isn't this last one meant only as a comment?

Sorry since this was structured as an RFC I didn't go back through the code with a fine tooth comb to ensure I removed or commented out any debugging statements. Stefan also pointed out to me that I left in an if (0) statement somewhere haha.

Tell a serve that protocol v2 can be used by sending an http header indicating this.

upload-pack.c int cmd_main(int argc, const char **argv) · line 1080

Quoted 5 lines · Brandon Williams

upload-pack.c +5 -0

+
+       version = getenv("GIT_PROTOCOL");
+       if (!strcmp(version, "2"))
+               upload_pack_v2();
+

I think the "if" check here needs some type of NULL check for "version" before calling "strcmp". Without that, if the "GIT_PROTOCOL" environment variable isn't set, git-upload-pack SEGVs.

This came up when I was testing the "protocol-v2" branch with Bitbucket Server. For performance reasons, we sometimes run ref advertisements as a separate process, when serving fetches, to allow throttling the ref advertisement separately from actually generating and sending a packfile. Since CI servers continuously poll for changes, but usually don't find any, we want to be able to serve ref advertisements, which have minimal overhead, with much higher parallelism than serving packs. To do that, we run "git-upload-pack --advertize-refs" directly, and that command has never *required* "GIT_PROTOCOL" before this change.

The best way to preserve functionality with old servers and clients would be to communicate using the same end point but have the client send a bit of extra information with its initial request. This extra information would need to be sent in such a way that old servers ignore it and operate normally (using protocol v1). The client would then need to be able to look at a server's response to determine whether the server understands and is speaking v2 or has ignored the clients request to use a newer protocol and is speaking v1.

Patches 1-5 enable a client to unconditionally send this back-channel information to a server. This is done by sending a version number after a second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://, and in an http header in http://, https://. Patches 6-7 teach a client and upload-pack to send and recognize a request to use protocol v2.

Quoted 7 lines · Brandon Williams

The best way to preserve functionality with old servers and clients would be to communicate using the same end point but have the client send a bit of extra information with its initial request. This extra information would need to be sent in such a way that old servers ignore it and operate normally (using protocol v1). The client would then need to be able to look at a server's response to determine whether the server understands and is speaking v2 or has ignored the clients request to use a newer protocol and is speaking v1.

Good. I think the idle talk last round was for the server to tell the v1 client "we are still doing the slow ls-remote comes first protocol with this exchange, but just for future reference, you can use the v2 endpoint with me", which was way less desirable (even though it may be safer).

Quoted 5 lines

Patches 1-5 enable a client to unconditionally send this back-channel information to a server. This is done by sending a version number after a second NUL byte in git://, in the envvar GIT_PROTOCOL in file:// and ssh://, and in an http header in http://, https://. Patches 6-7 teach a client and upload-pack to send and recognize a request to use protocol v2.

All sounds sensible.

- for git://, if you say you found a hole in the protocol to stuff this information, I simply believe you ;-) Good job.

- http:// and https:// should be a no-brainer as the HTTP headers give ample room to send information from the initiator side.

- For ssh://, I do not think it is sane to assume that we can convince server operators to allow passing any random environment variable. If the use of this specific variable turns out to be popular enough, however, and its benefit outweighs administrative burden, perhaps it is not impossible to convince them to allow AcceptEnv on this single variable.

Thanks.

For now I would suggest we put a protocol v2 in place that is the current protocol + a version number coming through the poked hole at the beginning; the goal and review of this series ought to focus on getting the version handshake right...

Quoted 4 lines · Stefan Beller

For now I would suggest we put a protocol v2 in place that is the current protocol + a version number coming through the poked hole at the beginning; the goal and review of this series ought to focus on getting the version handshake right...

Oh, we are in absolute agreement on that. It would be nice if we can have new tests to demonostrate three combinations working well (i.e. use 'installed git' whose path is given externally on one end of the connection, while the just-built binary sits on the other end, in addition to making sure just-built binary successfully talks with itself).

Another version of Git's wire protocol is a topic that has been discussed and attempted by many in the community over the years. The biggest challenge, as far as I understand, has been coming up with a transition plan to using the new server without breaking existing clients and servers. As such this RFC is really only concerned with solidifying a transition plan. Once it has been decided how we can transition to a new protocol we can get into decided what this new protocol would look like (though it would obviously eliminate the ref advertisement ;).

The biggest question I'm trying to answer is if these are reasonable ways with which to communicate a request to a server to use a newer protocol, without breaking current servers/clients. As far as I've tested, with patches 1-5 applied I can still communicate with current servers without causing any problems.

Quoted 8 lines · Brandon Williams

Another version of Git's wire protocol is a topic that has been discussed and attempted by many in the community over the years. The biggest challenge, as far as I understand, has been coming up with a transition plan to using the new server without breaking existing clients and servers. As such this RFC is really only concerned with solidifying a transition plan. Once it has been decided how we can transition to a new protocol we can get into decided what this new protocol would look like (though it would obviously eliminate the ref advertisement ;).

Sadly, while splitting these things apart makes the protocol conceptually cleaner, I'm not sure if we can consider them separately and avoid adding an extra round-trip to the protocol.

Given the techniques you've used here, I suspect the answer may be "yes". We could stick arbitrary data in each of those methods (though I suspect our length may be limited to about 1024 bytes if we want compatibility with very old git servers).

Quoted 5 lines

The biggest question I'm trying to answer is if these are reasonable ways with which to communicate a request to a server to use a newer protocol, without breaking current servers/clients. As far as I've tested, with patches 1-5 applied I can still communicate with current servers without causing any problems.

Current git.git servers, I assume?. How much do we want to care about alternate implementations? I would not be surprised if other git://
implementations are more picky about cruft after the virtual-host field (though I double-checked GitHub's implementation at least, and it is fine).

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

There's not really a spec here.

I dunno. Maybe it would be enough to have a config to switch off this feature, which would give people using those systems an escape hatch (until they upgrade).

Or alternatively, I guess make this optional to
start with, and let early adopters turn it on and complain to their server vendors for a while before flipping the default to on.

Quoted 3 lines

Sadly, while splitting these things apart makes the protocol conceptually cleaner, I'm not sure if we can consider them separately and avoid adding an extra round-trip to the protocol.

How about the idea of using this mechanism to implement a protocol "v1"?

The reply would be the same as today, except that it has a "protocol v1" pkt-line at the beginning. So this doesn't change the number of round-trips --- it just validates the protocol migration approach.

Quoted 5 lines

Current git.git servers, I assume?. How much do we want to care about alternate implementations? I would not be surprised if other git://
implementations are more picky about cruft after the virtual-host field (though I double-checked GitHub's implementation at least, and it is fine).

FWIW JGit copes fine with this.

Quoted 3 lines

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

I'd be happy if someone tests the patches against those. :)

Quoted 3 lines

Or alternatively, I guess make this optional to
start with, and let early adopters turn it on and complain to their server vendors for a while before flipping the default to on.

Can we do that by making it a patch / letting it cook for a while in 'next'? :)

Quoted 7 lines · Jonathan Nieder

Quoted 3 lines

Sadly, while splitting these things apart makes the protocol conceptually cleaner, I'm not sure if we can consider them separately and avoid adding an extra round-trip to the protocol.

How about the idea of using this mechanism to implement a protocol "v1"?

Quoted 3 lines

The reply would be the same as today, except that it has a "protocol v1" pkt-line at the beginning. So this doesn't change the number of round-trips --- it just validates the protocol migration approach.

I'm not that worried about validating the ideas here to shoe-horn a version field. I'm worried about what "step 2" is going to look like, and whether "we shoe-horned a version field" can be extended to "we shoe-horned arbitrary data".

Quoted 2 lines

Can we do that by making it a patch / letting it cook for a while in 'next'? :)

If people actually ran 'next', that would help. I was hoping that we could get it out to the masses behind a feature flag, and dangle it in front of them with "this will improve fetch performance if you turn it on". But that carrot implies going all the way through the follow-on steps of designing the performance-improving v2 extensions and getting them implemented on the server side.

Hi,

Quoted 11 lines · Jeff King

Quoted 8 lines · Brandon Williams

Another version of Git's wire protocol is a topic that has been discussed and attempted by many in the community over the years. The biggest challenge, as far as I understand, has been coming up with a transition plan to using the new server without breaking existing clients and servers. As such this RFC is really only concerned with solidifying a transition plan. Once it has been decided how we can transition to a new protocol we can get into decided what this new protocol would look like (though it would obviously eliminate the ref advertisement ;).

Quoted 3 lines

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

I'd be happy if someone tests the patches against those. :)

Quoted 15 lines · Jonathan Nieder

Hi,

Quoted 11 lines · Jeff King

Quoted 8 lines · Brandon Williams

Another version of Git's wire protocol is a topic that has been discussed and attempted by many in the community over the years. The biggest challenge, as far as I understand, has been coming up with a transition plan to using the new server without breaking existing clients and servers. As such this RFC is really only concerned with solidifying a transition plan. Once it has been decided how we can transition to a new protocol we can get into decided what this new protocol would look like (though it would obviously eliminate the ref advertisement ;).

Quoted 5 lines

Quoted 3 lines

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

I'd be happy if someone tests the patches against those. :)

I just wanted to jump in here and say I've done some initial testing of this against VSTS and so far it seems fine. And yes, we have a custom git server.

VSTS doesn't support the "git://" protocol, so the double-null trick isn't an issue for us. But "https://" worked just fine. I'm still asking around internally whether we support passing SSH environment variables.

Jeff

I just wanted to jump in here and say I've done some initial testing of this against VSTS and so far it seems fine. And yes, we have a custom git server.

VSTS doesn't support the "git://" protocol, so the double-null trick isn't an issue for us. But "https://" worked just fine. I'm still asking around internally whether we support passing SSH environment variables.

Quoted 3 lines · Jeff Hostetler

I just wanted to jump in here and say I've done some initial testing of this against VSTS and so far it seems fine. And yes, we have a custom git server.

Great, thank you for checking.

Quoted 4 lines

VSTS doesn't support the "git://" protocol, so the double-null trick isn't an issue for us. But "https://" worked just fine. I'm still asking around internally whether we support passing SSH environment variables.

The key thing for ssh is not whether you support passing environment variables. It's whether you quietly ignore unknown variables rather than cutting off the connection.

To support the v2 protocol you'd need to pass the new variables, but you'd also need to modify your server to actually do something useful with them anyway. At this point we're mostly concerned with whether we can safely pass the variables to current implementations unconditionally and get a reasonable outcome.

Quoted 17 lines · Jeff King

Quoted 3 lines · Jeff Hostetler

I just wanted to jump in here and say I've done some initial testing of this against VSTS and so far it seems fine. And yes, we have a custom git server.

Great, thank you for checking.

Quoted 4 lines

VSTS doesn't support the "git://" protocol, so the double-null trick isn't an issue for us. But "https://" worked just fine. I'm still asking around internally whether we support passing SSH environment variables.

The key thing for ssh is not whether you support passing environment variables. It's whether you quietly ignore unknown variables rather than cutting off the connection.

Quoted 5 lines

To support the v2 protocol you'd need to pass the new variables, but you'd also need to modify your server to actually do something useful with them anyway. At this point we're mostly concerned with whether we can safely pass the variables to current implementations unconditionally and get a reasonable outcome.

Right. I just spoke with our server folks and, currently, our SSH
support quietly eats ALL variables. So we're safe :-)

I'm starting a conversation with them to pass them thru so we can be ready for this. (Assuming we choose to go this way.)

But what if we instead think of it not as "protocol v2" but as "can I give the server some hints that it may end up ignoring", then we end up with something more like:

C: please run upload-pack (btw, I'm only interested in refs/heads/foo)
S: advertisement + caps (hopefully limited to foo, but client is prepared to receive all) ... etc, as before ...

Or alternatively, I guess make this optional to start with, and let early adopters turn it on and complain to their server vendors for a while before flipping the default to on.

Quoted 5 lines · Brandon Williams

The biggest question I'm trying to answer is if these are reasonable ways with which to communicate a request to a server to use a newer protocol, without breaking current servers/clients. As far as I've tested, with patches 1-5 applied I can still communicate with current servers without causing any problems.

Current git.git servers, I assume?. How much do we want to care about alternate implementations? I would not be surprised if other git://
implementations are more picky about cruft after the virtual-host field (though I double-checked GitHub's implementation at least, and it is fine).

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

Quoted 14 lines · Jeff King

Quoted 5 lines · Brandon Williams

The biggest question I'm trying to answer is if these are reasonable ways with which to communicate a request to a server to use a newer protocol, without breaking current servers/clients. As far as I've tested, with patches 1-5 applied I can still communicate with current servers without causing any problems.

Current git.git servers, I assume?. How much do we want to care about alternate implementations? I would not be surprised if other git://
implementations are more picky about cruft after the virtual-host field (though I double-checked GitHub's implementation at least, and it is fine).

Quoted 3 lines

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

Before I manually apply the patches to test how they work with Bitbucket Server, are they applied on a branch somewhere where I can just fetch them? If not, I'll apply them manually and verify.

Just based on the description, though, I expect no issues. We don't currently support the git:// protocol. Our HTTP handling passes headers through to the receive-pack and upload-pack processes as environment variables (with a little massaging), but doesn't consider them itself; it only considers the URL and "service" query parameter to decide what command to run and to detect "dumb" requests. Our SSH handling ignores any environment variables provided and does not forward them to the git process, similar to VSTS.

I'll confirm explicitly, to be certain, but just based on reading the overview and knowing our code I think the described approaches should work fine.

Quoted 19 lines · Bryan Turner

Quoted 17 lines · Jeff King

Quoted 5 lines · Brandon Williams

The biggest question I'm trying to answer is if these are reasonable ways with which to communicate a request to a server to use a newer protocol, without breaking current servers/clients. As far as I've tested, with patches 1-5 applied I can still communicate with current servers without causing any problems.

Current git.git servers, I assume?. How much do we want to care about alternate implementations? I would not be surprised if other git://
implementations are more picky about cruft after the virtual-host field (though I double-checked GitHub's implementation at least, and it is fine).

I don't think libgit2 implements the server side. That leaves probably JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian and GitLab use.

Quoted 3 lines

Before I manually apply the patches to test how they work with Bitbucket Server, are they applied on a branch somewhere where I can just fetch them? If not, I'll apply them manually and verify.

I just pushed this set of patches up to: https://github.com/bmwill/git/tree/protocol-v2
so you should be able to fetch them from there (saves you from having to manually applying the patches).

Quoted 10 lines

Just based on the description, though, I expect no issues. We don't currently support the git:// protocol. Our HTTP handling passes headers through to the receive-pack and upload-pack processes as environment variables (with a little massaging), but doesn't consider them itself; it only considers the URL and "service" query parameter to decide what command to run and to detect "dumb" requests. Our SSH handling ignores any environment variables provided and does not forward them to the git process, similar to VSTS.

Quoted 3 lines

I'll confirm explicitly, to be certain, but just based on reading the overview and knowing our code I think the described approaches should work fine.

Perfect! Thanks for taking the time to verify that this will work.

--
Brandon Williams