Re: [RFC 0/7] transitioning to protocol v2

[Bryan Turner <bturner@atlassian.com>]

On Wed, Aug 30, 2017 at 2:12 PM, Brandon Williams <bmwill@google.com> wrote:
> On 08/30, Bryan Turner wrote:
>> On Fri, Aug 25, 2017 at 10:29 AM, Jeff King <peff@peff.net> 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