Re: [PATCH] Update packfile transfer protocol documentation

From: Shawn O. Pearce <spearce@spearce.org>
Date: Nov 4, 2009, 00:56 UTC
Message-ID: <20091104005614.GD10505@spearce.org>
In-Reply-To: <7v4opbp1fa.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Scott Chacon <schacon@gmail.com> writes:
> 
> > diff --git a/Documentation/technical/protocol-capabilities.txt
> > +
> > +The client MUST send only maximum of one of "side-band" and "side-
> > +band-64k".  Server MUST favor side-band-64k if client requests both.
> 
> Again I think sending both is an error and should be diagnosed as such.
> This is not a three-way handshake where I say "I can handle both", you say
> "I can too", and then I finally pick "then we'll use this one".  There is
> no way for the requesting side to tell which one was chosen, and the
> requester who sent both assumed that the other end chose "side-band" and
> allocated only a 1000-byte buffer like older implementation did, the limit
> of buffer will be busted.

I think Scott borrowed the above from me.  The last sentance with that
server MUST is my error.

> Fix the last line "Server MUST favor" to "Server MUST diagnose it as an
> error".  Also drop "A client should ask for only one of them," near the
> beginning of this section, as it is redundant.  I think it is fine to keep
> "A modern client always favors".

Ack, I agree.

Show 12 quoted lines

> > +include-tag
> > +-----------
> > +
> > +The 'include-tag' capability is about sending tags if we are sending
> > +objects they point to.  If we pack an object to the client, and a tag
> > +points exactly at that object, we pack the tag too.  In general this
> > +allows a client to get all new tags when it fetches a branch, in a
> > +single network connection.
> > +
> > +Clients MAY always send include-tag, hardcoding it into a request.
> 
> "... when the server advertises this capability", no?

This is also my fault. Its rooted in the way the C implementation of upload-pack parses the want line, it doesn't care if there are unrecognized capabilities requested by the client. This is a bug in the C implementation.

I agree with you, and disagree with the original text I wrote.

Show 7 quoted lines

> > +Clients SHOULD NOT send include-tag if remote.name.tagopt was set to
> > +--no-tags, as the client doesn't want tag data.
> > +
> > +Servers MUST accept include-tag without error or warning, even if the
> > +server does not understand or support the option.
> 
> Why is this special case here?

Ack, I agree with you, this should be removd.

Show 9 quoted lines

> > +Servers SHOULD pack the tags if their referrant is packed and the
> > +client has requested include-tag.
> 
> Sorry, I do not understand the motivation to make make this so weak?  If
> the server claims to support this capability, and when a referrant is
> going to the client, the server MUST do so---if it cannot guarantee, why
> claim to support that capability?
> 
> Or am I missing something?

IIRC at one time the C implementation didn't fully ensure the tag is
packed when the referrant is packed.  This SHOULD exists because it
may have been possible for a tag to be omitted.  But I don't think
I've seen this happen under any condition, and it probably is now
a bug if it occurs, which means we likely can convert this to a MUST.

Show 8 quoted lines

> > diff --git a/Documentation/technical/protocol-common.txt
> > +...
> > +pkt-line Format
> > +---------------
> > +
> > +Implementations SHOULD NOT send an empty pkt-line ("0004").
> 
> Not an objection, but where is this coming from?

Me.  I think sending "0004" is stupid.

"0004" must immediately be followed by another pkt-len because there
is no data payload behind it.  "0004" is the same as having called
write(fd, buf, 0), which is equally pointless.  Such a kernel call
will be a no-op, my point here is that "0004" as a packet is also
stupid and shouldn't be sent.

Show 7 quoted lines

> > +A pkt-line with a length field of 0 ("0000"), called a flush-pkt,
> > +is a special case and MUST be handled differently than an empty
> > +pkt-line ("0004").
> 
> ...especially that this sentence makes it sound as if it is perfectly
> normal to send "0004" for "an empty line" (and I've always thought that is
> Ok), I am quite puzzled by that "SHOULD NOT".

I don't think we ever send an empty packet. If we have no data to send, why the hell did we create the packet header?

-- 
Shawn.