Re: [PATCH v4 0/1] Advertise multiple supported proto versions

2018-11-16 Thread Junio C Hamano
Josh Steadmon  writes:

>> This one unfortunately seems to interact rather badly with your
>> js/remote-archive-v2 topic which is already in 'next', when this
>> topic is merged to 'pu', and my attempt to mechanically resolve
>> conflicts breaks t5000.
>
> Hmm, should we drop js/remote-archive-v2 then? Any solution that
> unblocks js/remote-archive-v2 will almost certainly depend on this
> patch.

That one is already in 'next' for quite some time.  If you are
retrating it, that's OK.

Let me revert the topic out of 'next' and discard it, and then queue
this one in 'pu'.

Thanks.






Re: [PATCH v4 0/1] Advertise multiple supported proto versions

2018-11-14 Thread Josh Steadmon
On 2018.11.14 19:22, Junio C Hamano wrote:
> Josh Steadmon  writes:
> 
> > Fix several bugs identified in v3, clarify commit message, and clean up
> > extern keyword in protocol.h.
> 
> It is good to descirbe the change relative to v3 here, which would
> help those who are interested and reviewed v3.
> 
> To help those who missed the boat and v4 is their first encounter
> with this series, having the description relative to v3 alone and
> nothing else is not very friendly, though.

Ack. Will keep this in mind for future patches.

> > + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> > + --- a/builtin/upload-archive.c
> > + +++ b/builtin/upload-archive.c
> > +@@
> > + #include "builtin.h"
> > + #include "archive.h"
> > + #include "pkt-line.h"
> > ++#include "protocol.h"
> > + #include "sideband.h"
> > + #include "run-command.h"
> > + #include "argv-array.h"
> > +@@
> > +   if (argc == 2 && !strcmp(argv[1], "-h"))
> > +   usage(upload_archive_usage);
> > + 
> > ++  register_allowed_protocol_version(protocol_v0);
> > ++
> > +   /*
> > +* Set up sideband subprocess.
> > +*
> 
> This one unfortunately seems to interact rather badly with your
> js/remote-archive-v2 topic which is already in 'next', when this
> topic is merged to 'pu', and my attempt to mechanically resolve
> conflicts breaks t5000.

Hmm, should we drop js/remote-archive-v2 then? Any solution that
unblocks js/remote-archive-v2 will almost certainly depend on this
patch.


Re: [PATCH v4 0/1] Advertise multiple supported proto versions

2018-11-14 Thread Junio C Hamano
Josh Steadmon  writes:

> Fix several bugs identified in v3, clarify commit message, and clean up
> extern keyword in protocol.h.

It is good to descirbe the change relative to v3 here, which would
help those who are interested and reviewed v3.

To help those who missed the boat and v4 is their first encounter
with this series, having the description relative to v3 alone and
nothing else is not very friendly, though.

> + diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> + --- a/builtin/upload-archive.c
> + +++ b/builtin/upload-archive.c
> +@@
> + #include "builtin.h"
> + #include "archive.h"
> + #include "pkt-line.h"
> ++#include "protocol.h"
> + #include "sideband.h"
> + #include "run-command.h"
> + #include "argv-array.h"
> +@@
> + if (argc == 2 && !strcmp(argv[1], "-h"))
> + usage(upload_archive_usage);
> + 
> ++register_allowed_protocol_version(protocol_v0);
> ++
> + /*
> +  * Set up sideband subprocess.
> +  *

This one unfortunately seems to interact rather badly with your
js/remote-archive-v2 topic which is already in 'next', when this
topic is merged to 'pu', and my attempt to mechanically resolve
conflicts breaks t5000.




[PATCH v4 0/1] Advertise multiple supported proto versions

2018-11-13 Thread Josh Steadmon
Fix several bugs identified in v3, clarify commit message, and clean up
extern keyword in protocol.h.

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/receive-pack.c   |   3 +
 builtin/send-pack.c  |   3 +
 builtin/upload-archive.c |   3 +
 builtin/upload-pack.c|   4 ++
 connect.c|  47 +++
 protocol.c   | 122 ---
 protocol.h   |  23 +++-
 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 ++
 19 files changed, 237 insertions(+), 58 deletions(-)

Range-diff against v3:
1:  b9968e3fb0 ! 1:  3c023991c0 protocol: advertise multiple supported versions
@@ -4,22 +4,25 @@
 
 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.
+support the same set of protocol versions. For example, 
git-receive-pack
+supports v1 and v0, but not v2. If a client connects to 
git-receive-pack
+and requests v2, it will instead be downgraded to v0. Other services,
+such as git-upload-archive, do not do any version negotiation checks.
 
-This patch creates a protocol version registry. Individual operations
-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.
+This patch creates a protocol version registry. Individual client and
+server programs register all the protocol versions they support prior 
to
+communicating with a remote instance. 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.
+select the first allowed version from this advertisement.
+
+While we're at it, remove unnecessary externs from function 
declarations
+in protocol.h.
 
 Signed-off-by: Josh Steadmon 
@@ -165,6 +168,20 @@
git_config(git_push_config, );
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+ diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
+ --- a/builtin/receive-pack.c
+ +++ b/builtin/receive-pack.c
+@@
+ 
+   packet_trace_identity("receive-pack");
+ 
++  register_allowed_protocol_version(protocol_v1);
++  register_allowed_protocol_version(protocol_v0);
++
+   argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 
0);
+ 
+   if (argc > 1)
+
  diff --git a/builtin/send-pack.c b/builtin/send-pack.c
  --- a/builtin/send-pack.c
  +++ b/builtin/send-pack.c
@@ -179,6 +196,42 @@
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
if (argc > 0) {
 
+ diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
+ --- a/builtin/upload-archive.c
+ +++ b/builtin/upload-archive.c
+@@
+ #include "builtin.h"
+ #include "archive.h"
+ #include "pkt-line.h"
++#include "protocol.h"
+ #include "sideband.h"
+ #include "run-command.h"
+ #include "argv-array.h"
+@@
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(upload_archive_usage);
+ 
++  register_allowed_protocol_version(protocol_v0);
++
+   /*
+* Set up sideband subprocess.
+*
+
+ diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
+ --- a/builtin/upload-pack.c
+ +++ b/builtin/upload-pack.c
+@@
+   packet_trace_identity("upload-pack");
+   read_replace_refs = 0;
+ 
++  register_allowed_protocol_version(protocol_v2);
++  register_allowed_protocol_version(protocol_v1);
++  register_allowed_protocol_version(protocol_v0);
++
+   argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+ 
+   if (argc != 1)
+