Re: [PATCH] protocol upload-pack-v2

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2015, 20:36 UTC
Message-ID: <xmqqr3szql9r.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAGZ79kZBYFSwR6E86BF6Dt7xdh0zs07tkGnQAKfEJpSduTK-aw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 15 quoted lines

>> I do not see a good reason why we want "I am sending N caps"
>> upfront, instead of "this, that, and here is the end of the group".
>
> I thought about having an end marker, so something like
> capabilities start
> thin-pack
> lang
> ofs-delta
> capabilities done
>
> (Each line a pkt-line)
>
> Though all decisions I thought through I tried to put more weight on
> future expandability. Now that I think about it again, it makes no
> difference, whether to use a counter or an end marker.

One reason why I would suggest avoiding "count upfront" is to make sure we do not repeat the mistake of "Content-Length" which had to later be corrected by introducing chunked transfer by HTTP folks. Closer to home, our "type and then size upfront and then contents and hash the whole thing" loose object format makes it quite hard to produce without having the whole thing in-core, or otherwise having a separate way to know the size upfront.

For the capability list, the number of the capabilities you support may be limited, bounded and may even be known at the compile time, so count-upfront may not be a burden, but in other parts of the protocol where you need to feed the result of computation to the other end, you would need "the group ends here" marker. It would be easier for everybody if we can make all types of messages use the same syntax, regardless of type.

Show 14 quoted lines

>>> +  cap              =  PKT-LINE("capabilities" SP size LF list)
>>
>> Isn't a cap packet terminated by LF without any "list" following it?
>> The notation PKT-LINE(<blah>) is "wrap <blah> in a single packet",
>> and I do not think you meant to send the capability line and a series
>> of cap:foo entries in a single packet.
>
> Yeah I meant to use one packet per line
> So after considering your input, you'd want to have
> PKT-LINE("capabilities start")
> PKT-LINE("no-prefix-for-capabilities")
> PKT-LINE("ofs-delta")
> PKT-LINE("agent-as-capability-2.6")
> PKT-LINE("capabilities end")

OK, so that "list" at the end is just a typo; there shouldn't be "list at the end inside PKT-LINE().

Show 13 quoted lines

>>> +  size             =  *DIGIT
>>> +  capability-list  =  *(capability) [agent LF]
>>> +  capability       =  "cap:" keyvaluepair LF
>>> +  agent            =  keyvaluepair LF
>>
>> I do not see a reason to make 'agent' as part of capability.  That
>> was an implementation detail of v1 but v2 does not have an
>> obligation to consider agent announcement as capability.
>
> So I think we don't want to drop the agent announcement as it may
> reveal useful information ("How many outdated clients connect to our
> $HOSTING_SITE?", "I need to debug failures which happen only rarely,
> Oh! it can be narrowed down to clients with agent xyz.")

Don't be overly defensive and try not to misunderstand and see a criticism where there is none. All I am saying is that agent announcement is not annoucing capability. You may announce many things, and server or client version may be something you would want to announce.

I have a feeling that it is a bit too premature to specify the details at such a low level as "capaiblities are announced by prefixing four-byte 'c', 'a', 'p', ':' in front" and "a multi-record group has its element count at the beginning (or an end marker at the end, for that matter)", and it may be a better idea to outline all the necessary elements at a bit higher level first---that would avoid needs for useless exchanges like what we are having right now.

It's that when you write things in EBNF, you are writing something that you would eventually want to cast in stone, and the non-terminal names in EBNF matter (they convey the semantics, what these named things are), and I was reacting to that because I wanted to make sure we avoid mislabaling things as something that are not.

The "shallow" vs "reference advertisement" is the same. I think the former is _not_ part of reference announcement but is an optional phase of the protocol, but the level of the detail that would make the difference matter would appear only when you start writing it in EBNF and call both "reference advertisement". If you keep the discussion at the level like "fetch first asks capabilities it wants upload-pack-2 to enable, optionally gives the current shallow boundaries when the capaibilty says the other side supports it, and then starts showing what it has" while we are trying to achieve concensus on what kind of protocol elements we would need, and what information each element would carry, the discussion will help us reach a shared understanding on what to write down in EBNF form exactly faster, I would imagine.