Re: [PATCH] protocol upload-pack-v2

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

>> 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.

>>> +  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().

>>> +  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.