archive mirror
 help / color / mirror / Atom feed
From: "" <>
To: "Jeff King" <>, "Junio C Hamano" <>
Cc: git <>
Subject: Re: Re: [PATCH 2/2] remote-curl.c: handle v1 when check_smart_http
Date: Mon, 29 Mar 2021 17:30:29 +0800	[thread overview]
Message-ID: <> (raw)

>On Wed, Mar 24, 2021 at 01:28:32PM -0700, Junio C Hamano wrote:
>> > +	} else if (!strcmp(reader.line, "version 1")) {
>> > +	die(_("v1 is just the original protocol with a version string, use v0 or v2 instead."));
>> The user may no longer get "invalid response; got 'version 1'", but
>> the above does not still explain why v1 is bad and v0 or v2 is
>> welcome, either.  IOW, I do not think the patch improves the message
>> to achieve what it attempted to do, i.e.
>>     ... but the other side just treat it as "invalid response", this
>>     can't explain why is not ok.
>> I wonder if it is a sensible and better alternative to treat v1
>> response as if we got v0 (if v1 is truly the same as v0 except for
>> the initial version advertisement).
>> Input from those who are familiar with the protocol versions is very
>> much appreciated.
>Yes, "v1" is supposed to behave just like v0, except with the version
>advertisement (it is true that there is no point in normal people using
>it, but the purpose was to make sure the version advertisement worked).
>I am not sure who is rejecting it, though. Our test suite passes with
>GIT_TEST_PROTOCOL_VERSION=1. Running something like:
> $ GIT_TRACE_PACKET=1 git -c protocol.version=1 ls-remote
>yields a conversation like (cut down for clarity):
>  git< # service=git-upload-pack
>  git< 0000
>  git< version 1
>  git< 1234abcd[...etc, this is a normal v0/v1 advertisement]
>So the version string is there, but it does not trigger the problem
>described by this patch. That's because check_smart_http(), after seeing
>the "# service" line and the flush, takes all the rest of the packetized
>data and gives it to parse_git_refs(), which handles the version field
>line via discover_version().
>  Aside: on, the v1 response looks like a v0 response, with
>  no extra header. I guess they did not bother to implement v1, which is
>  OK, since it was not useful after the initial experiment.
>So everything seems to be working as intended. Is there some particular
>server that returns "version 1" in the wrong way, triggering the die()?
On, I got "version 1", and the process died here.

>One curiosity is that for v2, the response from does include
>the "service" line. So it follows the same path as v1, and never hits
>the "version 2" line check here. But http-backend omits the "service"
>line, due to 237ffedd46 (http: eliminate "# service" line when using
>protocol v2, 2018-03-15).
Keen observation :)

>So it's interesting that GitHub behaves differently than http-backend
>here. It's not surprising, since the HTTP framing is all done by a
>custom server there, which implemented off the spec.  What _is_
>surprising is that the client seems perfectly happy to see either form,
>and nobody has noticed the difference until just now.
>IMHO the spec is very unclear here; it says "client makes a smart
>info/refs request as described in http-protocol.txt", but doesn't call
>out the difference in the response. It's only implied by the example:
>  A v2 server would reply:
>     S: 200 OK
>     S: <Some headers>
>     S: ...
>     S:
>     S: 000eversion 2\n
>     S: <capability-advertisement>
>where it is unclear whether the blank line is separating HTTP headers
>from the body (and thus "..." is some headers), or if it is separating
>the "# service" line and matching flush from the rest of the response
>I note that also returns the "service" line for v2 (I don't
>know anything about their implementation, but I would not be at all
>surprised if they also use a custom HTTP endpoint; apache+http-backend
>is not very flexible or scalable).
> returns the "version 1" line for v1, so died for invalid server response
and it returns the "version 2" line for v2, which is expected.

>Anyway, that's all just an interesting side note. The client is happy
>with either form (though it might be nice if we had tests for the "#
>service" form; I suspect our tests don't cover that because they are all
>using http-backend).
>Getting back to the patch at hand, if there is a server saying "version
>1" without a "service" line, then I think that is a bug in that server.
If the problem is on the server side, then, is this patch worth continuing?



  parent reply	other threads:[~2021-03-29  9:31 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
2021-03-24  5:36 ` [PATCH 2/2] remote-curl.c: handle v1 when check_smart_http lilinchao
2021-03-24 20:28   ` Junio C Hamano
2021-03-24 20:33     ` Junio C Hamano
     [not found]     ` <>
2021-03-25  4:22       ` lilinchao
2021-03-26  6:24     ` Jeff King
2021-03-26  6:55       ` Jeff King
     [not found]     ` <>
2021-03-29  9:30       ` lilinchao [this message]
2021-03-29 10:40         ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).