Junio C Hamano <gitster@xxxxxxxxx> writes:
> Alex Neronskiy <zakmagnus@xxxxxxxxxx> writes:
>
>> The part of the document I'm referring to starts at line 221 and reads:
>>
>> Once all the "want"s (and optional 'deepen') are transferred,
>> clients MUST send a flush-pkt. If the client has all the references
>> on the server, client flushes and disconnects.
>>
>> And I believe this refers to the code path beginning at line 308 of fetch-pack.c:
>>
>> if (!fetching) {
>> strbuf_release(&req_buf);
>> packet_flush(fd[1]);
>> return 1;
>> }
>>
>> Am I wrong?
>
> Ah, I overlooked that codepath, but if that if statement triggered, that
> would mean fetching is still 0, which in turn means that you never sent
> any "want", so "Once all the 'want's' (and optional 'deepen') are
> transferred" is not even true, is it?
Also I suspect that this might not even trigger. fetching will stay 0 only
when all the refs->old_sha1 refer to commits already marked as COMPLETE, but
everything_local() would already have covered that case and returned 1.
The code dates back to 2759cbc (git-fetch-pack: avoid unnecessary zero
packing, 2005-10-18) and I tend to trust what Linus wrote, but in that
much simpler version of the code, I think the check is redundant.
In either case, I think what the server sees (in other words, what goes
over the wire at the protocol level) is the same. The client receives
references and capabilities, and sends the flush without giving any "want"
to the server.
And that is exactly what is described in the first paragraph of "Packfile
Negotiation" section.
I am inclined to conclude that the original documentation "If the client
has all the references on the server, client flushes and disconnects",
while technically not incorrect, is redundant (because that particular
behaviour on-the-wire is already spelled out in the first paragraph), and
misleading (because that condition is determined by the client without
sending "want" etc., but the second paragraph "Once the client has ..., it
will begin telling the server..." that comes way before this "client can
quit without requesting anything" gives a false impression that the client
will spend all these effort of sending "want" and can flush and disconnect.
In fact the original does not say the client should not ask for objects it
already has (as COMPLETE) with "want", and as you noticed, the expression
"has all the references" is probably the source of confusion. It should
have said "If the client did not send any "want", it may flush and
disconnect". Both the original and your update will allow an incorrectly
implemented client to send "want"s, realize that it does not want any, and
then flush to disconnect, but then the server will have to try to produce
a pack to send to the client, which is certainly not what we wanted to
specify.
So I would suggest removing the later part of the paragraph, and update
the first paragraph of the section instead.
Perhaps like this...
Documentation/technical/pack-protocol.txt | 21 +++++++++++----------
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 369f91d..f860f2a 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -179,14 +179,15 @@ and descriptions.
Packfile Negotiation
--------------------
-After reference and capabilities discovery, the client can decide
-to terminate the connection by sending a flush-pkt, telling the
-server it can now gracefully terminate (as happens with the ls-remote
-command) or it can enter the negotiation phase, where the client and
-server determine what the minimal packfile necessary for transport is.
-
-Once the client has the initial list of references that the server
-has, as well as the list of capabilities, it will begin telling the
+After reference and capabilities discovery, the client can decide to
+terminate the connection by sending a flush-pkt, telling the server it can
+now gracefully terminate, and disconnect, when it does not need any pack
+data. This can happen with the ls-remote command, and also can happen when
+the client already is up-to-date.
+
+Otherwise, it enters the negotiation phase, where the client and
+server determine what the minimal packfile necessary for transport is,
+by telling the
server what objects it wants and what objects it has, so the server
can make a packfile that only contains the objects that the client needs.
The client will also send a list of the capabilities it wants to be in
@@ -219,8 +220,8 @@ If client is requesting a shallow clone, it will now send a 'deepen'
line with the depth it is requesting.
Once all the "want"s (and optional 'deepen') are transferred,
-clients MUST send a flush-pkt. If the client has all the references
-on the server, client flushes and disconnects.
+clients MUST send a flush-pkt, to tell the server side that it is
+done sending the list.
TODO: shallow/unshallow response and document the deepen command in the ABNF.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html