Johannes Schindelin [off-list ref] writes:
after thinking about my earlier approach, I think there's a better, less
intrusive, and all in all just simpler approach:
- client asks for multi_ack protocol by sending " multi_ack" at the end
of at least one "want" line
- server appends " continue" to the ACK message, but continues sending
ACK (but not NAK) messages
- after receiving "done", server repeats last ACK message without
" continue" appended
After much fun with non-working, fragile code which had to be retracted
from master, I hope that this approach is less prone to errors.
Sorry for a late reply -- real life interrupts ;-).
I do not necessarily think the last round (still found in the
proposed updates branch) was a failure. One valuable thing you
found out was that there is a way to extend the protocol with
the appended "multi-ack" trick.
Sending "have you found a common yet" every 32 "have" like the
original protocol does has a nice batching property (I think we
could even tweak pkt-line.c::packet_write() to actually buffer
these "have"s and write them out in one-go, to put them in a
single network packet) and I do not want to lose it, but other
than that, since you have found a good way to extend the initial
handshake to find out if both ends can do a v2 protocol without
breaking older server nor clients, I have no objection against
doing things drastically differently in the v2 protocol.
I noticed the sketch in Documentation/pack-protocol.txt is
somewhat buggy. Here is my attempt to correct it:
upload-pack (S) | fetch/clone-pack (C) protocol (v1):
# Tell the puller what commits we have and what their names are
S: SHA1 name
S: ...
S: SHA1 name
S: # flush -- it's your turn
# Tell the pusher what commits we want, and what we have
C: want SHA1
C: ..
C: want SHA1
C: # flush -- done with "want" lines.
C: have SHA1
C: have SHA1
C: ...
C: # flush -- this occasionally asks "had enough?"
S: NAK
# and the server answers "notyet"
C: have SHA1
C: ...
C: have SHA1
S: ACK SHA1
C: done
S: XXXXXXX -- packfile contents.
I wrote as if "want" sends names, but it actually sends SHA1s.
Also I missed a place where "flush" was needed.
So let's illustrate the v2 the same way as I understand it.
upload-pack (S) | fetch/clone-pack (C) protocol (v2):
# Tell the puller what commits we have and what their names are
S: SHA1 name
S: ...
S: SHA1 name
S: # flush -- it's your turn
# Tell the pusher what commits we want, and what we have.
# In addition, we tell the other end that we support protocol
# extensions, without breaking the old servers.
C: want SHA1 extended
C: ..
C: want SHA1
C: # flush -- done with "want" lines.
Notice that until we hear from the server, we cannot tell if our
"extended" protocol wish will be granted, and in the original
protocol, "NAK" will come in fixed length, and the only thing we
could tack arbitrary garbage to was "ACK SHA1". That's why your
"ACK SHA1 continue" works nicely, but at the time you could not
find out if you are talking with updated server until you get at
least one ACK.
However, at this point, we *could* force the server to reveal
what it supports, by doing an extra flush here, before sending
*ANY* "have" lines yet:
C: # flush -- this is another one after "I'm done with wants".
Upon receiving this, if we were talking with an old upload-pack,
we would certanly get an NAK. Note that the server already
knows that we support extended protocol at this point, so our
updated server can send anything here to say it knows what
protocol extensions it supports. Let's say it says something
like this:
S: proto v2 v3 v5
to tell the puller it understands protocol v2, v3, and v5, to
which the puller responds:
C: proto v2
After this exchange, both ends know they understand and would
want to talk at protocol level v2. This leaves door open for
future protocol extension, but more importantly, I think this
arrangement would make things safer.
Both the server side and the client side code can be modified
from the current one by splitting the above handshake part and
the current trusty "only one ACK is supported" code, and the
exchange after this protocol negotiation part can be implemented
in totally separate functions. We could also give command line
option and/or .git/config item to limit the protocol level each
end supports when we find the v2 protocol implementation of the
day was buggy, in order to work problems around without
recompilation, reducing the risk of breaking things too much.
Hi,
On Thu, 27 Oct 2005, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
after thinking about my earlier approach, I think there's a better, less
intrusive, and all in all just simpler approach:
- client asks for multi_ack protocol by sending " multi_ack" at the end
of at least one "want" line
- server appends " continue" to the ACK message, but continues sending
ACK (but not NAK) messages
Here must be a correction: server continues to send NAK messages. Else we
must introduce some select() or poll() crud, or the client hangs forever.
quoted
- after receiving "done", server repeats last ACK message without
" continue" appended
In effect, an ACK message with "continue" switches into v2, which
just means: "I'll keep sendin'", and an ACK *without* "continue" switches
back to v1.
This way the patch looks almost trivial. (Will send soon).
quoted
After much fun with non-working, fragile code which had to be retracted
from master, I hope that this approach is less prone to errors.
Sorry for a late reply -- real life interrupts ;-).
No problem (hope you enjoyed it ;-). I tried to get some Zs anyway.
Sending "have you found a common yet" every 32 "have" like the
original protocol does has a nice batching property (I think we
could even tweak pkt-line.c::packet_write() to actually buffer
these "have"s and write them out in one-go, to put them in a
single network packet) and I do not want to lose it, but other
than that, since you have found a good way to extend the initial
handshake to find out if both ends can do a v2 protocol without
breaking older server nor clients, I have no objection against
doing things drastically differently in the v2 protocol.
I think that we don't need to extend packet_write(). Let's leave it like
that.
upload-pack (S) | fetch/clone-pack (C) protocol (v1):
# Tell the puller what commits we have and what their names are
S: SHA1 name
S: ...
S: SHA1 name
S: # flush -- it's your turn
# Tell the pusher what commits we want, and what we have
C: want SHA1
C: ..
C: want SHA1
C: # flush -- done with "want" lines.
C: have SHA1
C: have SHA1
C: ...
C: # flush -- this occasionally asks "had enough?"
S: NAK
# and the server answers "notyet"
C: have SHA1
C: ...
C: have SHA1
S: ACK SHA1
C: done
S: XXXXXXX -- packfile contents.
alternatively, when no "ACK" was sent, the response to "done" is "NAK".
I wrote as if "want" sends names, but it actually sends SHA1s.
You also ask if it is sensible in upload-pack.c to accept names. I think
not. SHA1s are supposed to be unique, names not (master~10 is likely to
denote different commits on server and client).
Also I missed a place where "flush" was needed.
Between "want" and "have". It is sent so that the server can handle them
in separate functions.
upload-pack (S) | fetch/clone-pack (C) protocol (v2):
# Tell the puller what commits we have and what their names are
S: SHA1 name
S: ...
S: SHA1 name
S: # flush -- it's your turn
# Tell the pusher what commits we want, and what we have.
# In addition, we tell the other end that we support protocol
# extensions, without breaking the old servers.
C: want SHA1 extended
C: ..
C: want SHA1
C: # flush -- done with "want" lines.
Notice that until we hear from the server, we cannot tell if our
"extended" protocol wish will be granted, and in the original
protocol, "NAK" will come in fixed length, and the only thing we
could tack arbitrary garbage to was "ACK SHA1". That's why your
"ACK SHA1 continue" works nicely, but at the time you could not
find out if you are talking with updated server until you get at
least one ACK.
However, at this point, we *could* force the server to reveal
what it supports, by doing an extra flush here, before sending
*ANY* "have" lines yet:
C: # flush -- this is another one after "I'm done with wants".
We don't need a second flush. The first can do. The client just has to
expect either "NAK" or "VER blabla blibli multi_ack". "VER" must be sent
by the server only when the client requested an extension the server has,
though.
S: proto v2 v3 v5
As written above, I'd prefer something like "VER" in order to stay with
the 3 capital letters convention. Also, I like to read what it is about,
i.e. "VER multi_ack" instead of "VER v2".
C: proto v2
I'd say that this is not necessary. Client can send that in the "want"
lines.
After this exchange, both ends know they understand and would
want to talk at protocol level v2. This leaves door open for
future protocol extension, but more importantly, I think this
arrangement would make things safer.
Yes. Nice.
I especially like that the client learns early that multi_ack protocol it
is. In my tests, non-multi_ack would only be happy when fed with commits
sorted by date, while multi_ack is much more efficient when fed commits
sorted by distance-to-tip (I cannot yet explain why this is so).
Ciao,
Dscho
On Thu, 27 Oct 2005 00:13:17 -0700 Junio C Hamano wrote:
[skip]
So let's illustrate the v2 the same way as I understand it.
upload-pack (S) | fetch/clone-pack (C) protocol (v2):
# Tell the puller what commits we have and what their names are
S: SHA1 name
S: ...
S: SHA1 name
S: # flush -- it's your turn
# Tell the pusher what commits we want, and what we have.
# In addition, we tell the other end that we support protocol
# extensions, without breaking the old servers.
C: want SHA1 extended
C: ..
C: want SHA1
C: # flush -- done with "want" lines.
Notice that until we hear from the server, we cannot tell if our
"extended" protocol wish will be granted, and in the original
protocol, "NAK" will come in fixed length, and the only thing we
could tack arbitrary garbage to was "ACK SHA1". That's why your
"ACK SHA1 continue" works nicely, but at the time you could not
find out if you are talking with updated server until you get at
least one ACK.
Actually, there is another way to pass some data from the server
which would be ignored by older clients - at the first stage,
when upload-pack sends the list of refs to the client:
packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname, '\0',
server_capabilities);
Old clients will ignore the additional data (functions which
work with the received name will happily stop at the added NUL
character; just some memory will be wasted), but the new
implementation of get_remote_heads() could look for that NUL
inside the received packet and find extended server capabilities
after it. Then the client could just use new commands at the
"want" stage.
However, at this point, we *could* force the server to reveal
what it supports, by doing an extra flush here, before sending
*ANY* "have" lines yet:
C: # flush -- this is another one after "I'm done with wants".
Upon receiving this, if we were talking with an old upload-pack,
we would certanly get an NAK. Note that the server already
knows that we support extended protocol at this point, so our
updated server can send anything here to say it knows what
protocol extensions it supports. Let's say it says something
like this:
S: proto v2 v3 v5
to tell the puller it understands protocol v2, v3, and v5, to
which the puller responds:
C: proto v2
After this exchange, both ends know they understand and would
want to talk at protocol level v2. This leaves door open for
future protocol extension, but more importantly, I think this
arrangement would make things safer.
This looks cleaner (no NUL bytes in the protocol), but does not
allow to replace the "want" stage with something entirely
different (if we ever want to do that).
Hi,
On Thu, 27 Oct 2005, Sergey Vlasov wrote:
Actually, there is another way to pass some data from the server
which would be ignored by older clients - at the first stage,
when upload-pack sends the list of refs to the client:
packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname, '\0',
server_capabilities);
That exploits that packet_write() uses vnsprintf() to find out the length,
not strlen(). Sweet.
get_remote_heads() would need to store the server_capabilities, maybe with
a function "server_supports(const char *extension_string)"
Actually, I like that even more than Junio's proposal. I was thinking hard
about how to enhance get_ack() to understand -- and most importantly to
return -- the capabilities. I could not think of an elegant way.
So, unless people complain, I'll go with that approach.
This looks cleaner (no NUL bytes in the protocol), but does not
allow to replace the "want" stage with something entirely
different (if we ever want to do that).
Also it feels wrong that a client should ask for capabilities, where it
does not yet know if the server supports them.
Ciao,
Dscho