[PATCH 1/2] transport-helper: no connection restriction in connect_helper

[PATCH 1/2] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

For protocol-v2, "stateless-connection" can be used to establish a
stateless connection between the client and the server, but trying to
establish http connection by calling "transport->vtable->connect" will
fail. This restriction was first introduced in commit b236752a87
(Support remote archive from all smart transports, 2009-12-09) by
adding a limitation in the "connect_helper()" function.

Remove the restriction in the "connect_helper()" function and use the
logic in the "process_connect_service()" function to check the protocol
version and service name. By this way, we can make a connection and do
something useful. E.g., in a later commit, implements remote archive
for a repository over HTTP protocol.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.40.1.49.g40e13c3520.dirty

[PATCH 2/2] archive: support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Even though we can establish a stateless connection, we still cannot
archive the remote repository using a stateless HTTP protocol. Try the
following steps to make it work.

 1. Add support for "git-upload-archive" service in "http-backend".

 2. Unable to access the URL ".../info/refs?service=git-upload-archive"
    to detect the protocol version, use the "git-upload-pack" service
    instead.

 3. "git-archive" does not resolve the protocol version and capabilities
    when connecting to remote-helper, so the remote-helper should not
    send them.

 4. "git-archive" may not be able to disconnect the stateless
    connection. Run "do_take_over()" to take_over the transfer for
    a graceful disconnect function.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 15 +++++++++++++--
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
 transport-helper.c     |  5 ++++-
 4 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..ed3bed965a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	const char *argv[4];
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	if (!strcmp(service_name, "git-upload-archive")) {
+		argv[1] = ".";
+		argv[2] = NULL;
+	} else {
+		argv[1] = "--stateless-rpc";
+		argv[2] = ".";
+		argv[3] = NULL;
+	}
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -723,7 +733,8 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.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/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..80123c1e06 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,34 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_when_finished "rm -f d5.zip" &&
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	test_when_finished "rm -f d5.zip" &&
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD &&
+	test_cmp_bin d.zip d5.zip
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
@@ -668,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.40.1.49.g40e13c3520.dirty

Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> For protocol-v2, "stateless-connection" can be used to establish a
> stateless connection between the client and the server, but trying to
> establish http connection by calling "transport->vtable->connect" will
> fail. This restriction was first introduced in commit b236752a87
> (Support remote archive from all smart transports, 2009-12-09) by
> adding a limitation in the "connect_helper()" function.

The above description may not be technically wrong per-se, but I
found it confusing.  The ".connect method must be defined" you are
removing was added back when there was no "stateless" variant of the
connection initiation.  Many codepaths added by that patch did "if
.connect is there, call it, but otherwise die()" and I think the
code you were removing was added as a safety valve, not a limitation
or restriction.  Later, process_connect_service() learned to handle
the .stateless_connect bit as a fallback for transports without
.connect method defined, and the commit added that feature, edc9caf7
(transport-helper: introduce stateless-connect, 2018-03-15), forgot
that the caller did not allow this fallback.

	When b236752a (Support remote archive from all smart
	transports, 2009-12-09) added "remote archive" support for
	"smart transports", it was for transport that supports the
	.connect method.  connect_helper() function protected itself
	from getting called for a transport without the method
	before calling process_connect_service(), which did not work
	wuth such a transport.

	Later, edc9caf7 (transport-helper: introduce
	stateless-connect, 2018-03-15) added a way for a transport
	without the .connect method to establish a "stateless"
	connection in protocol-v2, process_connect_service() was
	taught to handle the "stateless" connection, making the old
	safety valve in its caller that insisted that .connect
	method must be defined too strict, and forgot to loosen it.

or something along that line would have been easire to follow, at
least to me.

> Remove the restriction in the "connect_helper()" function and use the
> logic in the "process_connect_service()" function to check the protocol
> version and service name. By this way, we can make a connection and do
> something useful. E.g., in a later commit, implements remote archive
> for a repository over HTTP protocol.

OK.  

b236752a87 was to allow "remote archive from all smart transports",
but unfortunately HTTP was not among "smart transports".  This
series is to update smart HTTP transport (aka "stateless") to also
support it?  Interesting.

> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> -		die(_("operation not supported by protocol"));
>  
>  	if (!process_connect_service(transport, name, exec))
>  		die(_("can't connect to subservice %s"), name);

Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

On Wed, Sep 20, 2023 at 1:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > For protocol-v2, "stateless-connection" can be used to establish a
> > stateless connection between the client and the server, but trying to
> > establish http connection by calling "transport->vtable->connect" will
> > fail. This restriction was first introduced in commit b236752a87
> > (Support remote archive from all smart transports, 2009-12-09) by
> > adding a limitation in the "connect_helper()" function.
>
> The above description may not be technically wrong per-se, but I
> found it confusing.  The ".connect method must be defined" you are
> removing was added back when there was no "stateless" variant of the
> connection initiation.  Many codepaths added by that patch did "if
> .connect is there, call it, but otherwise die()" and I think the
> code you were removing was added as a safety valve, not a limitation
> or restriction.  Later, process_connect_service() learned to handle
> the .stateless_connect bit as a fallback for transports without
> .connect method defined, and the commit added that feature, edc9caf7
> (transport-helper: introduce stateless-connect, 2018-03-15), forgot
> that the caller did not allow this fallback.
>
>         When b236752a (Support remote archive from all smart
>         transports, 2009-12-09) added "remote archive" support for
>         "smart transports", it was for transport that supports the
>         .connect method.  connect_helper() function protected itself
>         from getting called for a transport without the method
>         before calling process_connect_service(), which did not work
>         wuth such a transport.
>
>         Later, edc9caf7 (transport-helper: introduce
>         stateless-connect, 2018-03-15) added a way for a transport
>         without the .connect method to establish a "stateless"
>         connection in protocol-v2, process_connect_service() was
>         taught to handle the "stateless" connection, making the old
>         safety valve in its caller that insisted that .connect
>         method must be defined too strict, and forgot to loosen it.
>
> or something along that line would have been easire to follow, at
> least to me.
>

These explanations are very clear and helpful, thank you.

--
Jiang Xin

[PATCH v2 0/3] support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Enable stateless-rpc connection in "connect_helper()", and add support
for remote archive from a stateless transport.

range-diff v1...v2:

1:  4457ca910b ! 1:  6fabd4dcab transport-helper: no connection restriction in connect_helper
    @@ Metadata
      ## Commit message ##
         transport-helper: no connection restriction in connect_helper
     
    -    For protocol-v2, "stateless-connection" can be used to establish a
    -    stateless connection between the client and the server, but trying to
    -    establish http connection by calling "transport->vtable->connect" will
    -    fail. This restriction was first introduced in commit b236752a87
    -    (Support remote archive from all smart transports, 2009-12-09) by
    -    adding a limitation in the "connect_helper()" function.
    +    When commit b236752a (Support remote archive from all smart transports,
    +    2009-12-09) added "remote archive" support for "smart transports", it
    +    was for transport that supports the ".connect" method. The
    +    "connect_helper()" function protected itself from getting called for a
    +    transport without the method before calling process_connect_service(),
    +    which did not work with such a transport.
     
    -    Remove the restriction in the "connect_helper()" function and use the
    -    logic in the "process_connect_service()" function to check the protocol
    -    version and service name. By this way, we can make a connection and do
    -    something useful. E.g., in a later commit, implements remote archive
    -    for a repository over HTTP protocol.
    +    Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
    +    2018-03-15) added a way for a transport without the ".connect" method
    +    to establish a "stateless" connection in protocol-v2, which
    +    process_connect_service() was taught to handle the "stateless"
    +    connection, making the old safety valve in its caller that insisted
    +    that ".connect" method must be defined too strict, and forgot to loosen
    +    it.
     
    +    Remove the restriction in the "connect_helper()" function and give the
    +    function "process_connect_service()" the opportunity to establish a
    +    connection using ".connect" or ".stateless_connect" for protocol v2. So
    +    we can connect with a stateless-rpc and do something useful. E.g., in a
    +    later commit, implements remote archive for a repository over HTTP
    +    protocol.
    +
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## transport-helper.c ##
-:  ---------- > 2:  1d687abc7e transport-helper: run do_take_over in connect_helper
2:  d4242d1f27 ! 3:  051d66f48e archive: support remote archive from stateless transport
    @@ Commit message
     
          1. Add support for "git-upload-archive" service in "http-backend".
     
    -     2. Unable to access the URL ".../info/refs?service=git-upload-archive"
    -        to detect the protocol version, use the "git-upload-pack" service
    -        instead.
    +     2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
    +        protocol version, instead of use the "git-upload-archive" service.
     
    -     3. "git-archive" does not resolve the protocol version and capabilities
    -        when connecting to remote-helper, so the remote-helper should not
    -        send them.
    -
    -     4. "git-archive" may not be able to disconnect the stateless
    -        connection. Run "do_take_over()" to take_over the transfer for
    -        a graceful disconnect function.
    +     3. "git-archive" does not expect to see protocol version and
    +        capabilities when connecting to remote-helper, so do not send them
    +        in "remote-curl.c" for the "git-upload-archive" service.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
    @@ transport-helper.c: static int process_connect_service(struct transport *transpo
      		strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
      		ret = run_connect(transport, &cmdbuf);
      		if (ret)
    -@@ transport-helper.c: static int connect_helper(struct transport *transport, const char *name,
    - 
    - 	fd[0] = data->helper->out;
    - 	fd[1] = data->helper->in;
    -+
    -+	do_take_over(transport);
    - 	return 0;
    - }
    - 

Jiang Xin (3):
  transport-helper: no connection restriction in connect_helper
  transport-helper: run do_take_over in connect_helper
  archive: support remote archive from stateless transport

 http-backend.c         | 15 +++++++++++++--
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
 transport-helper.c     |  7 ++++---
 4 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v2 1/3] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which did not work with such a transport.

Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol-v2, which
process_connect_service() was taught to handle the "stateless"
connection, making the old safety valve in its caller that insisted
that ".connect" method must be defined too strict, and forgot to loosen
it.

Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v2 2/3] transport-helper: run do_take_over in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

After successfully connecting to the smart transport by calling
"process_connect_service()" in "connect_helper()", run "do_take_over()"
to replace the old vtable with a new one which has methods ready for
the smart transport connection.

The subsequent commit introduces remote archive for a stateless-rpc
connection. But without running "do_take_over()", it may fail to call
"transport_disconnect()" in "run_remote_archiver()" of
"builtin/archive.c". This is because for a stateless connection or a
service like "git-upload-pack-archive", the remote helper may receive a
SIGPIPE signal and exit early. To have a graceful disconnect method by
calling "do_take_over()" will solve this issue.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..3c8802b7a3 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -668,6 +668,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v2 3/3] archive: support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Even though we can establish a stateless connection, we still cannot
archive the remote repository using a stateless HTTP protocol. Try the
following steps to make it work.

 1. Add support for "git-upload-archive" service in "http-backend".

 2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
    protocol version, instead of use the "git-upload-archive" service.

 3. "git-archive" does not expect to see protocol version and
    capabilities when connecting to remote-helper, so do not send them
    in "remote-curl.c" for the "git-upload-archive" service.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 15 +++++++++++++--
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
 transport-helper.c     |  3 ++-
 4 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..ed3bed965a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	const char *argv[4];
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	if (!strcmp(service_name, "git-upload-archive")) {
+		argv[1] = ".";
+		argv[2] = NULL;
+	} else {
+		argv[1] = "--stateless-rpc";
+		argv[2] = ".";
+		argv[3] = NULL;
+	}
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -723,7 +733,8 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.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/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..80123c1e06 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,34 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_when_finished "rm -f d5.zip" &&
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	test_when_finished "rm -f d5.zip" &&
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD &&
+	test_cmp_bin d.zip d5.zip
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 3c8802b7a3..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
-- 
2.40.1.50.gf560bcc116.dirty

Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

[Eric Sunshine]

On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> Even though we can establish a stateless connection, we still cannot
> archive the remote repository using a stateless HTTP protocol. Try the
> following steps to make it work.
> [...]
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> diff --git a/http-backend.c b/http-backend.c
> @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> -       const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
> +       const char *argv[4];
>
> +       if (!strcmp(service_name, "git-upload-archive")) {
> +               argv[1] = ".";
> +               argv[2] = NULL;
> +       } else {
> +               argv[1] = "--stateless-rpc";
> +               argv[2] = ".";
> +               argv[3] = NULL;
> +       }

It may not be worth a reroll, but since you're touching this code
anyhow, these days we'd use `strvec` for this:

    struct strvec argv = STRVEC_INIT;
    if (strcmp(service_name, "git-upload-archive"))
        strvec_push(&argv, "--stateless-rpc");
    strvec_push(&argv, ".");

Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

[Phillip Wood]

On 23/09/2023 16:22, Jiang Xin wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> Even though we can establish a stateless connection, we still cannot
> archive the remote repository using a stateless HTTP protocol. Try the
> following steps to make it work.
> 
>   1. Add support for "git-upload-archive" service in "http-backend".
> 
>   2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
>      protocol version, instead of use the "git-upload-archive" service.
> 
>   3. "git-archive" does not expect to see protocol version and
>      capabilities when connecting to remote-helper, so do not send them
>      in "remote-curl.c" for the "git-upload-archive" service.

I'm not familiar enough with the server side of git to comment on 
whether this patch is a good idea, but I did notice one C language issue 
below.

>   static struct string_list *get_parameters(void)
> @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
>   
>   static void service_rpc(struct strbuf *hdr, char *service_name)
>   {
> -	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};

In the pre-image argv[0] is initialized to NULL

> +	const char *argv[4];

In the post-image argv is not initialized and the first element is not 
set in the code below.

>   	struct rpc_service *svc = select_service(hdr, service_name);
>   	struct strbuf buf = STRBUF_INIT;
>   
> +	if (!strcmp(service_name, "git-upload-archive")) {
> +		argv[1] = ".";
> +		argv[2] = NULL;
> +	} else {
> +		argv[1] = "--stateless-rpc";
> +		argv[2] = ".";
> +		argv[3] = NULL;
> +	}

Best Wishes

Phillip

Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

[Jiang Xin]

On Sun, Sep 24, 2023 at 9:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 23/09/2023 16:22, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Even though we can establish a stateless connection, we still cannot
> > archive the remote repository using a stateless HTTP protocol. Try the
> > following steps to make it work.
> >
> >   1. Add support for "git-upload-archive" service in "http-backend".
> >
> >   2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
> >      protocol version, instead of use the "git-upload-archive" service.
> >
> >   3. "git-archive" does not expect to see protocol version and
> >      capabilities when connecting to remote-helper, so do not send them
> >      in "remote-curl.c" for the "git-upload-archive" service.
>
> I'm not familiar enough with the server side of git to comment on
> whether this patch is a good idea, but I did notice one C language issue
> below.
>
> >   static struct string_list *get_parameters(void)
> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> >
> >   static void service_rpc(struct strbuf *hdr, char *service_name)
> >   {
> > -     const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};

For the original implementation, the first NULL is used as a
placeholder, and will be initialized somewhere below.

> In the pre-image argv[0] is initialized to NULL
>
> > +     const char *argv[4];
>
> In the post-image argv is not initialized and the first element is not
> set in the code below.
>
> >       struct rpc_service *svc = select_service(hdr, service_name);
> >       struct strbuf buf = STRBUF_INIT;
> >
> > +     if (!strcmp(service_name, "git-upload-archive")) {
> > +             argv[1] = ".";
> > +             argv[2] = NULL;
> > +     } else {
> > +             argv[1] = "--stateless-rpc";
> > +             argv[2] = ".";
> > +             argv[3] = NULL;
> > +     }

It will be initialized in the code further below, see http-backend.c:668.

        argv[0] = svc->name;
        run_service(argv, svc->buffer_input);
        strbuf_release(&buf);

Anyway, I will rewrite these code in reroll v3 to follow Eric's suggestion.

> Best Wishes
>
> Phillip
>

Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

[Jiang Xin]

On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> > Even though we can establish a stateless connection, we still cannot
> > archive the remote repository using a stateless HTTP protocol. Try the
> > following steps to make it work.
> > [...]
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > ---
> > diff --git a/http-backend.c b/http-backend.c
> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> > -       const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
> > +       const char *argv[4];
> >
> > +       if (!strcmp(service_name, "git-upload-archive")) {
> > +               argv[1] = ".";
> > +               argv[2] = NULL;
> > +       } else {
> > +               argv[1] = "--stateless-rpc";
> > +               argv[2] = ".";
> > +               argv[3] = NULL;
> > +       }
>
> It may not be worth a reroll, but since you're touching this code
> anyhow, these days we'd use `strvec` for this:
>
>     struct strvec argv = STRVEC_INIT;
>     if (strcmp(service_name, "git-upload-archive"))
>         strvec_push(&argv, "--stateless-rpc");
>     strvec_push(&argv, ".");

Good suggestion, I'll queue this up as part of next reroll.

--
Jiang Xin

RE: [PATCH v2 3/3] archive: support remote archive from stateless transport

[rsbecker]

On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
>On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com>
>wrote:
>>
>> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
>> > Even though we can establish a stateless connection, we still cannot
>> > archive the remote repository using a stateless HTTP protocol. Try
>> > the following steps to make it work.
>> > [...]
>> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> > ---
>> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@
>> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)
>> > -       const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
>> > +       const char *argv[4];
>> >
>> > +       if (!strcmp(service_name, "git-upload-archive")) {
>> > +               argv[1] = ".";
>> > +               argv[2] = NULL;
>> > +       } else {
>> > +               argv[1] = "--stateless-rpc";
>> > +               argv[2] = ".";
>> > +               argv[3] = NULL;
>> > +       }
>>
>> It may not be worth a reroll, but since you're touching this code
>> anyhow, these days we'd use `strvec` for this:
>>
>>     struct strvec argv = STRVEC_INIT;
>>     if (strcmp(service_name, "git-upload-archive"))
>>         strvec_push(&argv, "--stateless-rpc");
>>     strvec_push(&argv, ".");
>
>Good suggestion, I'll queue this up as part of next reroll.

Which test covers this change?

Thanks,
Randall

Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

[Jiang Xin]

On Mon, Sep 25, 2023 at 7:58 AM <rsbecker@nexbridge.com> wrote:
>
> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com>
> >wrote:
> >>
> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> >> > Even though we can establish a stateless connection, we still cannot
> >> > archive the remote repository using a stateless HTTP protocol. Try
> >> > the following steps to make it work.
> >> > [...]
> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >> > ---
> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@
> >> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> >> > -       const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
> >> > +       const char *argv[4];
> >> >
> >> > +       if (!strcmp(service_name, "git-upload-archive")) {
> >> > +               argv[1] = ".";
> >> > +               argv[2] = NULL;
> >> > +       } else {
> >> > +               argv[1] = "--stateless-rpc";
> >> > +               argv[2] = ".";
> >> > +               argv[3] = NULL;
> >> > +       }
> >>
> >> It may not be worth a reroll, but since you're touching this code
> >> anyhow, these days we'd use `strvec` for this:
> >>
> >>     struct strvec argv = STRVEC_INIT;
> >>     if (strcmp(service_name, "git-upload-archive"))
> >>         strvec_push(&argv, "--stateless-rpc");
> >>     strvec_push(&argv, ".");
> >
> >Good suggestion, I'll queue this up as part of next reroll.
>
> Which test covers this change?

See: https://lore.kernel.org/git/20230923152201.14741-4-worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh

> Thanks,
> Randall
>

RE: [PATCH v2 3/3] archive: support remote archive from stateless transport

[rsbecker]

On Sunday, September 24, 2023 8:16 PM, Jiang Xin wrote:
>On Mon, Sep 25, 2023 at 7:58 AM <rsbecker@nexbridge.com> wrote:
>>
>> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
>> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine
>> ><sunshine@sunshineco.com>
>> >wrote:
>> >>
>> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com>
>wrote:
>> >> > Even though we can establish a stateless connection, we still
>> >> > cannot archive the remote repository using a stateless HTTP
>> >> > protocol. Try the following steps to make it work.
>> >> > [...]
>> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> >> > ---
>> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19
>> >> > @@ static void check_content_type(struct strbuf *hdr, const char
>*accepted_type)
>> >> > -       const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
>> >> > +       const char *argv[4];
>> >> >
>> >> > +       if (!strcmp(service_name, "git-upload-archive")) {
>> >> > +               argv[1] = ".";
>> >> > +               argv[2] = NULL;
>> >> > +       } else {
>> >> > +               argv[1] = "--stateless-rpc";
>> >> > +               argv[2] = ".";
>> >> > +               argv[3] = NULL;
>> >> > +       }
>> >>
>> >> It may not be worth a reroll, but since you're touching this code
>> >> anyhow, these days we'd use `strvec` for this:
>> >>
>> >>     struct strvec argv = STRVEC_INIT;
>> >>     if (strcmp(service_name, "git-upload-archive"))
>> >>         strvec_push(&argv, "--stateless-rpc");
>> >>     strvec_push(&argv, ".");
>> >
>> >Good suggestion, I'll queue this up as part of next reroll.
>>
>> Which test covers this change?
>
>See: https://lore.kernel.org/git/20230923152201.14741-4-
>worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh

Thanks. That is what I needed. Looking forward to the merge.
--Randall

Re: [PATCH v2 1/3] transport-helper: no connection restriction in connect_helper

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.

OK.  Given that process_connect_service() does this:

	if (data->connect) {
		strbuf_addf(&cmdbuf, "connect %s\n", name);
		ret = run_connect(transport, &cmdbuf);
	} else if (data->stateless_connect &&
		   (get_protocol_version_config() == protocol_v2) &&
		   (!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)
			transport->stateless_rpc = 1;
	}

in the spirit of the original "safety valve", it becomes tempting to
suggest we make sure at least .connect or .stateless_connect exists
in the transport to be safe, but then we will need to keep the logic
of safety valve in connect_helper() and the actual dispatching in
process_connect_service() in sync, which is a maintenance burden.

It however makes me wonder if we should add

	else
		die(_("operation not supported by protocol"));

at the end of the "if/else if" cascade in process_connect_service(),
so that callers that end up following this callpath with a transport
that defines neither would be caught.

Other than that, the patch is as good as the previous round, and the
explanation is vastly easier to understand.

Thanks.

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> -		die(_("operation not supported by protocol"));
>  
>  	if (!process_connect_service(transport, name, exec))
>  		die(_("can't connect to subservice %s"), name);

Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> "process_connect_service()" in "connect_helper()", run "do_take_over()"
> to replace the old vtable with a new one which has methods ready for
> the smart transport connection.

The existing pattern among all callers of process_connect() seems to
be

	if (process_connect(...)) {
		do_take_over();
		... dispatch to the underlying method ...
	}
	... otherwise implement the fallback ...

where the return value from process_connect() is the return value of
the call it makes to process_connect_service().

And the only other caller of process_connect_service() is
connect_helper(), so in that sense, making a call to do_take_over()
when process_connect_service() succeeds in the helper does make
things consistent.  The connect_helper() function being static, the
helper transport is the only transport that gets affected, but how
has it been working without having this do_take_over() step?  An
obvious related question is if it has been working so far, would it
break if we have do_take_over() added here?


In any case, this makes me wonder if we should do the following
patch to help developers who may want to add new callers to
process_connect_service() by adding calls to process_connect().

 transport-helper.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git c/transport-helper.c w/transport-helper.c
index 91381be622..566f7473df 100644
--- c/transport-helper.c
+++ w/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
 	struct helper_data *data = transport->data;
 	const char *name;
 	const char *exec;
+	int ret;
 
 	name = for_push ? "git-receive-pack" : "git-upload-pack";
 	if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
 	else
 		exec = data->transport_options.uploadpack;
 
-	return process_connect_service(transport, name, exec);
+	ret = process_connect_service(transport, name, exec);
+	if (ret)
+		do_take_over(transport);
+	return ret;
 }
 
 static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
 
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
-	}
 
 	/*
 	 * If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 
-	if (process_connect(transport, 1)) {
-		do_take_over(transport);
+	if (process_connect(transport, 1))
 		return transport->vtable->push_refs(transport, remote_refs, flags);
-	}
 
 	if (!remote_refs) {
 		fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
 {
 	get_helper(transport);
 
-	if (process_connect(transport, for_push)) {
-		do_take_over(transport);
+	if (process_connect(transport, for_push))
 		return transport->vtable->get_refs_list(transport, for_push,
 							transport_options);
-	}
 
 	return get_refs_list_using_list(transport, for_push);
 }
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
 {
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->get_bundle_uri(transport);
-	}
 
 	return -1;
 }

Re: [PATCH v2 0/3] support remote archive from stateless transport

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Enable stateless-rpc connection in "connect_helper()", and add support
> for remote archive from a stateless transport.
> ...

Administrivia.  Please make sure that your patches [1..N/N] appear
as a follow-up to the cover letter [0/N], instead of each of them
being individually a response to somebody else's message.

Thanks.

Re: [PATCH v2 0/3] support remote archive from stateless transport

[Jiang Xin]

On Tue, Sep 26, 2023 at 6:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Enable stateless-rpc connection in "connect_helper()", and add support
> > for remote archive from a stateless transport.
> > ...
>
> Administrivia.  Please make sure that your patches [1..N/N] appear
> as a follow-up to the cover letter [0/N], instead of each of them
> being individually a response to somebody else's message.
>

I will set the "format.thread" configuration variable so that I don't
have to worry about forgetting the "--thread" option when executing
git-format-patch.

        git config --global format.thread shallow

Thanks.

Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper

[Jiang Xin]

On Tue, Sep 26, 2023 at 5:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > After successfully connecting to the smart transport by calling
> > "process_connect_service()" in "connect_helper()", run "do_take_over()"
> > to replace the old vtable with a new one which has methods ready for
> > the smart transport connection.
>
> The existing pattern among all callers of process_connect() seems to
> be
>
>         if (process_connect(...)) {
>                 do_take_over();
>                 ... dispatch to the underlying method ...
>         }
>         ... otherwise implement the fallback ...
>
> where the return value from process_connect() is the return value of
> the call it makes to process_connect_service().
>
> And the only other caller of process_connect_service() is
> connect_helper(), so in that sense, making a call to do_take_over()
> when process_connect_service() succeeds in the helper does make
> things consistent.  The connect_helper() function being static, the
> helper transport is the only transport that gets affected, but how
> has it been working without having this do_take_over() step?  An
> obvious related question is if it has been working so far, would it
> break if we have do_take_over() added here?

The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and we use the function
"transport_connect()" in "transport.c" to call this connect method of
vtable. The only place that we call transport_connect() to setup a
connection is in "builtin/archive.c". So it won't break others if we
add do_take_over() in connect_helper().

In fact, it was not "git archive" that made me discover this issue.
When I implemented a fetch proxy and added a new caller for
transport_connect(), I found that the HTTP protocol didn't work, so I
dug it out.

[PATCH v3 0/4] support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.

range-diff v2...v3

1:  4497404900 = 1:  e660fc79b6 transport-helper: no connection restriction in connect_helper
-:  ---------- > 2:  e3dc18caa9 transport-helper: call do_take_over() in process_connect
2:  9bfaa1a904 ! 3:  01699822c3 transport-helper: run do_take_over in connect_helper
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## Commit message ##
    -    transport-helper: run do_take_over in connect_helper
    +    transport-helper: call do_take_over() in connect_helper
     
         After successfully connecting to the smart transport by calling
    -    "process_connect_service()" in "connect_helper()", run "do_take_over()"
    -    to replace the old vtable with a new one which has methods ready for
    -    the smart transport connection.
    +    process_connect_service() in connect_helper(), run do_take_over() to
    +    replace the old vtable with a new one which has methods ready for the
    +    smart transport connection.
     
    -    The subsequent commit introduces remote archive for a stateless-rpc
    -    connection. But without running "do_take_over()", it may fail to call
    -    "transport_disconnect()" in "run_remote_archiver()" of
    -    "builtin/archive.c". This is because for a stateless connection or a
    -    service like "git-upload-pack-archive", the remote helper may receive a
    -    SIGPIPE signal and exit early. To have a graceful disconnect method by
    -    calling "do_take_over()" will solve this issue.
    +    The connect_helper() function is used as the connect method of the
    +    vtable in "transport-helper.c", and it is called by transport_connect()
    +    in "transport.c" to setup a connection. The only place that we call
    +    transport_connect() so far is in "builtin/archive.c". Without running
    +    do_take_over(), it may fail to call transport_disconnect() in
    +    run_remote_archiver() of "builtin/archive.c". This is because for a
    +    stateless connection or a service like "git-upload-pack-archive", the
    +    remote helper may receive a SIGPIPE signal and exit early. To have a
    +    graceful disconnect method by calling do_take_over() will solve this
    +    issue.
    +
    +    The subsequent commit will introduce remote archive over a stateless-rpc
    +    connection.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
3:  1e305394ee ! 4:  a38ac182d6 archive: support remote archive from stateless transport
    @@ Commit message
             capabilities when connecting to remote-helper, so do not send them
             in "remote-curl.c" for the "git-upload-archive" service.
     
    +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## http-backend.c ##
    @@ http-backend.c: static void check_content_type(struct strbuf *hdr, const char *a
      static void service_rpc(struct strbuf *hdr, char *service_name)
      {
     -	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
    -+	const char *argv[4];
    ++	struct strvec argv = STRVEC_INIT;
      	struct rpc_service *svc = select_service(hdr, service_name);
      	struct strbuf buf = STRBUF_INIT;
      
    -+	if (!strcmp(service_name, "git-upload-archive")) {
    -+		argv[1] = ".";
    -+		argv[2] = NULL;
    -+	} else {
    -+		argv[1] = "--stateless-rpc";
    -+		argv[2] = ".";
    -+		argv[3] = NULL;
    -+	}
    ++	strvec_push(&argv, svc->name);
    ++	if (strcmp(service_name, "git-upload-archive"))
    ++		strvec_push(&argv, "--stateless-rpc");
    ++	strvec_push(&argv, ".");
     +
      	strbuf_reset(&buf);
      	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
      	check_content_type(hdr, buf.buf);
    +@@ http-backend.c: static void service_rpc(struct strbuf *hdr, char *service_name)
    + 
    + 	end_headers(hdr);
    + 
    +-	argv[0] = svc->name;
    +-	run_service(argv, svc->buffer_input);
    ++	run_service(argv.v, svc->buffer_input);
    + 	strbuf_release(&buf);
    ++	strvec_clear(&argv);
    + }
    + 
    + static int dead;
     @@ http-backend.c: static struct service_cmd {
      	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
      
---

Jiang Xin (4):
  transport-helper: no connection restriction in connect_helper
  transport-helper: call do_take_over() in process_connect
  transport-helper: call do_take_over() in connect_helper
  archive: support remote archive from stateless transport

 http-backend.c         | 15 +++++++++++----
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
 transport-helper.c     | 29 +++++++++++++----------------
 4 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v3 1/4] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which did not work with such a transport.

Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol-v2, which
process_connect_service() was taught to handle the "stateless"
connection, making the old safety valve in its caller that insisted
that ".connect" method must be defined too strict, and forgot to loosen
it.

Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v3 2/4] transport-helper: call do_take_over() in process_connect

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The existing pattern among all callers of process_connect() seems to be

        if (process_connect(...)) {
                do_take_over();
                ... dispatch to the underlying method ...
        }
        ... otherwise implement the fallback ...

where the return value from process_connect() is the return value of the
call it makes to process_connect_service().

It is safe to make a refactor by moving the call of do_take_over()
into the function process_connect().

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..51088cc03a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -645,6 +645,7 @@ static int process_connect(struct transport *transport,
 	struct helper_data *data = transport->data;
 	const char *name;
 	const char *exec;
+	int ret;
 
 	name = for_push ? "git-receive-pack" : "git-upload-pack";
 	if (for_push)
@@ -652,7 +653,10 @@ static int process_connect(struct transport *transport,
 	else
 		exec = data->transport_options.uploadpack;
 
-	return process_connect_service(transport, name, exec);
+	ret = process_connect_service(transport, name, exec);
+	if (ret)
+		do_take_over(transport);
+	return ret;
 }
 
 static int connect_helper(struct transport *transport, const char *name,
@@ -682,10 +686,8 @@ static int fetch_refs(struct transport *transport,
 
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
-	}
 
 	/*
 	 * If we reach here, then the server, the client, and/or the transport
@@ -1142,10 +1144,8 @@ static int push_refs(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 
-	if (process_connect(transport, 1)) {
-		do_take_over(transport);
+	if (process_connect(transport, 1))
 		return transport->vtable->push_refs(transport, remote_refs, flags);
-	}
 
 	if (!remote_refs) {
 		fprintf(stderr,
@@ -1186,11 +1186,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
 {
 	get_helper(transport);
 
-	if (process_connect(transport, for_push)) {
-		do_take_over(transport);
+	if (process_connect(transport, for_push))
 		return transport->vtable->get_refs_list(transport, for_push,
 							transport_options);
-	}
 
 	return get_refs_list_using_list(transport, for_push);
 }
@@ -1274,10 +1272,8 @@ static int get_bundle_uri(struct transport *transport)
 {
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->get_bundle_uri(transport);
-	}
 
 	return -1;
 }
-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v3 3/4] transport-helper: call do_take_over() in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection.

The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection or a service like "git-upload-pack-archive", the
remote helper may receive a SIGPIPE signal and exit early. To have a
graceful disconnect method by calling do_take_over() will solve this
issue.

The subsequent commit will introduce remote archive over a stateless-rpc
connection.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index 51088cc03a..3b036ae1ca 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.40.1.50.gf560bcc116.dirty

[PATCH v3 4/4] archive: support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Even though we can establish a stateless connection, we still cannot
archive the remote repository using a stateless HTTP protocol. Try the
following steps to make it work.

 1. Add support for "git-upload-archive" service in "http-backend".

 2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
    protocol version, instead of use the "git-upload-archive" service.

 3. "git-archive" does not expect to see protocol version and
    capabilities when connecting to remote-helper, so do not send them
    in "remote-curl.c" for the "git-upload-archive" service.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 15 +++++++++++----
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
 transport-helper.c     |  3 ++-
 4 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..6a2c919839 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	struct strvec argv = STRVEC_INIT;
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	strvec_push(&argv, svc->name);
+	if (strcmp(service_name, "git-upload-archive"))
+		strvec_push(&argv, "--stateless-rpc");
+	strvec_push(&argv, ".");
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
 
 	end_headers(hdr);
 
-	argv[0] = svc->name;
-	run_service(argv, svc->buffer_input);
+	run_service(argv.v, svc->buffer_input);
 	strbuf_release(&buf);
+	strvec_clear(&argv);
 }
 
 static int dead;
@@ -723,7 +729,8 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.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/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..80123c1e06 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,34 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_when_finished "rm -f d5.zip" &&
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	test_when_finished "rm -f d5.zip" &&
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=d5.zip HEAD &&
+	test_cmp_bin d.zip d5.zip
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 3b036ae1ca..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
-- 
2.40.1.50.gf560bcc116.dirty

Re: [PATCH v3 2/4] transport-helper: call do_take_over() in process_connect

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> It is safe to make a refactor by moving the call of do_take_over()
> into the function process_connect().

"It is safe" only explains why it does not hurt, and does not
explain why it is a good idea to do so, though.

[PATCH v4 0/4] support remote archive via stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

# Change since v3:

1. Update commit message of patch 2/4.
2. Add comments in t5003.

# range-diff v3...v4

1:  1818d8e30e = 1:  d343585cb5 transport-helper: no connection restriction in connect_helper
2:  b57524bc91 ! 2:  65fb67523c transport-helper: call do_take_over() in process_connect
    @@ Commit message
         where the return value from process_connect() is the return value of the
         call it makes to process_connect_service().
     
    -    It is safe to make a refactor by moving the call of do_take_over()
    -    into the function process_connect().
    +    Move the call of do_take_over() inside process_connect(), so that
    +    calling the process_connect() function is more concise and will not
    +    miss do_take_over().
     
         Suggested-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
3:  7ce60e3b9a = 3:  109a1fffde transport-helper: call do_take_over() in connect_helper
4:  626f903508 ! 4:  eb905259fe archive: support remote archive from stateless transport
    @@ Commit message
     
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    @@ t/t5003-archive-zip.sh: check_zip with_untracked2
      check_added with_untracked2 untracked one/untracked
      check_added with_untracked2 untracked two/untracked
      
    ++# Test remote archive over HTTP protocol.
    ++#
    ++# Note: this should be the last part of this test suite, because
    ++# by including lib-httpd.sh, the test may end early if httpd tests
    ++# should not be run.
    ++#
     +. "$TEST_DIRECTORY"/lib-httpd.sh
     +start_httpd
     +
    @@ t/t5003-archive-zip.sh: check_zip with_untracked2
     +setup_askpass_helper
     +
     +test_expect_success 'remote archive does not work with protocol v1' '
    -+	test_when_finished "rm -f d5.zip" &&
     +	test_must_fail git -c protocol.version=1 archive \
     +		--remote="$HTTPD_URL/auth/smart/bare.git" \
    -+		--output=d5.zip HEAD >actual 2>&1 &&
    ++		--output=remote-http.zip HEAD >actual 2>&1 &&
     +	cat >expect <<-EOF &&
     +	fatal: can${SQ}t connect to subservice git-upload-archive
     +	EOF
    @@ t/t5003-archive-zip.sh: check_zip with_untracked2
     +'
     +
     +test_expect_success 'archive remote http repository' '
    -+	test_when_finished "rm -f d5.zip" &&
     +	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
    -+		--output=d5.zip HEAD &&
    -+	test_cmp_bin d.zip d5.zip
    ++		--output=remote-http.zip HEAD &&
    ++	test_cmp_bin d.zip remote-http.zip
     +'
     +
      test_done

Jiang Xin (4):
  transport-helper: no connection restriction in connect_helper
  transport-helper: call do_take_over() in process_connect
  transport-helper: call do_take_over() in connect_helper
  archive: support remote archive from stateless transport

 http-backend.c         | 15 +++++++++++----
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 transport-helper.c     | 29 +++++++++++++----------------
 4 files changed, 69 insertions(+), 23 deletions(-)

-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which did not work with such a transport.

Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol-v2, which
process_connect_service() was taught to handle the "stateless"
connection, making the old safety valve in its caller that insisted
that ".connect" method must be defined too strict, and forgot to loosen
it.

Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v4 2/4] transport-helper: call do_take_over() in process_connect

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The existing pattern among all callers of process_connect() seems to be

        if (process_connect(...)) {
                do_take_over();
                ... dispatch to the underlying method ...
        }
        ... otherwise implement the fallback ...

where the return value from process_connect() is the return value of the
call it makes to process_connect_service().

Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..51088cc03a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -645,6 +645,7 @@ static int process_connect(struct transport *transport,
 	struct helper_data *data = transport->data;
 	const char *name;
 	const char *exec;
+	int ret;
 
 	name = for_push ? "git-receive-pack" : "git-upload-pack";
 	if (for_push)
@@ -652,7 +653,10 @@ static int process_connect(struct transport *transport,
 	else
 		exec = data->transport_options.uploadpack;
 
-	return process_connect_service(transport, name, exec);
+	ret = process_connect_service(transport, name, exec);
+	if (ret)
+		do_take_over(transport);
+	return ret;
 }
 
 static int connect_helper(struct transport *transport, const char *name,
@@ -682,10 +686,8 @@ static int fetch_refs(struct transport *transport,
 
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
-	}
 
 	/*
 	 * If we reach here, then the server, the client, and/or the transport
@@ -1142,10 +1144,8 @@ static int push_refs(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 
-	if (process_connect(transport, 1)) {
-		do_take_over(transport);
+	if (process_connect(transport, 1))
 		return transport->vtable->push_refs(transport, remote_refs, flags);
-	}
 
 	if (!remote_refs) {
 		fprintf(stderr,
@@ -1186,11 +1186,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
 {
 	get_helper(transport);
 
-	if (process_connect(transport, for_push)) {
-		do_take_over(transport);
+	if (process_connect(transport, for_push))
 		return transport->vtable->get_refs_list(transport, for_push,
 							transport_options);
-	}
 
 	return get_refs_list_using_list(transport, for_push);
 }
@@ -1274,10 +1272,8 @@ static int get_bundle_uri(struct transport *transport)
 {
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->get_bundle_uri(transport);
-	}
 
 	return -1;
 }
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection.

The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection or a service like "git-upload-pack-archive", the
remote helper may receive a SIGPIPE signal and exit early. To have a
graceful disconnect method by calling do_take_over() will solve this
issue.

The subsequent commit will introduce remote archive over a stateless-rpc
connection.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index 51088cc03a..3b036ae1ca 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

[PATCH v4 4/4] archive: support remote archive from stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Even though we can establish a stateless connection, we still cannot
archive the remote repository using a stateless HTTP protocol. Try the
following steps to make it work.

 1. Add support for "git-upload-archive" service in "http-backend".

 2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
    protocol version, instead of use the "git-upload-archive" service.

 3. "git-archive" does not expect to see protocol version and
    capabilities when connecting to remote-helper, so do not send them
    in "remote-curl.c" for the "git-upload-archive" service.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 15 +++++++++++----
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 transport-helper.c     |  3 ++-
 4 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..6a2c919839 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	struct strvec argv = STRVEC_INIT;
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	strvec_push(&argv, svc->name);
+	if (strcmp(service_name, "git-upload-archive"))
+		strvec_push(&argv, "--stateless-rpc");
+	strvec_push(&argv, ".");
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
 
 	end_headers(hdr);
 
-	argv[0] = svc->name;
-	run_service(argv, svc->buffer_input);
+	run_service(argv.v, svc->buffer_input);
 	strbuf_release(&buf);
+	strvec_clear(&argv);
 }
 
 static int dead;
@@ -723,7 +729,8 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.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/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..961c6aac25 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,38 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+# Test remote archive over HTTP protocol.
+#
+# Note: this should be the last part of this test suite, because
+# by including lib-httpd.sh, the test may end early if httpd tests
+# should not be run.
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD &&
+	test_cmp_bin d.zip remote-http.zip
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 3b036ae1ca..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
-- 
2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> When commit b236752a (Support remote archive from all smart transports,
> 2009-12-09) added "remote archive" support for "smart transports", it
> was for transport that supports the ".connect" method. The
> "connect_helper()" function protected itself from getting called for a
> transport without the method before calling process_connect_service(),

OK.

> which did not work with such a transport.

How about 'which only worked with the ".connect" method.' ?

>
> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
> 2018-03-15) added a way for a transport without the ".connect" method
> to establish a "stateless" connection in protocol-v2, which

s/which/where

> process_connect_service() was taught to handle the "stateless"
> connection,

I think using 'the ".stateless_connect" method' is more consistent with
the rest of this text.

> making the old safety valve in its caller that insisted
> that ".connect" method must be defined too strict, and forgot to loosen
> it.

I think just "...making the old protection too strict. But edc9caf7
forgot to adjust this protection accordingly." is simpler to read.

> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> -		die(_("operation not supported by protocol"));

Should we still terminate early here if both data->connect and
data->stateless_connect are not truthy? It would save a few CPU cycles,
but even better, remain true to the the original intent of the code.
Maybe there was a really good reason to terminate early here that we're
not aware of?

But also, what about the case where both are enabled? Should we print an
error message? (Maybe this concern is outside the scope of this series?)

>  	if (!process_connect_service(transport, name, exec))
>  		die(_("can't connect to subservice %s"), name);
> -- 
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> process_connect_service() in connect_helper(), run do_take_over() to
> replace the old vtable with a new one which has methods ready for the
> smart transport connection.
> 
>
> The connect_helper() function is used as the connect method of the
> vtable in "transport-helper.c", and it is called by transport_connect()
> in "transport.c" to setup a connection. The only place that we call
> transport_connect() so far is in "builtin/archive.c". Without running
> do_take_over(), it may fail to call transport_disconnect() in
> run_remote_archiver() of "builtin/archive.c". This is because for a
> stateless connection or a service like "git-upload-pack-archive", the

There is "git-upload-pack" and "git-upload-archive". Which one did you
mean here? Or did you mean both?

> remote helper may receive a SIGPIPE signal and exit early. To have a
> graceful disconnect method by calling do_take_over() will solve this
> issue.

Are you saying that this patch fixes an existing bug? That is, is this
patch independent of the first patch (transport-helper: no connection
restriction in connect_helper) in this series?

> The subsequent commit will introduce remote archive over a stateless-rpc
> connection.

Does the next commit depend on this patch? If not, I think you can drop
this paragraph.

> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 51088cc03a..3b036ae1ca 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	fd[0] = data->helper->out;
>  	fd[1] = data->helper->in;
> +
> +	do_take_over(transport);
>  	return 0;
>  }
>  
> -- 
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Re: [PATCH v4 4/4] archive: support remote archive from stateless transport

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Even though we can establish a stateless connection, we still cannot
> archive the remote repository using a stateless HTTP protocol. Try the
> following steps to make it work.

As Yoda once said, "Do or do not, there is no try". Here I think you
meant "Do".

>  1. Add support for "git-upload-archive" service in "http-backend".
>
>  2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
>     protocol version, instead of use the "git-upload-archive" service.
>
>  3. "git-archive" does not expect to see protocol version and
>     capabilities when connecting to remote-helper, so do not send them
>     in "remote-curl.c" for the "git-upload-archive" service.

It would be great if you could break up this patch into 3 smaller
patches. Or 4 patches if you decide to move the new test cases into their
own patch.

> @@ -723,7 +729,8 @@ static struct service_cmd {
>  	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.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}
>  };

Style nit: it might be cleaner to put the new "git-upload-archive" just
above "git-upload-pack" because the two have a special relationship now.

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Junio C Hamano]

Linus Arver <linusa@google.com> writes:

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>>
>> When commit b236752a (Support remote archive from all smart transports,
>> 2009-12-09) added "remote archive" support for "smart transports", it
>> was for transport that supports the ".connect" method. The
>> "connect_helper()" function protected itself from getting called for a
>> transport without the method before calling process_connect_service(),
>
> OK.
>
>> which did not work with such a transport.
>
> How about 'which only worked with the ".connect" method.' ?
>
>>
>> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
>> 2018-03-15) added a way for a transport without the ".connect" method
>> to establish a "stateless" connection in protocol-v2, which
>
> s/which/where
>
>> process_connect_service() was taught to handle the "stateless"
>> connection,
>
> I think using 'the ".stateless_connect" method' is more consistent with
> the rest of this text.
>
>> making the old safety valve in its caller that insisted
>> that ".connect" method must be defined too strict, and forgot to loosen
>> it.
>
> I think just "...making the old protection too strict. But edc9caf7
> forgot to adjust this protection accordingly." is simpler to read.
>
>> Remove the restriction in the "connect_helper()" function and give the
>> function "process_connect_service()" the opportunity to establish a
>> connection using ".connect" or ".stateless_connect" for protocol v2. So
>> we can connect with a stateless-rpc and do something useful. E.g., in a
>> later commit, implements remote archive for a repository over HTTP
>> protocol.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> ---
>>  transport-helper.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 49811ef176..2e127d24a5 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>>  
>>  	/* Get_helper so connect is inited. */
>>  	get_helper(transport);
>> -	if (!data->connect)
>> -		die(_("operation not supported by protocol"));
>
> Should we still terminate early here if both data->connect and
> data->stateless_connect are not truthy? It would save a few CPU cycles,
> but even better, remain true to the the original intent of the code.
> Maybe there was a really good reason to terminate early here that we're
> not aware of?
>
> But also, what about the case where both are enabled? Should we print an
> error message? (Maybe this concern is outside the scope of this series?)
>
>>  	if (!process_connect_service(transport, name, exec))
>>  		die(_("can't connect to subservice %s"), name);
>> -- 
>> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Thanks for a review to get the topic that hasn't seen much reviews
unstuck.  Very much appreciated.

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

On Fri, Jan 12, 2024 at 3:42 PM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > When commit b236752a (Support remote archive from all smart transports,
> > 2009-12-09) added "remote archive" support for "smart transports", it
> > was for transport that supports the ".connect" method. The
> > "connect_helper()" function protected itself from getting called for a
> > transport without the method before calling process_connect_service(),
>
> OK.
>
> > which did not work with such a transport.
>
> How about 'which only worked with the ".connect" method.' ?
>
> >
> > Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
> > 2018-03-15) added a way for a transport without the ".connect" method
> > to establish a "stateless" connection in protocol-v2, which
>
> s/which/where
>
> > process_connect_service() was taught to handle the "stateless"
> > connection,
>
> I think using 'the ".stateless_connect" method' is more consistent with
> the rest of this text.
>
> > making the old safety valve in its caller that insisted
> > that ".connect" method must be defined too strict, and forgot to loosen
> > it.
>
> I think just "...making the old protection too strict. But edc9caf7
> forgot to adjust this protection accordingly." is simpler to read.

Thanks for the above suggestions, and will update in next reroll.

> > Remove the restriction in the "connect_helper()" function and give the
> > function "process_connect_service()" the opportunity to establish a
> > connection using ".connect" or ".stateless_connect" for protocol v2. So
> > we can connect with a stateless-rpc and do something useful. E.g., in a
> > later commit, implements remote archive for a repository over HTTP
> > protocol.
> >
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > ---
> >  transport-helper.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 49811ef176..2e127d24a5 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
> >
> >       /* Get_helper so connect is inited. */
> >       get_helper(transport);
> > -     if (!data->connect)
> > -             die(_("operation not supported by protocol"));
>
> Should we still terminate early here if both data->connect and
> data->stateless_connect are not truthy? It would save a few CPU cycles,
> but even better, remain true to the the original intent of the code.
> Maybe there was a really good reason to terminate early here that we're
> not aware of?
>

It's not necessary to check data->connect here, because it will
terminate if fail to connect by calling the function
"process_connect_service()".

> But also, what about the case where both are enabled? Should we print an
> error message? (Maybe this concern is outside the scope of this series?)

In the function "process_connect_service()", we can see that "connect"
has a higher priority than "stateless-connect".

>
> >       if (!process_connect_service(transport, name, exec))
> >               die(_("can't connect to subservice %s"), name);

Regardless of whether "connect" or "stateless-connect" is used, the
function process_connect_service() will return 1 if the connection is
successful. If the connection fails, it will terminate here.

Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper

[Jiang Xin]

On Fri, Jan 12, 2024 at 3:56 PM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > After successfully connecting to the smart transport by calling
> > process_connect_service() in connect_helper(), run do_take_over() to
> > replace the old vtable with a new one which has methods ready for the
> > smart transport connection.
> >
> >
> > The connect_helper() function is used as the connect method of the
> > vtable in "transport-helper.c", and it is called by transport_connect()
> > in "transport.c" to setup a connection. The only place that we call
> > transport_connect() so far is in "builtin/archive.c". Without running
> > do_take_over(), it may fail to call transport_disconnect() in
> > run_remote_archiver() of "builtin/archive.c". This is because for a
> > stateless connection or a service like "git-upload-pack-archive", the
>
> There is "git-upload-pack" and "git-upload-archive". Which one did you
> mean here? Or did you mean both?
>

Should be "git-upload-archive".

> > remote helper may receive a SIGPIPE signal and exit early. To have a
> > graceful disconnect method by calling do_take_over() will solve this
> > issue.
>
> Are you saying that this patch fixes an existing bug? That is, is this
> patch independent of the first patch (transport-helper: no connection
> restriction in connect_helper) in this series?
>
> > The subsequent commit will introduce remote archive over a stateless-rpc
> > connection.
>
> Does the next commit depend on this patch? If not, I think you can drop
> this paragraph.

One test case in next commit will break without this patch. I will
move this patch to the end of this series.

[PATCH v5 0/6] support remote archive via stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.

# Changes since v4

1. Change commit messages and order of commits.
2. Split the last commit of v4 into three seperate commits.


# range-diff v4...v5

1:  da80391037 ! 1:  f3fef46c05 transport-helper: no connection restriction in connect_helper
    @@ Commit message
         was for transport that supports the ".connect" method. The
         "connect_helper()" function protected itself from getting called for a
         transport without the method before calling process_connect_service(),
    -    which did not work with such a transport.
    +    which only worked with the ".connect" method.
     
         Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
         2018-03-15) added a way for a transport without the ".connect" method
    -    to establish a "stateless" connection in protocol-v2, which
    -    process_connect_service() was taught to handle the "stateless"
    -    connection, making the old safety valve in its caller that insisted
    -    that ".connect" method must be defined too strict, and forgot to loosen
    -    it.
    +    to establish a "stateless" connection in protocol-v2, where
    +    process_connect_service() was taught to handle the ".stateless_connect"
    +    method, making the old protection too strict. But commit edc9caf7 forgot
    +    to adjust this protection accordingly.
     
         Remove the restriction in the "connect_helper()" function and give the
         function "process_connect_service()" the opportunity to establish a
    @@ Commit message
         protocol.
     
         Helped-by: Junio C Hamano <gitster@pobox.com>
    +    Helped-by: Linus Arver <linusa@google.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## transport-helper.c ##
-:  ---------- > 2:  6be331b22d remote-curl: supports git-upload-archive service
-:  ---------- > 3:  aabc8e1a2a transport-helper: protocol-v2 supports upload-archive
4:  a21a80dae9 ! 4:  fdab4abb43 archive: support remote archive from stateless transport
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## Commit message ##
    -    archive: support remote archive from stateless transport
    +    http-backend: new rpc-service for git-upload-archive
     
    -    Even though we can establish a stateless connection, we still cannot
    -    archive the remote repository using a stateless HTTP protocol. Try the
    -    following steps to make it work.
    +    Add new rpc-service "upload-archive" in http-backend to add server side
    +    support for remote archive over HTTP/HTTPS protocols.
     
    -     1. Add support for "git-upload-archive" service in "http-backend".
    -
    -     2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
    -        protocol version, instead of use the "git-upload-archive" service.
    -
    -     3. "git-archive" does not expect to see protocol version and
    -        capabilities when connecting to remote-helper, so do not send them
    -        in "remote-curl.c" for the "git-upload-archive" service.
    +    Also add new test cases in t5003. In the test case "archive remote http
    +    repository", git-archive exits with a non-0 exit code even though we
    +    create the archive correctly. It will be fixed in a later commit.
     
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
 		if (ret)

    ... (omitted) ...

3:  870dc5fd21 ! 5:  6ac0c8e105 transport-helper: call do_take_over() in connect_helper
    @@ Commit message
         After successfully connecting to the smart transport by calling
         process_connect_service() in connect_helper(), run do_take_over() to
         replace the old vtable with a new one which has methods ready for the
    -    smart transport connection.
    +    smart transport connection. This will fix the exit code of git-archive
    +    in test case "archive remote http repository" of t5003.
     
         The connect_helper() function is used as the connect method of the
         vtable in "transport-helper.c", and it is called by transport_connect()
    @@ Commit message
         transport_connect() so far is in "builtin/archive.c". Without running
         do_take_over(), it may fail to call transport_disconnect() in
         run_remote_archiver() of "builtin/archive.c". This is because for a
    -    stateless connection or a service like "git-upload-pack-archive", the
    +    stateless connection and a service like "git-upload-archive", the
         remote helper may receive a SIGPIPE signal and exit early. To have a
         graceful disconnect method by calling do_take_over() will solve this
         issue.
     
    -    The subsequent commit will introduce remote archive over a stateless-rpc
    -    connection.
    -
    +    Helped-by: Linus Arver <linusa@google.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
    + ## t/t5003-archive-zip.sh ##
    +@@ t/t5003-archive-zip.sh: test_expect_success 'remote archive does not work with protocol v1' '
    + '
    + 
    + test_expect_success 'archive remote http repository' '
    +-	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
    ++	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
    + 		--output=remote-http.zip HEAD &&
    + 	test_cmp_bin d.zip remote-http.zip
    + '
    +
      ## transport-helper.c ##
     @@ transport-helper.c: static int connect_helper(struct transport *transport, const char *name,
      
2:  2f7060f7c5 = 6:  423a89c593 transport-helper: call do_take_over() in process_connect

Jiang Xin (6):
  transport-helper: no connection restriction in connect_helper
  remote-curl: supports git-upload-archive service
  transport-helper: protocol-v2 supports upload-archive
  http-backend: new rpc-service for git-upload-archive
  transport-helper: call do_take_over() in connect_helper
  transport-helper: call do_take_over() in process_connect

 http-backend.c         | 13 ++++++++++---
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 transport-helper.c     | 29 +++++++++++++----------------
 4 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH v5 1/6] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which only worked with the ".connect" method.

Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol-v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
to adjust this protection accordingly.

Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.43.0

[PATCH v5 2/6] remote-curl: supports git-upload-archive service

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
git-upload-archive and other serices:

 1. The git-archive command does not expect to see protocol version and
    capabilities when connecting to remote-helper, so do not send them
    in remote-curl for the git-upload-archive service.

 2. We need to detect protocol version by calling discover_refs(),
    Fallback to use the git-upload-pack service (which, like
    git-upload-archive, is a read-only operation) to discover protocol
    version.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 remote-curl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
-- 
2.43.0

[PATCH v5 3/6] transport-helper: protocol-v2 supports upload-archive

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

We used to support only git-upload-pack service for protocol-v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
service support for git-upload-archive in protocol-v2.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..6fe9f4f208 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
-- 
2.43.0

[PATCH v5 4/6] http-backend: new rpc-service for git-upload-archive

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Add new rpc-service "upload-archive" in http-backend to add server side
support for remote archive over HTTP/HTTPS protocols.

Also add new test cases in t5003. In the test case "archive remote http
repository", git-archive exits with a non-0 exit code even though we
create the archive correctly. It will be fixed in a later commit.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 13 ++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..1ed1e29d07 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	struct strvec argv = STRVEC_INIT;
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	strvec_push(&argv, svc->name);
+	if (strcmp(service_name, "git-upload-archive"))
+		strvec_push(&argv, "--stateless-rpc");
+	strvec_push(&argv, ".");
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
 
 	end_headers(hdr);
 
-	argv[0] = svc->name;
-	run_service(argv, svc->buffer_input);
+	run_service(argv.v, svc->buffer_input);
 	strbuf_release(&buf);
+	strvec_clear(&argv);
 }
 
 static int dead;
@@ -723,6 +729,7 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
 
 	{"POST", "/git-upload-pack$", service_rpc},
+	{"POST", "/git-upload-archive$", service_rpc},
 	{"POST", "/git-receive-pack$", service_rpc}
 };
 
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..6f85bd3463 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,38 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+# Test remote archive over HTTP protocol.
+#
+# Note: this should be the last part of this test suite, because
+# by including lib-httpd.sh, the test may end early if httpd tests
+# should not be run.
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD &&
+	test_cmp_bin d.zip remote-http.zip
+'
+
 test_done
-- 
2.43.0

[PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection. This will fix the exit code of git-archive
in test case "archive remote http repository" of t5003.

The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection and a service like "git-upload-archive", the
remote helper may receive a SIGPIPE signal and exit early. To have a
graceful disconnect method by calling do_take_over() will solve this
issue.

Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5003-archive-zip.sh | 2 +-
 transport-helper.c     | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 6f85bd3463..961c6aac25 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
 '
 
 test_expect_success 'archive remote http repository' '
-	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
 		--output=remote-http.zip HEAD &&
 	test_cmp_bin d.zip remote-http.zip
 '
diff --git a/transport-helper.c b/transport-helper.c
index 6fe9f4f208..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.43.0

[PATCH v5 6/6] transport-helper: call do_take_over() in process_connect

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The existing pattern among all callers of process_connect() seems to be

        if (process_connect(...)) {
                do_take_over();
                ... dispatch to the underlying method ...
        }
        ... otherwise implement the fallback ...

where the return value from process_connect() is the return value of the
call it makes to process_connect_service().

Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 91381be622..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
 	struct helper_data *data = transport->data;
 	const char *name;
 	const char *exec;
+	int ret;
 
 	name = for_push ? "git-receive-pack" : "git-upload-pack";
 	if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
 	else
 		exec = data->transport_options.uploadpack;
 
-	return process_connect_service(transport, name, exec);
+	ret = process_connect_service(transport, name, exec);
+	if (ret)
+		do_take_over(transport);
+	return ret;
 }
 
 static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
 
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
-	}
 
 	/*
 	 * If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 
-	if (process_connect(transport, 1)) {
-		do_take_over(transport);
+	if (process_connect(transport, 1))
 		return transport->vtable->push_refs(transport, remote_refs, flags);
-	}
 
 	if (!remote_refs) {
 		fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
 {
 	get_helper(transport);
 
-	if (process_connect(transport, for_push)) {
-		do_take_over(transport);
+	if (process_connect(transport, for_push))
 		return transport->vtable->get_refs_list(transport, for_push,
 							transport_options);
-	}
 
 	return get_refs_list_using_list(transport, for_push);
 }
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
 {
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->get_bundle_uri(transport);
-	}
 
 	return -1;
 }
-- 
2.43.0

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

>> > Remove the restriction in the "connect_helper()" function and give the
>> > function "process_connect_service()" the opportunity to establish a
>> > connection using ".connect" or ".stateless_connect" for protocol v2. So
>> > we can connect with a stateless-rpc and do something useful. E.g., in a
>> > later commit, implements remote archive for a repository over HTTP
>> > protocol.
>> >
>> > Helped-by: Junio C Hamano <gitster@pobox.com>
>> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> > ---
>> >  transport-helper.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> >
>> > diff --git a/transport-helper.c b/transport-helper.c
>> > index 49811ef176..2e127d24a5 100644
>> > --- a/transport-helper.c
>> > +++ b/transport-helper.c
>> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>> >
>> >       /* Get_helper so connect is inited. */
>> >       get_helper(transport);
>> > -     if (!data->connect)
>> > -             die(_("operation not supported by protocol"));
>>
>> Should we still terminate early here if both data->connect and
>> data->stateless_connect are not truthy? It would save a few CPU cycles,
>> but even better, remain true to the the original intent of the code.
>> Maybe there was a really good reason to terminate early here that we're
>> not aware of?
>>
>
> It's not necessary to check data->connect here, because it will
> terminate if fail to connect by calling the function
> "process_connect_service()".

In the process_connect_service() we have

    if (data->connect) {
       ...
    } else if (data->stateless_connect && ...) {
       ...
    }

    strbuf_release(&cmdbuf);
    return ret;

and so if both data->connect and data->stateless_connect are false, that
function could silently do nothing. IOW that function expects the
connection type to be guaranteed to be set, so it makes sense to check
for the correctness of this in the connect_helper().

>> But also, what about the case where both are enabled? Should we print an
>> error message? (Maybe this concern is outside the scope of this series?)
>
> In the function "process_connect_service()", we can see that "connect"
> has a higher priority than "stateless-connect".

What I mean is, does it make sense for connect_helper() to recognize
invalid or possibly buggy states? IOW, is having both data->connect and
data->stateless_connect enabled a bug? If we only ever set one or the
other (we treat them as mutually exclusive) elsewhere in the codebase,
and if we are doing the sort of "correctness" check in the
connect_helper(), then it makes sense to detect that both are set and
print an error or warning (as a programmer bug).

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

On Fri, Jan 19, 2024 at 6:26 AM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> >> > Remove the restriction in the "connect_helper()" function and give the
> >> > function "process_connect_service()" the opportunity to establish a
> >> > connection using ".connect" or ".stateless_connect" for protocol v2. So
> >> > we can connect with a stateless-rpc and do something useful. E.g., in a
> >> > later commit, implements remote archive for a repository over HTTP
> >> > protocol.
> >> >
> >> > Helped-by: Junio C Hamano <gitster@pobox.com>
> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >> > ---
> >> >  transport-helper.c | 2 --
> >> >  1 file changed, 2 deletions(-)
> >> >
> >> > diff --git a/transport-helper.c b/transport-helper.c
> >> > index 49811ef176..2e127d24a5 100644
> >> > --- a/transport-helper.c
> >> > +++ b/transport-helper.c
> >> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
> >> >
> >> >       /* Get_helper so connect is inited. */
> >> >       get_helper(transport);
> >> > -     if (!data->connect)
> >> > -             die(_("operation not supported by protocol"));
> >>
> >> Should we still terminate early here if both data->connect and
> >> data->stateless_connect are not truthy? It would save a few CPU cycles,
> >> but even better, remain true to the the original intent of the code.
> >> Maybe there was a really good reason to terminate early here that we're
> >> not aware of?
> >>
> >
> > It's not necessary to check data->connect here, because it will
> > terminate if fail to connect by calling the function
> > "process_connect_service()".
>
> In the process_connect_service() we have
>
>     if (data->connect) {
>        ...
>     } else if (data->stateless_connect && ...) {
>        ...
>     }
>
>     strbuf_release(&cmdbuf);
>     return ret;
>
> and so if both data->connect and data->stateless_connect are false, that
> function could silently do nothing. IOW that function expects the
> connection type to be guaranteed to be set, so it makes sense to check
> for the correctness of this in the connect_helper().

If both data->connect and data->stateless_connect are false,
process_connect_service() will return 0 instead of making a connection
and returning 1. The return value will be checked in the function
connect_helper() as follows:

        if (!process_connect_service(transport, name, exec))
                die(_("can't connect to subservice %s"), name);

So I think it's not necessary to make double check in connect_helper().

>
> >> But also, what about the case where both are enabled? Should we print an
> >> error message? (Maybe this concern is outside the scope of this series?)
> >
> > In the function "process_connect_service()", we can see that "connect"
> > has a higher priority than "stateless-connect".
>
> What I mean is, does it make sense for connect_helper() to recognize
> invalid or possibly buggy states? IOW, is having both data->connect and
> data->stateless_connect enabled a bug? If we only ever set one or the
> other (we treat them as mutually exclusive) elsewhere in the codebase,
> and if we are doing the sort of "correctness" check in the
> connect_helper(), then it makes sense to detect that both are set and
> print an error or warning (as a programmer bug).

The best position to address the bug that both data->connect and
data->stateless_connect are enabled is in the function get_helper() as
below:

        } else if (!strcmp(capname, "connect")) {
                data->connect = 1;
        } else if (!strcmp(capname, "stateless-connect")) {
                data->stateless_connect = 1;
        }
        ... ...
        if (data->connect && data->stateless_connect)
                die("cannot have both connect and stateless_connect enabled");

I consider this change to be off-topic and it will not be introduced
in this series.

--
Jiang Xin

Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> If both data->connect and data->stateless_connect are false,
> process_connect_service() will return 0 instead of making a connection
> and returning 1. The return value will be checked in the function
> connect_helper() as follows:
>
>         if (!process_connect_service(transport, name, exec))
>                 die(_("can't connect to subservice %s"), name);
>
> So I think it's not necessary to make double check in connect_helper().

Ah, thank you for the clarification.

> The best position to address the bug that both data->connect and
> data->stateless_connect are enabled is in the function get_helper() as
> below:
>
>         } else if (!strcmp(capname, "connect")) {
>                 data->connect = 1;
>         } else if (!strcmp(capname, "stateless-connect")) {
>                 data->stateless_connect = 1;
>         }
>         ... ...
>         if (data->connect && data->stateless_connect)
>                 die("cannot have both connect and stateless_connect enabled");
>
> I consider this change to be off-topic and it will not be introduced
> in this series.

SG.

Re: [PATCH v5 1/6] transport-helper: no connection restriction in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.

Nit: Perhaps add something like the following for the commit message?

    Removing the restriction does not change behavior, because
    process_connect_service() will return 0 if both data->connect and
    data->stateless_connect are false, and we'll still die() early.

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	/* Get_helper so connect is inited. */
>  	get_helper(transport);
> -	if (!data->connect)
> -		die(_("operation not supported by protocol"));
>  
>  	if (!process_connect_service(transport, name, exec))
>  		die(_("can't connect to subservice %s"), name);
> -- 
> 2.43.0

Re: [PATCH v5 2/6] remote-curl: supports git-upload-archive service

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Add new service (git-upload-archive) support in remote-curl, so we can
> support remote archive over HTTP/HTTPS protocols. Differences between
> git-upload-archive and other serices:

s/serices/services

>  1. The git-archive command does not expect to see protocol version and
>     capabilities when connecting to remote-helper, so do not send them
>     in remote-curl for the git-upload-archive service.
>
>  2. We need to detect protocol version by calling discover_refs(),

s/,/.

>     Fallback to use the git-upload-pack service (which, like
>     git-upload-archive, is a read-only operation) to discover protocol
>     version.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  remote-curl.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index ef05752ca5..ce6cb8ac05 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
>  	 * establish a stateless connection, otherwise we need to tell the
>  	 * client to fallback to using other transport helper functions to
>  	 * complete their request.
> +	 *
> +	 * The "git-upload-archive" service is a read-only operation. Fallback
> +	 * to use "git-upload-pack" service to discover protocol version.
>  	 */
> -	discover = discover_refs(service_name, 0);
> +	if (!strcmp(service_name, "git-upload-archive"))
> +		discover = discover_refs("git-upload-pack", 0);
> +	else
> +		discover = discover_refs(service_name, 0);
>  	if (discover->version != protocol_v2) {
>  		printf("fallback\n");
>  		fflush(stdout);
> @@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
>  
>  	/*
>  	 * Dump the capability listing that we got from the server earlier
> -	 * during the info/refs request.
> +	 * during the info/refs request. This does not work with the
> +	 * "git-upload-archive" service.
>  	 */
> -	write_or_die(rpc.in, discover->buf, discover->len);
> +	if (strcmp(service_name, "git-upload-archive"))
> +		write_or_die(rpc.in, discover->buf, discover->len);
>  
>  	/* Until we see EOF keep sending POSTs */
>  	while (1) {
> -- 
> 2.43.0

Re: [PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> process_connect_service() in connect_helper(), run do_take_over() to
> replace the old vtable with a new one which has methods ready for the
> smart transport connection. This will fix the exit code of git-archive

s/will fix/fixes

> in test case "archive remote http repository" of t5003.
>
> The connect_helper() function is used as the connect method of the
> vtable in "transport-helper.c", and it is called by transport_connect()
> in "transport.c" to setup a connection. The only place that we call
> transport_connect() so far is in "builtin/archive.c". Without running
> do_take_over(), it may fail to call transport_disconnect() in
> run_remote_archiver() of "builtin/archive.c". This is because for a
> stateless connection and a service like "git-upload-archive", the
> remote helper may receive a SIGPIPE signal and exit early.

OK.

> To have a
> graceful disconnect method by calling do_take_over() will solve this
> issue.

How about rewording to

    Call do_take_over() to have a graceful disconnect method, so that we
    still call transport_disconnect() even if the remote helper exits
    early.

to make "this issue" more explicit?

> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t5003-archive-zip.sh | 2 +-
>  transport-helper.c     | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 6f85bd3463..961c6aac25 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
>  '
>  
>  test_expect_success 'archive remote http repository' '
> -	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
> +	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
>  		--output=remote-http.zip HEAD &&
>  	test_cmp_bin d.zip remote-http.zip
>  '
> diff --git a/transport-helper.c b/transport-helper.c
> index 6fe9f4f208..91381be622 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	fd[0] = data->helper->out;
>  	fd[1] = data->helper->in;
> +
> +	do_take_over(transport);
>  	return 0;
>  }
>  
> -- 
> 2.43.0

Re: [PATCH v5 0/6] support remote archive via stateless transport

[Linus Arver]

Jiang Xin <worldhello.net@gmail.com> writes:

This v5 looks good code-wise. I've made small suggestions to make the
commit messages better, but they are just nits and you are free to
ignore them.

If you choose to reroll one more time, one additional thing you could do
is to use the word "protocol_v2" in all commit messages because that's
how that term looks like in the code (unless the "protocol-v2" string is
already the standard term used elsewhere).

Thanks.

Re: [PATCH v5 0/6] support remote archive via stateless transport

[Jiang Xin]

On Sun, Jan 21, 2024 at 4:43 AM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> This v5 looks good code-wise. I've made small suggestions to make the
> commit messages better, but they are just nits and you are free to
> ignore them.

Thanks for helping me refine commit messages. I will update them based
on your suggestions in next reroll.

> If you choose to reroll one more time, one additional thing you could do
> is to use the word "protocol_v2" in all commit messages because that's
> how that term looks like in the code (unless the "protocol-v2" string is
> already the standard term used elsewhere).

Will s/protocol_v2/protocol v2/

[PATCH v6 0/6] support remote archive via stateless transport

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.

# Changes since v5

Change commit messages.


# range-diff v5...v6

1:  f3fef46c05 ! 1:  d75e6d27ac transport-helper: no connection restriction in connect_helper
    @@ Commit message
     
         Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
         2018-03-15) added a way for a transport without the ".connect" method
    -    to establish a "stateless" connection in protocol-v2, where
    +    to establish a "stateless" connection in protocol v2, where
         process_connect_service() was taught to handle the ".stateless_connect"
         method, making the old protection too strict. But commit edc9caf7 forgot
    -    to adjust this protection accordingly.
    +    to adjust this protection accordingly. Even at the time of commit
    +    b236752a, this protection seemed redundant, since
    +    process_connect_service() would return 0 if the connection could not be
    +    established, and connect_helper() would still die() early.
     
    -    Remove the restriction in the "connect_helper()" function and give the
    -    function "process_connect_service()" the opportunity to establish a
    -    connection using ".connect" or ".stateless_connect" for protocol v2. So
    -    we can connect with a stateless-rpc and do something useful. E.g., in a
    -    later commit, implements remote archive for a repository over HTTP
    -    protocol.
    +    Remove the restriction in connect_helper() and give the function
    +    process_connect_service() the opportunity to establish a connection
    +    using ".connect" or ".stateless_connect" for protocol v2. So we can
    +    connect with a stateless-rpc and do something useful. E.g., in a later
    +    commit, implements remote archive for a repository over HTTP protocol.
     
         Helped-by: Junio C Hamano <gitster@pobox.com>
         Helped-by: Linus Arver <linusa@google.com>
2:  6be331b22d ! 2:  320526dc56 remote-curl: supports git-upload-archive service
    @@ Commit message
     
         Add new service (git-upload-archive) support in remote-curl, so we can
         support remote archive over HTTP/HTTPS protocols. Differences between
    -    git-upload-archive and other serices:
    +    git-upload-archive and other services:
     
          1. The git-archive program does not expect to see protocol version and
             capabilities when connecting to remote-helper, so do not send them
             in remote-curl for the git-upload-archive service.
     
    -     2. We need to detect protocol version by calling discover_refs(),
    +     2. We need to detect protocol version by calling discover_refs().
             Fallback to use the git-upload-pack service (which, like
             git-upload-archive, is a read-only operation) to discover protocol
             version.
     
    +    Helped-by: Linus Arver <linusa@google.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## remote-curl.c ##
3:  aabc8e1a2a ! 3:  72e575d28a transport-helper: protocol-v2 supports upload-archive
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## Commit message ##
    -    transport-helper: protocol-v2 supports upload-archive
    +    transport-helper: protocol v2 supports upload-archive
     
    -    We used to support only git-upload-pack service for protocol-v2. In
    +    We used to support only git-upload-pack service for protocol v2. In
         order to support remote archive over HTTP/HTTPS protocols, add new
    -    service support for git-upload-archive in protocol-v2.
    +    service support for git-upload-archive in protocol v2.
     
    +    Helped-by: Linus Arver <linusa@google.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## transport-helper.c ##
4:  fdab4abb43 = 4:  390d13c074 http-backend: new rpc-service for git-upload-archive
5:  6ac0c8e105 ! 5:  1c9f7755d3 transport-helper: call do_take_over() in connect_helper
    @@ Commit message
         After successfully connecting to the smart transport by calling
         process_connect_service() in connect_helper(), run do_take_over() to
         replace the old vtable with a new one which has methods ready for the
    -    smart transport connection. This will fix the exit code of git-archive
    +    smart transport connection. This fixes the exit code of git-archive
         in test case "archive remote http repository" of t5003.
     
         The connect_helper() function is used as the connect method of the
    @@ Commit message
         do_take_over(), it may fail to call transport_disconnect() in
         run_remote_archiver() of "builtin/archive.c". This is because for a
         stateless connection and a service like "git-upload-archive", the
    -    remote helper may receive a SIGPIPE signal and exit early. To have a
    -    graceful disconnect method by calling do_take_over() will solve this
    -    issue.
    +    remote helper may receive a SIGPIPE signal and exit early. Call
    +    do_take_over() to have a graceful disconnect method, so that we still
    +    call transport_disconnect() even if the remote helper exits early.
     
         Helped-by: Linus Arver <linusa@google.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
6:  423a89c593 = 6:  18bc8753df transport-helper: call do_take_over() in process_connect

---

Jiang Xin (6):
  transport-helper: no connection restriction in connect_helper
  remote-curl: supports git-upload-archive service
  transport-helper: protocol v2 supports upload-archive
  http-backend: new rpc-service for git-upload-archive
  transport-helper: call do_take_over() in connect_helper
  transport-helper: call do_take_over() in process_connect

 http-backend.c         | 13 ++++++++++---
 remote-curl.c          | 14 +++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 transport-helper.c     | 29 +++++++++++++----------------
 4 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH v6 1/6] transport-helper: no connection restriction in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which only worked with the ".connect" method.

Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
to adjust this protection accordingly. Even at the time of commit
b236752a, this protection seemed redundant, since
process_connect_service() would return 0 if the connection could not be
established, and connect_helper() would still die() early.

Remove the restriction in connect_helper() and give the function
process_connect_service() the opportunity to establish a connection
using ".connect" or ".stateless_connect" for protocol v2. So we can
connect with a stateless-rpc and do something useful. E.g., in a later
commit, implements remote archive for a repository over HTTP protocol.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	/* Get_helper so connect is inited. */
 	get_helper(transport);
-	if (!data->connect)
-		die(_("operation not supported by protocol"));
 
 	if (!process_connect_service(transport, name, exec))
 		die(_("can't connect to subservice %s"), name);
-- 
2.43.0

[PATCH v6 2/6] remote-curl: supports git-upload-archive service

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
git-upload-archive and other services:

 1. The git-archive program does not expect to see protocol version and
    capabilities when connecting to remote-helper, so do not send them
    in remote-curl for the git-upload-archive service.

 2. We need to detect protocol version by calling discover_refs().
    Fallback to use the git-upload-pack service (which, like
    git-upload-archive, is a read-only operation) to discover protocol
    version.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 remote-curl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
 	 * establish a stateless connection, otherwise we need to tell the
 	 * client to fallback to using other transport helper functions to
 	 * complete their request.
+	 *
+	 * The "git-upload-archive" service is a read-only operation. Fallback
+	 * to use "git-upload-pack" service to discover protocol version.
 	 */
-	discover = discover_refs(service_name, 0);
+	if (!strcmp(service_name, "git-upload-archive"))
+		discover = discover_refs("git-upload-pack", 0);
+	else
+		discover = discover_refs(service_name, 0);
 	if (discover->version != protocol_v2) {
 		printf("fallback\n");
 		fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
-	 * during the info/refs request.
+	 * during the info/refs request. This does not work with the
+	 * "git-upload-archive" service.
 	 */
-	write_or_die(rpc.in, discover->buf, discover->len);
+	if (strcmp(service_name, "git-upload-archive"))
+		write_or_die(rpc.in, discover->buf, discover->len);
 
 	/* Until we see EOF keep sending POSTs */
 	while (1) {
-- 
2.43.0

[PATCH v6 3/6] transport-helper: protocol v2 supports upload-archive

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

We used to support only git-upload-pack service for protocol v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
service support for git-upload-archive in protocol v2.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..6fe9f4f208 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,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)
-- 
2.43.0

[PATCH v6 4/6] http-backend: new rpc-service for git-upload-archive

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Add new rpc-service "upload-archive" in http-backend to add server side
support for remote archive over HTTP/HTTPS protocols.

Also add new test cases in t5003. In the test case "archive remote http
repository", git-archive exits with a non-0 exit code even though we
create the archive correctly. It will be fixed in a later commit.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 http-backend.c         | 13 ++++++++++---
 t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..1ed1e29d07 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
 static struct rpc_service rpc_service[] = {
 	{ "upload-pack", "uploadpack", 1, 1 },
 	{ "receive-pack", "receivepack", 0, -1 },
+	{ "upload-archive", "uploadarchive", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
 
 static void service_rpc(struct strbuf *hdr, char *service_name)
 {
-	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	struct strvec argv = STRVEC_INIT;
 	struct rpc_service *svc = select_service(hdr, service_name);
 	struct strbuf buf = STRBUF_INIT;
 
+	strvec_push(&argv, svc->name);
+	if (strcmp(service_name, "git-upload-archive"))
+		strvec_push(&argv, "--stateless-rpc");
+	strvec_push(&argv, ".");
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
 
 	end_headers(hdr);
 
-	argv[0] = svc->name;
-	run_service(argv, svc->buffer_input);
+	run_service(argv.v, svc->buffer_input);
 	strbuf_release(&buf);
+	strvec_clear(&argv);
 }
 
 static int dead;
@@ -723,6 +729,7 @@ static struct service_cmd {
 	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
 
 	{"POST", "/git-upload-pack$", service_rpc},
+	{"POST", "/git-upload-archive$", service_rpc},
 	{"POST", "/git-receive-pack$", service_rpc}
 };
 
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..6f85bd3463 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,38 @@ check_zip with_untracked2
 check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
+# Test remote archive over HTTP protocol.
+#
+# Note: this should be the last part of this test suite, because
+# by including lib-httpd.sh, the test may end early if httpd tests
+# should not be run.
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+	cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+		config http.uploadpack true &&
+	set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+	test_must_fail git -c protocol.version=1 archive \
+		--remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	fatal: can${SQ}t connect to subservice git-upload-archive
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+		--output=remote-http.zip HEAD &&
+	test_cmp_bin d.zip remote-http.zip
+'
+
 test_done
-- 
2.43.0

[PATCH v6 5/6] transport-helper: call do_take_over() in connect_helper

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection. This fixes the exit code of git-archive
in test case "archive remote http repository" of t5003.

The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection and a service like "git-upload-archive", the
remote helper may receive a SIGPIPE signal and exit early. Call
do_take_over() to have a graceful disconnect method, so that we still
call transport_disconnect() even if the remote helper exits early.

Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5003-archive-zip.sh | 2 +-
 transport-helper.c     | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 6f85bd3463..961c6aac25 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
 '
 
 test_expect_success 'archive remote http repository' '
-	test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+	git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
 		--output=remote-http.zip HEAD &&
 	test_cmp_bin d.zip remote-http.zip
 '
diff --git a/transport-helper.c b/transport-helper.c
index 6fe9f4f208..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
 
 	fd[0] = data->helper->out;
 	fd[1] = data->helper->in;
+
+	do_take_over(transport);
 	return 0;
 }
 
-- 
2.43.0

[PATCH v6 6/6] transport-helper: call do_take_over() in process_connect

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The existing pattern among all callers of process_connect() seems to be

        if (process_connect(...)) {
                do_take_over();
                ... dispatch to the underlying method ...
        }
        ... otherwise implement the fallback ...

where the return value from process_connect() is the return value of the
call it makes to process_connect_service().

Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 transport-helper.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 91381be622..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
 	struct helper_data *data = transport->data;
 	const char *name;
 	const char *exec;
+	int ret;
 
 	name = for_push ? "git-receive-pack" : "git-upload-pack";
 	if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
 	else
 		exec = data->transport_options.uploadpack;
 
-	return process_connect_service(transport, name, exec);
+	ret = process_connect_service(transport, name, exec);
+	if (ret)
+		do_take_over(transport);
+	return ret;
 }
 
 static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
 
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
-	}
 
 	/*
 	 * If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 
-	if (process_connect(transport, 1)) {
-		do_take_over(transport);
+	if (process_connect(transport, 1))
 		return transport->vtable->push_refs(transport, remote_refs, flags);
-	}
 
 	if (!remote_refs) {
 		fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
 {
 	get_helper(transport);
 
-	if (process_connect(transport, for_push)) {
-		do_take_over(transport);
+	if (process_connect(transport, for_push))
 		return transport->vtable->get_refs_list(transport, for_push,
 							transport_options);
-	}
 
 	return get_refs_list_using_list(transport, for_push);
 }
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
 {
 	get_helper(transport);
 
-	if (process_connect(transport, 0)) {
-		do_take_over(transport);
+	if (process_connect(transport, 0))
 		return transport->vtable->get_bundle_uri(transport);
-	}
 
 	return -1;
 }
-- 
2.43.0

Re: [PATCH v6 0/6] support remote archive via stateless transport

[Linus Arver]

This v6 version LGTM. Thanks!

Re: [PATCH v6 0/6] support remote archive via stateless transport

[Junio C Hamano]

Linus Arver <linusa@google.com> writes:

> This v6 version LGTM. Thanks!

Thanks, both of you.  Will queue.