git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Josh Steadmon <steadmon@google.com>
Cc: git <git@vger.kernel.org>, Jonathan Nieder <jrnieder@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] protocol: limit max protocol version per service
Date: Wed, 3 Oct 2018 15:47:05 -0700
Message-ID: <CAGZ79kZP7y3v8hrhKKP_FT7+uyUSWO5s8w13-c67QWdzme1-Qw@mail.gmail.com> (raw)
In-Reply-To: <20181003213349.GA32105@google.com>

On Wed, Oct 3, 2018 at 2:34 PM Josh Steadmon <steadmon@google.com> wrote:

>
> I believe that git-upload-archive can still speak version 1 without any
> trouble, but it at least doesn't break anything in the test suite to
> limit this to v0 either.

ok, let me just play around with archive to follow along:

Running the excerpt that I found in one of the tests in t5000

    GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar

(whoooa ... that spews a lot of text into my terminal, which makes
sense as transported tar files unlike packets starting with PACK are
not cut short; so onwards:)

    $ git init test && cd test
    $ GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar
    ...
    git< \2fatal: no such ref: HEAD
    ...
    fatal: sent error to the client: git upload-archive: archiver died
with error
    remote: git upload-archive: archiver died with error

This sounds similar to a bug that I have on my todo list for
clone recursing into submodules. Maybe we need to talk
about HEAD-less repositories and how to solve handling them
in general. Usually it doesn't happen except for corner cases like
now, so:

    $ git commit --allow-empty -m "commit"
    [master (root-commit) 24d7967] commit
    $ GIT_TRACE_PACKET=1 git archive --remote=. HEAD >b5.tar
15:28:00.762615 pkt-line.c:80           packet:          git> argument HEAD
15:28:00.762704 pkt-line.c:80           packet:          git> 0000
15:28:00.766336 pkt-line.c:80           packet:          git> ACK
15:28:00.766428 pkt-line.c:80           packet:          git> 0000
15:28:00.766483 pkt-line.c:80           packet:          git< ACK
15:28:00.766508 pkt-line.c:80           packet:          git< 0000
15:28:00.767694 pkt-line.c:80           packet:          git< \2
15:28:00.767524 pkt-line.c:80           packet:          git< argument HEAD
15:28:00.767583 pkt-line.c:80           packet:          git< 0000
remote: 15:28:00.767524 pkt-line.c:80           packet:          git<
argument HEAD
remote: 15:28:00.767583 pkt-line.c:80           packet:          git< 0000
15:28:00.768758 pkt-line.c:80           packet:          git> 0000
15:28:00.770475 pkt-line.c:80           packet:          git<
\1pax_global_header
    ... \0\0\0\0\0\0\0\0\0\0\0\0\ ...
 # not too bad but the tar file contains a lot of zeros
    $

Ah I forgot the crucial part, so

    $ GIT_TRACE_PACKET=1 git -c protocol.version=1 archive --remote=.
HEAD >b5.tar
15:33:10.030508 pkt-line.c:80           packet:          git> argument HEAD
...

This pretty much looks like v0 as v1 would send a "version 1", c.f.

    $ GIT_TRACE_PACKET=1 git -c protocol.version=1 fetch .
15:34:26.111013 pkt-line.c:80           packet:  upload-pack> version 1
15:34:26.111140 pkt-line.c:80           packet:        fetch< version 1
....


>
> Is there a method or design for advertising multiple acceptable versions
> from the client?

I think the client can send multiple versions, looking through protocol.c
(and not the documentation as I should for this:)

  /*
   * Determine which protocol version the client has requested.  Since
   * multiple 'version' keys can be sent by the client, indicating that
   * the client is okay to speak any of them, select the greatest version
   * that the client has requested.  This is due to the assumption that
   * the most recent protocol version will be the most state-of-the-art.
   */
    ...
    const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
    string_list_split(&list, git_protocol, ':', -1);
    ...
    for_each_string_list_item(item, &list) {
        ...
        if (skip_prefix(item->string, "version=", &value))
            ...

in determine_protocol_version_server which already had the client
speak to it, so I think at least the server can deal with multiple versions.

But given that we transport the version in env variables, we'd
need to be extra careful if we just did not see the version
in the --remote=. above?

> From my understanding, we can only add a single
> version=X field in the advertisement, but IIUC we can extend this fairly
> easily? Perhaps we can have "version=X" to mean the preferred version,
> and then a repeatable "acceptable_version=Y" field or similar?

Just re-use "version X", separated by colons as above.

> > From a maintenance perspective, do we want to keep
> > this part of the code central, as it ties protocol (as proxied
> > by service name) to the max version number?
> > I would think that we'd rather have the decision local to the
> > code, i.e. builtin/fetch would need to tell protocol.c that it
> > can do (0,1,2) and builtin/push can do (0,1), and then the
> > networking layers of code would figure out by the input
> > from the caller and the input from the user (configured
> > protocol.version) what is the best to go forward from
> > then on.
>
> I like having it centralized, because enforcing this in git_connect()
> and discover_refs() catches all the outgoing version advertisements, but
> there's lots of code paths that lead to those two functions that would
> all have to have the acceptable version numbers plumbed through.

Makes sense.

> I suppose we could also have a registry of services to version numbers,
> but I tend to dislike non-local sources of data. But if the list likes
> that approach better, I'll be happy to implement it.

> > But I guess having the central place here is not to
> > bad either. How will it cope with the desire of protocol v2
> > to have only one end point (c.f. serve.{c,h} via builtin/serve
> > as "git serve") ?
>
> I'm not sure about this. In my series to add a v2 archive command, I
> added support for a new endpoint for proto v2 and I don't recall seeing
> any complaints, but that is still open for review.

Ah I guess new end points would imply that you can speak at least
a given version N.

> I suppose if we are strict about serving from a single endpoint, the
> version registry makes more sense, and individual operations can declare
> acceptable version numbers before calling any network code?

Ah yeah, that makes sense!

Thanks for your explanations and prodding,
Stefan

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02 21:59 [PATCH 0/1] Limit client version advertisements Josh Steadmon
2018-10-02 21:59 ` [PATCH 1/1] protocol: limit max protocol version per service Josh Steadmon
2018-10-02 22:28   ` Stefan Beller
2018-10-03 21:33     ` Josh Steadmon
2018-10-03 22:47       ` Stefan Beller [this message]
2018-10-05  0:18         ` Josh Steadmon
2018-10-05 19:25           ` Stefan Beller
2018-10-10 23:53             ` Josh Steadmon
2018-10-12 23:30               ` Jonathan Nieder
2018-10-12 23:32               ` Jonathan Nieder
2018-10-12 23:45                 ` Josh Steadmon
2018-10-12 23:53                   ` Jonathan Nieder
2018-10-13  7:58                     ` Junio C Hamano
2018-10-12  1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
2018-10-12  1:02   ` [PATCH v2 1/1] protocol: advertise multiple supported versions steadmon
2018-10-12 22:30     ` Stefan Beller
2018-10-22 22:55       ` Josh Steadmon
2018-10-23  0:37         ` Stefan Beller
2018-11-12 21:49   ` [PATCH v3 0/1] Advertise multiple supported proto versions steadmon
2018-11-12 21:49     ` [PATCH v3 1/1] protocol: advertise multiple supported versions steadmon
2018-11-12 22:33       ` Stefan Beller
2018-11-13  3:24         ` Re* " Junio C Hamano
2018-11-13 19:21           ` Stefan Beller
2018-11-14  2:31             ` Junio C Hamano
2018-11-13  4:01       ` Junio C Hamano
2018-11-13 22:53         ` Josh Steadmon
2018-11-14  2:38           ` Junio C Hamano
2018-11-14 19:57             ` Josh Steadmon
2018-11-16  2:45               ` Junio C Hamano
2018-11-16 19:59                 ` Josh Steadmon
2018-11-13 13:35       ` Junio C Hamano
2018-11-13 18:28       ` SZEDER Gábor
2018-11-13 23:03         ` Josh Steadmon
2018-11-14  0:47           ` SZEDER Gábor
2018-11-14  2:44         ` Junio C Hamano
2018-11-14 11:01           ` SZEDER Gábor
2018-11-14  2:30     ` [PATCH v4 0/1] Advertise multiple supported proto versions Josh Steadmon
2018-11-14  2:30       ` [PATCH v4 1/1] protocol: advertise multiple supported versions Josh Steadmon
2018-11-14 10:22       ` [PATCH v4 0/1] Advertise multiple supported proto versions Junio C Hamano
2018-11-14 19:51         ` Josh Steadmon
2018-11-16 10:46           ` Junio C Hamano
2018-11-16 22:34       ` [PATCH v5 " Josh Steadmon
2018-11-16 22:34         ` [PATCH v5 1/1] protocol: advertise multiple supported versions Josh Steadmon

Reply instructions:

You may reply publically 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=CAGZ79kZP7y3v8hrhKKP_FT7+uyUSWO5s8w13-c67QWdzme1-Qw@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox