Re: [RFC] protocol version 2

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

Brandon Williams <bmwill@google.com> writes:

>   * Capabilities were implemented as a hack and are hidden behind a NUL
>     byte after the first ref sent from the server during the ref
>     advertisement:
> ...
>
>   * Various other technical debt (e.g. abusing capabilities to
>     communicate agent and symref data, service name set using a query
>     parameter).

This sounds like a duplicate of the above.

>  Special Packets
> -----------------
>
> In protocol v2 these special packets will have the following semantics:
>
>   * '0000' Flush Packet (flush-pkt) - indicates the end of a message
>   * '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list

After reading the remainder of the document twice, I do not think
the reason why we want to introduce a new type delim-pkt is
explained and justified well enough.  If a command request takes a
command packet, zero or more capability packets, a mandatory
delimiter packet, zero or more parameter packets and a mandatory
flush packet, then you can use the same "flush" as delimiter in the
middle.  The delimiter will of course become useful if you can omit
it when not necessary (e.g. after seeing capabilities, you may see a
flush and you will know there is no parameters and save the need to
send one "delim").

I actually have a reasonable guess why you want to have a separate
delimiter (which has nothing to do with "optional delim can be
omitted"), but I want to see it explained in this document clearly
by its designer(s).

>     command-request = command
>                       capability-list
>                       delim-pkt
>                       (command specific parameters)
>                       flush-pkt
>     command = PKT-LINE("command=" key LF)
>
> The server will then acknowledge the command and requested capabilities
> by echoing them back to the client and then launch into the command.
>
>     acknowledge-request = command
>                           capability-list
>                           delim-pkt
>                           execute-command
>     execute-command = <defined by each command>

It is not quite clear what the value of "echoing them back" is,
especially if that is done by always echoing verbatim.  A reader may
naturally expect, when capabilities are exchanged between two
parties, these are negotiated so that the only ones that are
commonly supported by both ends would be used, or something like
that.

> A particular command can last for as many rounds as are required to
> complete the service (multiple for negotiation during fetch and push or
> no additional trips in the case of ls-refs).

OK.

>  Commands in v2
> ~~~~~~~~~~~~~~~~
>
> Services are the core actions that a client wants to perform (fetch,
> push, etc).  Each service has its own set of capabilities and its own
> language of commands (think 'want' lines in fetch).  Optionally a
> service can take in initial parameters or data when a client sends it
> service request.

So a service (like "fetch") employ a set of "command"s (like "want",
"have, etc)?  In the earlier part of the document, we did not see
any mention of "service" and instead saw only "command" mentioned.
Is the state machine on both ends and the transition between states
implicit?  E.g. when one side throws "want" command and the other
side acknowledges, they understand implicitly that they are now in a
"fetch" service session, even though there is nothing that said over
the wire that they are doing so?  One reason I am wondering about
this is what we should do if a command verb may be applicable in
multiple services.

After reading the earlier protocol exchange explanation, I was sort
of expecting that "fetch" would be the command and "want", "have",
and "ack" lines would be exchanged as "command specific parameters",
so a sudden introduction of "services" here was a bit of impedance
mismatch to me.

>  Ls-refs
> ---------
>
> Ls-refs can be looked at as the equivalent of the current ls-remote as
> it is a way to query a remote for the references that it has.  Unlike
> the current ls-remote, the filtering of the output is done on the server
> side by passing a number of parameters to the server-side command
> instead of the filtering occurring on the client.
>
> Ls-ref takes in the following parameters:
>
>   --head, --tags: Limit to only refs/heads or refs/tags

I see no need for the above two as long as "refs/heads/*", etc. are
understood.

>   --refs: Do not show peeled tags or pseudorefs like HEAD

So showing peeled tags is the default?  Then I can sort-of see why
"I am not interested in peeled result".  

I do not see why "do not show HEAD, MERGE_HEAD, etc." is needed,
though.  It should be sufficient to just ask for refs/* if you are
interested only in other things, no?

>   --symref: In addition to the object pointed by it, show the underlying
>             ref pointed by it when showing a symbolic ref

Sort of OK--it probably is easier to always send this, as symbolic
refs are minority anyway, though.

>   <refspec>: When specified, only references matching the given patterns
>              are displayed.

I do not think you meant <refspec> here.

The side that is listing what it has has no reason to know what the
recipient plans to do with the result, so you must be only sending
the LHS of a refspec.  If your explanation says "given patterns",
then replace <refspec> with <pattern>.  Do not abuse a term that has
specific and established meaning for something else.

> The output of ls-refs is as follows:
>
>     output = (no-refs / list-of-refs)
> 	     *symref
>              *shallow
>              flush-pkt
>
>     no-refs = PKT-LINE(zero-id SP no-refs LF)

Can't your list-of-refs have 0 element?  I do not see why you need
no-refs here.  It's not like you need a dummy line, to the end of
which you need to append NUL plus hidden capabilities ;-)

>     list-of-refs = *ref
>     ref = PKT-LINE((tip / peeled) LF)
>     tip = obj-id SP refname
>     peeled = obj-id SP refname "^{}"
>
>     symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
>     shallow = PKT-LINE("shallow" SP obj-id LF)

Thanks.