On Wed, Aug 30, 2017 at 2:12 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote: > On 08/30, Bryan Turner wrote: >> On Fri, Aug 25, 2017 at 10:29 AM, Jeff King <peff@xxxxxxxx> wrote: >> > On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote: >> > >> >> 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. >> >> 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). Thanks for that! It made testing a lot simpler. > >> 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. > > Perfect! Thanks for taking the time to verify that this will work. With 2 small tweaks on top of "protocol-v2", I was able to run our full command and hosting (HTTP and SSH) test suites without issues. diff --git a/transport.c b/transport.c index c05e167..37b5d83 100644 --- a/transport.c +++ b/transport.c @@ -222,7 +222,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus switch(determine_version(data->fd[0], &buf)) { case 2: /* The server understands Protocol v2 */ - fprintf(stderr, "Server understands Protocol v2!\n"); + if (transport->verbose >= 0) + fprintf(stderr, "Server understands Protocol v2!\n"); break; case 1: /* Server is speaking Protocol v1 and sent a ref so process it */ diff --git a/upload-pack.c b/upload-pack.c index 0f85315..7c59495 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1075,7 +1075,7 @@ int cmd_main(int argc, const char **argv) git_config(upload_pack_config, NULL); version = getenv("GIT_PROTOCOL"); - if (!strcmp(version, "2")) + if (version && !strcmp(version, "2")) upload_pack_v2(); upload_pack(); I'd imagine the "Server understands Protocol v2!" message won't actually *ship* in Git, so it's likely making that honor "--quiet" doesn't actually matter; I only adjusted it because we have a test that verifies "git clone --quiet" is quiet. The "if (version" change I commented on separately in the 7/7 patch that introduced the check. Thanks again for publishing this, and for letting me test it! > > -- > Brandon Williams
