Re: [PATCH 3/4] Add 'promisor-remote' capability to protocol v2

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 10, 2024, 16:32 UTC
Message-ID: <CAP8UFD1DQ6jGcnc_t=GQPmnfx2s63NLxyvSJ6V+vVQBU9m_MCA@mail.gmail.com>
In-Reply-To: <CAP8UFD2OR=5QvyFT6EGQkekFBtjRWoMShQ=_eT=ypH=FswUaRA@mail.gmail.com>

On Tue, Aug 20, 2024 at 1:32 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 20 quoted lines

>
> On Wed, Jul 31, 2024 at 6:16 PM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > On Wed, Jul 31, 2024 at 03:40:13PM +0200, Christian Couder wrote:
> > > diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
> > > index 414bc625d5..4d8d3839c4 100644
> > > --- a/Documentation/gitprotocol-v2.txt
> > > +++ b/Documentation/gitprotocol-v2.txt
> > > @@ -781,6 +781,43 @@ retrieving the header from a bundle at the indicated URI, and thus
> > >  save themselves and the server(s) the request(s) needed to inspect the
> > >  headers of that bundle or bundles.
> > >
> > > +promisor-remote=<pr-infos>
> > > +~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > +
> > > +The server may advertise some promisor remotes it is using, if it's OK
> > > +for the server that a client uses them too. In this case <pr-infos>
> > > +should be of the form:
> > > +
> > > +     pr-infos = pr-info | pr-infos ";" pr-info

[...]

Show 6 quoted lines

> > I wonder if it would instead be useful to have <pr-infos> first write
> > out how many <pr-info>s it contains, and then write out each <pr-info>
> > separated by a NUL byte, so that none of the files in the <pr-info>
> > itself are restricted in what characters they can use.
>
> I am not sure how NUL bytes would interfere with the pkt-line.[c,h] code though.

As Junio said pkt-line.[ch] is about <length> and <bytes> and it is used to transfer the pack data stream that can have arbitrary bytes, so there is no problem with NUL bytes. Sorry for not checking.

However I still think that capabilities have been using a simple text format for now which works well, and that it's better to respect that format and not introduce complexity in it if it's not necessary.

For example t5555-http-smart-common.sh has:

cat >expect <<-EOF &&
    version 2
    agent=FAKE
    ls-refs=unborn
    fetch=shallow wait-for-done
    server-option
    object-format=$(test_oid algo)
    0000
    EOF

to check the capabilities sent by `git upload-pack --advertise-refs`.

t5701 also uses similar instructions to check protocol v2 server commands.

So I think it's nice for tests and debugging if we keep using a simple text format.

Also writing the number of <pr-info>s and then each <pr-info> separated by a NUL byte might not save a lot of bytes compared to urlencoded content if necessary, as I don't think many special characters will need to be urlencoded most of the time.

So in the version 2 of this patch series, I haven't changed this.

Thanks for the review.