Re: [PATCH v3 12/35] serve: introduce git-serve

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 21, 2018, 22:45 UTC
Message-ID: <20180221144515.d180a26627a33168b19e0a4f@google.com>
In-Reply-To: <20180207011312.189834-13-bmwill@google.com>

On Tue, 6 Feb 2018 17:12:49 -0800 Brandon Williams <bmwill@google.com> wrote:

Show 15 quoted lines

>  .gitignore                              |   1 +
>  Documentation/technical/protocol-v2.txt | 114 +++++++++++++++
>  Makefile                                |   2 +
>  builtin.h                               |   1 +
>  builtin/serve.c                         |  30 ++++
>  git.c                                   |   1 +
>  serve.c                                 | 250 ++++++++++++++++++++++++++++++++
>  serve.h                                 |  15 ++
>  t/t5701-git-serve.sh                    |  60 ++++++++
>  9 files changed, 474 insertions(+)
>  create mode 100644 Documentation/technical/protocol-v2.txt
>  create mode 100644 builtin/serve.c
>  create mode 100644 serve.c
>  create mode 100644 serve.h
>  create mode 100755 t/t5701-git-serve.sh

As someone who is implementing the server side of protocol V2 in JGit, I now have a bit more insight into this :-)

First of all, I used to not have a strong opinion on the existence of a new endpoint, but now I think that it's better to *not* have git-serve. As it is, as far as I can tell, upload-pack also needs to support (and does support, as of the end of this patch set) protocol v2 anyway, so it might be better to merely upgrade upload-pack.

Show 7 quoted lines

> +A client then responds to select the command it wants with any particular
> +capabilities or arguments.  There is then an optional section where the
> +client can provide any command specific parameters or queries.
> +
> +    command-request = command
> +		      capability-list
> +		      (command-args)

If you are stating that this is optional, write "*1command-args". (RFC 5234 also supports square brackets, but "*1" is already used in pack-protocol.txt and http-protocol.txt.)

Show 5 quoted lines

> +		      flush-pkt
> +    command = PKT-LINE("command=" key LF)
> +    command-args = delim-pkt
> +		   *arg
> +    arg = 1*CHAR

arg should be wrapped in PKT-LINE, I think, and terminated by an LF.