Add proto v2 archive command with HTTP support

>
> Using packet_reader will simplify version detection and capability
> handling, which will make implementation of protocol v2 support in
> git-archive easier.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>

> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e74f67539..e54fc39ad 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,
>  			       const char *remote, const char *exec,
>  			       const char *name_hint)
>  {
> -	char *buf;
>  	int fd[2], i, rv;
>  	struct transport *transport;
>  	struct remote *_remote;
> +	struct packet_reader reader;
> +	enum packet_read_status status;
>  
>  	_remote = remote_get(remote);
>  	if (!_remote->url[0])
> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,
>  	transport = transport_get(_remote, _remote->url[0]);
>  	transport_connect(transport, "git-upload-archive", exec, fd);
>  
> +	packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +
>  	/*
>  	 * Inject a fake --format field at the beginning of the
>  	 * arguments, with the format inferred from our output
> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,
>  		packet_write_fmt(fd[1], "argument %s\n", argv[i]);
>  	packet_flush(fd[1]);
>  
> -	buf = packet_read_line(fd[0], NULL);
> -	if (!buf)
> +	status = packet_reader_read(&reader);
> +
> +	if (status == PACKET_READ_FLUSH)
>  		die(_("git archive: expected ACK/NAK, got a flush packet"));

> -	if (strcmp(buf, "ACK")) {
> -		if (starts_with(buf, "NACK "))
> -			die(_("git archive: NACK %s"), buf + 5);
> -		if (starts_with(buf, "ERR "))
> -			die(_("remote error: %s"), buf + 4);
> +	if (strcmp(reader.buffer, "ACK")) {

> +		if (starts_with(reader.buffer, "NACK "))
> +			die(_("git archive: NACK %s"), reader.buffer + 5);
> +		if (starts_with(reader.buffer, "ERR "))
> +			die(_("remote error: %s"), reader.buffer + 4);
>  		die(_("git archive: protocol error"));
>  	}
>  
> -	if (packet_read_line(fd[0], NULL))
> +	status = packet_reader_read(&reader);
> +	if (status != PACKET_READ_FLUSH)
>  		die(_("git archive: expected a flush"));

>> -	if (packet_read_line(fd[0], NULL))
>> +	status = packet_reader_read(&reader);
>> +	if (status != PACKET_READ_FLUSH)
>>  		die(_("git archive: expected a flush"));
>
> This makes me wonder what happens if we got an EOF instead.  We fail
> to notice protocol error here, but do the code after this part
> correctly handle the situation?

>
> This adds a new archive command for protocol v2. The command expects
> arguments in the form "argument X" which are passed unmodified to
> git-upload-archive--writer.
>
> This command works over the file://, Git, and SSH transports. HTTP
> support will be added in a separate patch.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c        | 45 +++++++++++++++++++++++++++-------------
>  builtin/upload-archive.c | 44 ++++++++++++++++++++++++++++++++++++---
>  t/t5000-tar-tree.sh      |  5 +++++
>  3 files changed, 77 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e54fc39ad..73831887d 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -5,9 +5,11 @@
>  #include "cache.h"
>  #include "builtin.h"
>  #include "archive.h"
> +#include "connect.h"
>  #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)
> @@ -23,6 +25,13 @@ static void create_output_file(const char *output_file)
>         }
>  }
>
> +static int do_v2_command_and_cap(int out)
> +{
> +       packet_write_fmt(out, "command=archive\n");
> +       /* Capability list would go here, if we had any. */
> +       packet_delim(out);
> +}
> +
>  static int run_remote_archiver(int argc, const char **argv,
>                                const char *remote, const char *exec,
>                                const char *name_hint)
> @@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,
>         struct remote *_remote;
>         struct packet_reader reader;
>         enum packet_read_status status;
> +       enum protocol_version version;
>
>         _remote = remote_get(remote);
>         if (!_remote->url[0])
> @@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,
>
>         packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
>
> +       version = discover_version(&reader);
> +
> +       if (version == protocol_v2)
> +               do_v2_command_and_cap(fd[1]);
> +
>         /*
>          * Inject a fake --format field at the beginning of the
>          * arguments, with the format inferred from our output
> @@ -56,22 +71,24 @@ static int run_remote_archiver(int argc, const char **argv,
>                 packet_write_fmt(fd[1], "argument %s\n", argv[i]);
>         packet_flush(fd[1]);
>
> -       status = packet_reader_read(&reader);
> -
> -       if (status == PACKET_READ_FLUSH)
> -               die(_("git archive: expected ACK/NAK, got a flush packet"));
> -       if (strcmp(reader.buffer, "ACK")) {
> -               if (starts_with(reader.buffer, "NACK "))
> -                       die(_("git archive: NACK %s"), reader.buffer + 5);
> -               if (starts_with(reader.buffer, "ERR "))
> -                       die(_("remote error: %s"), reader.buffer + 4);
> -               die(_("git archive: protocol error"));

> +       if (version == protocol_v0) {
> +               status = packet_reader_read(&reader);
> +
> +               if (status == PACKET_READ_FLUSH)
> +                       die(_("git archive: expected ACK/NAK, got a flush packet"));
> +               if (strcmp(reader.buffer, "ACK")) {
> +                       if (starts_with(reader.buffer, "NACK "))
> +                               die(_("git archive: NACK %s"), reader.buffer + 5);
> +                       if (starts_with(reader.buffer, "ERR "))
> +                               die(_("remote error: %s"), reader.buffer + 4);
> +                       die(_("git archive: protocol error"));
> +               }
> +
> +               status = packet_reader_read(&reader);
> +               if (status != PACKET_READ_FLUSH)
> +                       die(_("git archive: expected a flush"));
>         }
>
> -       status = packet_reader_read(&reader);
> -       if (status != PACKET_READ_FLUSH)
> -               die(_("git archive: expected a flush"));
> -
>         /* Now, start reading from fd[0] and spit it out to stdout */
>         rv = recv_sideband("archive", fd[0], 1);
>         rv |= transport_disconnect(transport);
> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 25d911635..534e8fd56 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -5,6 +5,7 @@
>  #include "builtin.h"
>  #include "archive.h"
>  #include "pkt-line.h"
> +#include "protocol.h"
>  #include "sideband.h"
>  #include "run-command.h"
>  #include "argv-array.h"
> @@ -73,13 +74,53 @@ static ssize_t process_input(int child_fd, int band)
>         return sz;
>  }
>
> +static int handle_v2_command_and_cap(void)
> +{
> +       struct packet_reader reader;
> +       enum packet_read_status status;
> +
> +       packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +
> +       packet_write_fmt(1, "version 2\n");
> +       /*
> +        * We don't currently send any capabilities, but maybe we could list
> +        * supported archival formats?
> +        */
> +       packet_flush(1);
> +
> +       status = packet_reader_read(&reader);
> +       if (status != PACKET_READ_NORMAL ||
> +           strcmp(reader.buffer, "command=archive"))
> +               die(_("upload-archive: expected command=archive"));
> +       while (status == PACKET_READ_NORMAL) {
> +               /* We don't currently expect any client capabilities, but we
> +                * should still read (and ignore) any that happen to get sent.

> +                */
> +               status = packet_reader_read(&reader);
> +       }
> +       if (status != PACKET_READ_DELIM)
> +               die(_("upload-archive: expected delim packet"));

>  int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>  {
>         struct child_process writer = { argv };
> +       enum protocol_version version = determine_protocol_version_server();
>
>         if (argc == 2 && !strcmp(argv[1], "-h"))
>                 usage(upload_archive_usage);
>
> +       if (version == protocol_v2)
> +               handle_v2_command_and_cap();
> +       else {

> +               packet_write_fmt(1, "ACK\n");
> +               packet_flush(1);
> +       }
> +
>         /*
>          * Set up sideband subprocess.
>          *
> @@ -96,9 +137,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>                 die("upload-archive: %s", strerror(err));
>         }
>
> -       packet_write_fmt(1, "ACK\n");
> -       packet_flush(1);
> -
>         while (1) {
>                 struct pollfd pfd[2];
>
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0..4be74d6e9 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -145,6 +145,11 @@ test_expect_success \
>
>  check_tar b
>
> +test_expect_success 'protocol v2 for remote' '
> +       GIT_PROTOCOL="version=2" git archive --remote=. HEAD >v2_remote.tar
> +'
> +check_tar v2_remote

> On Tue, Sep 11, 2018 at 10:36 PM Josh Steadmon <steadmon@google.com> wrote:
>> +                */
>> +               status = packet_reader_read(&reader);
>> +       }
>> +       if (status != PACKET_READ_DELIM)
>> +               die(_("upload-archive: expected delim packet"));
>
> This is upload-archive, which is a low level plumbing command
> (see the main man page of git for an explanation of that category),
> so we do not translate the error/die() calls. Besides, this is executed
> on the server, which might have a different locale than the requesting
> client?
>
> Would asking for a setlocale() on the server side be an unreasonable
> feature request for the capabilities (in a follow up patch, and then not
> just for archive but also fetch/push, etc.)?

>> Would asking for a setlocale() on the server side be an unreasonable
>> feature request for the capabilities (in a follow up patch, and then not
>> just for archive but also fetch/push, etc.)?
>
> This would be very nice to have, but as you suggest in some follow-up
> change.

>  4) Aside from translation purposes, getting a machine-readable
>     "push/pull" etc. mode would be very handy. E.g. now you need to
>     parse stderr to see why exactly your push failed (hook denied, or
>     non-fast-forward, or non-fast-forward where there was a lock race
>     condition? ...).

> Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Sep 12 2018, Stefan Beller wrote:
>
>>> Would asking for a setlocale() on the server side be an unreasonable
>>> feature request for the capabilities (in a follow up patch, and then not
>>> just for archive but also fetch/push, etc.)?
>>
>> This would be very nice to have, but as you suggest in some follow-up
>> change.
>
> Indeed, I think we've gone pretty far afield from the goal of this
> patch series.
>
>> I think though that instead of doing setlocale() it would be better to
>> pass some flag saying we're operating in a machine-readable mode, and
>> then we'd (as part of the protocol defintion) say we're going to emit
>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.
>
> I think you're suggesting client-side message generation, and that is
> one way to handle internationalization of server output.
>
> The main downside is when the server really does want to provide a
> custom error message.  For that, we'd need

>  1. To propagate LANG to the server, so it knows what human language
>     to generate messages in.
>
>  2. On the server side, to produce messages in that language if
>     available, with an appropriate fallback if not.
>
> We've been thinking of doing at least (1) using the same trick as
> server-options use (cramming it into client capabilities).
>
> It is difficult to use setlocale for this because it affects the whole
> program (problematic for a threaded server) and affects features like
> collation order instead of just message generation (problematic for
> many things).  Does gettext have a variant that takes a locale_t
> argument?

> [...]
>>  4) Aside from translation purposes, getting a machine-readable
>>     "push/pull" etc. mode would be very handy. E.g. now you need to
>>     parse stderr to see why exactly your push failed (hook denied, or
>>     non-fast-forward, or non-fast-forward where there was a lock race
>>     condition? ...).
>
> Indeed, this is a good reason to provide error codes instead of (in
> the case where the message doesn't add anything to it) or alongside
> (in case the error message is more specialized) human-oriented error
> messages.

>> I think though that instead of doing setlocale() it would be better to
>> pass some flag saying we're operating in a machine-readable mode, and
>> then we'd (as part of the protocol defintion) say we're going to emit
>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.
>
> I think you're suggesting client-side message generation, and that is
> one way to handle internationalization of server output.
>
> The main downside is when the server really does want to provide a
> custom error message.  For that, we'd need
>
>  1. To propagate LANG to the server, so it knows what human language
>     to generate messages in.
>
>  2. On the server side, to produce messages in that language if
>     available, with an appropriate fallback if not.

>>> I think though that instead of doing setlocale() it would be better to
>>> pass some flag saying we're operating in a machine-readable mode, and
>>> then we'd (as part of the protocol defintion) say we're going to emit
>>> GIT_ERR_UPLOAD_ARCHIVE_EXPECTED_DELIM_PACKET or whatever.
>>
>> I think you're suggesting client-side message generation, and that is
>> one way to handle internationalization of server output.
>>
>> The main downside is when the server really does want to provide a
>> custom error message.  For that, we'd need
>>
>>  1. To propagate LANG to the server, so it knows what human language
>>     to generate messages in.
>>
>>  2. On the server side, to produce messages in that language if
>>     available, with an appropriate fallback if not.
>
> That is one way to do so, but it does not have to be the only way, I
> would think.  You can send a machine parsable message in pieces, and
> assemble the parts of speech into a message at the receiving end.
> Like sending a msgid to identify an entry in the .pot file, and
> values to be filled in.

> +static int do_v2_command_and_cap(int out)
> +{
> +	packet_write_fmt(out, "command=archive\n");
> +	/* Capability list would go here, if we had any. */
> +	packet_delim(out);
> +}
> +
>  static int run_remote_archiver(int argc, const char **argv,
>  			       const char *remote, const char *exec,
>  			       const char *name_hint)
> @@ -32,6 +41,7 @@ static int run_remote_archiver(int argc, const char **argv,
>  	struct remote *_remote;
>  	struct packet_reader reader;
>  	enum packet_read_status status;
> +	enum protocol_version version;
>  
>  	_remote = remote_get(remote);
>  	if (!_remote->url[0])
> @@ -41,6 +51,11 @@ static int run_remote_archiver(int argc, const char **argv,
>  
>  	packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
>  
> +	version = discover_version(&reader);

> -	status = packet_reader_read(&reader);
> -
> -	if (status == PACKET_READ_FLUSH)
> -		die(_("git archive: expected ACK/NAK, got a flush packet"));
> -	if (strcmp(reader.buffer, "ACK")) {
> -		if (starts_with(reader.buffer, "NACK "))
> -			die(_("git archive: NACK %s"), reader.buffer + 5);
> -		if (starts_with(reader.buffer, "ERR "))
> -			die(_("remote error: %s"), reader.buffer + 4);
> -		die(_("git archive: protocol error"));
> +	if (version == protocol_v0) {
> +		status = packet_reader_read(&reader);
> +
> +		if (status == PACKET_READ_FLUSH)
> +			die(_("git archive: expected ACK/NAK, got a flush packet"));
> +		if (strcmp(reader.buffer, "ACK")) {
> +			if (starts_with(reader.buffer, "NACK "))
> +				die(_("git archive: NACK %s"), reader.buffer + 5);
> +			if (starts_with(reader.buffer, "ERR "))
> +				die(_("remote error: %s"), reader.buffer + 4);
> +			die(_("git archive: protocol error"));
> +		}
> +
> +		status = packet_reader_read(&reader);
> +		if (status != PACKET_READ_FLUSH)
> +			die(_("git archive: expected a flush"));
>  	}

> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 25d911635..534e8fd56 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -5,6 +5,7 @@
>  #include "builtin.h"
>  #include "archive.h"
>  #include "pkt-line.h"
> +#include "protocol.h"
>  #include "sideband.h"
>  #include "run-command.h"
>  #include "argv-array.h"
> @@ -73,13 +74,53 @@ static ssize_t process_input(int child_fd, int band)
>  	return sz;
>  }
>  
> +static int handle_v2_command_and_cap(void)
> +{
> +	struct packet_reader reader;
> +	enum packet_read_status status;
> +
> +	packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +
> +	packet_write_fmt(1, "version 2\n");

> +	/*
> +	 * We don't currently send any capabilities, but maybe we could list
> +	 * supported archival formats?
> +	 */
> +	packet_flush(1);

> +	while (status == PACKET_READ_NORMAL) {
> +		/* We don't currently expect any client capabilities, but we
> +		 * should still read (and ignore) any that happen to get sent.
> +		 */
> +		status = packet_reader_read(&reader);

> +	}
> +	if (status != PACKET_READ_DELIM)
> +		die(_("upload-archive: expected delim packet"));
> +
> +	/* Let git-upload-archive--writer handle the arguments. */

> +	return 0;
> +}
> +
>  int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>  {
>  	struct child_process writer = { argv };
> +	enum protocol_version version = determine_protocol_version_server();
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage(upload_archive_usage);
>  
> +	if (version == protocol_v2)
> +		handle_v2_command_and_cap();
> +	else {
> +		packet_write_fmt(1, "ACK\n");
> +		packet_flush(1);
> +	}
> +

>  	/*
>  	 * Set up sideband subprocess.
>  	 *
> @@ -96,9 +137,6 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
>  		die("upload-archive: %s", strerror(err));
>  	}
>  
> -	packet_write_fmt(1, "ACK\n");
> -	packet_flush(1);
> -
>  	while (1) {
>  		struct pollfd pfd[2];
>  
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0..4be74d6e9 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -145,6 +145,11 @@ test_expect_success \
>  
>  check_tar b
>  
> +test_expect_success 'protocol v2 for remote' '
> +	GIT_PROTOCOL="version=2" git archive --remote=. HEAD >v2_remote.tar
> +'
> +check_tar v2_remote
> +
>  test_expect_success 'git archive --prefix=prefix/' '
>  	git archive --prefix=prefix/ HEAD >with_prefix.tar
>  '

> This adds a new archive command for protocol v2. The command expects
> arguments in the form "argument X" which are passed unmodified to
> git-upload-archive--writer.
>
> This command works over the file://, Git, and SSH transports. HTTP
> support will be added in a separate patch.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c        | 45 +++++++++++++++++++++++++++-------------
>  builtin/upload-archive.c | 44 ++++++++++++++++++++++++++++++++++++---
>  t/t5000-tar-tree.sh      |  5 +++++
>  3 files changed, 77 insertions(+), 17 deletions(-)

>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c  |  8 +++++++-
>  http-backend.c     | 10 +++++++++-
>  transport-helper.c |  5 +++--
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 73831887d..5fa75b3f7 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,
>                 status = packet_reader_read(&reader);
>                 if (status != PACKET_READ_FLUSH)
>                         die(_("git archive: expected a flush"));
> -       }
> +       } else if (version == protocol_v2 &&
> +                  starts_with(transport->url, "http"))

> +               /*
> +                * Commands over HTTP require two requests, so there's an
> +                * additional server response to parse.
> +                */
> +               discover_version(&reader);
>
>         /* Now, start reading from fd[0] and spit it out to stdout */
>         rv = recv_sideband("archive", fd[0], 1);
> diff --git a/http-backend.c b/http-backend.c
> index 458642ef7..d62d583c7 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -32,6 +32,7 @@ struct rpc_service {
>  static struct rpc_service rpc_service[] = {
>         { "upload-pack", "uploadpack", 1, 1 },
>         { "receive-pack", "receivepack", 0, -1 },
> +       { "upload-archive", "uploadarchive", 1, 1 },

> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c  |  8 +++++++-
>  http-backend.c     | 10 +++++++++-
>  transport-helper.c |  5 +++--
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 73831887d..5fa75b3f7 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,
>  		status = packet_reader_read(&reader);
>  		if (status != PACKET_READ_FLUSH)
>  			die(_("git archive: expected a flush"));
> -	}
> +	} else if (version == protocol_v2 &&
> +		   starts_with(transport->url, "http"))
> +		/*
> +		 * Commands over HTTP require two requests, so there's an
> +		 * additional server response to parse.
> +		 */
> +		discover_version(&reader);

> diff --git a/http-backend.c b/http-backend.c
> index 458642ef7..d62d583c7 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -32,6 +32,7 @@ struct rpc_service {
>  static struct rpc_service rpc_service[] = {
>  	{ "upload-pack", "uploadpack", 1, 1 },
>  	{ "receive-pack", "receivepack", 0, -1 },
> +	{ "upload-archive", "uploadarchive", 1, 1 },
>  };
>  
>  static struct string_list *get_parameters(void)
> @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
>  	struct rpc_service *svc = select_service(hdr, service_name);
>  	struct strbuf buf = STRBUF_INIT;
>  
> +	if (!strcmp(service_name, "git-upload-archive")) {
> +		/* git-upload-archive doesn't need --stateless-rpc */
> +		argv[1] = ".";
> +		argv[2] = NULL;
> +	}
> +
>  	strbuf_reset(&buf);
>  	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
>  	check_content_type(hdr, buf.buf);
> @@ -713,7 +720,8 @@ static struct service_cmd {
>  	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file},
>  
>  	{"POST", "/git-upload-pack$", service_rpc},
> -	{"POST", "/git-receive-pack$", service_rpc}
> +	{"POST", "/git-receive-pack$", service_rpc},
> +	{"POST", "/git-upload-archive$", service_rpc},
>  };
>  
>  static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
> diff --git a/transport-helper.c b/transport-helper.c
> index 143ca008c..b4b96fc89 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,
>  		ret = run_connect(transport, &cmdbuf);
>  	} else if (data->stateless_connect &&
>  		   (get_protocol_version_config() == protocol_v2) &&
> -		   !strcmp("git-upload-pack", name)) {
> +		   (!strcmp("git-upload-pack", name) ||
> +		    !strcmp("git-upload-archive", name))) {
>  		strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
>  		ret = run_connect(transport, &cmdbuf);
>  		if (ret)
> @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> +	if (!data->connect && !data->stateless_connect)
>  		die(_("operation not supported by protocol"));

> Josh Steadmon <steadmon@google.com> writes:
> 
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  builtin/archive.c  |  8 +++++++-
> >  http-backend.c     | 10 +++++++++-
> >  transport-helper.c |  5 +++--
> >  3 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/builtin/archive.c b/builtin/archive.c
> > index 73831887d..5fa75b3f7 100644
> > --- a/builtin/archive.c
> > +++ b/builtin/archive.c
> > @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,
> >  		status = packet_reader_read(&reader);
> >  		if (status != PACKET_READ_FLUSH)
> >  			die(_("git archive: expected a flush"));
> > -	}
> > +	} else if (version == protocol_v2 &&
> > +		   starts_with(transport->url, "http"))
> > +		/*
> > +		 * Commands over HTTP require two requests, so there's an
> > +		 * additional server response to parse.
> > +		 */
> > +		discover_version(&reader);
> 
> What should happen if the version discovered here is different from
> v2 or the capabilities offered are different from what we saw
> before?  Perhaps we need some sanity checks, as we on this side of
> the connection know we are making two requests, and may even end up
> talking with an instance of "upload-archive" that is different from
> the one we talked with earlier.
> 
> > diff --git a/http-backend.c b/http-backend.c
> > index 458642ef7..d62d583c7 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -32,6 +32,7 @@ struct rpc_service {
> >  static struct rpc_service rpc_service[] = {
> >  	{ "upload-pack", "uploadpack", 1, 1 },
> >  	{ "receive-pack", "receivepack", 0, -1 },
> > +	{ "upload-archive", "uploadarchive", 1, 1 },
> >  };
> >  
> >  static struct string_list *get_parameters(void)
> > @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
> >  	struct rpc_service *svc = select_service(hdr, service_name);
> >  	struct strbuf buf = STRBUF_INIT;
> >  
> > +	if (!strcmp(service_name, "git-upload-archive")) {
> > +		/* git-upload-archive doesn't need --stateless-rpc */
> > +		argv[1] = ".";
> > +		argv[2] = NULL;
> > +	}
> > +
> >  	strbuf_reset(&buf);
> >  	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
> >  	check_content_type(hdr, buf.buf);
> > @@ -713,7 +720,8 @@ static struct service_cmd {
> >  	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file},
> >  
> >  	{"POST", "/git-upload-pack$", service_rpc},
> > -	{"POST", "/git-receive-pack$", service_rpc}
> > +	{"POST", "/git-receive-pack$", service_rpc},
> > +	{"POST", "/git-upload-archive$", service_rpc},
> >  };
> >  
> >  static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 143ca008c..b4b96fc89 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,
> >  		ret = run_connect(transport, &cmdbuf);
> >  	} else if (data->stateless_connect &&
> >  		   (get_protocol_version_config() == protocol_v2) &&
> > -		   !strcmp("git-upload-pack", name)) {
> > +		   (!strcmp("git-upload-pack", name) ||
> > +		    !strcmp("git-upload-archive", name))) {
> >  		strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
> >  		ret = run_connect(transport, &cmdbuf);
> >  		if (ret)
> > @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,
> >  
> >  	/* Get_helper so connect is inited. */
> >  	get_helper(transport);
> > -	if (!data->connect)
> > +	if (!data->connect && !data->stateless_connect)
> >  		die(_("operation not supported by protocol"));
> 
> This is somewhat curious.  The upload-pack going over HTTP also is
> triggered by the same "stateless-connect" remote helper command, as
> we just saw in the previous hunk, and that support is not new.  Why
> do we need this change then?  What's different between the "data"
> that is obtained by get_helper(transport) for driving upload-pack
> and upload-archive?  Presumably upload-pack was working without this
> change because it also sets the connect bit on, and upload-archive
> does not work without this change because it does not?  Why do these
> two behave differently?

> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/archive.c  |  8 +++++++-
>  http-backend.c     | 10 +++++++++-
>  transport-helper.c |  5 +++--
>  3 files changed, 19 insertions(+), 4 deletions(-)

> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char **argv,
>  		status = packet_reader_read(&reader);
>  		if (status != PACKET_READ_FLUSH)
>  			die(_("git archive: expected a flush"));
> -	}
> +	} else if (version == protocol_v2 &&
> +		   starts_with(transport->url, "http"))

> +		/*
> +		 * Commands over HTTP require two requests, so there's an
> +		 * additional server response to parse.
> +		 */
> +		discover_version(&reader);

> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -32,6 +32,7 @@ struct rpc_service {
>  static struct rpc_service rpc_service[] = {
>  	{ "upload-pack", "uploadpack", 1, 1 },
>  	{ "receive-pack", "receivepack", 0, -1 },
> +	{ "upload-archive", "uploadarchive", 1, 1 },

> @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
>  	struct rpc_service *svc = select_service(hdr, service_name);
>  	struct strbuf buf = STRBUF_INIT;
>  
> +	if (!strcmp(service_name, "git-upload-archive")) {
> +		/* git-upload-archive doesn't need --stateless-rpc */

> @@ -713,7 +720,8 @@ static struct service_cmd {
>  	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file},
>  
>  	{"POST", "/git-upload-pack$", service_rpc},
> -	{"POST", "/git-receive-pack$", service_rpc}
> +	{"POST", "/git-receive-pack$", service_rpc},
> +	{"POST", "/git-upload-archive$", service_rpc},

> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -605,7 +605,8 @@ static int process_connect_service(struct transport *transport,
>  		ret = run_connect(transport, &cmdbuf);
>  	} else if (data->stateless_connect &&
>  		   (get_protocol_version_config() == protocol_v2) &&

> @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> +	if (!data->connect && !data->stateless_connect)
>  		die(_("operation not supported by protocol"));

> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 2a97b27b0a..c408e3a23d 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -62,11 +62,9 @@ check_tar() {
>         dir=$1
>         dir_with_prefix=$dir/$2
>
> -       test_expect_success ' extract tar archive' '
> -               (mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile
> -       '
> +       (mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile &&
>
> -       test_expect_success TAR_NEEDS_PAX_FALLBACK ' interpret pax headers' '
> +       if test_have_prereq TAR_NEEDS_PAX_FALLBACK ; then
>                 (
>                         cd $dir &&
>                         for header in *.paxheader
> @@ -82,16 +80,11 @@ check_tar() {
>                                 fi
>                         done
>                 )
> -       '
> +       fi &&
>
> -       test_expect_success ' validate filenames' '
> -               (cd ${dir_with_prefix}a && find .) | sort >$listfile &&
> -               test_cmp a.lst $listfile
> -       '
> -
> -       test_expect_success ' validate file contents' '
> -               diff -r a ${dir_with_prefix}a
> -       '
> +       (cd ${dir_with_prefix}a && find .) | sort >$listfile &&
> +       test_cmp a.lst $listfile &&
> +       diff -r a ${dir_with_prefix}a

>  }
>
>  test_expect_success \
> @@ -143,19 +136,20 @@ test_expect_success \
>      'git archive' \
>      'git archive HEAD >b.tar'
>
> -check_tar b
> +test_expect_success 'extract archive' 'check_tar b'

>
>  test_expect_success 'git archive --prefix=prefix/' '
>         git archive --prefix=prefix/ HEAD >with_prefix.tar
>  '
>
> -check_tar with_prefix prefix/
> +test_expect_success 'extract with prefix' 'check_tar with_prefix prefix/'
>
>  test_expect_success 'git-archive --prefix=olde-' '
>         git archive --prefix=olde- HEAD >with_olde-prefix.tar
>  '
>
> -check_tar with_olde-prefix olde-
> +test_expect_success 'extract with olde- prefix' \
> +       'check_tar with_olde-prefix olde-'

>
> Using packet_reader will simplify version detection and capability
> handling, which will make implementation of protocol v2 support in
> git-archive easier.
>
> This refactoring does not change the behavior of "git archive".
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>

> ---
>  builtin/archive.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e74f675390..4eb547c5b7 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char **argv,
>                                const char *remote, const char *exec,
>                                const char *name_hint)
>  {
> -       char *buf;
>         int fd[2], i, rv;
>         struct transport *transport;
>         struct remote *_remote;
> +       struct packet_reader reader;
> +       enum packet_read_status status;
>
>         _remote = remote_get(remote);
>         if (!_remote->url[0])
> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,
>         transport = transport_get(_remote, _remote->url[0]);
>         transport_connect(transport, "git-upload-archive", exec, fd);
>
> +       packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +
>         /*
>          * Inject a fake --format field at the beginning of the
>          * arguments, with the format inferred from our output
> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char **argv,
>                 packet_write_fmt(fd[1], "argument %s\n", argv[i]);
>         packet_flush(fd[1]);
>
> -       buf = packet_read_line(fd[0], NULL);
> -       if (!buf)
> +       status = packet_reader_read(&reader);
> +
> +       if (status != PACKET_READ_NORMAL || reader.pktlen <= 0)
>                 die(_("git archive: expected ACK/NAK, got a flush packet"));
> -       if (strcmp(buf, "ACK")) {
> -               if (starts_with(buf, "NACK "))
> -                       die(_("git archive: NACK %s"), buf + 5);
> -               if (starts_with(buf, "ERR "))
> -                       die(_("remote error: %s"), buf + 4);
> +       if (strcmp(reader.line, "ACK")) {
> +               if (starts_with(reader.line, "NACK "))
> +                       die(_("git archive: NACK %s"), reader.line + 5);
> +               if (starts_with(reader.line, "ERR "))
> +                       die(_("remote error: %s"), reader.line + 4);
>                 die(_("git archive: protocol error"));
>         }
>
> -       if (packet_read_line(fd[0], NULL))
> +       status = packet_reader_read(&reader);
> +       if (status == PACKET_READ_NORMAL && reader.pktlen > 0)
>                 die(_("git archive: expected a flush"));
>
>         /* Now, start reading from fd[0] and spit it out to stdout */
> --
> 2.19.0.605.g01d371f741-goog
>

>
> This is the second version of my series to add a new protocol v2 command
> for archiving, with support for HTTP(S).
>
> NEEDSWORK: a server built with this series is not backwards-compatible
> with clients that set GIT_PROTOCOL=version=2 or configure
> protocol.version=2. The old client will unconditionally send "argument
> ..." packet lines, which breaks the server's expectations of a
> "command=archive" request,

>> I've been discussing workarounds for this with Jonathan Nieder, but
>> please let me know if you have any suggestions for v3 of this series.
>
> Care to open the discussion to the list? What are the different
> approaches, what are the pros/cons?

>  1. Clients sending version=2 when they do not, in fact, speak protocol
>     v2 for a service is a (serious) bug.  (Separately from this
>     series) we should fix it.
>
>  2. That bug is already in the wild, alas.  Fortunately the semantics of
>     GIT_PROTOCOL as a list of key/value pairs is well defined.  So we
>     have choices of (a) bump version to version=3 (b) pass another
>     value 'version=2:yesreallyversion=2' (c) etc.
>
>  3. This is likely to affect push, too.

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >  1. Clients sending version=2 when they do not, in fact, speak protocol
> >     v2 for a service is a (serious) bug.  (Separately from this
> >     series) we should fix it.
> >
> >  2. That bug is already in the wild, alas.  Fortunately the semantics of
> >     GIT_PROTOCOL as a list of key/value pairs is well defined.  So we
> >     have choices of (a) bump version to version=3 (b) pass another
> >     value 'version=2:yesreallyversion=2' (c) etc.
> >
> >  3. This is likely to affect push, too.
> 
> Do you mean that existing "git push", "git fetch" and "git archive"
> sends version=2 even when they are not capable of speaking protocol
> v2?  I thought that "git archive [--remote]" was left outside of the
> protocol update (that was the reason why the earlier attempt took a
> hacky route of "shallow clone followed by local archive"), so there
> is no "git archive" in the wild that can even say "version=$n"
> (which requires you to be at least version=1)?

> Yes, the version on my desktop sends version=2 when archiving:
>
> ∫ which git
> /usr/bin/git
> ∫ git --version
> git version 2.19.0.605.g01d371f741-goog
> ∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \
>   --enable=upload-archive \
>   --base-path=${HOME}/src/bare-repos &
> [1] 258496
> ∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar
> ∫ grep version ~/server_trace
> 15:31:22.377869 pkt-line.c:80           packet:          git< git-upload-archive /test-repo.git\0host=localhost\0\0version=2\0

> On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon <steadmon@google.com> wrote:
> >
> > This is the second version of my series to add a new protocol v2 command
> > for archiving, with support for HTTP(S).
> >
> > NEEDSWORK: a server built with this series is not backwards-compatible
> > with clients that set GIT_PROTOCOL=version=2 or configure
> > protocol.version=2. The old client will unconditionally send "argument
> > ..." packet lines, which breaks the server's expectations of a
> > "command=archive" request,
> 
> So if an old client sets protocol to v2, it would only apply that
> protocol version
> to fetch, not archive, so it would start following a v0 conversation, but
> as the protocol version is set, it would be transmitted to the server.
> This sounds like a bug in the client?

> >  while the server's capability advertisement
> > in turn breaks the clients expectation of either an ACK or NACK.
> 
> Could a modern client send either another protocol version (3?)
> or a special capability along the protocol version ("fixed_archive")
> 
> > I've been discussing workarounds for this with Jonathan Nieder, but
> > please let me know if you have any suggestions for v3 of this series.
> 
> Care to open the discussion to the list? What are the different
> approaches, what are the pros/cons?