[PATCH v2 0/4] Add proto v2 archive command with HTTP support
This is the second version of my series to add a new protocol v2 command
for archiving, with support for HTTP(S).
NEEDSWORK: a server built with this series is not backwards-compatible
with clients that set GIT_PROTOCOL=version=2 or configure
protocol.version=2. The old client will unconditionally send "argument
..." packet lines, which breaks the server's expectations of a
"command=archive" request, while the server's capability advertisement
in turn breaks the clients expectation of either an ACK or NACK.
I've been discussing workarounds for this with Jonathan Nieder, but
please let me know if you have any suggestions for v3 of this series.
Josh Steadmon (4):
archive: follow test standards around assertions
archive: use packet_reader for communications
archive: implement protocol v2 archive command
archive: allow archive over HTTP(S) with proto v2
Documentation/technical/protocol-v2.txt | 21 -
builtin/archive.c | 58 +++--
builtin/upload-archive.c| 27 ++--
http-backend.c | 13 +-
serve.c | 7 +++
t/t5000-tar-tree.sh | 33 +++---
t/t5701-git-serve.sh| 1 +
transport-helper.c | 7 +--
8 files changed, 130 insertions(+), 37 deletions(-)
Range-diff against v1:
-: -- > 1: c2e371ad24 archive: follow test standards around assertions
1: b514184273 ! 2: a65f73f627 archive: use packet_reader for communications
@@ -6,7 +6,10 @@
handling, which will make implementation of protocol v2 support in
git-archive easier.
+This refactoring does not change the behavior of "git archive".
+
Signed-off-by: Josh Steadmon
+Reviewed-by: Stefan Beller
diff --git a/builtin/archive.c b/builtin/archive.c
@@ -42,24 +45,24 @@
- if (!buf)
+ status = packet_reader_read(&reader);
+
-+ if (status == PACKET_READ_FLUSH)
++ if (status != PACKET_READ_NORMAL || reader.pktlen <= 0)
die(_("git archive: expected ACK/NAK, got a flush packet"));
- if (strcmp(buf, "ACK")) {
- if (starts_with(buf, "NACK "))
- die(_("git archive: NACK %s"), buf + 5);
- if (starts_with(buf, "ERR "))
- die(_("remote error: %s"), buf + 4);
-+ if (strcmp(reader.buffer, "ACK")) {
-+ if (starts_with(reader.buffer, "NACK "))
-+ die(_("git archive: NACK %s"), reader.buffer + 5);
-+ if (starts_with(reader.buffer, "ERR "))
-+ die(_("remote error: %s"), reader.buffer + 4);
++ if (strcmp(reader.line, "ACK")) {
++ if (starts_with(reader.line, "NACK "))
++ die(_("git archive: NACK %s"), reader.line + 5);
++ if (starts_with(reader.line, "ERR "))
++ die(_("remote error: %s"), reader.line + 4);
die(_("git archive: protocol error"));
}
- if (packet_read_line(fd[0], NULL))
+ status = packet_reader_read(&reader);
-+ if (status != PACKET_READ_FLUSH)
++ if (status == PACKET_READ_NORMAL && reader.pktlen > 0)
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
2: 1518c15dc1 < -: -- archive: implement protocol v2 archive command
-: -- > 3: 0a8cc5e331 archive: implement protocol v2 archive command
3: 1b7ad8d8f6 ! 4: 97a1424f32 archive: allow archive over HTTP(S) with proto
v2
@@ -10,16 +10,20 @@
+++ b/builtin/archive.c
@@
status = packet_reader_read(&reader);
- if (status != PACKET_READ_FLUSH)
+ if (status == PACKET_READ_NORMAL && reader.pktlen > 0)
die(_("git archive: expected a flush"));
- }
+ } else if (version == protocol_v2 &&
-+ starts_with(transport->url, "http"))
++ (starts_with(transport->url, "http://";) ||
++ starts_with(transport->url, "https://";)))
+ /*
+ * Commands over HTTP require two requests, so there's an
-+ * additional server response to parse.
++ * additional server response to parse. We do only basic sanity
++ * checking here that the versions presented match across
++ * requests.
+ */
-+ discover_version(&reader);
++ if (version != discover_version(&reader))
++ die(_("git archive: received different protocol
versions in subsequent requests"));
/* Now, start reading from fd[0] and spit it out to stdout */
rv = recv_sideband("archive", fd[0], 1);
@@ -40,7 +44,10 @@
struct strbuf buf = STRBUF_INIT;
+ if (!strcmp(servic
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:
>
> This is the second version of my series to add a new protocol v2 command
> for archiving, with support for HTTP(S).
>
> NEEDSWORK: a server built with this series is not backwards-compatible
> with clients that set GIT_PROTOCOL=version=2 or configure
> protocol.version=2. The old client will unconditionally send "argument
> ..." packet lines, which breaks the server's expectations of a
> "command=archive" request,
So if an old client sets protocol to v2, it would only apply that
protocol version
to fetch, not archive, so it would start following a v0 conversation, but
as the protocol version is set, it would be transmitted to the server.
This sounds like a bug in the client?
> while the server's capability advertisement
> in turn breaks the clients expectation of either an ACK or NACK.
Could a modern client send either another protocol version (3?)
or a special capability along the protocol version ("fixed_archive")
> I've been discussing workarounds for this with Jonathan Nieder, but
> please let me know if you have any suggestions for v3 of this series.
Care to open the discussion to the list? What are the different
approaches, what are the pros/cons?
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
On 2018.09.27 11:20, Stefan Beller wrote:
> On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote:
> >
> > This is the second version of my series to add a new protocol v2 command
> > for archiving, with support for HTTP(S).
> >
> > NEEDSWORK: a server built with this series is not backwards-compatible
> > with clients that set GIT_PROTOCOL=version=2 or configure
> > protocol.version=2. The old client will unconditionally send "argument
> > ..." packet lines, which breaks the server's expectations of a
> > "command=archive" request,
>
> So if an old client sets protocol to v2, it would only apply that
> protocol version
> to fetch, not archive, so it would start following a v0 conversation, but
> as the protocol version is set, it would be transmitted to the server.
> This sounds like a bug in the client?
Yeah, basically. We're telling the server we support v2, even if the
specific operation we're trying to do doesn't have a v2 implementation
on the client. So this is going to make it ugly to replace existing
commands.
> > while the server's capability advertisement
> > in turn breaks the clients expectation of either an ACK or NACK.
>
> Could a modern client send either another protocol version (3?)
> or a special capability along the protocol version ("fixed_archive")
>
> > I've been discussing workarounds for this with Jonathan Nieder, but
> > please let me know if you have any suggestions for v3 of this series.
>
> Care to open the discussion to the list? What are the different
> approaches, what are the pros/cons?
Jonathan suggested something along the lines of what you said above,
adding a new field in GIT_PROTOCOL. So we'd send something like
"version=2:archive_version=2" and have the server detect the latter.
I'm not sure if that's the best way to go about this since I'm not
familiar with the version detection code for other parts of the system.
I worry that it will lead us down the path of having to specify a
version for every command that we eventually convert to protocol v2. On
the other hand, I don't see any other way to work around this, at least
in the archive case. We can't peek at the client's transmissions on the
server, because v2 requires that the server speaks first...
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
Stefan Beller wrote: > On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon wrote: >> I've been discussing workarounds for this with Jonathan Nieder, but >> please let me know if you have any suggestions for v3 of this series. > > Care to open the discussion to the list? What are the different > approaches, what are the pros/cons? Do you mean sending video of chatting in the office? Josh and I discussed that 1. Clients sending version=2 when they do not, in fact, speak protocol v2 for a service is a (serious) bug. (Separately from this series) we should fix it. 2. That bug is already in the wild, alas. Fortunately the semantics of GIT_PROTOCOL as a list of key/value pairs is well defined. So we have choices of (a) bump version to version=3 (b) pass another value 'version=2:yesreallyversion=2' (c) etc. 3. This is likely to affect push, too. Thanks and hope that helps, Jonathan
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
Jonathan Nieder writes: > 1. Clients sending version=2 when they do not, in fact, speak protocol > v2 for a service is a (serious) bug. (Separately from this > series) we should fix it. > > 2. That bug is already in the wild, alas. Fortunately the semantics of > GIT_PROTOCOL as a list of key/value pairs is well defined. So we > have choices of (a) bump version to version=3 (b) pass another > value 'version=2:yesreallyversion=2' (c) etc. > > 3. This is likely to affect push, too. Do you mean that existing "git push", "git fetch" and "git archive" sends version=2 even when they are not capable of speaking protocol v2? I thought that "git archive [--remote]" was left outside of the protocol update (that was the reason why the earlier attempt took a hacky route of "shallow clone followed by local archive"), so there is no "git archive" in the wild that can even say "version=$n" (which requires you to be at least version=1)?
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
On 2018.09.27 15:20, Junio C Hamano wrote:
> Jonathan Nieder writes:
>
> > 1. Clients sending version=2 when they do not, in fact, speak protocol
> > v2 for a service is a (serious) bug. (Separately from this
> > series) we should fix it.
> >
> > 2. That bug is already in the wild, alas. Fortunately the semantics of
> > GIT_PROTOCOL as a list of key/value pairs is well defined. So we
> > have choices of (a) bump version to version=3 (b) pass another
> > value 'version=2:yesreallyversion=2' (c) etc.
> >
> > 3. This is likely to affect push, too.
>
> Do you mean that existing "git push", "git fetch" and "git archive"
> sends version=2 even when they are not capable of speaking protocol
> v2? I thought that "git archive [--remote]" was left outside of the
> protocol update (that was the reason why the earlier attempt took a
> hacky route of "shallow clone followed by local archive"), so there
> is no "git archive" in the wild that can even say "version=$n"
> (which requires you to be at least version=1)?
Yes, the version on my desktop sends version=2 when archiving:
∫ which git
/usr/bin/git
∫ git --version
git version 2.19.0.605.g01d371f741-goog
∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \
--enable=upload-archive \
--base-path=${HOME}/src/bare-repos &
[1] 258496
∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar
∫ grep version ~/server_trace
15:31:22.377869 pkt-line.c:80 packet: git<
git-upload-archive /test-repo.git\0host=localhost\0\0version=2\0
Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support
Josh Steadmon writes:
> Yes, the version on my desktop sends version=2 when archiving:
>
> ∫ which git
> /usr/bin/git
> ∫ git --version
> git version 2.19.0.605.g01d371f741-goog
> ∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \
> --enable=upload-archive \
> --base-path=${HOME}/src/bare-repos &
> [1] 258496
> ∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar
> ∫ grep version ~/server_trace
> 15:31:22.377869 pkt-line.c:80 packet: git<
> git-upload-archive /test-repo.git\0host=localhost\0\0version=2\0
Ah, that's truly broken.
Come to think of it, do we need to be using uniform versions across
different endpoints? The archive request could be at v3 while fetch
request could still be at v2, in which case the design to use a
single protocol.version variable is probably the root cause of the
confusion? Perhaps like protocol..allow, we would want
protocol..version or something like that (and no
protocol.version) to make it clear that protocol v2 used for
fetching has nothing to do with protocol v1 or v2 or v3 used for
archiving?
Luckily, protocol.version is still marked as experimental so it is
not too bad that we caught the design mistake (if it is one) and can
now correct it before the damage spreads too widely.
