mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Brandon Williams <>
Subject: Re: [RFC] protocol version 2
Date: Tue, 24 Oct 2017 15:48:10 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Brandon Williams's message of "Fri, 20 Oct 2017 10:18:39 -0700")

Brandon Williams <> 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

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


>  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

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


  reply	other threads:[~2017-10-24  6:48 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-20 17:18 [RFC] protocol version 2 Brandon Williams
2017-10-24  6:48 ` Junio C Hamano [this message]
2017-10-24 18:35   ` Brandon Williams
2017-10-25  1:22     ` Junio C Hamano
2017-10-26  0:59     ` Junio C Hamano
2017-10-25 13:09 ` Derrick Stolee
2017-10-25 18:10   ` Brandon Williams
2017-10-28 22:57 ` Philip Oakley
2017-10-31 18:42   ` Brandon Williams
2017-11-10 20:13 ` Jonathan Tan
2017-12-04 23:58 ` [WIP 00/15] " Brandon Williams
2017-12-04 23:58   ` [WIP 01/15] pkt-line: introduce packet_read_with_status Brandon Williams
2017-12-07 20:53     ` Stefan Beller
2017-12-08 18:03       ` Brandon Williams
2017-12-04 23:58   ` [WIP 02/15] pkt-line: introduce struct packet_reader Brandon Williams
2017-12-07 22:01     ` Stefan Beller
2017-12-08 18:11       ` Brandon Williams
2017-12-04 23:58   ` [WIP 03/15] pkt-line: add delim packet support Brandon Williams
2017-12-07 22:30     ` Stefan Beller
2017-12-08 20:08       ` Brandon Williams
2017-12-04 23:58   ` [WIP 04/15] upload-pack: convert to a builtin Brandon Williams
2017-12-06 21:59     ` Junio C Hamano
2017-12-07 16:14       ` Johannes Schindelin
2017-12-08 20:26         ` Junio C Hamano
2017-12-08 20:12       ` Brandon Williams
2017-12-04 23:58   ` [WIP 05/15] upload-pack: factor out processing lines Brandon Williams
2017-12-04 23:58   ` [WIP 06/15] transport: use get_refs_via_connect to get refs Brandon Williams
2017-12-06 22:10     ` Junio C Hamano
2017-12-07 18:40       ` Brandon Williams
2017-12-04 23:58   ` [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader Brandon Williams
2017-12-06 22:39     ` Junio C Hamano
2017-12-08 20:19       ` Brandon Williams
2017-12-04 23:58   ` [WIP 08/15] connect: discover protocol version outside of get_remote_heads Brandon Williams
2017-12-07 18:50     ` Junio C Hamano
2017-12-07 19:04       ` Brandon Williams
2017-12-07 19:30         ` Junio C Hamano
2017-12-08 20:11           ` Brandon Williams
2017-12-04 23:58   ` [WIP 09/15] transport: store protocol version Brandon Williams
2017-12-04 23:58   ` [WIP 10/15] protocol: introduce enum protocol_version value protocol_v2 Brandon Williams
2017-12-04 23:58   ` [WIP 11/15] serve: introduce git-serve Brandon Williams
2017-12-07 23:42     ` Junio C Hamano
2017-12-08 20:25       ` Brandon Williams
2017-12-04 23:58   ` [WIP 12/15] ls-refs: introduce ls-refs server command Brandon Williams
2017-12-13 16:30     ` Philip Oakley
2017-12-04 23:58   ` [WIP 13/15] connect: request remote refs using v2 Brandon Williams
2017-12-04 23:58   ` [WIP 14/15] upload_pack: introduce fetch server command Brandon Williams
2017-12-04 23:58   ` [WIP 15/15] fetch-pack: perform a fetch using v2 Brandon Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).