git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 0/1] Limit client version advertisements
@ 2018-10-02 21:59 Josh Steadmon
  2018-10-02 21:59 ` [PATCH 1/1] protocol: limit max protocol version per service Josh Steadmon
  2018-10-12  1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
  0 siblings, 2 replies; 17+ messages in thread
From: Josh Steadmon @ 2018-10-02 21:59 UTC (permalink / raw)
  To: git; +Cc: sbeller, jrnieder, gitster, steadmon

As discussed in [1], clients will incorrectly advertise support for
protocol version 2 even when the service in question does not have a v2
implementation. This patch sets maximum protocol versions for
git-receive-pack, git-upload-archive, and git-upload-pack.

[1]: https://public-inbox.org/git/20180927012455.234876-1-steadmon@google.com/

Josh Steadmon (1):
  protocol: limit max protocol version per service

 connect.c     | 11 ++++-------
 protocol.c    | 13 +++++++++++++
 protocol.h    |  7 +++++++
 remote-curl.c | 11 ++++-------
 4 files changed, 28 insertions(+), 14 deletions(-)

-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-02 21:59 [PATCH 0/1] Limit client version advertisements Josh Steadmon
@ 2018-10-02 21:59 ` Josh Steadmon
  2018-10-02 22:28   ` Stefan Beller
  2018-10-12  1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
  1 sibling, 1 reply; 17+ messages in thread
From: Josh Steadmon @ 2018-10-02 21:59 UTC (permalink / raw)
  To: git; +Cc: sbeller, jrnieder, gitster, steadmon

For services other than git-receive-pack, clients currently advertise
that they support the version set in the protocol.version config,
regardless of whether or not there is actually an implementation of that
service for the given protocol version. This causes backwards-
compatibility problems when a new implementation for the given
protocol version is added.

This patch sets maximum allowed protocol versions for git-receive-pack,
git-upload-archive, and git-upload-pack.

Previously, git-receive-pack would downgrade from v2 to v0, but would
allow v1 if set in protocol.version. Now, it will downgrade from v2 to
v1.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 connect.c     | 11 ++++-------
 protocol.c    | 13 +++++++++++++
 protocol.h    |  7 +++++++
 remote-curl.c | 11 ++++-------
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/connect.c b/connect.c
index 94547e5056..4a8cd78239 100644
--- a/connect.c
+++ b/connect.c
@@ -1228,14 +1228,11 @@ struct child_process *git_connect(int fd[2], const char *url,
 	struct child_process *conn;
 	enum protocol protocol;
 	enum protocol_version version = get_protocol_version_config();
+	enum protocol_version max_version;
 
-	/*
-	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
-	 * to perform a push, then fallback to v0 since the client doesn't know
-	 * how to push yet using v2.
-	 */
-	if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
-		version = protocol_v0;
+	max_version = determine_maximum_protocol_version(prog, version);
+	if (version > max_version)
+		version = max_version;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
diff --git a/protocol.c b/protocol.c
index 5e636785d1..1c553d8b99 100644
--- a/protocol.c
+++ b/protocol.c
@@ -79,3 +79,16 @@ enum protocol_version determine_protocol_version_client(const char *server_respo
 
 	return version;
 }
+
+enum protocol_version determine_maximum_protocol_version(
+		const char *service, enum protocol_version default_version)
+{
+	if (!strcmp(service, "git-receive-pack"))
+		return protocol_v1;
+	else if (!strcmp(service, "git-upload-archive"))
+		return protocol_v1;
+	else if (!strcmp(service, "git-upload-pack"))
+		return protocol_v2;
+
+	return default_version;
+}
diff --git a/protocol.h b/protocol.h
index 2ad35e433c..eabc0c5fab 100644
--- a/protocol.h
+++ b/protocol.h
@@ -31,4 +31,11 @@ extern enum protocol_version determine_protocol_version_server(void);
  */
 extern enum protocol_version determine_protocol_version_client(const char *server_response);
 
+/*
+ * Used by a client to determine the maximum protocol version to advertise for a
+ * particular service. If the service is unrecognized, return default_version.
+ */
+extern enum protocol_version determine_maximum_protocol_version(
+		const char *service, enum protocol_version default_version);
+
 #endif /* PROTOCOL_H */
diff --git a/remote-curl.c b/remote-curl.c
index fb28309e85..028adb76ae 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -344,6 +344,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	int http_ret, maybe_smart = 0;
 	struct http_get_options http_options;
 	enum protocol_version version = get_protocol_version_config();
+	enum protocol_version max_version;
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -360,13 +361,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
-	/*
-	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
-	 * to perform a push, then fallback to v0 since the client doesn't know
-	 * how to push yet using v2.
-	 */
-	if (version == protocol_v2 && !strcmp("git-receive-pack", service))
-		version = protocol_v0;
+	max_version = determine_maximum_protocol_version(service, version);
+	if (version > max_version)
+		version = max_version;
 
 	/* Add the extra Git-Protocol header */
 	if (get_protocol_http_header(version, &protocol_header))
-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  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
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Beller @ 2018-10-02 22:28 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git, Jonathan Nieder, Junio C Hamano

On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote:
>
> For services other than git-receive-pack, clients currently advertise
> that they support the version set in the protocol.version config,
> regardless of whether or not there is actually an implementation of that
> service for the given protocol version. This causes backwards-
> compatibility problems when a new implementation for the given
> protocol version is added.
>
> This patch sets maximum allowed protocol versions for git-receive-pack,
> git-upload-archive, and git-upload-pack.
>
> Previously, git-receive-pack would downgrade from v2 to v0, but would
> allow v1 if set in protocol.version. Now, it will downgrade from v2 to
> v1.

But does git-receive-pack understand v1?
As to my understanding we have not even defined v1
for push (receive-pack) and archive --remote (upload-archive).
v1 is only known to fetch (upload-pack).

> +enum protocol_version determine_maximum_protocol_version(
> +               const char *service, enum protocol_version default_version)
> +{
> +       if (!strcmp(service, "git-receive-pack"))
> +               return protocol_v1;
> +       else if (!strcmp(service, "git-upload-archive"))
> +               return protocol_v1;

so I would think these two would be _v0.
... goes and checks ...
aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1,
2017-10-16) seems to actually teach v1 to receive-pack as well,
but upload-archive was completely off radar, so I think returning
(v1, v0, v2 in the order as in the code) would make sense?

Asides from this, I thought there was a deliberate decision
that we'd want to avoid a strict order on the protocol versions,
but I could not find prior discussion on list to back up this claim. :/

For example we'd go with e.g. enums instead of integers
for version numbers, as then some internal setup could
also have things like protocol_v2018-10-02 or protocol_vWhatever;
some protocol version may be advantageous to the client, some to
the server, and we'd need to negotiate the best version that both
are happy with. (e.g. the server may like version 0, 2 and 3, and
the client may like 0,2,4 as 3 is bad security wise for the client,
so both would negotiate to 2 as their best case)

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.

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

Stefan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-02 22:28   ` Stefan Beller
@ 2018-10-03 21:33     ` Josh Steadmon
  2018-10-03 22:47       ` Stefan Beller
  0 siblings, 1 reply; 17+ messages in thread
From: Josh Steadmon @ 2018-10-03 21:33 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, Jonathan Nieder, Junio C Hamano

On 2018.10.02 15:28, Stefan Beller wrote:
> On Tue, Oct 2, 2018 at 3:00 PM Josh Steadmon <steadmon@google.com> wrote:
> >
> > For services other than git-receive-pack, clients currently advertise
> > that they support the version set in the protocol.version config,
> > regardless of whether or not there is actually an implementation of that
> > service for the given protocol version. This causes backwards-
> > compatibility problems when a new implementation for the given
> > protocol version is added.
> >
> > This patch sets maximum allowed protocol versions for git-receive-pack,
> > git-upload-archive, and git-upload-pack.
> >
> > Previously, git-receive-pack would downgrade from v2 to v0, but would
> > allow v1 if set in protocol.version. Now, it will downgrade from v2 to
> > v1.
> 
> But does git-receive-pack understand v1?
> As to my understanding we have not even defined v1
> for push (receive-pack) and archive --remote (upload-archive).
> v1 is only known to fetch (upload-pack).
> 
> > +enum protocol_version determine_maximum_protocol_version(
> > +               const char *service, enum protocol_version default_version)
> > +{
> > +       if (!strcmp(service, "git-receive-pack"))
> > +               return protocol_v1;
> > +       else if (!strcmp(service, "git-upload-archive"))
> > +               return protocol_v1;
> 
> so I would think these two would be _v0.
> ... goes and checks ...
> aa9bab29b8 (upload-pack, receive-pack: introduce protocol version 1,
> 2017-10-16) seems to actually teach v1 to receive-pack as well,
> but upload-archive was completely off radar, so I think returning
> (v1, v0, v2 in the order as in the code) would make sense?

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.


> Asides from this, I thought there was a deliberate decision
> that we'd want to avoid a strict order on the protocol versions,
> but I could not find prior discussion on list to back up this claim. :/
> 
> For example we'd go with e.g. enums instead of integers
> for version numbers, as then some internal setup could
> also have things like protocol_v2018-10-02 or protocol_vWhatever;
> some protocol version may be advantageous to the client, some to
> the server, and we'd need to negotiate the best version that both
> are happy with. (e.g. the server may like version 0, 2 and 3, and
> the client may like 0,2,4 as 3 is bad security wise for the client,
> so both would negotiate to 2 as their best case)

Is there a method or design for advertising multiple acceptable versions
from the client? 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?


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

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.

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?



Thanks for the review,
Josh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-03 21:33     ` Josh Steadmon
@ 2018-10-03 22:47       ` Stefan Beller
  2018-10-05  0:18         ` Josh Steadmon
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Beller @ 2018-10-03 22:47 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git, Jonathan Nieder, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-03 22:47       ` Stefan Beller
@ 2018-10-05  0:18         ` Josh Steadmon
  2018-10-05 19:25           ` Stefan Beller
  0 siblings, 1 reply; 17+ messages in thread
From: Josh Steadmon @ 2018-10-05  0:18 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, Jonathan Nieder, Junio C Hamano

On 2018.10.03 15:47, Stefan Beller wrote:
> On Wed, Oct 3, 2018 at 2:34 PM Josh Steadmon <steadmon@google.com> wrote:
> >
> > 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?

Sorry, I'm not sure I understand this. What about env variables requires
caution?


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

Thinking out loud here. Please let me know if I say something stupid :)

So we'll have (up to) three pieces of version information we'll care
about for version negotiation:

1. (maybe) a client-side protocol.version config entry
2. a list of acceptable proto versions for the currently running
   operation on the client
3. a list of acceptable proto versions for the server endpoint that
   handles the request

According to the doc on protocol.version: "If set, clients will attempt
to communicate with a server using the specified protocol version. If
unset, no attempt will be made by the client to communicate using a
particular protocol version, this results in protocol version 0 being
used."

So, if protocol.version is not set, or set to 0, the client should not
attempt any sort of version negotiation. Otherwise, the client prefers a
particular version, but we don't guarantee that they will actually use
that version after the (unspecified) version negotiation procedure.

If protocol.version is set to something other than 0, we construct a
list of acceptable versions for the given operation. If the
protocol.version entry is present in that list, we move it to the front
of the list to note that it is the preferred version. We send all of
these, in order, in the request.

When the server endpoint begins to handle a request, it first constructs
a list of acceptable versions. If the client specifies a list of
versions, we check them one-by-one to see if they are acceptable. If we
find a match, we use that version. If we don't find any matches or if
the client did not send a version list, we default to v0.

Seem reasonable?


Thanks,
Josh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-05  0:18         ` Josh Steadmon
@ 2018-10-05 19:25           ` Stefan Beller
  2018-10-10 23:53             ` Josh Steadmon
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Beller @ 2018-10-05 19:25 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git, Jonathan Nieder, Junio C Hamano

> > 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?
>
> Sorry, I'm not sure I understand this. What about env variables requires
> caution?

By locally transporting the version via env variables means the absence of
"version X" lines in the GIT_TRACE output is not a clear indication
of version 0, I would think. It is a strong indicator, but now we'd have to dig
and see if the env variables were set outside?


> >
> > > 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!
>
> Thinking out loud here. Please let me know if I say something stupid :)
>
> So we'll have (up to) three pieces of version information we'll care
> about for version negotiation:
>
> 1. (maybe) a client-side protocol.version config entry

(and in case we don't, we have it implicit ly hardcoded, as
we have to choose the default for users that don't care themselves about
this setting)

> 2. a list of acceptable proto versions for the currently running
>    operation on the client
> 3. a list of acceptable proto versions for the server endpoint that
>    handles the request

Yes that matches my understanding. The issue is between (1) and (2)
as (1) is in a generic config, whereas (2) is specific to the command,
such that it may differ. And as a user you may want to express things
like: "use the highest version", which is done by setting (1) to "version 2"
despite (2) not having support of all commands for v2.

> According to the doc on protocol.version: "If set, clients will attempt
> to communicate with a server using the specified protocol version. If
> unset, no attempt will be made by the client to communicate using a
> particular protocol version, this results in protocol version 0 being
> used."
>
> So, if protocol.version is not set, or set to 0, the client should not
> attempt any sort of version negotiation.

Yes, as version 0 is unaware of versions, i.e. there are old installations
out there where all the versioning code is not there, so in case of an
old client the new server *must* speak v0 to be able to communicate
(and vice versa).


> Otherwise, the client prefers a
> particular version, but we don't guarantee that they will actually use
> that version after the (unspecified) version negotiation procedure.
>
> If protocol.version is set to something other than 0, we construct a
> list of acceptable versions for the given operation. If the
> protocol.version entry is present in that list, we move it to the front
> of the list to note that it is the preferred version. We send all of
> these, in order, in the request.
>
> When the server endpoint begins to handle a request, it first constructs
> a list of acceptable versions. If the client specifies a list of
> versions, we check them one-by-one to see if they are acceptable. If we
> find a match, we use that version. If we don't find any matches or if
> the client did not send a version list, we default to v0.
>
> Seem reasonable?

This sounds super reasonable!

Thanks
Stefan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  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
  0 siblings, 2 replies; 17+ messages in thread
From: Josh Steadmon @ 2018-10-10 23:53 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, Jonathan Nieder, Junio C Hamano

On 2018.10.05 12:25, Stefan Beller wrote:
> > > > 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!
> >
> > Thinking out loud here. Please let me know if I say something stupid :)
> >
> > So we'll have (up to) three pieces of version information we'll care
> > about for version negotiation:
> >
> > 1. (maybe) a client-side protocol.version config entry
> 
> (and in case we don't, we have it implicit ly hardcoded, as
> we have to choose the default for users that don't care themselves about
> this setting)
> 
> > 2. a list of acceptable proto versions for the currently running
> >    operation on the client
> > 3. a list of acceptable proto versions for the server endpoint that
> >    handles the request
> 
> Yes that matches my understanding. The issue is between (1) and (2)
> as (1) is in a generic config, whereas (2) is specific to the command,
> such that it may differ. And as a user you may want to express things
> like: "use the highest version", which is done by setting (1) to "version 2"
> despite (2) not having support of all commands for v2.
> 
> > According to the doc on protocol.version: "If set, clients will attempt
> > to communicate with a server using the specified protocol version. If
> > unset, no attempt will be made by the client to communicate using a
> > particular protocol version, this results in protocol version 0 being
> > used."
> >
> > So, if protocol.version is not set, or set to 0, the client should not
> > attempt any sort of version negotiation.
> 
> Yes, as version 0 is unaware of versions, i.e. there are old installations
> out there where all the versioning code is not there, so in case of an
> old client the new server *must* speak v0 to be able to communicate
> (and vice versa).
> 
> 
> > Otherwise, the client prefers a
> > particular version, but we don't guarantee that they will actually use
> > that version after the (unspecified) version negotiation procedure.
> >
> > If protocol.version is set to something other than 0, we construct a
> > list of acceptable versions for the given operation. If the
> > protocol.version entry is present in that list, we move it to the front
> > of the list to note that it is the preferred version. We send all of
> > these, in order, in the request.
> >
> > When the server endpoint begins to handle a request, it first constructs
> > a list of acceptable versions. If the client specifies a list of
> > versions, we check them one-by-one to see if they are acceptable. If we
> > find a match, we use that version. If we don't find any matches or if
> > the client did not send a version list, we default to v0.
> >
> > Seem reasonable?
> 
> This sounds super reasonable!

So this runs into problems with remote-curl (and possibly other remote
helpers):

builtin/push.c can declare whatever allowed versions it wants, but those
are not carried over when remote-curl is started to actually talk to the
remote. What's worse, remote-curl starts its HTTP connection before it
knows what command it's actually acting as a helper for.

For now, I'm going to try adding an --allowed_versions flag for the
remote helpers, but if anyone has a better idea, let me know.


Thanks,
Josh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 0/1] Advertise multiple supported proto versions
  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-12  1:02 ` steadmon
  2018-10-12  1:02   ` [PATCH v2 1/1] protocol: advertise multiple supported versions steadmon
  1 sibling, 1 reply; 17+ messages in thread
From: steadmon @ 2018-10-12  1:02 UTC (permalink / raw)
  To: git; +Cc: sbeller, jrnieder, gitster, Josh Steadmon

From: Josh Steadmon <steadmon@google.com>

This is an alternate approach to the previous series. We add a registry
of supported wire protocol versions that individual commands can use to
declare supported versions before contacting a server. The client will
then advertise all supported versions, while the server will choose the
first recognized version from the advertised list.

Compared to the previous series, this approach is more convenient for
protocol_v2, which is intended to work on a single server endpoint.
However, it has the drawback that every command that acts as a client
must register its supported versions; it is not always obvious which (if
any) network operations a given command will perform.

Thank you to Stefan for his review of the previous series and for
helping me think through the requirements for this new approach.

Josh Steadmon (1):
  protocol: advertise multiple supported versions

 builtin/archive.c      |   3 ++
 builtin/clone.c        |   4 ++
 builtin/fetch-pack.c   |   4 ++
 builtin/fetch.c        |   5 ++
 builtin/ls-remote.c    |   5 ++
 builtin/pull.c         |   5 ++
 builtin/push.c         |   4 ++
 builtin/send-pack.c    |   3 ++
 connect.c              |  47 ++++++++---------
 protocol.c             | 115 ++++++++++++++++++++++++++++++++++++++---
 protocol.h             |  17 ++++++
 remote-curl.c          |  28 ++++++----
 t/t5570-git-daemon.sh  |   2 +-
 t/t5700-protocol-v1.sh |   8 +--
 t/t5702-protocol-v2.sh |  16 +++---
 transport-helper.c     |   6 +++
 16 files changed, 217 insertions(+), 55 deletions(-)

-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/1] protocol: advertise multiple supported versions
  2018-10-12  1:02 ` [PATCH v2 0/1] Advertise multiple supported proto versions steadmon
@ 2018-10-12  1:02   ` steadmon
  2018-10-12 22:30     ` Stefan Beller
  0 siblings, 1 reply; 17+ messages in thread
From: steadmon @ 2018-10-12  1:02 UTC (permalink / raw)
  To: git; +Cc: sbeller, jrnieder, gitster, Josh Steadmon

From: Josh Steadmon <steadmon@google.com>

Currently the client advertises that it supports the wire protocol
version set in the protocol.version config. However, not all services
support the same set of protocol versions. When connecting to
git-receive-pack, the client automatically downgrades to v0 if
config.protocol is set to v2, but this check is not performed for other
services.

This patch creates a protocol version registry. Individual commands
register all the protocol versions they support prior to communicating
with a server. Versions should be listed in preference order; the
version specified in protocol.version will automatically be moved to the
front of the registry.

The protocol version registry is passed to remote helpers via the
GIT_PROTOCOL environment variable.

Clients now advertise the full list of registered versions. Servers
select the first recognized version from this advertisement.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 builtin/archive.c      |   3 ++
 builtin/clone.c        |   4 ++
 builtin/fetch-pack.c   |   4 ++
 builtin/fetch.c        |   5 ++
 builtin/ls-remote.c    |   5 ++
 builtin/pull.c         |   5 ++
 builtin/push.c         |   4 ++
 builtin/send-pack.c    |   3 ++
 connect.c              |  47 ++++++++---------
 protocol.c             | 115 ++++++++++++++++++++++++++++++++++++++---
 protocol.h             |  17 ++++++
 remote-curl.c          |  28 ++++++----
 t/t5570-git-daemon.sh  |   2 +-
 t/t5700-protocol-v1.sh |   8 +--
 t/t5702-protocol-v2.sh |  16 +++---
 transport-helper.c     |   6 +++
 16 files changed, 217 insertions(+), 55 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..8adb9f381b 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -8,6 +8,7 @@
 #include "transport.h"
 #include "parse-options.h"
 #include "pkt-line.h"
+#include "protocol.h"
 #include "sideband.h"
 
 static void create_output_file(const char *output_file)
@@ -94,6 +95,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	register_allowed_protocol_version(protocol_v0);
+
 	argc = parse_options(argc, argv, prefix, local_opts, NULL,
 			     PARSE_OPT_KEEP_ALL);
 
diff --git a/builtin/clone.c b/builtin/clone.c
index fd2c3ef090..1651a950b6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -900,6 +900,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct refspec rs = REFSPEC_INIT_FETCH;
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
+	register_allowed_protocol_version(protocol_v2);
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	fetch_if_missing = 0;
 
 	packet_trace_identity("clone");
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 1a1bc63566..cba935e4d3 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -57,6 +57,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	fetch_if_missing = 0;
 
+	register_allowed_protocol_version(protocol_v2);
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	packet_trace_identity("fetch-pack");
 
 	memset(&args, 0, sizeof(args));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..2a20cf8bfd 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -21,6 +21,7 @@
 #include "argv-array.h"
 #include "utf8.h"
 #include "packfile.h"
+#include "protocol.h"
 #include "list-objects-filter-options.h"
 
 static const char * const builtin_fetch_usage[] = {
@@ -1476,6 +1477,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	int prune_tags_ok = 1;
 	struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
 
+	register_allowed_protocol_version(protocol_v2);
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	packet_trace_identity("fetch");
 
 	fetch_if_missing = 0;
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1a25df7ee1..ea685e8bb9 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "protocol.h"
 #include "transport.h"
 #include "ref-filter.h"
 #include "remote.h"
@@ -80,6 +81,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	memset(&ref_array, 0, sizeof(ref_array));
 
+	register_allowed_protocol_version(protocol_v2);
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
diff --git a/builtin/pull.c b/builtin/pull.c
index 681c127a07..cb64146834 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "protocol.h"
 #include "exec-cmd.h"
 #include "run-command.h"
 #include "sha1-array.h"
@@ -849,6 +850,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct object_id rebase_fork_point;
 	int autostash;
 
+	register_allowed_protocol_version(protocol_v2);
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
diff --git a/builtin/push.c b/builtin/push.c
index d09a42062c..10d8abe829 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -10,6 +10,7 @@
 #include "remote.h"
 #include "transport.h"
 #include "parse-options.h"
+#include "protocol.h"
 #include "submodule.h"
 #include "submodule-config.h"
 #include "send-pack.h"
@@ -587,6 +588,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8e3c7490f7..f48bd1306b 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -184,6 +184,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	register_allowed_protocol_version(protocol_v1);
+	register_allowed_protocol_version(protocol_v0);
+
 	git_config(send_pack_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
 	if (argc > 0) {
diff --git a/connect.c b/connect.c
index 94547e5056..a170ba1e90 100644
--- a/connect.c
+++ b/connect.c
@@ -1046,7 +1046,7 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
  */
 static struct child_process *git_connect_git(int fd[2], char *hostandport,
 					     const char *path, const char *prog,
-					     enum protocol_version version,
+					     const struct strbuf *version_advert,
 					     int flags)
 {
 	struct child_process *conn;
@@ -1085,10 +1085,9 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 		    target_host, 0);
 
 	/* If using a new version put that stuff here after a second null byte */
-	if (version > 0) {
+	if (strcmp(version_advert->buf, "version=0")) {
 		strbuf_addch(&request, '\0');
-		strbuf_addf(&request, "version=%d%c",
-			    version, '\0');
+		strbuf_addf(&request, "%s%c", version_advert->buf, '\0');
 	}
 
 	packet_write(fd[1], request.buf, request.len);
@@ -1104,14 +1103,14 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
  */
 static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 			     enum ssh_variant variant, const char *port,
-			     enum protocol_version version, int flags)
+			     const struct strbuf *version_advert, int flags)
 {
 	if (variant == VARIANT_SSH &&
-	    version > 0) {
+	    strcmp(version_advert->buf, "version=0")) {
 		argv_array_push(args, "-o");
 		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
-		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-				 version);
+		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
+				 version_advert->buf);
 	}
 
 	if (flags & CONNECT_IPV4) {
@@ -1164,7 +1163,7 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
-			  const char *port, enum protocol_version version,
+			  const char *port, const struct strbuf *version_advert,
 			  int flags)
 {
 	const char *ssh;
@@ -1198,15 +1197,16 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
 
 		argv_array_push(&detect.args, ssh);
 		argv_array_push(&detect.args, "-G");
-		push_ssh_options(&detect.args, &detect.env_array,
-				 VARIANT_SSH, port, version, flags);
+		push_ssh_options(&detect.args, &detect.env_array, VARIANT_SSH,
+				 port, version_advert, flags);
 		argv_array_push(&detect.args, ssh_host);
 
 		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
 	}
 
 	argv_array_push(&conn->args, ssh);
-	push_ssh_options(&conn->args, &conn->env_array, variant, port, version, flags);
+	push_ssh_options(&conn->args, &conn->env_array, variant, port,
+			 version_advert, flags);
 	argv_array_push(&conn->args, ssh_host);
 }
 
@@ -1226,16 +1226,10 @@ struct child_process *git_connect(int fd[2], const char *url,
 {
 	char *hostandport, *path;
 	struct child_process *conn;
+	struct strbuf version_advert = STRBUF_INIT;
 	enum protocol protocol;
-	enum protocol_version version = get_protocol_version_config();
 
-	/*
-	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
-	 * to perform a push, then fallback to v0 since the client doesn't know
-	 * how to push yet using v2.
-	 */
-	if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
-		version = protocol_v0;
+	get_client_protocol_version_advertisement(&version_advert);
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -1250,7 +1244,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
-		conn = git_connect_git(fd, hostandport, path, prog, version, flags);
+		conn = git_connect_git(fd, hostandport, path, prog,
+				       &version_advert, flags);
 	} else {
 		struct strbuf cmd = STRBUF_INIT;
 		const char *const *var;
@@ -1293,12 +1288,14 @@ struct child_process *git_connect(int fd[2], const char *url,
 				strbuf_release(&cmd);
 				return NULL;
 			}
-			fill_ssh_args(conn, ssh_host, port, version, flags);
+			fill_ssh_args(conn, ssh_host, port, &version_advert,
+				      flags);
 		} else {
 			transport_check_allowed("file");
-			if (version > 0) {
-				argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-						 version);
+			if (strcmp(version_advert.buf, "version=0")) {
+				argv_array_pushf(&conn->env_array,
+						 GIT_PROTOCOL_ENVIRONMENT "=%s",
+						 version_advert.buf);
 			}
 		}
 		argv_array_push(&conn->args, cmd.buf);
diff --git a/protocol.c b/protocol.c
index 5e636785d1..f788269c47 100644
--- a/protocol.c
+++ b/protocol.c
@@ -2,18 +2,43 @@
 #include "config.h"
 #include "protocol.h"
 
+static enum protocol_version *allowed_versions;
+static int nr_allowed_versions;
+static int alloc_allowed_versions;
+static int have_advertised_versions_already = 0;
+
+static const char protocol_v0_string[] = "0";
+static const char protocol_v1_string[] = "1";
+static const char protocol_v2_string[] = "2";
+
 static enum protocol_version parse_protocol_version(const char *value)
 {
-	if (!strcmp(value, "0"))
+	if (!strcmp(value, protocol_v0_string))
 		return protocol_v0;
-	else if (!strcmp(value, "1"))
+	else if (!strcmp(value, protocol_v1_string))
 		return protocol_v1;
-	else if (!strcmp(value, "2"))
+	else if (!strcmp(value, protocol_v2_string))
 		return protocol_v2;
 	else
 		return protocol_unknown_version;
 }
 
+/* Return the text representation of a wire protocol version. */
+static const char *format_protocol_version(enum protocol_version version)
+{
+	switch (version) {
+	case protocol_v0:
+		return protocol_v0_string;
+	case protocol_v1:
+		return protocol_v1_string;
+	case protocol_v2:
+		return protocol_v2_string;
+	case protocol_unknown_version:
+		die(_("Unrecognized protocol version"));
+	}
+	die(_("Unrecognized protocol_version"));
+}
+
 enum protocol_version get_protocol_version_config(void)
 {
 	const char *value;
@@ -30,6 +55,79 @@ enum protocol_version get_protocol_version_config(void)
 	return protocol_v0;
 }
 
+void register_allowed_protocol_version(enum protocol_version version)
+{
+	if (have_advertised_versions_already)
+		error(_("attempting to register an allowed protocol version after advertisement"));
+
+	ALLOC_GROW(allowed_versions, nr_allowed_versions + 1,
+		   alloc_allowed_versions);
+	allowed_versions[nr_allowed_versions++] = version;
+}
+
+void register_allowed_protocol_versions_from_env(void)
+{
+	const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
+	const char *version_str;
+	struct string_list version_list = STRING_LIST_INIT_DUP;
+	struct string_list_item *version;
+
+	if (!git_protocol)
+		return;
+
+	string_list_split(&version_list, git_protocol, ':', -1);
+	for_each_string_list_item(version, &version_list) {
+		if (skip_prefix(version->string, "version=", &version_str))
+			register_allowed_protocol_version(
+				parse_protocol_version(version_str));
+	}
+	string_list_clear(&version_list, 0);
+}
+
+void get_client_protocol_version_advertisement(struct strbuf *advert)
+{
+	int tmp_nr = nr_allowed_versions;
+	enum protocol_version *tmp_allowed_versions, config_version;
+	strbuf_reset(advert);
+
+	have_advertised_versions_already = 1;
+
+	config_version = get_protocol_version_config();
+	if (config_version == protocol_v0) {
+		strbuf_addstr(advert, "version=0");
+		return;
+	}
+
+	if (tmp_nr > 0) {
+		ALLOC_ARRAY(tmp_allowed_versions, tmp_nr);
+		copy_array(tmp_allowed_versions, allowed_versions, tmp_nr,
+			   sizeof(enum protocol_version));
+	} else {
+		ALLOC_ARRAY(tmp_allowed_versions, 1);
+		tmp_nr = 1;
+		tmp_allowed_versions[0] = config_version;
+	}
+
+	if (tmp_allowed_versions[0] != config_version)
+		for (int i = 1; i < nr_allowed_versions; i++)
+			if (tmp_allowed_versions[i] == config_version) {
+				enum protocol_version swap =
+					tmp_allowed_versions[0];
+				tmp_allowed_versions[0] =
+					tmp_allowed_versions[i];
+				tmp_allowed_versions[i] = swap;
+			}
+
+	strbuf_addf(advert, "version=%s",
+		    format_protocol_version(tmp_allowed_versions[0]));
+	for (int i = 1; i < tmp_nr; i++)
+		strbuf_addf(advert, ":version=%s",
+			    format_protocol_version(tmp_allowed_versions[i]));
+
+}
+
 enum protocol_version determine_protocol_version_server(void)
 {
 	const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
@@ -38,9 +136,10 @@ enum protocol_version determine_protocol_version_server(void)
 	/*
 	 * 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.
+	 * the client is okay to speak any of them, select the first
+	 * recognizable version that the client has requested.  This is due to
+	 * the assumption that the protocol versions will be listed in
+	 * preference order.
 	 */
 	if (git_protocol) {
 		struct string_list list = STRING_LIST_INIT_DUP;
@@ -53,8 +152,10 @@ enum protocol_version determine_protocol_version_server(void)
 
 			if (skip_prefix(item->string, "version=", &value)) {
 				v = parse_protocol_version(value);
-				if (v > version)
+				if (v != protocol_unknown_version) {
 					version = v;
+					break;
+				}
 			}
 		}
 
diff --git a/protocol.h b/protocol.h
index 2ad35e433c..b67b2259de 100644
--- a/protocol.h
+++ b/protocol.h
@@ -16,6 +16,23 @@ enum protocol_version {
  */
 extern enum protocol_version get_protocol_version_config(void);
 
+/*
+ * Register an allowable protocol version for a given operation. Registration
+ * must occur before attempting to advertise a version to a server process.
+ */
+extern void register_allowed_protocol_version(enum protocol_version version);
+
+/*
+ * Register allowable protocol versions from the GIT_PROTOCOL environment var.
+ */
+extern void register_allowed_protocol_versions_from_env(void);
+
+/*
+ * Fill a strbuf with a version advertisement string suitable for use in the
+ * GIT_PROTOCOL environment variable or similar version negotiation field.
+ */
+extern void get_client_protocol_version_advertisement(struct strbuf *advert);
+
 /*
  * Used by a server to determine which protocol version should be used based on
  * a client's request, communicated via the 'GIT_PROTOCOL' environment variable
diff --git a/remote-curl.c b/remote-curl.c
index fb28309e85..537f8c7aac 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,6 +330,20 @@ static int get_protocol_http_header(enum protocol_version version,
 	return 0;
 }
 
+static int get_client_protocol_http_header(const struct strbuf *version_advert,
+					   struct strbuf *header)
+{
+	if (version_advert->len > 0 &&
+	    strcmp(version_advert->buf, "version=0")) {
+		strbuf_addf(header, GIT_PROTOCOL_HEADER ": %s",
+			    version_advert->buf);
+
+		return 1;
+	}
+
+	return 0;
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
 	struct strbuf exp = STRBUF_INIT;
@@ -339,11 +353,11 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct strbuf refs_url = STRBUF_INIT;
 	struct strbuf effective_url = STRBUF_INIT;
 	struct strbuf protocol_header = STRBUF_INIT;
+	struct strbuf version_advert = STRBUF_INIT;
 	struct string_list extra_headers = STRING_LIST_INIT_DUP;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options http_options;
-	enum protocol_version version = get_protocol_version_config();
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -360,16 +374,10 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
-	/*
-	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
-	 * to perform a push, then fallback to v0 since the client doesn't know
-	 * how to push yet using v2.
-	 */
-	if (version == protocol_v2 && !strcmp("git-receive-pack", service))
-		version = protocol_v0;
+	get_client_protocol_version_advertisement(&version_advert);
 
 	/* Add the extra Git-Protocol header */
-	if (get_protocol_http_header(version, &protocol_header))
+	if (get_client_protocol_http_header(&version_advert, &protocol_header))
 		string_list_append(&extra_headers, protocol_header.buf);
 
 	memset(&http_options, 0, sizeof(http_options));
@@ -1327,6 +1335,8 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	int nongit;
 
+	register_allowed_protocol_versions_from_env();
+
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		error("remote-curl: usage: git remote-curl <remote> [<url>]");
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..d528e91630 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -186,7 +186,7 @@ test_expect_success 'hostname cannot break out of directory' '
 test_expect_success 'daemon log records all attributes' '
 	cat >expect <<-\EOF &&
 	Extended attribute "host": localhost
-	Extended attribute "protocol": version=1
+	Extended attribute "protocol": version=1:version=2:version=0
 	EOF
 	>daemon.log &&
 	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..2e56c79233 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -26,7 +26,7 @@ test_expect_success 'clone with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "clone> .*\\\0\\\0version=1\\\0$" log &&
+	grep "clone> .*\\\0\\\0version=1.*\\\0$" log &&
 	# Server responded using protocol v1
 	grep "clone< version 1" log
 '
@@ -42,7 +42,7 @@ test_expect_success 'fetch with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "fetch> .*\\\0\\\0version=1\\\0$" log &&
+	grep "fetch> .*\\\0\\\0version=1.*\\\0$" log &&
 	# Server responded using protocol v1
 	grep "fetch< version 1" log
 '
@@ -56,7 +56,7 @@ test_expect_success 'pull with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "fetch> .*\\\0\\\0version=1\\\0$" log &&
+	grep "fetch> .*\\\0\\\0version=1.*\\\0$" log &&
 	# Server responded using protocol v1
 	grep "fetch< version 1" log
 '
@@ -74,7 +74,7 @@ test_expect_success 'push with git:// using protocol v1' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v1
-	grep "push> .*\\\0\\\0version=1\\\0$" log &&
+	grep "push> .*\\\0\\\0version=1.*\\\0$" log &&
 	# Server responded using protocol v1
 	grep "push< version 1" log
 '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 3beeed4546..78c17c25e4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -24,7 +24,7 @@ test_expect_success 'list refs with git:// using protocol v2' '
 		ls-remote --symref "$GIT_DAEMON_URL/parent" >actual &&
 
 	# Client requested to use protocol v2
-	grep "git> .*\\\0\\\0version=2\\\0$" log &&
+	grep "git> .*\\\0\\\0version=2.*\\\0$" log &&
 	# Server responded using protocol v2
 	grep "git< version 2" log &&
 
@@ -56,7 +56,7 @@ test_expect_success 'clone with git:// using protocol v2' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v2
-	grep "clone> .*\\\0\\\0version=2\\\0$" log &&
+	grep "clone> .*\\\0\\\0version=2.*\\\0$" log &&
 	# Server responded using protocol v2
 	grep "clone< version 2" log
 '
@@ -74,7 +74,7 @@ test_expect_success 'fetch with git:// using protocol v2' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v2
-	grep "fetch> .*\\\0\\\0version=2\\\0$" log &&
+	grep "fetch> .*\\\0\\\0version=2.*\\\0$" log &&
 	# Server responded using protocol v2
 	grep "fetch< version 2" log
 '
@@ -90,7 +90,7 @@ test_expect_success 'pull with git:// using protocol v2' '
 	test_cmp expect actual &&
 
 	# Client requested to use protocol v2
-	grep "fetch> .*\\\0\\\0version=2\\\0$" log &&
+	grep "fetch> .*\\\0\\\0version=2.*\\\0$" log &&
 	# Server responded using protocol v2
 	grep "fetch< version 2" log
 '
@@ -476,7 +476,7 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
 	test_when_finished "rm -f log" &&
 	# Till v2 for push is designed, make sure that if a client has
 	# protocol.version configured to use v2, that the client instead falls
-	# back and uses v0.
+	# back to previous versions.
 
 	test_commit -C http_child three &&
 
@@ -489,10 +489,8 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
 	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s client_branch >expect &&
 	test_cmp expect actual &&
 
-	# Client didnt request to use protocol v2
-	! grep "Git-Protocol: version=2" log &&
-	# Server didnt respond using protocol v2
-	! grep "git< version 2" log
+	# Server responded with version 1
+	grep "git< version 1" log
 '
 
 
diff --git a/transport-helper.c b/transport-helper.c
index 143ca008c8..ac1937f1e1 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -105,6 +105,7 @@ static struct child_process *get_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf version_advert = STRBUF_INIT;
 	struct child_process *helper;
 	int duped;
 	int code;
@@ -127,6 +128,11 @@ static struct child_process *get_helper(struct transport *transport)
 		argv_array_pushf(&helper->env_array, "%s=%s",
 				 GIT_DIR_ENVIRONMENT, get_git_dir());
 
+	get_client_protocol_version_advertisement(&version_advert);
+	if (version_advert.len > 0)
+		argv_array_pushf(&helper->env_array, "%s=%s",
+				 GIT_PROTOCOL_ENVIRONMENT, version_advert.buf);
+
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)
 		die(_("unable to find remote helper for '%s'"), data->name);
-- 
2.19.0.605.g01d371f741-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/1] protocol: advertise multiple supported versions
  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
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Beller @ 2018-10-12 22:30 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: git, Jonathan Nieder, Junio C Hamano

On Thu, Oct 11, 2018 at 6:02 PM <steadmon@google.com> wrote:
>
> From: Josh Steadmon <steadmon@google.com>
>
> Currently the client advertises that it supports the wire protocol
> version set in the protocol.version config. However, not all services
> support the same set of protocol versions. When connecting to
> git-receive-pack, the client automatically downgrades to v0 if
> config.protocol is set to v2, but this check is not performed for other
> services.
>
> This patch creates a protocol version registry. Individual commands
> register all the protocol versions they support prior to communicating
> with a server. Versions should be listed in preference order; the
> version specified in protocol.version will automatically be moved to the
> front of the registry.
>
> The protocol version registry is passed to remote helpers via the
> GIT_PROTOCOL environment variable.
>
> Clients now advertise the full list of registered versions. Servers
> select the first recognized version from this advertisement.

I like this patch from the users point of view (i.e. inside fetch/push etc),
and I need to through the details in connect/protocol as that seems
to be a lot of new code, but hides the complexity very well.

> +       register_allowed_protocol_version(protocol_v2);
> +       register_allowed_protocol_version(protocol_v1);
> +       register_allowed_protocol_version(protocol_v0);

Would it make sense to have a

    register_allowed_protocol_versions(protocol_v2, protocol_v1,
protocol_v0, NULL);

similar to argv_array_pushl  or would that be overengineered?

> @@ -1085,10 +1085,9 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
>                     target_host, 0);
>
>         /* If using a new version put that stuff here after a second null byte */
> -       if (version > 0) {
> +       if (strcmp(version_advert->buf, "version=0")) {
>                 strbuf_addch(&request, '\0');
> -               strbuf_addf(&request, "version=%d%c",
> -                           version, '\0');
> +               strbuf_addf(&request, "%s%c", version_advert->buf, '\0');

It's a bit unfortunate to pass around the string, but reading through
the following
lines seems like it is easiest.


> @@ -1226,16 +1226,10 @@ struct child_process *git_connect(int fd[2], const char *url,
>  {
>         char *hostandport, *path;
>         struct child_process *conn;
> +       struct strbuf version_advert = STRBUF_INIT;
>         enum protocol protocol;
> -       enum protocol_version version = get_protocol_version_config();
>
> -       /*
> -        * NEEDSWORK: If we are trying to use protocol v2 and we are planning
> -        * to perform a push, then fallback to v0 since the client doesn't know
> -        * how to push yet using v2.
> -        */
> -       if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
> -               version = protocol_v0;
> +       get_client_protocol_version_advertisement(&version_advert);

Nice to have this special casing gone!

> diff --git a/protocol.c b/protocol.c
> index 5e636785d1..f788269c47 100644
> +
> +static const char protocol_v0_string[] = "0";
> +static const char protocol_v1_string[] = "1";
> +static const char protocol_v2_string[] = "2";
> +
...
> +/* Return the text representation of a wire protocol version. */
> +static const char *format_protocol_version(enum protocol_version version)
> +{
> +       switch (version) {
> +       case protocol_v0:
> +               return protocol_v0_string;
> +       case protocol_v1:
> +               return protocol_v1_string;
> +       case protocol_v2:
> +               return protocol_v2_string;
> +       case protocol_unknown_version:
> +               die(_("Unrecognized protocol version"));
> +       }
> +       die(_("Unrecognized protocol_version"));
> +}

Up to now we have the textual representation that can easily
be constructed from protocol_version by using e.g.
    sprintf(buf, "%d", version);
which is why I initially thought this could be worded way
shorter, but I guess this is more future proof?


> +
>  enum protocol_version get_protocol_version_config(void)
>  {
>         const char *value;
> @@ -30,6 +55,79 @@ enum protocol_version get_protocol_version_config(void)
>         return protocol_v0;
>  }
>
> +void register_allowed_protocol_version(enum protocol_version version)
> +{
> +       if (have_advertised_versions_already)
> +               error(_("attempting to register an allowed protocol version after advertisement"));

This would be a
    BUG(...)
instead?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-10 23:53             ` Josh Steadmon
@ 2018-10-12 23:30               ` Jonathan Nieder
  2018-10-12 23:32               ` Jonathan Nieder
  1 sibling, 0 replies; 17+ messages in thread
From: Jonathan Nieder @ 2018-10-12 23:30 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: Stefan Beller, git, Junio C Hamano

Hi,

Josh Steadmon wrote:

> So this runs into problems with remote-curl (and possibly other remote
> helpers):
>
> builtin/push.c can declare whatever allowed versions it wants, but those
> are not carried over when remote-curl is started to actually talk to the
> remote. What's worse, remote-curl starts its HTTP connection before it
> knows what command it's actually acting as a helper for.
>
> For now, I'm going to try adding an --allowed_versions flag for the
> remote helpers, but if anyone has a better idea, let me know.

There are some external remote helpers (see "git help remote-helpers"
for the documented interface), so alas, they can't take new flags.

That said, you can add a new remote helper capability and then
communicate on stdin, or in a pinch you can use the existing "option"
capability.  Remote helpers are also allowed to query "git config" if
they want to (either using the config machinery in config.h or by
running "git config").

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  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
  1 sibling, 1 reply; 17+ messages in thread
From: Jonathan Nieder @ 2018-10-12 23:32 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: Stefan Beller, git, Junio C Hamano

Josh Steadmon wrote:

> For now, I'm going to try adding an --allowed_versions flag for the
> remote helpers, but if anyone has a better idea, let me know.

I forgot to mention: the stateless-connect remote helper capability is
still experimental so you're free to change its interface (e.g. to
change the syntax of the stateless-connect command it provides).

Thanks again,
Jonathan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-12 23:32               ` Jonathan Nieder
@ 2018-10-12 23:45                 ` Josh Steadmon
  2018-10-12 23:53                   ` Jonathan Nieder
  0 siblings, 1 reply; 17+ messages in thread
From: Josh Steadmon @ 2018-10-12 23:45 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Stefan Beller, git, Junio C Hamano

On 2018.10.12 16:32, Jonathan Nieder wrote:
> Josh Steadmon wrote:
> 
> > For now, I'm going to try adding an --allowed_versions flag for the
> > remote helpers, but if anyone has a better idea, let me know.
> 
> I forgot to mention: the stateless-connect remote helper capability is
> still experimental so you're free to change its interface (e.g. to
> change the syntax of the stateless-connect command it provides).

For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version
string to the remote helpers.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-12 23:45                 ` Josh Steadmon
@ 2018-10-12 23:53                   ` Jonathan Nieder
  2018-10-13  7:58                     ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Nieder @ 2018-10-12 23:53 UTC (permalink / raw)
  To: Josh Steadmon; +Cc: Stefan Beller, git, Junio C Hamano

Josh Steadmon wrote:
> On 2018.10.12 16:32, Jonathan Nieder wrote:
>> Josh Steadmon wrote:

>>> For now, I'm going to try adding an --allowed_versions flag for the
>>> remote helpers, but if anyone has a better idea, let me know.
>>
>> I forgot to mention: the stateless-connect remote helper capability is
>> still experimental so you're free to change its interface (e.g. to
>> change the syntax of the stateless-connect command it provides).
>
> For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version
> string to the remote helpers.

Makes sense.  Thanks. :)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] protocol: limit max protocol version per service
  2018-10-12 23:53                   ` Jonathan Nieder
@ 2018-10-13  7:58                     ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2018-10-13  7:58 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Josh Steadmon, Stefan Beller, git

Jonathan Nieder <jrnieder@gmail.com> writes:

> Josh Steadmon wrote:
>> On 2018.10.12 16:32, Jonathan Nieder wrote:
>>> Josh Steadmon wrote:
>
>>>> For now, I'm going to try adding an --allowed_versions flag for the
>>>> remote helpers, but if anyone has a better idea, let me know.
>>>
>>> I forgot to mention: the stateless-connect remote helper capability is
>>> still experimental so you're free to change its interface (e.g. to
>>> change the syntax of the stateless-connect command it provides).
>>
>> For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version
>> string to the remote helpers.
>
> Makes sense.  Thanks. :)

Yeah, it does.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/1] protocol: advertise multiple supported versions
  2018-10-12 22:30     ` Stefan Beller
@ 2018-10-22 22:55       ` Josh Steadmon
  0 siblings, 0 replies; 17+ messages in thread
From: Josh Steadmon @ 2018-10-22 22:55 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, Jonathan Nieder, Junio C Hamano

On 2018.10.12 15:30, Stefan Beller wrote:
> On Thu, Oct 11, 2018 at 6:02 PM <steadmon@google.com> wrote:
> >
> > From: Josh Steadmon <steadmon@google.com>
> >
> > Currently the client advertises that it supports the wire protocol
> > version set in the protocol.version config. However, not all services
> > support the same set of protocol versions. When connecting to
> > git-receive-pack, the client automatically downgrades to v0 if
> > config.protocol is set to v2, but this check is not performed for other
> > services.
> >
> > This patch creates a protocol version registry. Individual commands
> > register all the protocol versions they support prior to communicating
> > with a server. Versions should be listed in preference order; the
> > version specified in protocol.version will automatically be moved to the
> > front of the registry.
> >
> > The protocol version registry is passed to remote helpers via the
> > GIT_PROTOCOL environment variable.
> >
> > Clients now advertise the full list of registered versions. Servers
> > select the first recognized version from this advertisement.
> 
> I like this patch from the users point of view (i.e. inside fetch/push etc),
> and I need to through the details in connect/protocol as that seems
> to be a lot of new code, but hides the complexity very well.
> 
> > +       register_allowed_protocol_version(protocol_v2);
> > +       register_allowed_protocol_version(protocol_v1);
> > +       register_allowed_protocol_version(protocol_v0);
> 
> Would it make sense to have a
> 
>     register_allowed_protocol_versions(protocol_v2, protocol_v1,
> protocol_v0, NULL);
> 
> similar to argv_array_pushl  or would that be overengineered?

Hmm, well it wouldn't be quite as clean as the argv_* versions, since we
can't take the pointer of an enum value, so we don't have a good
stop-value for the va_list. I suppose we could use
protocol_unknown_version, but that seems unintuitive to me. We could
also pass in an explicit argument count, but... ugh.

Am I missing some other way to do this cleanly? I'll admit I'm not very
familiar with va_lists.

> > @@ -1085,10 +1085,9 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
> >                     target_host, 0);
> >
> >         /* If using a new version put that stuff here after a second null byte */
> > -       if (version > 0) {
> > +       if (strcmp(version_advert->buf, "version=0")) {
> >                 strbuf_addch(&request, '\0');
> > -               strbuf_addf(&request, "version=%d%c",
> > -                           version, '\0');
> > +               strbuf_addf(&request, "%s%c", version_advert->buf, '\0');
> 
> It's a bit unfortunate to pass around the string, but reading through
> the following
> lines seems like it is easiest.
> 
> 
> > @@ -1226,16 +1226,10 @@ struct child_process *git_connect(int fd[2], const char *url,
> >  {
> >         char *hostandport, *path;
> >         struct child_process *conn;
> > +       struct strbuf version_advert = STRBUF_INIT;
> >         enum protocol protocol;
> > -       enum protocol_version version = get_protocol_version_config();
> >
> > -       /*
> > -        * NEEDSWORK: If we are trying to use protocol v2 and we are planning
> > -        * to perform a push, then fallback to v0 since the client doesn't know
> > -        * how to push yet using v2.
> > -        */
> > -       if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
> > -               version = protocol_v0;
> > +       get_client_protocol_version_advertisement(&version_advert);
> 
> Nice to have this special casing gone!
> 
> > diff --git a/protocol.c b/protocol.c
> > index 5e636785d1..f788269c47 100644
> > +
> > +static const char protocol_v0_string[] = "0";
> > +static const char protocol_v1_string[] = "1";
> > +static const char protocol_v2_string[] = "2";
> > +
> ...
> > +/* Return the text representation of a wire protocol version. */
> > +static const char *format_protocol_version(enum protocol_version version)
> > +{
> > +       switch (version) {
> > +       case protocol_v0:
> > +               return protocol_v0_string;
> > +       case protocol_v1:
> > +               return protocol_v1_string;
> > +       case protocol_v2:
> > +               return protocol_v2_string;
> > +       case protocol_unknown_version:
> > +               die(_("Unrecognized protocol version"));
> > +       }
> > +       die(_("Unrecognized protocol_version"));
> > +}
> 
> Up to now we have the textual representation that can easily
> be constructed from protocol_version by using e.g.
>     sprintf(buf, "%d", version);
> which is why I initially thought this could be worded way
> shorter, but I guess this is more future proof?

Yeah, my understanding is that we don't want to assume that version
identifiers will always be simple integers. We could get away with
sprintf() for now, but I figured I didn't want to cause future pain
if/when the version identifiers become complex.

> > +
> >  enum protocol_version get_protocol_version_config(void)
> >  {
> >         const char *value;
> > @@ -30,6 +55,79 @@ enum protocol_version get_protocol_version_config(void)
> >         return protocol_v0;
> >  }
> >
> > +void register_allowed_protocol_version(enum protocol_version version)
> > +{
> > +       if (have_advertised_versions_already)
> > +               error(_("attempting to register an allowed protocol version after advertisement"));
> 
> This would be a
>     BUG(...)
> instead?

Ack, will fix in the next version.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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