threads / patch / 60243

patch, 2 partstransport-helper: no connection restriction in connect_helper

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

## tl;dr

61 messages between Sep 19, 2023 and Jan 22, 2024. Diffs are folded; open one to read it.

replies: 60people: 6as markdown or json

Jiang Xin· Sep 19, 2023, 06:41 UTC · lore
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(-)
Show changes to transport-helper.c +0 −2
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
Jiang Xin· Sep 19, 2023, 06:41 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 4 files +58 −6

http-backend.c, remote-curl.c, t/t5003-archive-zip.sh, transport-helper.c

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
Junio C Hamano· Sep 19, 2023, 17:18 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 8 quoted lines
> 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.

Show 5 quoted lines
> 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.

Show 18 quoted lines
> 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);
Jiang Xin· Sep 20, 2023, 00:20 UTC · re: Junio C Hamano · lore

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

On Wed, Sep 20, 2023 at 1:19 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
>
> 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

Jiang Xin· Sep 23, 2023, 15:21 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Sep 25, 2023, 22:21 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 5 quoted lines
> 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.
Jiang Xin· Sep 26, 2023, 00:43 UTC · re: Junio C Hamano · lore

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

On Tue, Sep 26, 2023 at 6:21 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> 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.
Jiang Xin· Sep 23, 2023, 15:21 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to transport-helper.c +0 −2
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
Junio C Hamano· Sep 25, 2023, 21:11 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 19 quoted lines
> 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);
Jiang Xin· Sep 23, 2023, 15:22 UTC · re: Junio C Hamano · lore

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

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(+)
Show changes to transport-helper.c +2 −0
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
Junio C Hamano· Sep 25, 2023, 21:34 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 6 quoted lines
> 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(-)
Show changes to diff +9 −13
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;
 }
Jiang Xin· Oct 4, 2023, 14:00 UTC · re: Junio C Hamano · lore

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

On Tue, Sep 26, 2023 at 5:34 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
>
> 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.

Jiang Xin· Oct 4, 2023, 15:21 UTC · re: Junio C Hamano · lore

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

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
Jiang Xin· Oct 4, 2023, 15:21 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +0 −2
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
Jiang Xin· Oct 4, 2023, 15:21 UTC · re: Jiang Xin · lore

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

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(+)
Show changes to transport-helper.c +2 −0
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
Jiang Xin· Oct 4, 2023, 15:21 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 4 files +54 −8

http-backend.c, remote-curl.c, t/t5003-archive-zip.sh, transport-helper.c

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
Jiang Xin· Oct 4, 2023, 15:21 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +9 −13
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
Junio C Hamano· Oct 4, 2023, 18:29 UTC · re: Jiang Xin · lore

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

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.

Jiang Xin· Dec 14, 2023, 14:13 UTC · re: Jiang Xin · lore

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

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
Jiang Xin· Dec 14, 2023, 14:13 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +0 −2
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
Linus Arver· Jan 12, 2024, 07:42 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 7 quoted lines
> 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.

Show 23 quoted lines
> 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
Junio C Hamano· Jan 12, 2024, 21:50 UTC · re: Linus Arver · lore

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

Linus Arver <linusa@google.com> writes:
Show 73 quoted lines
> 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.

Jiang Xin· Jan 16, 2024, 09:04 UTC · re: Linus Arver · lore

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

On Fri, Jan 12, 2024 at 3:42 PM Linus Arver <linusa@google.com> wrote:
Show 36 quoted lines
>
> 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.
Show 30 quoted lines
> > 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.

Linus Arver· Jan 18, 2024, 22:26 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 34 quoted lines
>> > 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().

Show 5 quoted lines
>> 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).

Jiang Xin· Jan 19, 2024, 10:56 UTC · re: Linus Arver · lore

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

On Fri, Jan 19, 2024 at 6:26 AM Linus Arver <linusa@google.com> wrote:
Show 53 quoted lines
>
> 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().
Show 14 quoted lines
>
> >> 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

Linus Arver· Jan 20, 2024, 20:25 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 9 quoted lines
> 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.
Show 15 quoted lines
> 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.
Jiang Xin· Dec 14, 2023, 14:13 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +9 −13
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
Jiang Xin· Dec 14, 2023, 14:13 UTC · re: Jiang Xin · lore

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

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(+)
Show changes to transport-helper.c +2 −0
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
Linus Arver· Jan 12, 2024, 07:56 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 15 quoted lines
> 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.

Show 20 quoted lines
> 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
Jiang Xin· Jan 16, 2024, 09:41 UTC · re: Linus Arver · lore

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

On Fri, Jan 12, 2024 at 3:56 PM Linus Arver <linusa@google.com> wrote:
Show 22 quoted lines
>
> 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".
Show 13 quoted lines
> > 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.

Jiang Xin· Dec 14, 2023, 14:13 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 4 files +58 −8

http-backend.c, remote-curl.c, t/t5003-archive-zip.sh, transport-helper.c

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
Linus Arver· Jan 12, 2024, 08:12 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 5 quoted lines
> 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".

Show 8 quoted lines
>  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.

Show 8 quoted lines
> @@ -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.

Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +0 −2
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
Linus Arver· Jan 20, 2024, 20:28 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 22 quoted lines
> 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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to remote-curl.c +11 −3
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
Linus Arver· Jan 20, 2024, 20:30 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 5 quoted lines
> 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
Show 5 quoted lines
>  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/,/.
Show 45 quoted lines
>     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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +2 −1
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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +44 −3

http-backend.c, t/t5003-archive-zip.sh

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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +3 −1

t/t5003-archive-zip.sh, transport-helper.c

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
Linus Arver· Jan 20, 2024, 20:37 UTC · re: Jiang Xin · lore

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

Jiang Xin <worldhello.net@gmail.com> writes:
Show 6 quoted lines
> 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
Show 10 quoted lines
> 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?
Show 35 quoted lines
> 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
Jiang Xin· Jan 16, 2024, 13:39 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +9 −13
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
Linus Arver· Jan 20, 2024, 20:43 UTC · re: Jiang Xin · lore

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

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.
Jiang Xin· Jan 21, 2024, 04:09 UTC · re: Linus Arver · lore

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

On Sun, Jan 21, 2024 at 4:43 AM Linus Arver <linusa@google.com> wrote:
Show 6 quoted lines
>
> 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/
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +0 −2
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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to remote-curl.c +11 −3
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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +2 −1
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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +44 −3

http-backend.c, t/t5003-archive-zip.sh

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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to 2 files +3 −1

t/t5003-archive-zip.sh, transport-helper.c

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
Jiang Xin· Jan 21, 2024, 13:15 UTC · re: Jiang Xin · lore

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

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(-)
Show changes to transport-helper.c +9 −13
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
Jiang Xin· Sep 23, 2023, 15:22 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 4 files +56 −6

http-backend.c, remote-curl.c, t/t5003-archive-zip.sh, transport-helper.c

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
Eric Sunshine· Sep 24, 2023, 06:52 UTC · re: Jiang Xin · lore

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

On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
Show 19 quoted lines
> 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, ".");
Jiang Xin· Sep 24, 2023, 23:39 UTC · re: Eric Sunshine · lore

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

On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 29 quoted lines
>
> 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

rsbecker@nexbridge.com· Sep 24, 2023, 23:58 UTC · re: Jiang Xin · lore

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

On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
Show 33 quoted lines
>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

Jiang Xin· Sep 25, 2023, 00:15 UTC · re: rsbecker@nexbridge.com · lore

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

On Mon, Sep 25, 2023 at 7:58 AM <rsbecker@nexbridge.com> wrote:
Show 37 quoted lines
>
> 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
>
rsbecker@nexbridge.com· Sep 25, 2023, 01:04 UTC · re: Jiang Xin · lore

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

On Sunday, September 24, 2023 8:16 PM, Jiang Xin wrote:
Show 44 quoted lines
>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

Phillip Wood· Sep 24, 2023, 13:41 UTC · re: Jiang Xin · lore

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

On 23/09/2023 16:22, Jiang Xin wrote:
Show 14 quoted lines
> 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.

Show 6 quoted lines
>   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.

Show 11 quoted lines
>   	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
Jiang Xin· Sep 24, 2023, 23:36 UTC · re: Phillip Wood · lore

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

On Sun, Sep 24, 2023 at 9:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 27 quoted lines
>
> 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.

Show 18 quoted lines
> 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
>

← back to recent threads