{"thread":{"id":"60243","subject":"[PATCH 1/2] transport-helper: no connection restriction in connect_helper","startedAt":"2023-09-19T06:42:07Z","lastAt":"2024-01-22T15:55:01Z","messageCount":61,"participants":["Jiang Xin","Junio C Hamano","Eric Sunshine","Phillip Wood","rsbecker@nexbridge.com","Linus Arver"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"482002","messageId":"20230919064156.13892-1-worldhello.net@gmail.com","threadId":"60243","inReplyTo":null,"subject":"[PATCH 1/2] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-19T06:41:55Z","receivedAt":"2023-09-19T06:42:07Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nFor protocol-v2, \"stateless-connection\" can be used to establish a\nstateless connection between the client and the server, but trying to\nestablish http connection by calling \"transport->vtable->connect\" will\nfail. This restriction was first introduced in commit b236752a87\n(Support remote archive from all smart transports, 2009-12-09) by\nadding a limitation in the \"connect_helper()\" function.\n\nRemove the restriction in the \"connect_helper()\" function and use the\nlogic in the \"process_connect_service()\" function to check the protocol\nversion and service name. By this way, we can make a connection and do\nsomething useful. E.g., in a later commit, implements remote archive\nfor a repository over HTTP protocol.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.40.1.49.g40e13c3520.dirty\n\n"},{"id":"482003","messageId":"20230919064156.13892-2-worldhello.net@gmail.com","threadId":"60243","inReplyTo":"20230919064156.13892-1-worldhello.net@gmail.com","subject":"[PATCH 2/2] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-19T06:41:56Z","receivedAt":"2023-09-19T06:42:08Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nEven though we can establish a stateless connection, we still cannot\narchive the remote repository using a stateless HTTP protocol. Try the\nfollowing steps to make it work.\n\n 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n\n 2. Unable to access the URL \".../info/refs?service=git-upload-archive\"\n to detect the protocol version, use the \"git-upload-pack\" service\n instead.\n\n 3. \"git-archive\" does not resolve the protocol version and capabilities\n when connecting to remote-helper, so the remote-helper should not\n send them.\n\n 4. \"git-archive\" may not be able to disconnect the stateless\n connection. Run \"do_take_over()\" to take_over the transfer for\n a graceful disconnect function.\n\nSigned-off-by: Jiang Xin \n---\n http-backend.c | 15 +++++++++++++--\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++\n transport-helper.c | 5 ++++-\n 4 files changed, 58 insertions(+), 6 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..ed3bed965a 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tconst char *argv[4];\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!strcmp(service_name, \"git-upload-archive\")) {\n+\t\targv[1] = \".\";\n+\t\targv[2] = NULL;\n+\t} else {\n+\t\targv[1] = \"--stateless-rpc\";\n+\t\targv[2] = \".\";\n+\t\targv[3] = NULL;\n+\t}\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -723,7 +733,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc}\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..80123c1e06 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,34 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD &&\n+\ttest_cmp_bin d.zip d5.zip\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..91381be622 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n@@ -668,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.40.1.49.g40e13c3520.dirty\n\n"},{"id":"482018","messageId":"xmqqy1h2f5dv.fsf@gitster.g","threadId":"60243","inReplyTo":"20230919064156.13892-1-worldhello.net@gmail.com","subject":"Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-19T17:18:52Z","receivedAt":"2023-09-19T17:19:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> For protocol-v2, \"stateless-connection\" can be used to establish a\n> stateless connection between the client and the server, but trying to\n> establish http connection by calling \"transport->vtable->connect\" will\n> fail. This restriction was first introduced in commit b236752a87\n> (Support remote archive from all smart transports, 2009-12-09) by\n> adding a limitation in the \"connect_helper()\" function.\n\nThe above description may not be technically wrong per-se, but I\nfound it confusing. The \".connect method must be defined\" you are\nremoving was added back when there was no \"stateless\" variant of the\nconnection initiation. Many codepaths added by that patch did \"if\n.connect is there, call it, but otherwise die()\" and I think the\ncode you were removing was added as a safety valve, not a limitation\nor restriction. Later, process_connect_service() learned to handle\nthe .stateless_connect bit as a fallback for transports without\n.connect method defined, and the commit added that feature, edc9caf7\n(transport-helper: introduce stateless-connect, 2018-03-15), forgot\nthat the caller did not allow this fallback.\n\n\tWhen b236752a (Support remote archive from all smart\n\ttransports, 2009-12-09) added \"remote archive\" support for\n\t\"smart transports\", it was for transport that supports the\n\t.connect method. connect_helper() function protected itself\n\tfrom getting called for a transport without the method\n\tbefore calling process_connect_service(), which did not work\n\twuth such a transport.\n\n\tLater, edc9caf7 (transport-helper: introduce\n\tstateless-connect, 2018-03-15) added a way for a transport\n\twithout the .connect method to establish a \"stateless\"\n\tconnection in protocol-v2, process_connect_service() was\n\ttaught to handle the \"stateless\" connection, making the old\n\tsafety valve in its caller that insisted that .connect\n\tmethod must be defined too strict, and forgot to loosen it.\n\nor something along that line would have been easire to follow, at\nleast to me.\n\n> Remove the restriction in the \"connect_helper()\" function and use the\n> logic in the \"process_connect_service()\" function to check the protocol\n> version and service name. By this way, we can make a connection and do\n> something useful. E.g., in a later commit, implements remote archive\n> for a repository over HTTP protocol.\n\nOK. \n\nb236752a87 was to allow \"remote archive from all smart transports\",\nbut unfortunately HTTP was not among \"smart transports\". This\nseries is to update smart HTTP transport (aka \"stateless\") to also\nsupport it? Interesting.\n\n> Signed-off-by: Jiang Xin \n> ---\n> transport-helper.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 49811ef176..2e127d24a5 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> -\t\tdie(_(\"operation not supported by protocol\"));\n> \n> \tif (!process_connect_service(transport, name, exec))\n> \t\tdie(_(\"can't connect to subservice %s\"), name);\n"},{"id":"482043","messageId":"CANYiYbFKaiHS5PQSjB9V6dWLCP=wiJJRNLK=v=x1_mTLXBft=A@mail.gmail.com","threadId":"60243","inReplyTo":"xmqqy1h2f5dv.fsf@gitster.g","subject":"Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-20T00:20:20Z","receivedAt":"2023-09-20T00:20:37Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Wed, Sep 20, 2023 at 1:19 AM Junio C Hamano wrote:\n>\n> Jiang Xin writes:\n>\n> > From: Jiang Xin \n> >\n> > For protocol-v2, \"stateless-connection\" can be used to establish a\n> > stateless connection between the client and the server, but trying to\n> > establish http connection by calling \"transport->vtable->connect\" will\n> > fail. This restriction was first introduced in commit b236752a87\n> > (Support remote archive from all smart transports, 2009-12-09) by\n> > adding a limitation in the \"connect_helper()\" function.\n>\n> The above description may not be technically wrong per-se, but I\n> found it confusing. The \".connect method must be defined\" you are\n> removing was added back when there was no \"stateless\" variant of the\n> connection initiation. Many codepaths added by that patch did \"if\n> .connect is there, call it, but otherwise die()\" and I think the\n> code you were removing was added as a safety valve, not a limitation\n> or restriction. Later, process_connect_service() learned to handle\n> the .stateless_connect bit as a fallback for transports without\n> .connect method defined, and the commit added that feature, edc9caf7\n> (transport-helper: introduce stateless-connect, 2018-03-15), forgot\n> that the caller did not allow this fallback.\n>\n> When b236752a (Support remote archive from all smart\n> transports, 2009-12-09) added \"remote archive\" support for\n> \"smart transports\", it was for transport that supports the\n> .connect method. connect_helper() function protected itself\n> from getting called for a transport without the method\n> before calling process_connect_service(), which did not work\n> wuth such a transport.\n>\n> Later, edc9caf7 (transport-helper: introduce\n> stateless-connect, 2018-03-15) added a way for a transport\n> without the .connect method to establish a \"stateless\"\n> connection in protocol-v2, process_connect_service() was\n> taught to handle the \"stateless\" connection, making the old\n> safety valve in its caller that insisted that .connect\n> method must be defined too strict, and forgot to loosen it.\n>\n> or something along that line would have been easire to follow, at\n> least to me.\n>\n\nThese explanations are very clear and helpful, thank you.\n\n--\nJiang Xin\n"},{"id":"482210","messageId":"20230923152201.14741-1-worldhello.net@gmail.com","threadId":"60243","inReplyTo":"xmqqy1h2f5dv.fsf@gitster.g","subject":"[PATCH v2 0/3] support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-23T15:21:58Z","receivedAt":"2023-09-23T15:22:09Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nEnable stateless-rpc connection in \"connect_helper()\", and add support\nfor remote archive from a stateless transport.\n\nrange-diff v1...v2:\n\n1: 4457ca910b ! 1: 6fabd4dcab transport-helper: no connection restriction in connect_helper\n @@ Metadata\n ## Commit message ##\n transport-helper: no connection restriction in connect_helper\n \n - For protocol-v2, \"stateless-connection\" can be used to establish a\n - stateless connection between the client and the server, but trying to\n - establish http connection by calling \"transport->vtable->connect\" will\n - fail. This restriction was first introduced in commit b236752a87\n - (Support remote archive from all smart transports, 2009-12-09) by\n - adding a limitation in the \"connect_helper()\" function.\n + When commit b236752a (Support remote archive from all smart transports,\n + 2009-12-09) added \"remote archive\" support for \"smart transports\", it\n + was for transport that supports the \".connect\" method. The\n + \"connect_helper()\" function protected itself from getting called for a\n + transport without the method before calling process_connect_service(),\n + which did not work with such a transport.\n \n - Remove the restriction in the \"connect_helper()\" function and use the\n - logic in the \"process_connect_service()\" function to check the protocol\n - version and service name. By this way, we can make a connection and do\n - something useful. E.g., in a later commit, implements remote archive\n - for a repository over HTTP protocol.\n + Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n + 2018-03-15) added a way for a transport without the \".connect\" method\n + to establish a \"stateless\" connection in protocol-v2, which\n + process_connect_service() was taught to handle the \"stateless\"\n + connection, making the old safety valve in its caller that insisted\n + that \".connect\" method must be defined too strict, and forgot to loosen\n + it.\n \n + Remove the restriction in the \"connect_helper()\" function and give the\n + function \"process_connect_service()\" the opportunity to establish a\n + connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n + we can connect with a stateless-rpc and do something useful. E.g., in a\n + later commit, implements remote archive for a repository over HTTP\n + protocol.\n +\n + Helped-by: Junio C Hamano \n Signed-off-by: Jiang Xin \n \n ## transport-helper.c ##\n-: ---------- > 2: 1d687abc7e transport-helper: run do_take_over in connect_helper\n2: d4242d1f27 ! 3: 051d66f48e archive: support remote archive from stateless transport\n @@ Commit message\n \n 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n \n - 2. Unable to access the URL \".../info/refs?service=git-upload-archive\"\n - to detect the protocol version, use the \"git-upload-pack\" service\n - instead.\n + 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n + protocol version, instead of use the \"git-upload-archive\" service.\n \n - 3. \"git-archive\" does not resolve the protocol version and capabilities\n - when connecting to remote-helper, so the remote-helper should not\n - send them.\n -\n - 4. \"git-archive\" may not be able to disconnect the stateless\n - connection. Run \"do_take_over()\" to take_over the transfer for\n - a graceful disconnect function.\n + 3. \"git-archive\" does not expect to see protocol version and\n + capabilities when connecting to remote-helper, so do not send them\n + in \"remote-curl.c\" for the \"git-upload-archive\" service.\n \n Signed-off-by: Jiang Xin \n \n @@ transport-helper.c: static int process_connect_service(struct transport *transpo\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n -@@ transport-helper.c: static int connect_helper(struct transport *transport, const char *name,\n - \n - \tfd[0] = data->helper->out;\n - \tfd[1] = data->helper->in;\n -+\n -+\tdo_take_over(transport);\n - \treturn 0;\n - }\n - \n\nJiang Xin (3):\n transport-helper: no connection restriction in connect_helper\n transport-helper: run do_take_over in connect_helper\n archive: support remote archive from stateless transport\n\n http-backend.c | 15 +++++++++++++--\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++\n transport-helper.c | 7 ++++---\n 4 files changed, 58 insertions(+), 8 deletions(-)\n\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482211","messageId":"20230923152201.14741-2-worldhello.net@gmail.com","threadId":"60243","inReplyTo":"xmqqy1h2f5dv.fsf@gitster.g","subject":"[PATCH v2 1/3] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-23T15:21:59Z","receivedAt":"2023-09-23T15:22:12Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen commit b236752a (Support remote archive from all smart transports,\n2009-12-09) added \"remote archive\" support for \"smart transports\", it\nwas for transport that supports the \".connect\" method. The\n\"connect_helper()\" function protected itself from getting called for a\ntransport without the method before calling process_connect_service(),\nwhich did not work with such a transport.\n\nLater, commit edc9caf7 (transport-helper: introduce stateless-connect,\n2018-03-15) added a way for a transport without the \".connect\" method\nto establish a \"stateless\" connection in protocol-v2, which\nprocess_connect_service() was taught to handle the \"stateless\"\nconnection, making the old safety valve in its caller that insisted\nthat \".connect\" method must be defined too strict, and forgot to loosen\nit.\n\nRemove the restriction in the \"connect_helper()\" function and give the\nfunction \"process_connect_service()\" the opportunity to establish a\nconnection using \".connect\" or \".stateless_connect\" for protocol v2. So\nwe can connect with a stateless-rpc and do something useful. E.g., in a\nlater commit, implements remote archive for a repository over HTTP\nprotocol.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482212","messageId":"20230923152201.14741-3-worldhello.net@gmail.com","threadId":"60243","inReplyTo":"xmqqy1h2f5dv.fsf@gitster.g","subject":"[PATCH v2 2/3] transport-helper: run do_take_over in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-23T15:22:00Z","receivedAt":"2023-09-23T15:22:19Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAfter successfully connecting to the smart transport by calling\n\"process_connect_service()\" in \"connect_helper()\", run \"do_take_over()\"\nto replace the old vtable with a new one which has methods ready for\nthe smart transport connection.\n\nThe subsequent commit introduces remote archive for a stateless-rpc\nconnection. But without running \"do_take_over()\", it may fail to call\n\"transport_disconnect()\" in \"run_remote_archiver()\" of\n\"builtin/archive.c\". This is because for a stateless connection or a\nservice like \"git-upload-pack-archive\", the remote helper may receive a\nSIGPIPE signal and exit early. To have a graceful disconnect method by\ncalling \"do_take_over()\" will solve this issue.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..3c8802b7a3 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -668,6 +668,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482213","messageId":"20230923152201.14741-4-worldhello.net@gmail.com","threadId":"60243","inReplyTo":"xmqqy1h2f5dv.fsf@gitster.g","subject":"[PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-23T15:22:01Z","receivedAt":"2023-09-23T15:22:20Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nEven though we can establish a stateless connection, we still cannot\narchive the remote repository using a stateless HTTP protocol. Try the\nfollowing steps to make it work.\n\n 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n\n 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n protocol version, instead of use the \"git-upload-archive\" service.\n\n 3. \"git-archive\" does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in \"remote-curl.c\" for the \"git-upload-archive\" service.\n\nSigned-off-by: Jiang Xin \n---\n http-backend.c | 15 +++++++++++++--\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++\n transport-helper.c | 3 ++-\n 4 files changed, 56 insertions(+), 6 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..ed3bed965a 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tconst char *argv[4];\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!strcmp(service_name, \"git-upload-archive\")) {\n+\t\targv[1] = \".\";\n+\t\targv[2] = NULL;\n+\t} else {\n+\t\targv[1] = \"--stateless-rpc\";\n+\t\targv[2] = \".\";\n+\t\targv[3] = NULL;\n+\t}\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -723,7 +733,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc}\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..80123c1e06 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,34 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD &&\n+\ttest_cmp_bin d.zip d5.zip\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 3c8802b7a3..91381be622 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482219","messageId":"CAPig+cTRByz10ySknTxPB2yVJf5Snz29LNRq5MtPk2MF3nMziQ@mail.gmail.com","threadId":"60243","inReplyTo":"20230923152201.14741-4-worldhello.net@gmail.com","subject":"Re: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-09-24T06:52:19Z","receivedAt":"2023-09-24T06:52:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin wrote:\n> Even though we can establish a stateless connection, we still cannot\n> archive the remote repository using a stateless HTTP protocol. Try the\n> following steps to make it work.\n> [...]\n> Signed-off-by: Jiang Xin \n> ---\n> diff --git a/http-backend.c b/http-backend.c\n> @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n> - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n> + const char *argv[4];\n>\n> + if (!strcmp(service_name, \"git-upload-archive\")) {\n> + argv[1] = \".\";\n> + argv[2] = NULL;\n> + } else {\n> + argv[1] = \"--stateless-rpc\";\n> + argv[2] = \".\";\n> + argv[3] = NULL;\n> + }\n\nIt may not be worth a reroll, but since you're touching this code\nanyhow, these days we'd use `strvec` for this:\n\n struct strvec argv = STRVEC_INIT;\n if (strcmp(service_name, \"git-upload-archive\"))\n strvec_push(&argv, \"--stateless-rpc\");\n strvec_push(&argv, \".\");\n"},{"id":"482227","messageId":"f4877c36-ff26-4f81-b5dd-63c929ba30c9@gmail.com","threadId":"60243","inReplyTo":"20230923152201.14741-4-worldhello.net@gmail.com","subject":"Re: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-24T13:41:04Z","receivedAt":"2023-09-24T13:41:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/09/2023 16:22, Jiang Xin wrote:\n> From: Jiang Xin \n> \n> Even though we can establish a stateless connection, we still cannot\n> archive the remote repository using a stateless HTTP protocol. Try the\n> following steps to make it work.\n> \n> 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n> \n> 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n> protocol version, instead of use the \"git-upload-archive\" service.\n> \n> 3. \"git-archive\" does not expect to see protocol version and\n> capabilities when connecting to remote-helper, so do not send them\n> in \"remote-curl.c\" for the \"git-upload-archive\" service.\n\nI'm not familiar enough with the server side of git to comment on \nwhether this patch is a good idea, but I did notice one C language issue \nbelow.\n\n> static struct string_list *get_parameters(void)\n> @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n> \n> static void service_rpc(struct strbuf *hdr, char *service_name)\n> {\n> -\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n\nIn the pre-image argv[0] is initialized to NULL\n\n> +\tconst char *argv[4];\n\nIn the post-image argv is not initialized and the first element is not \nset in the code below.\n\n> \tstruct rpc_service *svc = select_service(hdr, service_name);\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> +\tif (!strcmp(service_name, \"git-upload-archive\")) {\n> +\t\targv[1] = \".\";\n> +\t\targv[2] = NULL;\n> +\t} else {\n> +\t\targv[1] = \"--stateless-rpc\";\n> +\t\targv[2] = \".\";\n> +\t\targv[3] = NULL;\n> +\t}\n\nBest Wishes\n\nPhillip\n\n"},{"id":"482232","messageId":"CANYiYbHjk4CX4Uswn4sX-tH3e22uLSHk_4bwjqVO=9MWcfoHnw@mail.gmail.com","threadId":"60243","inReplyTo":"f4877c36-ff26-4f81-b5dd-63c929ba30c9@gmail.com","subject":"Re: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-24T23:36:59Z","receivedAt":"2023-09-24T23:37:14Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Sun, Sep 24, 2023 at 9:41 PM Phillip Wood wrote:\n>\n> On 23/09/2023 16:22, Jiang Xin wrote:\n> > From: Jiang Xin \n> >\n> > Even though we can establish a stateless connection, we still cannot\n> > archive the remote repository using a stateless HTTP protocol. Try the\n> > following steps to make it work.\n> >\n> > 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n> >\n> > 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n> > protocol version, instead of use the \"git-upload-archive\" service.\n> >\n> > 3. \"git-archive\" does not expect to see protocol version and\n> > capabilities when connecting to remote-helper, so do not send them\n> > in \"remote-curl.c\" for the \"git-upload-archive\" service.\n>\n> I'm not familiar enough with the server side of git to comment on\n> whether this patch is a good idea, but I did notice one C language issue\n> below.\n>\n> > static struct string_list *get_parameters(void)\n> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n> >\n> > static void service_rpc(struct strbuf *hdr, char *service_name)\n> > {\n> > - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n\nFor the original implementation, the first NULL is used as a\nplaceholder, and will be initialized somewhere below.\n\n> In the pre-image argv[0] is initialized to NULL\n>\n> > + const char *argv[4];\n>\n> In the post-image argv is not initialized and the first element is not\n> set in the code below.\n>\n> > struct rpc_service *svc = select_service(hdr, service_name);\n> > struct strbuf buf = STRBUF_INIT;\n> >\n> > + if (!strcmp(service_name, \"git-upload-archive\")) {\n> > + argv[1] = \".\";\n> > + argv[2] = NULL;\n> > + } else {\n> > + argv[1] = \"--stateless-rpc\";\n> > + argv[2] = \".\";\n> > + argv[3] = NULL;\n> > + }\n\nIt will be initialized in the code further below, see http-backend.c:668.\n\n argv[0] = svc->name;\n run_service(argv, svc->buffer_input);\n strbuf_release(&buf);\n\nAnyway, I will rewrite these code in reroll v3 to follow Eric's suggestion.\n\n> Best Wishes\n>\n> Phillip\n>\n"},{"id":"482233","messageId":"CANYiYbFkG+CvrNFBkdNewZs7ADROVsjd051SDQsU0zVq8eBhew@mail.gmail.com","threadId":"60243","inReplyTo":"CAPig+cTRByz10ySknTxPB2yVJf5Snz29LNRq5MtPk2MF3nMziQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-24T23:39:33Z","receivedAt":"2023-09-24T23:39:47Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine wrote:\n>\n> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin wrote:\n> > Even though we can establish a stateless connection, we still cannot\n> > archive the remote repository using a stateless HTTP protocol. Try the\n> > following steps to make it work.\n> > [...]\n> > Signed-off-by: Jiang Xin \n> > ---\n> > diff --git a/http-backend.c b/http-backend.c\n> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n> > - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n> > + const char *argv[4];\n> >\n> > + if (!strcmp(service_name, \"git-upload-archive\")) {\n> > + argv[1] = \".\";\n> > + argv[2] = NULL;\n> > + } else {\n> > + argv[1] = \"--stateless-rpc\";\n> > + argv[2] = \".\";\n> > + argv[3] = NULL;\n> > + }\n>\n> It may not be worth a reroll, but since you're touching this code\n> anyhow, these days we'd use `strvec` for this:\n>\n> struct strvec argv = STRVEC_INIT;\n> if (strcmp(service_name, \"git-upload-archive\"))\n> strvec_push(&argv, \"--stateless-rpc\");\n> strvec_push(&argv, \".\");\n\nGood suggestion, I'll queue this up as part of next reroll.\n\n--\nJiang Xin\n"},{"id":"482234","messageId":"007601d9ef43$00731690$015943b0$@nexbridge.com","threadId":"60243","inReplyTo":"CANYiYbFkG+CvrNFBkdNewZs7ADROVsjd051SDQsU0zVq8eBhew@mail.gmail.com","subject":"RE: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-09-24T23:58:17Z","receivedAt":"2023-09-24T23:58:41Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:\n>On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine \n>wrote:\n>>\n>> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin wrote:\n>> > Even though we can establish a stateless connection, we still cannot\n>> > archive the remote repository using a stateless HTTP protocol. Try\n>> > the following steps to make it work.\n>> > [...]\n>> > Signed-off-by: Jiang Xin \n>> > ---\n>> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@\n>> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n>> > - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n>> > + const char *argv[4];\n>> >\n>> > + if (!strcmp(service_name, \"git-upload-archive\")) {\n>> > + argv[1] = \".\";\n>> > + argv[2] = NULL;\n>> > + } else {\n>> > + argv[1] = \"--stateless-rpc\";\n>> > + argv[2] = \".\";\n>> > + argv[3] = NULL;\n>> > + }\n>>\n>> It may not be worth a reroll, but since you're touching this code\n>> anyhow, these days we'd use `strvec` for this:\n>>\n>> struct strvec argv = STRVEC_INIT;\n>> if (strcmp(service_name, \"git-upload-archive\"))\n>> strvec_push(&argv, \"--stateless-rpc\");\n>> strvec_push(&argv, \".\");\n>\n>Good suggestion, I'll queue this up as part of next reroll.\n\nWhich test covers this change?\n\nThanks,\nRandall\n\n"},{"id":"482235","messageId":"CANYiYbGf4U2_UG674GqfauNPg+TgOtzRT=xCJ=x0gHM+TcrNpQ@mail.gmail.com","threadId":"60243","inReplyTo":"007601d9ef43$00731690$015943b0$@nexbridge.com","subject":"Re: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-25T00:15:47Z","receivedAt":"2023-09-25T00:16:05Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Mon, Sep 25, 2023 at 7:58 AM wrote:\n>\n> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:\n> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine \n> >wrote:\n> >>\n> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin wrote:\n> >> > Even though we can establish a stateless connection, we still cannot\n> >> > archive the remote repository using a stateless HTTP protocol. Try\n> >> > the following steps to make it work.\n> >> > [...]\n> >> > Signed-off-by: Jiang Xin \n> >> > ---\n> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@\n> >> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n> >> > - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n> >> > + const char *argv[4];\n> >> >\n> >> > + if (!strcmp(service_name, \"git-upload-archive\")) {\n> >> > + argv[1] = \".\";\n> >> > + argv[2] = NULL;\n> >> > + } else {\n> >> > + argv[1] = \"--stateless-rpc\";\n> >> > + argv[2] = \".\";\n> >> > + argv[3] = NULL;\n> >> > + }\n> >>\n> >> It may not be worth a reroll, but since you're touching this code\n> >> anyhow, these days we'd use `strvec` for this:\n> >>\n> >> struct strvec argv = STRVEC_INIT;\n> >> if (strcmp(service_name, \"git-upload-archive\"))\n> >> strvec_push(&argv, \"--stateless-rpc\");\n> >> strvec_push(&argv, \".\");\n> >\n> >Good suggestion, I'll queue this up as part of next reroll.\n>\n> Which test covers this change?\n\nSee: https://lore.kernel.org/git/20230923152201.14741-4-worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh\n\n> Thanks,\n> Randall\n>\n"},{"id":"482237","messageId":"007b01d9ef4c$4c4e8eb0$e4ebac10$@nexbridge.com","threadId":"60243","inReplyTo":"CANYiYbGf4U2_UG674GqfauNPg+TgOtzRT=xCJ=x0gHM+TcrNpQ@mail.gmail.com","subject":"RE: [PATCH v2 3/3] archive: support remote archive from stateless transport","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2023-09-25T01:04:49Z","receivedAt":"2023-09-25T01:05:18Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, September 24, 2023 8:16 PM, Jiang Xin wrote:\n>On Mon, Sep 25, 2023 at 7:58 AM wrote:\n>>\n>> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:\n>> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine\n>> >\n>> >wrote:\n>> >>\n>> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin \n>wrote:\n>> >> > Even though we can establish a stateless connection, we still\n>> >> > cannot archive the remote repository using a stateless HTTP\n>> >> > protocol. Try the following steps to make it work.\n>> >> > [...]\n>> >> > Signed-off-by: Jiang Xin \n>> >> > ---\n>> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19\n>> >> > @@ static void check_content_type(struct strbuf *hdr, const char\n>*accepted_type)\n>> >> > - const char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n>> >> > + const char *argv[4];\n>> >> >\n>> >> > + if (!strcmp(service_name, \"git-upload-archive\")) {\n>> >> > + argv[1] = \".\";\n>> >> > + argv[2] = NULL;\n>> >> > + } else {\n>> >> > + argv[1] = \"--stateless-rpc\";\n>> >> > + argv[2] = \".\";\n>> >> > + argv[3] = NULL;\n>> >> > + }\n>> >>\n>> >> It may not be worth a reroll, but since you're touching this code\n>> >> anyhow, these days we'd use `strvec` for this:\n>> >>\n>> >> struct strvec argv = STRVEC_INIT;\n>> >> if (strcmp(service_name, \"git-upload-archive\"))\n>> >> strvec_push(&argv, \"--stateless-rpc\");\n>> >> strvec_push(&argv, \".\");\n>> >\n>> >Good suggestion, I'll queue this up as part of next reroll.\n>>\n>> Which test covers this change?\n>\n>See: https://lore.kernel.org/git/20230923152201.14741-4-\n>worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh\n\nThanks. That is what I needed. Looking forward to the merge.\n--Randall\n\n"},{"id":"482305","messageId":"xmqqjzserma3.fsf@gitster.g","threadId":"60243","inReplyTo":"20230923152201.14741-2-worldhello.net@gmail.com","subject":"Re: [PATCH v2 1/3] transport-helper: no connection restriction in connect_helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-25T21:11:16Z","receivedAt":"2023-09-25T21:11:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> Remove the restriction in the \"connect_helper()\" function and give the\n> function \"process_connect_service()\" the opportunity to establish a\n> connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n> we can connect with a stateless-rpc and do something useful. E.g., in a\n> later commit, implements remote archive for a repository over HTTP\n> protocol.\n\nOK. Given that process_connect_service() does this:\n\n\tif (data->connect) {\n\t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\", name);\n\t\tret = run_connect(transport, &cmdbuf);\n\t} else if (data->stateless_connect &&\n\t\t (get_protocol_version_config() == protocol_v2) &&\n\t\t (!strcmp(\"git-upload-pack\", name) ||\n\t\t !strcmp(\"git-upload-archive\", name))) {\n\t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n\t\tret = run_connect(transport, &cmdbuf);\n\t\tif (ret)\n\t\t\ttransport->stateless_rpc = 1;\n\t}\n\nin the spirit of the original \"safety valve\", it becomes tempting to\nsuggest we make sure at least .connect or .stateless_connect exists\nin the transport to be safe, but then we will need to keep the logic\nof safety valve in connect_helper() and the actual dispatching in\nprocess_connect_service() in sync, which is a maintenance burden.\n\nIt however makes me wonder if we should add\n\n\telse\n\t\tdie(_(\"operation not supported by protocol\"));\n\nat the end of the \"if/else if\" cascade in process_connect_service(),\nso that callers that end up following this callpath with a transport\nthat defines neither would be caught.\n\nOther than that, the patch is as good as the previous round, and the\nexplanation is vastly easier to understand.\n\nThanks.\n\n> Helped-by: Junio C Hamano \n> Signed-off-by: Jiang Xin \n> ---\n> transport-helper.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 49811ef176..2e127d24a5 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> -\t\tdie(_(\"operation not supported by protocol\"));\n> \n> \tif (!process_connect_service(transport, name, exec))\n> \t\tdie(_(\"can't connect to subservice %s\"), name);\n"},{"id":"482306","messageId":"xmqqil7yq6ms.fsf@gitster.g","threadId":"60243","inReplyTo":"20230923152201.14741-3-worldhello.net@gmail.com","subject":"Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-25T21:34:35Z","receivedAt":"2023-09-25T21:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> After successfully connecting to the smart transport by calling\n> \"process_connect_service()\" in \"connect_helper()\", run \"do_take_over()\"\n> to replace the old vtable with a new one which has methods ready for\n> the smart transport connection.\n\nThe existing pattern among all callers of process_connect() seems to\nbe\n\n\tif (process_connect(...)) {\n\t\tdo_take_over();\n\t\t... dispatch to the underlying method ...\n\t}\n\t... otherwise implement the fallback ...\n\nwhere the return value from process_connect() is the return value of\nthe call it makes to process_connect_service().\n\nAnd the only other caller of process_connect_service() is\nconnect_helper(), so in that sense, making a call to do_take_over()\nwhen process_connect_service() succeeds in the helper does make\nthings consistent. The connect_helper() function being static, the\nhelper transport is the only transport that gets affected, but how\nhas it been working without having this do_take_over() step? An\nobvious related question is if it has been working so far, would it\nbreak if we have do_take_over() added here?\n\n\nIn any case, this makes me wonder if we should do the following\npatch to help developers who may want to add new callers to\nprocess_connect_service() by adding calls to process_connect().\n\n transport-helper.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git c/transport-helper.c w/transport-helper.c\nindex 91381be622..566f7473df 100644\n--- c/transport-helper.c\n+++ w/transport-helper.c\n@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tconst char *name;\n \tconst char *exec;\n+\tint ret;\n \n \tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n \tif (for_push)\n@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\treturn process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, name, exec);\n+\tif (ret)\n+\t\tdo_take_over(transport);\n+\treturn ret;\n }\n \n static int connect_helper(struct transport *transport, const char *name,\n@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,\n \n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->fetch_refs(transport, nr_heads, to_fetch);\n-\t}\n \n \t/*\n \t * If we reach here, then the server, the client, and/or the transport\n@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \n-\tif (process_connect(transport, 1)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 1))\n \t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n-\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr,\n@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, for_push)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, for_push))\n \t\treturn transport->vtable->get_refs_list(transport, for_push,\n \t\t\t\t\t\t\ttransport_options);\n-\t}\n \n \treturn get_refs_list_using_list(transport, for_push);\n }\n@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->get_bundle_uri(transport);\n-\t}\n \n \treturn -1;\n }\n\n"},{"id":"482309","messageId":"xmqq5y3xrj19.fsf@gitster.g","threadId":"60243","inReplyTo":"20230923152201.14741-1-worldhello.net@gmail.com","subject":"Re: [PATCH v2 0/3] support remote archive from stateless transport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-25T22:21:22Z","receivedAt":"2023-09-25T22:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> Enable stateless-rpc connection in \"connect_helper()\", and add support\n> for remote archive from a stateless transport.\n> ...\n\nAdministrivia. Please make sure that your patches [1..N/N] appear\nas a follow-up to the cover letter [0/N], instead of each of them\nbeing individually a response to somebody else's message.\n\nThanks.\n"},{"id":"482314","messageId":"CANYiYbFwtJ=pT=TfWmkOfzKLNeFzoT2ofXsjKihfUt-awv6K4A@mail.gmail.com","threadId":"60243","inReplyTo":"xmqq5y3xrj19.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-09-26T00:43:26Z","receivedAt":"2023-09-26T00:43:44Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Tue, Sep 26, 2023 at 6:21 AM Junio C Hamano wrote:\n>\n> Jiang Xin writes:\n>\n> > From: Jiang Xin \n> >\n> > Enable stateless-rpc connection in \"connect_helper()\", and add support\n> > for remote archive from a stateless transport.\n> > ...\n>\n> Administrivia. Please make sure that your patches [1..N/N] appear\n> as a follow-up to the cover letter [0/N], instead of each of them\n> being individually a response to somebody else's message.\n>\n\nI will set the \"format.thread\" configuration variable so that I don't\nhave to worry about forgetting the \"--thread\" option when executing\ngit-format-patch.\n\n git config --global format.thread shallow\n\nThanks.\n"},{"id":"482648","messageId":"CANYiYbF70=8tyuah+wwp2EYiBrExSzHbfEYkEcZz2gesXkJ-vw@mail.gmail.com","threadId":"60243","inReplyTo":"xmqqil7yq6ms.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T14:00:34Z","receivedAt":"2023-10-04T14:00:50Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Tue, Sep 26, 2023 at 5:34 AM Junio C Hamano wrote:\n>\n> Jiang Xin writes:\n>\n> > From: Jiang Xin \n> >\n> > After successfully connecting to the smart transport by calling\n> > \"process_connect_service()\" in \"connect_helper()\", run \"do_take_over()\"\n> > to replace the old vtable with a new one which has methods ready for\n> > the smart transport connection.\n>\n> The existing pattern among all callers of process_connect() seems to\n> be\n>\n> if (process_connect(...)) {\n> do_take_over();\n> ... dispatch to the underlying method ...\n> }\n> ... otherwise implement the fallback ...\n>\n> where the return value from process_connect() is the return value of\n> the call it makes to process_connect_service().\n>\n> And the only other caller of process_connect_service() is\n> connect_helper(), so in that sense, making a call to do_take_over()\n> when process_connect_service() succeeds in the helper does make\n> things consistent. The connect_helper() function being static, the\n> helper transport is the only transport that gets affected, but how\n> has it been working without having this do_take_over() step? An\n> obvious related question is if it has been working so far, would it\n> break if we have do_take_over() added here?\n\nThe connect_helper() function is used as the connect method of the\nvtable in \"transport-helper.c\", and we use the function\n\"transport_connect()\" in \"transport.c\" to call this connect method of\nvtable. The only place that we call transport_connect() to setup a\nconnection is in \"builtin/archive.c\". So it won't break others if we\nadd do_take_over() in connect_helper().\n\nIn fact, it was not \"git archive\" that made me discover this issue.\nWhen I implemented a fetch proxy and added a new caller for\ntransport_connect(), I found that the HTTP protocol didn't work, so I\ndug it out.\n"},{"id":"482651","messageId":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"xmqqil7yq6ms.fsf@gitster.g","subject":"[PATCH v3 0/4] support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T15:21:39Z","receivedAt":"2023-10-04T15:21:51Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\n\"git archive --remote=\" learned to talk over the smart\nhttp (aka stateless) transport.\n\nrange-diff v2...v3\n\n1: 4497404900 = 1: e660fc79b6 transport-helper: no connection restriction in connect_helper\n-: ---------- > 2: e3dc18caa9 transport-helper: call do_take_over() in process_connect\n2: 9bfaa1a904 ! 3: 01699822c3 transport-helper: run do_take_over in connect_helper\n @@ Metadata\n Author: Jiang Xin \n \n ## Commit message ##\n - transport-helper: run do_take_over in connect_helper\n + transport-helper: call do_take_over() in connect_helper\n \n After successfully connecting to the smart transport by calling\n - \"process_connect_service()\" in \"connect_helper()\", run \"do_take_over()\"\n - to replace the old vtable with a new one which has methods ready for\n - the smart transport connection.\n + process_connect_service() in connect_helper(), run do_take_over() to\n + replace the old vtable with a new one which has methods ready for the\n + smart transport connection.\n \n - The subsequent commit introduces remote archive for a stateless-rpc\n - connection. But without running \"do_take_over()\", it may fail to call\n - \"transport_disconnect()\" in \"run_remote_archiver()\" of\n - \"builtin/archive.c\". This is because for a stateless connection or a\n - service like \"git-upload-pack-archive\", the remote helper may receive a\n - SIGPIPE signal and exit early. To have a graceful disconnect method by\n - calling \"do_take_over()\" will solve this issue.\n + The connect_helper() function is used as the connect method of the\n + vtable in \"transport-helper.c\", and it is called by transport_connect()\n + in \"transport.c\" to setup a connection. The only place that we call\n + transport_connect() so far is in \"builtin/archive.c\". Without running\n + do_take_over(), it may fail to call transport_disconnect() in\n + run_remote_archiver() of \"builtin/archive.c\". This is because for a\n + stateless connection or a service like \"git-upload-pack-archive\", the\n + remote helper may receive a SIGPIPE signal and exit early. To have a\n + graceful disconnect method by calling do_take_over() will solve this\n + issue.\n +\n + The subsequent commit will introduce remote archive over a stateless-rpc\n + connection.\n \n Signed-off-by: Jiang Xin \n \n3: 1e305394ee ! 4: a38ac182d6 archive: support remote archive from stateless transport\n @@ Commit message\n capabilities when connecting to remote-helper, so do not send them\n in \"remote-curl.c\" for the \"git-upload-archive\" service.\n \n + Helped-by: Eric Sunshine \n Signed-off-by: Jiang Xin \n \n ## http-backend.c ##\n @@ http-backend.c: static void check_content_type(struct strbuf *hdr, const char *a\n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n -\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n -+\tconst char *argv[4];\n ++\tstruct strvec argv = STRVEC_INIT;\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n -+\tif (!strcmp(service_name, \"git-upload-archive\")) {\n -+\t\targv[1] = \".\";\n -+\t\targv[2] = NULL;\n -+\t} else {\n -+\t\targv[1] = \"--stateless-rpc\";\n -+\t\targv[2] = \".\";\n -+\t\targv[3] = NULL;\n -+\t}\n ++\tstrvec_push(&argv, svc->name);\n ++\tif (strcmp(service_name, \"git-upload-archive\"))\n ++\t\tstrvec_push(&argv, \"--stateless-rpc\");\n ++\tstrvec_push(&argv, \".\");\n +\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n +@@ http-backend.c: static void service_rpc(struct strbuf *hdr, char *service_name)\n + \n + \tend_headers(hdr);\n + \n +-\targv[0] = svc->name;\n +-\trun_service(argv, svc->buffer_input);\n ++\trun_service(argv.v, svc->buffer_input);\n + \tstrbuf_release(&buf);\n ++\tstrvec_clear(&argv);\n + }\n + \n + static int dead;\n @@ http-backend.c: static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n---\n\nJiang Xin (4):\n transport-helper: no connection restriction in connect_helper\n transport-helper: call do_take_over() in process_connect\n transport-helper: call do_take_over() in connect_helper\n archive: support remote archive from stateless transport\n\n http-backend.c | 15 +++++++++++----\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++\n transport-helper.c | 29 +++++++++++++----------------\n 4 files changed, 65 insertions(+), 23 deletions(-)\n\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482652","messageId":"e660fc79b64a1bd02bdb1e1ea6f95701ae31a68f.1696432594.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T15:21:40Z","receivedAt":"2023-10-04T15:21:52Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen commit b236752a (Support remote archive from all smart transports,\n2009-12-09) added \"remote archive\" support for \"smart transports\", it\nwas for transport that supports the \".connect\" method. The\n\"connect_helper()\" function protected itself from getting called for a\ntransport without the method before calling process_connect_service(),\nwhich did not work with such a transport.\n\nLater, commit edc9caf7 (transport-helper: introduce stateless-connect,\n2018-03-15) added a way for a transport without the \".connect\" method\nto establish a \"stateless\" connection in protocol-v2, which\nprocess_connect_service() was taught to handle the \"stateless\"\nconnection, making the old safety valve in its caller that insisted\nthat \".connect\" method must be defined too strict, and forgot to loosen\nit.\n\nRemove the restriction in the \"connect_helper()\" function and give the\nfunction \"process_connect_service()\" the opportunity to establish a\nconnection using \".connect\" or \".stateless_connect\" for protocol v2. So\nwe can connect with a stateless-rpc and do something useful. E.g., in a\nlater commit, implements remote archive for a repository over HTTP\nprotocol.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482653","messageId":"01699822c3f8ecda2c25dc2e2922c1f993c8beb2.1696432594.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 3/4] transport-helper: call do_take_over() in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T15:21:42Z","receivedAt":"2023-10-04T15:21:54Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAfter successfully connecting to the smart transport by calling\nprocess_connect_service() in connect_helper(), run do_take_over() to\nreplace the old vtable with a new one which has methods ready for the\nsmart transport connection.\n\nThe connect_helper() function is used as the connect method of the\nvtable in \"transport-helper.c\", and it is called by transport_connect()\nin \"transport.c\" to setup a connection. The only place that we call\ntransport_connect() so far is in \"builtin/archive.c\". Without running\ndo_take_over(), it may fail to call transport_disconnect() in\nrun_remote_archiver() of \"builtin/archive.c\". This is because for a\nstateless connection or a service like \"git-upload-pack-archive\", the\nremote helper may receive a SIGPIPE signal and exit early. To have a\ngraceful disconnect method by calling do_take_over() will solve this\nissue.\n\nThe subsequent commit will introduce remote archive over a stateless-rpc\nconnection.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 51088cc03a..3b036ae1ca 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482654","messageId":"a38ac182d6895f83fd6b92995ea08c5473ca24bb.1696432594.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 4/4] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T15:21:43Z","receivedAt":"2023-10-04T15:22:00Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nEven though we can establish a stateless connection, we still cannot\narchive the remote repository using a stateless HTTP protocol. Try the\nfollowing steps to make it work.\n\n 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n\n 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n protocol version, instead of use the \"git-upload-archive\" service.\n\n 3. \"git-archive\" does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in \"remote-curl.c\" for the \"git-upload-archive\" service.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n http-backend.c | 15 +++++++++++----\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++\n transport-helper.c | 3 ++-\n 4 files changed, 54 insertions(+), 8 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..6a2c919839 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tstruct strvec argv = STRVEC_INIT;\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tstrvec_push(&argv, svc->name);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\tstrvec_push(&argv, \"--stateless-rpc\");\n+\tstrvec_push(&argv, \".\");\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \n \tend_headers(hdr);\n \n-\targv[0] = svc->name;\n-\trun_service(argv, svc->buffer_input);\n+\trun_service(argv.v, svc->buffer_input);\n \tstrbuf_release(&buf);\n+\tstrvec_clear(&argv);\n }\n \n static int dead;\n@@ -723,7 +729,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc}\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..80123c1e06 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,34 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\ttest_when_finished \"rm -f d5.zip\" &&\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=d5.zip HEAD &&\n+\ttest_cmp_bin d.zip d5.zip\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 3b036ae1ca..566f7473df 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482655","messageId":"e3dc18caa91bd16d95dc7c2bbd0e6eceedefe636.1696432594.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v3 2/4] transport-helper: call do_take_over() in process_connect","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-10-04T15:21:41Z","receivedAt":"2023-10-04T15:22:01Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe existing pattern among all callers of process_connect() seems to be\n\n if (process_connect(...)) {\n do_take_over();\n ... dispatch to the underlying method ...\n }\n ... otherwise implement the fallback ...\n\nwhere the return value from process_connect() is the return value of the\ncall it makes to process_connect_service().\n\nIt is safe to make a refactor by moving the call of do_take_over()\ninto the function process_connect().\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..51088cc03a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -645,6 +645,7 @@ static int process_connect(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tconst char *name;\n \tconst char *exec;\n+\tint ret;\n \n \tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n \tif (for_push)\n@@ -652,7 +653,10 @@ static int process_connect(struct transport *transport,\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\treturn process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, name, exec);\n+\tif (ret)\n+\t\tdo_take_over(transport);\n+\treturn ret;\n }\n \n static int connect_helper(struct transport *transport, const char *name,\n@@ -682,10 +686,8 @@ static int fetch_refs(struct transport *transport,\n \n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->fetch_refs(transport, nr_heads, to_fetch);\n-\t}\n \n \t/*\n \t * If we reach here, then the server, the client, and/or the transport\n@@ -1142,10 +1144,8 @@ static int push_refs(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \n-\tif (process_connect(transport, 1)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 1))\n \t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n-\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr,\n@@ -1186,11 +1186,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, for_push)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, for_push))\n \t\treturn transport->vtable->get_refs_list(transport, for_push,\n \t\t\t\t\t\t\ttransport_options);\n-\t}\n \n \treturn get_refs_list_using_list(transport, for_push);\n }\n@@ -1274,10 +1272,8 @@ static int get_bundle_uri(struct transport *transport)\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->get_bundle_uri(transport);\n-\t}\n \n \treturn -1;\n }\n-- \n2.40.1.50.gf560bcc116.dirty\n\n"},{"id":"482667","messageId":"xmqqlecip7gx.fsf@gitster.g","threadId":"60243","inReplyTo":"e3dc18caa91bd16d95dc7c2bbd0e6eceedefe636.1696432594.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v3 2/4] transport-helper: call do_take_over() in process_connect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-04T18:29:02Z","receivedAt":"2023-10-04T18:29:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> It is safe to make a refactor by moving the call of do_take_over()\n> into the function process_connect().\n\n\"It is safe\" only explains why it does not hurt, and does not\nexplain why it is a good idea to do so, though.\n"},{"id":"485650","messageId":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1696432593.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 0/4] support remote archive via stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T14:13:41Z","receivedAt":"2023-12-14T14:13:48Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\n# Change since v3:\n\n1. Update commit message of patch 2/4.\n2. Add comments in t5003.\n\n# range-diff v3...v4\n\n1: 1818d8e30e = 1: d343585cb5 transport-helper: no connection restriction in connect_helper\n2: b57524bc91 ! 2: 65fb67523c transport-helper: call do_take_over() in process_connect\n @@ Commit message\n where the return value from process_connect() is the return value of the\n call it makes to process_connect_service().\n \n - It is safe to make a refactor by moving the call of do_take_over()\n - into the function process_connect().\n + Move the call of do_take_over() inside process_connect(), so that\n + calling the process_connect() function is more concise and will not\n + miss do_take_over().\n \n Suggested-by: Junio C Hamano \n Signed-off-by: Jiang Xin \n3: 7ce60e3b9a = 3: 109a1fffde transport-helper: call do_take_over() in connect_helper\n4: 626f903508 ! 4: eb905259fe archive: support remote archive from stateless transport\n @@ Commit message\n \n Helped-by: Eric Sunshine \n Signed-off-by: Jiang Xin \n @@ t/t5003-archive-zip.sh: check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n ++# Test remote archive over HTTP protocol.\n ++#\n ++# Note: this should be the last part of this test suite, because\n ++# by including lib-httpd.sh, the test may end early if httpd tests\n ++# should not be run.\n ++#\n +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n +start_httpd\n +\n @@ t/t5003-archive-zip.sh: check_zip with_untracked2\n +setup_askpass_helper\n +\n +test_expect_success 'remote archive does not work with protocol v1' '\n -+\ttest_when_finished \"rm -f d5.zip\" &&\n +\ttest_must_fail git -c protocol.version=1 archive \\\n +\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n -+\t\t--output=d5.zip HEAD >actual 2>&1 &&\n ++\t\t--output=remote-http.zip HEAD >actual 2>&1 &&\n +\tcat >expect <<-EOF &&\n +\tfatal: can${SQ}t connect to subservice git-upload-archive\n +\tEOF\n @@ t/t5003-archive-zip.sh: check_zip with_untracked2\n +'\n +\n +test_expect_success 'archive remote http repository' '\n -+\ttest_when_finished \"rm -f d5.zip\" &&\n +\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n -+\t\t--output=d5.zip HEAD &&\n -+\ttest_cmp_bin d.zip d5.zip\n ++\t\t--output=remote-http.zip HEAD &&\n ++\ttest_cmp_bin d.zip remote-http.zip\n +'\n +\n test_done\n\nJiang Xin (4):\n transport-helper: no connection restriction in connect_helper\n transport-helper: call do_take_over() in process_connect\n transport-helper: call do_take_over() in connect_helper\n archive: support remote archive from stateless transport\n\n http-backend.c | 15 +++++++++++----\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n transport-helper.c | 29 +++++++++++++----------------\n 4 files changed, 69 insertions(+), 23 deletions(-)\n\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485651","messageId":"d343585cb5e696f521c2ee1dd6c0f0c2d86de113.1702562879.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T14:13:42Z","receivedAt":"2023-12-14T14:13:49Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen commit b236752a (Support remote archive from all smart transports,\n2009-12-09) added \"remote archive\" support for \"smart transports\", it\nwas for transport that supports the \".connect\" method. The\n\"connect_helper()\" function protected itself from getting called for a\ntransport without the method before calling process_connect_service(),\nwhich did not work with such a transport.\n\nLater, commit edc9caf7 (transport-helper: introduce stateless-connect,\n2018-03-15) added a way for a transport without the \".connect\" method\nto establish a \"stateless\" connection in protocol-v2, which\nprocess_connect_service() was taught to handle the \"stateless\"\nconnection, making the old safety valve in its caller that insisted\nthat \".connect\" method must be defined too strict, and forgot to loosen\nit.\n\nRemove the restriction in the \"connect_helper()\" function and give the\nfunction \"process_connect_service()\" the opportunity to establish a\nconnection using \".connect\" or \".stateless_connect\" for protocol v2. So\nwe can connect with a stateless-rpc and do something useful. E.g., in a\nlater commit, implements remote archive for a repository over HTTP\nprotocol.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485652","messageId":"65fb67523c5c052fae466cbd8ce966e0f6265297.1702562879.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 2/4] transport-helper: call do_take_over() in process_connect","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T14:13:43Z","receivedAt":"2023-12-14T14:13:50Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe existing pattern among all callers of process_connect() seems to be\n\n if (process_connect(...)) {\n do_take_over();\n ... dispatch to the underlying method ...\n }\n ... otherwise implement the fallback ...\n\nwhere the return value from process_connect() is the return value of the\ncall it makes to process_connect_service().\n\nMove the call of do_take_over() inside process_connect(), so that\ncalling the process_connect() function is more concise and will not\nmiss do_take_over().\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..51088cc03a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -645,6 +645,7 @@ static int process_connect(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tconst char *name;\n \tconst char *exec;\n+\tint ret;\n \n \tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n \tif (for_push)\n@@ -652,7 +653,10 @@ static int process_connect(struct transport *transport,\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\treturn process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, name, exec);\n+\tif (ret)\n+\t\tdo_take_over(transport);\n+\treturn ret;\n }\n \n static int connect_helper(struct transport *transport, const char *name,\n@@ -682,10 +686,8 @@ static int fetch_refs(struct transport *transport,\n \n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->fetch_refs(transport, nr_heads, to_fetch);\n-\t}\n \n \t/*\n \t * If we reach here, then the server, the client, and/or the transport\n@@ -1142,10 +1144,8 @@ static int push_refs(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \n-\tif (process_connect(transport, 1)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 1))\n \t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n-\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr,\n@@ -1186,11 +1186,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, for_push)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, for_push))\n \t\treturn transport->vtable->get_refs_list(transport, for_push,\n \t\t\t\t\t\t\ttransport_options);\n-\t}\n \n \treturn get_refs_list_using_list(transport, for_push);\n }\n@@ -1274,10 +1272,8 @@ static int get_bundle_uri(struct transport *transport)\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->get_bundle_uri(transport);\n-\t}\n \n \treturn -1;\n }\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485653","messageId":"af8fd2a4eb8783be4a62973bfd2135da4568570e.1702562879.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T14:13:44Z","receivedAt":"2023-12-14T14:13:51Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAfter successfully connecting to the smart transport by calling\nprocess_connect_service() in connect_helper(), run do_take_over() to\nreplace the old vtable with a new one which has methods ready for the\nsmart transport connection.\n\nThe connect_helper() function is used as the connect method of the\nvtable in \"transport-helper.c\", and it is called by transport_connect()\nin \"transport.c\" to setup a connection. The only place that we call\ntransport_connect() so far is in \"builtin/archive.c\". Without running\ndo_take_over(), it may fail to call transport_disconnect() in\nrun_remote_archiver() of \"builtin/archive.c\". This is because for a\nstateless connection or a service like \"git-upload-pack-archive\", the\nremote helper may receive a SIGPIPE signal and exit early. To have a\ngraceful disconnect method by calling do_take_over() will solve this\nissue.\n\nThe subsequent commit will introduce remote archive over a stateless-rpc\nconnection.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 51088cc03a..3b036ae1ca 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"485654","messageId":"18b9a11d3be9d804e8d22d054ea881b8336d170c.1702562879.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v4 4/4] archive: support remote archive from stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2023-12-14T14:13:45Z","receivedAt":"2023-12-14T14:13:52Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nEven though we can establish a stateless connection, we still cannot\narchive the remote repository using a stateless HTTP protocol. Try the\nfollowing steps to make it work.\n\n 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n\n 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n protocol version, instead of use the \"git-upload-archive\" service.\n\n 3. \"git-archive\" does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in \"remote-curl.c\" for the \"git-upload-archive\" service.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n http-backend.c | 15 +++++++++++----\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n transport-helper.c | 3 ++-\n 4 files changed, 58 insertions(+), 8 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..6a2c919839 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tstruct strvec argv = STRVEC_INIT;\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tstrvec_push(&argv, svc->name);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\tstrvec_push(&argv, \"--stateless-rpc\");\n+\tstrvec_push(&argv, \".\");\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \n \tend_headers(hdr);\n \n-\targv[0] = svc->name;\n-\trun_service(argv, svc->buffer_input);\n+\trun_service(argv.v, svc->buffer_input);\n \tstrbuf_release(&buf);\n+\tstrvec_clear(&argv);\n }\n \n static int dead;\n@@ -723,7 +729,8 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n-\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n+\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc}\n };\n \n static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..961c6aac25 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,38 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+# Test remote archive over HTTP protocol.\n+#\n+# Note: this should be the last part of this test suite, because\n+# by including lib-httpd.sh, the test may end early if httpd tests\n+# should not be run.\n+#\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD &&\n+\ttest_cmp_bin d.zip remote-http.zip\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 3b036ae1ca..566f7473df 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n-- \n2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\n"},{"id":"486694","messageId":"owlyy1cvhua5.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"d343585cb5e696f521c2ee1dd6c0f0c2d86de113.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-12T07:42:42Z","receivedAt":"2024-01-12T07:42:45Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> When commit b236752a (Support remote archive from all smart transports,\n> 2009-12-09) added \"remote archive\" support for \"smart transports\", it\n> was for transport that supports the \".connect\" method. The\n> \"connect_helper()\" function protected itself from getting called for a\n> transport without the method before calling process_connect_service(),\n\nOK.\n\n> which did not work with such a transport.\n\nHow about 'which only worked with the \".connect\" method.' ?\n\n>\n> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n> 2018-03-15) added a way for a transport without the \".connect\" method\n> to establish a \"stateless\" connection in protocol-v2, which\n\ns/which/where\n\n> process_connect_service() was taught to handle the \"stateless\"\n> connection,\n\nI think using 'the \".stateless_connect\" method' is more consistent with\nthe rest of this text.\n\n> making the old safety valve in its caller that insisted\n> that \".connect\" method must be defined too strict, and forgot to loosen\n> it.\n\nI think just \"...making the old protection too strict. But edc9caf7\nforgot to adjust this protection accordingly.\" is simpler to read.\n\n> Remove the restriction in the \"connect_helper()\" function and give the\n> function \"process_connect_service()\" the opportunity to establish a\n> connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n> we can connect with a stateless-rpc and do something useful. E.g., in a\n> later commit, implements remote archive for a repository over HTTP\n> protocol.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Jiang Xin \n> ---\n> transport-helper.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 49811ef176..2e127d24a5 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> -\t\tdie(_(\"operation not supported by protocol\"));\n\nShould we still terminate early here if both data->connect and\ndata->stateless_connect are not truthy? It would save a few CPU cycles,\nbut even better, remain true to the the original intent of the code.\nMaybe there was a really good reason to terminate early here that we're\nnot aware of?\n\nBut also, what about the case where both are enabled? Should we print an\nerror message? (Maybe this concern is outside the scope of this series?)\n\n> \tif (!process_connect_service(transport, name, exec))\n> \t\tdie(_(\"can't connect to subservice %s\"), name);\n> -- \n> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n"},{"id":"486696","messageId":"owlyttnjhtmz.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"af8fd2a4eb8783be4a62973bfd2135da4568570e.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-12T07:56:36Z","receivedAt":"2024-01-12T07:56:38Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> After successfully connecting to the smart transport by calling\n> process_connect_service() in connect_helper(), run do_take_over() to\n> replace the old vtable with a new one which has methods ready for the\n> smart transport connection.\n> \n>\n> The connect_helper() function is used as the connect method of the\n> vtable in \"transport-helper.c\", and it is called by transport_connect()\n> in \"transport.c\" to setup a connection. The only place that we call\n> transport_connect() so far is in \"builtin/archive.c\". Without running\n> do_take_over(), it may fail to call transport_disconnect() in\n> run_remote_archiver() of \"builtin/archive.c\". This is because for a\n> stateless connection or a service like \"git-upload-pack-archive\", the\n\nThere is \"git-upload-pack\" and \"git-upload-archive\". Which one did you\nmean here? Or did you mean both?\n\n> remote helper may receive a SIGPIPE signal and exit early. To have a\n> graceful disconnect method by calling do_take_over() will solve this\n> issue.\n\nAre you saying that this patch fixes an existing bug? That is, is this\npatch independent of the first patch (transport-helper: no connection\nrestriction in connect_helper) in this series?\n\n> The subsequent commit will introduce remote archive over a stateless-rpc\n> connection.\n\nDoes the next commit depend on this patch? If not, I think you can drop\nthis paragraph.\n\n> Signed-off-by: Jiang Xin \n> ---\n> transport-helper.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 51088cc03a..3b036ae1ca 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \tfd[0] = data->helper->out;\n> \tfd[1] = data->helper->in;\n> +\n> +\tdo_take_over(transport);\n> \treturn 0;\n> }\n> \n> -- \n> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n"},{"id":"486699","messageId":"owlyr0inhswp.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"18b9a11d3be9d804e8d22d054ea881b8336d170c.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v4 4/4] archive: support remote archive from stateless transport","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-12T08:12:22Z","receivedAt":"2024-01-12T08:12:24Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> Even though we can establish a stateless connection, we still cannot\n> archive the remote repository using a stateless HTTP protocol. Try the\n> following steps to make it work.\n\nAs Yoda once said, \"Do or do not, there is no try\". Here I think you\nmeant \"Do\".\n\n> 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n>\n> 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n> protocol version, instead of use the \"git-upload-archive\" service.\n>\n> 3. \"git-archive\" does not expect to see protocol version and\n> capabilities when connecting to remote-helper, so do not send them\n> in \"remote-curl.c\" for the \"git-upload-archive\" service.\n\nIt would be great if you could break up this patch into 3 smaller\npatches. Or 4 patches if you decide to move the new test cases into their\nown patch.\n\n> @@ -723,7 +729,8 @@ static struct service_cmd {\n> \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n> \n> \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n> -\t{\"POST\", \"/git-receive-pack$\", service_rpc}\n> +\t{\"POST\", \"/git-receive-pack$\", service_rpc},\n> +\t{\"POST\", \"/git-upload-archive$\", service_rpc}\n> };\n\nStyle nit: it might be cleaner to put the new \"git-upload-archive\" just\nabove \"git-upload-pack\" because the two have a special relationship now.\n"},{"id":"486739","messageId":"xmqqfrz28bn9.fsf@gitster.g","threadId":"60243","inReplyTo":"owlyy1cvhua5.fsf@fine.c.googlers.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T21:50:02Z","receivedAt":"2024-01-12T21:50:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Arver writes:\n\n> Jiang Xin writes:\n>\n>> From: Jiang Xin \n>>\n>> When commit b236752a (Support remote archive from all smart transports,\n>> 2009-12-09) added \"remote archive\" support for \"smart transports\", it\n>> was for transport that supports the \".connect\" method. The\n>> \"connect_helper()\" function protected itself from getting called for a\n>> transport without the method before calling process_connect_service(),\n>\n> OK.\n>\n>> which did not work with such a transport.\n>\n> How about 'which only worked with the \".connect\" method.' ?\n>\n>>\n>> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n>> 2018-03-15) added a way for a transport without the \".connect\" method\n>> to establish a \"stateless\" connection in protocol-v2, which\n>\n> s/which/where\n>\n>> process_connect_service() was taught to handle the \"stateless\"\n>> connection,\n>\n> I think using 'the \".stateless_connect\" method' is more consistent with\n> the rest of this text.\n>\n>> making the old safety valve in its caller that insisted\n>> that \".connect\" method must be defined too strict, and forgot to loosen\n>> it.\n>\n> I think just \"...making the old protection too strict. But edc9caf7\n> forgot to adjust this protection accordingly.\" is simpler to read.\n>\n>> Remove the restriction in the \"connect_helper()\" function and give the\n>> function \"process_connect_service()\" the opportunity to establish a\n>> connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n>> we can connect with a stateless-rpc and do something useful. E.g., in a\n>> later commit, implements remote archive for a repository over HTTP\n>> protocol.\n>>\n>> Helped-by: Junio C Hamano \n>> Signed-off-by: Jiang Xin \n>> ---\n>> transport-helper.c | 2 --\n>> 1 file changed, 2 deletions(-)\n>>\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index 49811ef176..2e127d24a5 100644\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n>> \n>> \t/* Get_helper so connect is inited. */\n>> \tget_helper(transport);\n>> -\tif (!data->connect)\n>> -\t\tdie(_(\"operation not supported by protocol\"));\n>\n> Should we still terminate early here if both data->connect and\n> data->stateless_connect are not truthy? It would save a few CPU cycles,\n> but even better, remain true to the the original intent of the code.\n> Maybe there was a really good reason to terminate early here that we're\n> not aware of?\n>\n> But also, what about the case where both are enabled? Should we print an\n> error message? (Maybe this concern is outside the scope of this series?)\n>\n>> \tif (!process_connect_service(transport, name, exec))\n>> \t\tdie(_(\"can't connect to subservice %s\"), name);\n>> -- \n>> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev\n\nThanks for a review to get the topic that hasn't seen much reviews\nunstuck. Very much appreciated.\n"},{"id":"486824","messageId":"CANYiYbFOa-E8Pivhgn_nmy982fn7VPtb803bewnC_UV7qY3xcw@mail.gmail.com","threadId":"60243","inReplyTo":"owlyy1cvhua5.fsf@fine.c.googlers.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T09:04:28Z","receivedAt":"2024-01-16T09:04:40Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Jan 12, 2024 at 3:42 PM Linus Arver wrote:\n>\n> Jiang Xin writes:\n>\n> > From: Jiang Xin \n> >\n> > When commit b236752a (Support remote archive from all smart transports,\n> > 2009-12-09) added \"remote archive\" support for \"smart transports\", it\n> > was for transport that supports the \".connect\" method. The\n> > \"connect_helper()\" function protected itself from getting called for a\n> > transport without the method before calling process_connect_service(),\n>\n> OK.\n>\n> > which did not work with such a transport.\n>\n> How about 'which only worked with the \".connect\" method.' ?\n>\n> >\n> > Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n> > 2018-03-15) added a way for a transport without the \".connect\" method\n> > to establish a \"stateless\" connection in protocol-v2, which\n>\n> s/which/where\n>\n> > process_connect_service() was taught to handle the \"stateless\"\n> > connection,\n>\n> I think using 'the \".stateless_connect\" method' is more consistent with\n> the rest of this text.\n>\n> > making the old safety valve in its caller that insisted\n> > that \".connect\" method must be defined too strict, and forgot to loosen\n> > it.\n>\n> I think just \"...making the old protection too strict. But edc9caf7\n> forgot to adjust this protection accordingly.\" is simpler to read.\n\nThanks for the above suggestions, and will update in next reroll.\n\n> > Remove the restriction in the \"connect_helper()\" function and give the\n> > function \"process_connect_service()\" the opportunity to establish a\n> > connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n> > we can connect with a stateless-rpc and do something useful. E.g., in a\n> > later commit, implements remote archive for a repository over HTTP\n> > protocol.\n> >\n> > Helped-by: Junio C Hamano \n> > Signed-off-by: Jiang Xin \n> > ---\n> > transport-helper.c | 2 --\n> > 1 file changed, 2 deletions(-)\n> >\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index 49811ef176..2e127d24a5 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> >\n> > /* Get_helper so connect is inited. */\n> > get_helper(transport);\n> > - if (!data->connect)\n> > - die(_(\"operation not supported by protocol\"));\n>\n> Should we still terminate early here if both data->connect and\n> data->stateless_connect are not truthy? It would save a few CPU cycles,\n> but even better, remain true to the the original intent of the code.\n> Maybe there was a really good reason to terminate early here that we're\n> not aware of?\n>\n\nIt's not necessary to check data->connect here, because it will\nterminate if fail to connect by calling the function\n\"process_connect_service()\".\n\n> But also, what about the case where both are enabled? Should we print an\n> error message? (Maybe this concern is outside the scope of this series?)\n\nIn the function \"process_connect_service()\", we can see that \"connect\"\nhas a higher priority than \"stateless-connect\".\n\n>\n> > if (!process_connect_service(transport, name, exec))\n> > die(_(\"can't connect to subservice %s\"), name);\n\nRegardless of whether \"connect\" or \"stateless-connect\" is used, the\nfunction process_connect_service() will return 1 if the connection is\nsuccessful. If the connection fails, it will terminate here.\n"},{"id":"486826","messageId":"CANYiYbGk6v1dUASxMGpCAC6rtumm3i=ybUC3C_43HRtvBHyc1w@mail.gmail.com","threadId":"60243","inReplyTo":"owlyttnjhtmz.fsf@fine.c.googlers.com","subject":"Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T09:41:27Z","receivedAt":"2024-01-16T09:41:39Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Jan 12, 2024 at 3:56 PM Linus Arver wrote:\n>\n> Jiang Xin writes:\n>\n> > From: Jiang Xin \n> >\n> > After successfully connecting to the smart transport by calling\n> > process_connect_service() in connect_helper(), run do_take_over() to\n> > replace the old vtable with a new one which has methods ready for the\n> > smart transport connection.\n> >\n> >\n> > The connect_helper() function is used as the connect method of the\n> > vtable in \"transport-helper.c\", and it is called by transport_connect()\n> > in \"transport.c\" to setup a connection. The only place that we call\n> > transport_connect() so far is in \"builtin/archive.c\". Without running\n> > do_take_over(), it may fail to call transport_disconnect() in\n> > run_remote_archiver() of \"builtin/archive.c\". This is because for a\n> > stateless connection or a service like \"git-upload-pack-archive\", the\n>\n> There is \"git-upload-pack\" and \"git-upload-archive\". Which one did you\n> mean here? Or did you mean both?\n>\n\nShould be \"git-upload-archive\".\n\n> > remote helper may receive a SIGPIPE signal and exit early. To have a\n> > graceful disconnect method by calling do_take_over() will solve this\n> > issue.\n>\n> Are you saying that this patch fixes an existing bug? That is, is this\n> patch independent of the first patch (transport-helper: no connection\n> restriction in connect_helper) in this series?\n>\n> > The subsequent commit will introduce remote archive over a stateless-rpc\n> > connection.\n>\n> Does the next commit depend on this patch? If not, I think you can drop\n> this paragraph.\n\nOne test case in next commit will break without this patch. I will\nmove this patch to the end of this series.\n"},{"id":"486831","messageId":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1702562879.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 0/6] support remote archive via stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:24Z","receivedAt":"2024-01-16T13:39:34Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\n\"git archive --remote=\" learned to talk over the smart\nhttp (aka stateless) transport.\n\n# Changes since v4\n\n1. Change commit messages and order of commits.\n2. Split the last commit of v4 into three seperate commits.\n\n\n# range-diff v4...v5\n\n1: da80391037 ! 1: f3fef46c05 transport-helper: no connection restriction in connect_helper\n @@ Commit message\n was for transport that supports the \".connect\" method. The\n \"connect_helper()\" function protected itself from getting called for a\n transport without the method before calling process_connect_service(),\n - which did not work with such a transport.\n + which only worked with the \".connect\" method.\n \n Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n 2018-03-15) added a way for a transport without the \".connect\" method\n - to establish a \"stateless\" connection in protocol-v2, which\n - process_connect_service() was taught to handle the \"stateless\"\n - connection, making the old safety valve in its caller that insisted\n - that \".connect\" method must be defined too strict, and forgot to loosen\n - it.\n + to establish a \"stateless\" connection in protocol-v2, where\n + process_connect_service() was taught to handle the \".stateless_connect\"\n + method, making the old protection too strict. But commit edc9caf7 forgot\n + to adjust this protection accordingly.\n \n Remove the restriction in the \"connect_helper()\" function and give the\n function \"process_connect_service()\" the opportunity to establish a\n @@ Commit message\n protocol.\n \n Helped-by: Junio C Hamano \n + Helped-by: Linus Arver \n Signed-off-by: Jiang Xin \n \n ## transport-helper.c ##\n-: ---------- > 2: 6be331b22d remote-curl: supports git-upload-archive service\n-: ---------- > 3: aabc8e1a2a transport-helper: protocol-v2 supports upload-archive\n4: a21a80dae9 ! 4: fdab4abb43 archive: support remote archive from stateless transport\n @@ Metadata\n Author: Jiang Xin \n \n ## Commit message ##\n - archive: support remote archive from stateless transport\n + http-backend: new rpc-service for git-upload-archive\n \n - Even though we can establish a stateless connection, we still cannot\n - archive the remote repository using a stateless HTTP protocol. Try the\n - following steps to make it work.\n + Add new rpc-service \"upload-archive\" in http-backend to add server side\n + support for remote archive over HTTP/HTTPS protocols.\n \n - 1. Add support for \"git-upload-archive\" service in \"http-backend\".\n -\n - 2. Use the URL \".../info/refs?service=git-upload-pack\" to detect the\n - protocol version, instead of use the \"git-upload-archive\" service.\n -\n - 3. \"git-archive\" does not expect to see protocol version and\n - capabilities when connecting to remote-helper, so do not send them\n - in \"remote-curl.c\" for the \"git-upload-archive\" service.\n + Also add new test cases in t5003. In the test case \"archive remote http\n + repository\", git-archive exits with a non-0 exit code even though we\n + create the archive correctly. It will be fixed in a later commit.\n \n Helped-by: Eric Sunshine \n Signed-off-by: Jiang Xin \n \t\tif (ret)\n\n ... (omitted) ...\n\n3: 870dc5fd21 ! 5: 6ac0c8e105 transport-helper: call do_take_over() in connect_helper\n @@ Commit message\n After successfully connecting to the smart transport by calling\n process_connect_service() in connect_helper(), run do_take_over() to\n replace the old vtable with a new one which has methods ready for the\n - smart transport connection.\n + smart transport connection. This will fix the exit code of git-archive\n + in test case \"archive remote http repository\" of t5003.\n \n The connect_helper() function is used as the connect method of the\n vtable in \"transport-helper.c\", and it is called by transport_connect()\n @@ Commit message\n transport_connect() so far is in \"builtin/archive.c\". Without running\n do_take_over(), it may fail to call transport_disconnect() in\n run_remote_archiver() of \"builtin/archive.c\". This is because for a\n - stateless connection or a service like \"git-upload-pack-archive\", the\n + stateless connection and a service like \"git-upload-archive\", the\n remote helper may receive a SIGPIPE signal and exit early. To have a\n graceful disconnect method by calling do_take_over() will solve this\n issue.\n \n - The subsequent commit will introduce remote archive over a stateless-rpc\n - connection.\n -\n + Helped-by: Linus Arver \n Signed-off-by: Jiang Xin \n \n + ## t/t5003-archive-zip.sh ##\n +@@ t/t5003-archive-zip.sh: test_expect_success 'remote archive does not work with protocol v1' '\n + '\n + \n + test_expect_success 'archive remote http repository' '\n +-\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n ++\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n + \t\t--output=remote-http.zip HEAD &&\n + \ttest_cmp_bin d.zip remote-http.zip\n + '\n +\n ## transport-helper.c ##\n @@ transport-helper.c: static int connect_helper(struct transport *transport, const char *name,\n \n2: 2f7060f7c5 = 6: 423a89c593 transport-helper: call do_take_over() in process_connect\n\nJiang Xin (6):\n transport-helper: no connection restriction in connect_helper\n remote-curl: supports git-upload-archive service\n transport-helper: protocol-v2 supports upload-archive\n http-backend: new rpc-service for git-upload-archive\n transport-helper: call do_take_over() in connect_helper\n transport-helper: call do_take_over() in process_connect\n\n http-backend.c | 13 ++++++++++---\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n transport-helper.c | 29 +++++++++++++----------------\n 4 files changed, 68 insertions(+), 22 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486832","messageId":"f3fef46c058968f6d0ad5a48776bd2f59ab45868.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 1/6] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:25Z","receivedAt":"2024-01-16T13:39:35Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen commit b236752a (Support remote archive from all smart transports,\n2009-12-09) added \"remote archive\" support for \"smart transports\", it\nwas for transport that supports the \".connect\" method. The\n\"connect_helper()\" function protected itself from getting called for a\ntransport without the method before calling process_connect_service(),\nwhich only worked with the \".connect\" method.\n\nLater, commit edc9caf7 (transport-helper: introduce stateless-connect,\n2018-03-15) added a way for a transport without the \".connect\" method\nto establish a \"stateless\" connection in protocol-v2, where\nprocess_connect_service() was taught to handle the \".stateless_connect\"\nmethod, making the old protection too strict. But commit edc9caf7 forgot\nto adjust this protection accordingly.\n\nRemove the restriction in the \"connect_helper()\" function and give the\nfunction \"process_connect_service()\" the opportunity to establish a\nconnection using \".connect\" or \".stateless_connect\" for protocol v2. So\nwe can connect with a stateless-rpc and do something useful. E.g., in a\nlater commit, implements remote archive for a repository over HTTP\nprotocol.\n\nHelped-by: Junio C Hamano \nHelped-by: Linus Arver \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.43.0\n\n"},{"id":"486833","messageId":"6be331b22d51e1f6f96cb0035d99db5b8cede676.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 2/6] remote-curl: supports git-upload-archive service","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:26Z","receivedAt":"2024-01-16T13:39:35Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAdd new service (git-upload-archive) support in remote-curl, so we can\nsupport remote archive over HTTP/HTTPS protocols. Differences between\ngit-upload-archive and other serices:\n\n 1. The git-archive command does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in remote-curl for the git-upload-archive service.\n\n 2. We need to detect protocol version by calling discover_refs(),\n Fallback to use the git-upload-pack service (which, like\n git-upload-archive, is a read-only operation) to discover protocol\n version.\n\nSigned-off-by: Jiang Xin \n---\n remote-curl.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\n-- \n2.43.0\n\n"},{"id":"486834","messageId":"aabc8e1a2a191059b88b439d53bc7b3735b979ca.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 3/6] transport-helper: protocol-v2 supports upload-archive","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:27Z","receivedAt":"2024-01-16T13:39:36Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe used to support only git-upload-pack service for protocol-v2. In\norder to support remote archive over HTTP/HTTPS protocols, add new\nservice support for git-upload-archive in protocol-v2.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..6fe9f4f208 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n-- \n2.43.0\n\n"},{"id":"486835","messageId":"fdab4abb43d4601006ff40f0f5ed89014f811b85.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 4/6] http-backend: new rpc-service for git-upload-archive","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:28Z","receivedAt":"2024-01-16T13:39:37Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAdd new rpc-service \"upload-archive\" in http-backend to add server side\nsupport for remote archive over HTTP/HTTPS protocols.\n\nAlso add new test cases in t5003. In the test case \"archive remote http\nrepository\", git-archive exits with a non-0 exit code even though we\ncreate the archive correctly. It will be fixed in a later commit.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n http-backend.c | 13 ++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..1ed1e29d07 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tstruct strvec argv = STRVEC_INIT;\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tstrvec_push(&argv, svc->name);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\tstrvec_push(&argv, \"--stateless-rpc\");\n+\tstrvec_push(&argv, \".\");\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \n \tend_headers(hdr);\n \n-\targv[0] = svc->name;\n-\trun_service(argv, svc->buffer_input);\n+\trun_service(argv.v, svc->buffer_input);\n \tstrbuf_release(&buf);\n+\tstrvec_clear(&argv);\n }\n \n static int dead;\n@@ -723,6 +729,7 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n \t{\"POST\", \"/git-receive-pack$\", service_rpc}\n };\n \ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..6f85bd3463 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,38 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+# Test remote archive over HTTP protocol.\n+#\n+# Note: this should be the last part of this test suite, because\n+# by including lib-httpd.sh, the test may end early if httpd tests\n+# should not be run.\n+#\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD &&\n+\ttest_cmp_bin d.zip remote-http.zip\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"486836","messageId":"6ac0c8e105febe526dc64182845832297656a8a5.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:29Z","receivedAt":"2024-01-16T13:39:38Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAfter successfully connecting to the smart transport by calling\nprocess_connect_service() in connect_helper(), run do_take_over() to\nreplace the old vtable with a new one which has methods ready for the\nsmart transport connection. This will fix the exit code of git-archive\nin test case \"archive remote http repository\" of t5003.\n\nThe connect_helper() function is used as the connect method of the\nvtable in \"transport-helper.c\", and it is called by transport_connect()\nin \"transport.c\" to setup a connection. The only place that we call\ntransport_connect() so far is in \"builtin/archive.c\". Without running\ndo_take_over(), it may fail to call transport_disconnect() in\nrun_remote_archiver() of \"builtin/archive.c\". This is because for a\nstateless connection and a service like \"git-upload-archive\", the\nremote helper may receive a SIGPIPE signal and exit early. To have a\ngraceful disconnect method by calling do_take_over() will solve this\nissue.\n\nHelped-by: Linus Arver \nSigned-off-by: Jiang Xin \n---\n t/t5003-archive-zip.sh | 2 +-\n transport-helper.c | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 6f85bd3463..961c6aac25 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '\n '\n \n test_expect_success 'archive remote http repository' '\n-\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n \t\t--output=remote-http.zip HEAD &&\n \ttest_cmp_bin d.zip remote-http.zip\n '\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6fe9f4f208..91381be622 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.43.0\n\n"},{"id":"486837","messageId":"423a89c59306e9c33851b7d36c685ddfce45736c.1705411391.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v5 6/6] transport-helper: call do_take_over() in process_connect","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-16T13:39:30Z","receivedAt":"2024-01-16T13:39:38Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe existing pattern among all callers of process_connect() seems to be\n\n if (process_connect(...)) {\n do_take_over();\n ... dispatch to the underlying method ...\n }\n ... otherwise implement the fallback ...\n\nwhere the return value from process_connect() is the return value of the\ncall it makes to process_connect_service().\n\nMove the call of do_take_over() inside process_connect(), so that\ncalling the process_connect() function is more concise and will not\nmiss do_take_over().\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91381be622..566f7473df 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tconst char *name;\n \tconst char *exec;\n+\tint ret;\n \n \tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n \tif (for_push)\n@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\treturn process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, name, exec);\n+\tif (ret)\n+\t\tdo_take_over(transport);\n+\treturn ret;\n }\n \n static int connect_helper(struct transport *transport, const char *name,\n@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,\n \n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->fetch_refs(transport, nr_heads, to_fetch);\n-\t}\n \n \t/*\n \t * If we reach here, then the server, the client, and/or the transport\n@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \n-\tif (process_connect(transport, 1)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 1))\n \t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n-\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr,\n@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, for_push)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, for_push))\n \t\treturn transport->vtable->get_refs_list(transport, for_push,\n \t\t\t\t\t\t\ttransport_options);\n-\t}\n \n \treturn get_refs_list_using_list(transport, for_push);\n }\n@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->get_bundle_uri(transport);\n-\t}\n \n \treturn -1;\n }\n-- \n2.43.0\n\n"},{"id":"487011","messageId":"owly1qaei8hw.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"CANYiYbFOa-E8Pivhgn_nmy982fn7VPtb803bewnC_UV7qY3xcw@mail.gmail.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-18T22:26:03Z","receivedAt":"2024-01-18T22:26:05Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n>> > Remove the restriction in the \"connect_helper()\" function and give the\n>> > function \"process_connect_service()\" the opportunity to establish a\n>> > connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n>> > we can connect with a stateless-rpc and do something useful. E.g., in a\n>> > later commit, implements remote archive for a repository over HTTP\n>> > protocol.\n>> >\n>> > Helped-by: Junio C Hamano \n>> > Signed-off-by: Jiang Xin \n>> > ---\n>> > transport-helper.c | 2 --\n>> > 1 file changed, 2 deletions(-)\n>> >\n>> > diff --git a/transport-helper.c b/transport-helper.c\n>> > index 49811ef176..2e127d24a5 100644\n>> > --- a/transport-helper.c\n>> > +++ b/transport-helper.c\n>> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n>> >\n>> > /* Get_helper so connect is inited. */\n>> > get_helper(transport);\n>> > - if (!data->connect)\n>> > - die(_(\"operation not supported by protocol\"));\n>>\n>> Should we still terminate early here if both data->connect and\n>> data->stateless_connect are not truthy? It would save a few CPU cycles,\n>> but even better, remain true to the the original intent of the code.\n>> Maybe there was a really good reason to terminate early here that we're\n>> not aware of?\n>>\n>\n> It's not necessary to check data->connect here, because it will\n> terminate if fail to connect by calling the function\n> \"process_connect_service()\".\n\nIn the process_connect_service() we have\n\n if (data->connect) {\n ...\n } else if (data->stateless_connect && ...) {\n ...\n }\n\n strbuf_release(&cmdbuf);\n return ret;\n\nand so if both data->connect and data->stateless_connect are false, that\nfunction could silently do nothing. IOW that function expects the\nconnection type to be guaranteed to be set, so it makes sense to check\nfor the correctness of this in the connect_helper().\n\n>> But also, what about the case where both are enabled? Should we print an\n>> error message? (Maybe this concern is outside the scope of this series?)\n>\n> In the function \"process_connect_service()\", we can see that \"connect\"\n> has a higher priority than \"stateless-connect\".\n\nWhat I mean is, does it make sense for connect_helper() to recognize\ninvalid or possibly buggy states? IOW, is having both data->connect and\ndata->stateless_connect enabled a bug? If we only ever set one or the\nother (we treat them as mutually exclusive) elsewhere in the codebase,\nand if we are doing the sort of \"correctness\" check in the\nconnect_helper(), then it makes sense to detect that both are set and\nprint an error or warning (as a programmer bug).\n"},{"id":"487066","messageId":"CANYiYbGy-APMD7Cw=m-=8dkMcXCp9c+x_6OCoBhWBfvUUWm2ow@mail.gmail.com","threadId":"60243","inReplyTo":"owly1qaei8hw.fsf@fine.c.googlers.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-19T10:56:19Z","receivedAt":"2024-01-19T10:56:31Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Fri, Jan 19, 2024 at 6:26 AM Linus Arver wrote:\n>\n> Jiang Xin writes:\n>\n> >> > Remove the restriction in the \"connect_helper()\" function and give the\n> >> > function \"process_connect_service()\" the opportunity to establish a\n> >> > connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n> >> > we can connect with a stateless-rpc and do something useful. E.g., in a\n> >> > later commit, implements remote archive for a repository over HTTP\n> >> > protocol.\n> >> >\n> >> > Helped-by: Junio C Hamano \n> >> > Signed-off-by: Jiang Xin \n> >> > ---\n> >> > transport-helper.c | 2 --\n> >> > 1 file changed, 2 deletions(-)\n> >> >\n> >> > diff --git a/transport-helper.c b/transport-helper.c\n> >> > index 49811ef176..2e127d24a5 100644\n> >> > --- a/transport-helper.c\n> >> > +++ b/transport-helper.c\n> >> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> >> >\n> >> > /* Get_helper so connect is inited. */\n> >> > get_helper(transport);\n> >> > - if (!data->connect)\n> >> > - die(_(\"operation not supported by protocol\"));\n> >>\n> >> Should we still terminate early here if both data->connect and\n> >> data->stateless_connect are not truthy? It would save a few CPU cycles,\n> >> but even better, remain true to the the original intent of the code.\n> >> Maybe there was a really good reason to terminate early here that we're\n> >> not aware of?\n> >>\n> >\n> > It's not necessary to check data->connect here, because it will\n> > terminate if fail to connect by calling the function\n> > \"process_connect_service()\".\n>\n> In the process_connect_service() we have\n>\n> if (data->connect) {\n> ...\n> } else if (data->stateless_connect && ...) {\n> ...\n> }\n>\n> strbuf_release(&cmdbuf);\n> return ret;\n>\n> and so if both data->connect and data->stateless_connect are false, that\n> function could silently do nothing. IOW that function expects the\n> connection type to be guaranteed to be set, so it makes sense to check\n> for the correctness of this in the connect_helper().\n\nIf both data->connect and data->stateless_connect are false,\nprocess_connect_service() will return 0 instead of making a connection\nand returning 1. The return value will be checked in the function\nconnect_helper() as follows:\n\n if (!process_connect_service(transport, name, exec))\n die(_(\"can't connect to subservice %s\"), name);\n\nSo I think it's not necessary to make double check in connect_helper().\n\n>\n> >> But also, what about the case where both are enabled? Should we print an\n> >> error message? (Maybe this concern is outside the scope of this series?)\n> >\n> > In the function \"process_connect_service()\", we can see that \"connect\"\n> > has a higher priority than \"stateless-connect\".\n>\n> What I mean is, does it make sense for connect_helper() to recognize\n> invalid or possibly buggy states? IOW, is having both data->connect and\n> data->stateless_connect enabled a bug? If we only ever set one or the\n> other (we treat them as mutually exclusive) elsewhere in the codebase,\n> and if we are doing the sort of \"correctness\" check in the\n> connect_helper(), then it makes sense to detect that both are set and\n> print an error or warning (as a programmer bug).\n\nThe best position to address the bug that both data->connect and\ndata->stateless_connect are enabled is in the function get_helper() as\nbelow:\n\n } else if (!strcmp(capname, \"connect\")) {\n data->connect = 1;\n } else if (!strcmp(capname, \"stateless-connect\")) {\n data->stateless_connect = 1;\n }\n ... ...\n if (data->connect && data->stateless_connect)\n die(\"cannot have both connect and stateless_connect enabled\");\n\nI consider this change to be off-topic and it will not be introduced\nin this series.\n\n--\nJiang Xin\n"},{"id":"487152","messageId":"owlycytvhhw0.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"CANYiYbGy-APMD7Cw=m-=8dkMcXCp9c+x_6OCoBhWBfvUUWm2ow@mail.gmail.com","subject":"Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-20T20:25:19Z","receivedAt":"2024-01-20T20:25:21Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> If both data->connect and data->stateless_connect are false,\n> process_connect_service() will return 0 instead of making a connection\n> and returning 1. The return value will be checked in the function\n> connect_helper() as follows:\n>\n> if (!process_connect_service(transport, name, exec))\n> die(_(\"can't connect to subservice %s\"), name);\n>\n> So I think it's not necessary to make double check in connect_helper().\n\nAh, thank you for the clarification.\n\n> The best position to address the bug that both data->connect and\n> data->stateless_connect are enabled is in the function get_helper() as\n> below:\n>\n> } else if (!strcmp(capname, \"connect\")) {\n> data->connect = 1;\n> } else if (!strcmp(capname, \"stateless-connect\")) {\n> data->stateless_connect = 1;\n> }\n> ... ...\n> if (data->connect && data->stateless_connect)\n> die(\"cannot have both connect and stateless_connect enabled\");\n>\n> I consider this change to be off-topic and it will not be introduced\n> in this series.\n\nSG.\n"},{"id":"487153","messageId":"owlya5ozhhr4.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"f3fef46c058968f6d0ad5a48776bd2f59ab45868.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v5 1/6] transport-helper: no connection restriction in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-20T20:28:15Z","receivedAt":"2024-01-20T20:28:17Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> Remove the restriction in the \"connect_helper()\" function and give the\n> function \"process_connect_service()\" the opportunity to establish a\n> connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n> we can connect with a stateless-rpc and do something useful. E.g., in a\n> later commit, implements remote archive for a repository over HTTP\n> protocol.\n\nNit: Perhaps add something like the following for the commit message?\n\n Removing the restriction does not change behavior, because\n process_connect_service() will return 0 if both data->connect and\n data->stateless_connect are false, and we'll still die() early.\n\n> Helped-by: Junio C Hamano \n> Helped-by: Linus Arver \n> Signed-off-by: Jiang Xin \n> ---\n> transport-helper.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 49811ef176..2e127d24a5 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \t/* Get_helper so connect is inited. */\n> \tget_helper(transport);\n> -\tif (!data->connect)\n> -\t\tdie(_(\"operation not supported by protocol\"));\n> \n> \tif (!process_connect_service(transport, name, exec))\n> \t\tdie(_(\"can't connect to subservice %s\"), name);\n> -- \n> 2.43.0\n"},{"id":"487154","messageId":"owly7ck3hhnh.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"6be331b22d51e1f6f96cb0035d99db5b8cede676.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v5 2/6] remote-curl: supports git-upload-archive service","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-20T20:30:26Z","receivedAt":"2024-01-20T20:30:28Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> Add new service (git-upload-archive) support in remote-curl, so we can\n> support remote archive over HTTP/HTTPS protocols. Differences between\n> git-upload-archive and other serices:\n\ns/serices/services\n\n> 1. The git-archive command does not expect to see protocol version and\n> capabilities when connecting to remote-helper, so do not send them\n> in remote-curl for the git-upload-archive service.\n>\n> 2. We need to detect protocol version by calling discover_refs(),\n\ns/,/.\n\n> Fallback to use the git-upload-pack service (which, like\n> git-upload-archive, is a read-only operation) to discover protocol\n> version.\n>\n> Signed-off-by: Jiang Xin \n> ---\n> remote-curl.c | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index ef05752ca5..ce6cb8ac05 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n> \t * establish a stateless connection, otherwise we need to tell the\n> \t * client to fallback to using other transport helper functions to\n> \t * complete their request.\n> +\t *\n> +\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n> +\t * to use \"git-upload-pack\" service to discover protocol version.\n> \t */\n> -\tdiscover = discover_refs(service_name, 0);\n> +\tif (!strcmp(service_name, \"git-upload-archive\"))\n> +\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n> +\telse\n> +\t\tdiscover = discover_refs(service_name, 0);\n> \tif (discover->version != protocol_v2) {\n> \t\tprintf(\"fallback\\n\");\n> \t\tfflush(stdout);\n> @@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n> \n> \t/*\n> \t * Dump the capability listing that we got from the server earlier\n> -\t * during the info/refs request.\n> +\t * during the info/refs request. This does not work with the\n> +\t * \"git-upload-archive\" service.\n> \t */\n> -\twrite_or_die(rpc.in, discover->buf, discover->len);\n> +\tif (strcmp(service_name, \"git-upload-archive\"))\n> +\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n> \n> \t/* Until we see EOF keep sending POSTs */\n> \twhile (1) {\n> -- \n> 2.43.0\n"},{"id":"487155","messageId":"owly4jf7hhaw.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"6ac0c8e105febe526dc64182845832297656a8a5.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-20T20:37:59Z","receivedAt":"2024-01-20T20:38:01Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> After successfully connecting to the smart transport by calling\n> process_connect_service() in connect_helper(), run do_take_over() to\n> replace the old vtable with a new one which has methods ready for the\n> smart transport connection. This will fix the exit code of git-archive\n\ns/will fix/fixes\n\n> in test case \"archive remote http repository\" of t5003.\n>\n> The connect_helper() function is used as the connect method of the\n> vtable in \"transport-helper.c\", and it is called by transport_connect()\n> in \"transport.c\" to setup a connection. The only place that we call\n> transport_connect() so far is in \"builtin/archive.c\". Without running\n> do_take_over(), it may fail to call transport_disconnect() in\n> run_remote_archiver() of \"builtin/archive.c\". This is because for a\n> stateless connection and a service like \"git-upload-archive\", the\n> remote helper may receive a SIGPIPE signal and exit early.\n\nOK.\n\n> To have a\n> graceful disconnect method by calling do_take_over() will solve this\n> issue.\n\nHow about rewording to\n\n Call do_take_over() to have a graceful disconnect method, so that we\n still call transport_disconnect() even if the remote helper exits\n early.\n\nto make \"this issue\" more explicit?\n\n> Helped-by: Linus Arver \n> Signed-off-by: Jiang Xin \n> ---\n> t/t5003-archive-zip.sh | 2 +-\n> transport-helper.c | 2 ++\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n> index 6f85bd3463..961c6aac25 100755\n> --- a/t/t5003-archive-zip.sh\n> +++ b/t/t5003-archive-zip.sh\n> @@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '\n> '\n> \n> test_expect_success 'archive remote http repository' '\n> -\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n> +\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n> \t\t--output=remote-http.zip HEAD &&\n> \ttest_cmp_bin d.zip remote-http.zip\n> '\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 6fe9f4f208..91381be622 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,\n> \n> \tfd[0] = data->helper->out;\n> \tfd[1] = data->helper->in;\n> +\n> +\tdo_take_over(transport);\n> \treturn 0;\n> }\n> \n> -- \n> 2.43.0\n"},{"id":"487156","messageId":"owly1qabhh19.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v5 0/6] support remote archive via stateless transport","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-20T20:43:46Z","receivedAt":"2024-01-20T20:43:48Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jiang Xin writes:\n\nThis v5 looks good code-wise. I've made small suggestions to make the\ncommit messages better, but they are just nits and you are free to\nignore them.\n\nIf you choose to reroll one more time, one additional thing you could do\nis to use the word \"protocol_v2\" in all commit messages because that's\nhow that term looks like in the code (unless the \"protocol-v2\" string is\nalready the standard term used elsewhere).\n\nThanks.\n"},{"id":"487169","messageId":"CANYiYbF63Nc=ehHmp0c3K=Xa9qrzzV+K-5u5KR50pDy3AuDS3A@mail.gmail.com","threadId":"60243","inReplyTo":"owly1qabhh19.fsf@fine.c.googlers.com","subject":"Re: [PATCH v5 0/6] support remote archive via stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T04:09:00Z","receivedAt":"2024-01-21T04:09:13Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"On Sun, Jan 21, 2024 at 4:43 AM Linus Arver wrote:\n>\n> Jiang Xin writes:\n>\n> This v5 looks good code-wise. I've made small suggestions to make the\n> commit messages better, but they are just nits and you are free to\n> ignore them.\n\nThanks for helping me refine commit messages. I will update them based\non your suggestions in next reroll.\n\n> If you choose to reroll one more time, one additional thing you could do\n> is to use the word \"protocol_v2\" in all commit messages because that's\n> how that term looks like in the code (unless the \"protocol-v2\" string is\n> already the standard term used elsewhere).\n\nWill s/protocol_v2/protocol v2/\n"},{"id":"487171","messageId":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705411391.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 0/6] support remote archive via stateless transport","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:32Z","receivedAt":"2024-01-21T13:15:43Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\n\"git archive --remote=\" learned to talk over the smart\nhttp (aka stateless) transport.\n\n# Changes since v5\n\nChange commit messages.\n\n\n# range-diff v5...v6\n\n1: f3fef46c05 ! 1: d75e6d27ac transport-helper: no connection restriction in connect_helper\n @@ Commit message\n \n Later, commit edc9caf7 (transport-helper: introduce stateless-connect,\n 2018-03-15) added a way for a transport without the \".connect\" method\n - to establish a \"stateless\" connection in protocol-v2, where\n + to establish a \"stateless\" connection in protocol v2, where\n process_connect_service() was taught to handle the \".stateless_connect\"\n method, making the old protection too strict. But commit edc9caf7 forgot\n - to adjust this protection accordingly.\n + to adjust this protection accordingly. Even at the time of commit\n + b236752a, this protection seemed redundant, since\n + process_connect_service() would return 0 if the connection could not be\n + established, and connect_helper() would still die() early.\n \n - Remove the restriction in the \"connect_helper()\" function and give the\n - function \"process_connect_service()\" the opportunity to establish a\n - connection using \".connect\" or \".stateless_connect\" for protocol v2. So\n - we can connect with a stateless-rpc and do something useful. E.g., in a\n - later commit, implements remote archive for a repository over HTTP\n - protocol.\n + Remove the restriction in connect_helper() and give the function\n + process_connect_service() the opportunity to establish a connection\n + using \".connect\" or \".stateless_connect\" for protocol v2. So we can\n + connect with a stateless-rpc and do something useful. E.g., in a later\n + commit, implements remote archive for a repository over HTTP protocol.\n \n Helped-by: Junio C Hamano \n Helped-by: Linus Arver \n2: 6be331b22d ! 2: 320526dc56 remote-curl: supports git-upload-archive service\n @@ Commit message\n \n Add new service (git-upload-archive) support in remote-curl, so we can\n support remote archive over HTTP/HTTPS protocols. Differences between\n - git-upload-archive and other serices:\n + git-upload-archive and other services:\n \n 1. The git-archive program does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in remote-curl for the git-upload-archive service.\n \n - 2. We need to detect protocol version by calling discover_refs(),\n + 2. We need to detect protocol version by calling discover_refs().\n Fallback to use the git-upload-pack service (which, like\n git-upload-archive, is a read-only operation) to discover protocol\n version.\n \n + Helped-by: Linus Arver \n Signed-off-by: Jiang Xin \n \n ## remote-curl.c ##\n3: aabc8e1a2a ! 3: 72e575d28a transport-helper: protocol-v2 supports upload-archive\n @@ Metadata\n Author: Jiang Xin \n \n ## Commit message ##\n - transport-helper: protocol-v2 supports upload-archive\n + transport-helper: protocol v2 supports upload-archive\n \n - We used to support only git-upload-pack service for protocol-v2. In\n + We used to support only git-upload-pack service for protocol v2. In\n order to support remote archive over HTTP/HTTPS protocols, add new\n - service support for git-upload-archive in protocol-v2.\n + service support for git-upload-archive in protocol v2.\n \n + Helped-by: Linus Arver \n Signed-off-by: Jiang Xin \n \n ## transport-helper.c ##\n4: fdab4abb43 = 4: 390d13c074 http-backend: new rpc-service for git-upload-archive\n5: 6ac0c8e105 ! 5: 1c9f7755d3 transport-helper: call do_take_over() in connect_helper\n @@ Commit message\n After successfully connecting to the smart transport by calling\n process_connect_service() in connect_helper(), run do_take_over() to\n replace the old vtable with a new one which has methods ready for the\n - smart transport connection. This will fix the exit code of git-archive\n + smart transport connection. This fixes the exit code of git-archive\n in test case \"archive remote http repository\" of t5003.\n \n The connect_helper() function is used as the connect method of the\n @@ Commit message\n do_take_over(), it may fail to call transport_disconnect() in\n run_remote_archiver() of \"builtin/archive.c\". This is because for a\n stateless connection and a service like \"git-upload-archive\", the\n - remote helper may receive a SIGPIPE signal and exit early. To have a\n - graceful disconnect method by calling do_take_over() will solve this\n - issue.\n + remote helper may receive a SIGPIPE signal and exit early. Call\n + do_take_over() to have a graceful disconnect method, so that we still\n + call transport_disconnect() even if the remote helper exits early.\n \n Helped-by: Linus Arver \n Signed-off-by: Jiang Xin \n6: 423a89c593 = 6: 18bc8753df transport-helper: call do_take_over() in process_connect\n\n---\n\nJiang Xin (6):\n transport-helper: no connection restriction in connect_helper\n remote-curl: supports git-upload-archive service\n transport-helper: protocol v2 supports upload-archive\n http-backend: new rpc-service for git-upload-archive\n transport-helper: call do_take_over() in connect_helper\n transport-helper: call do_take_over() in process_connect\n\n http-backend.c | 13 ++++++++++---\n remote-curl.c | 14 +++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n transport-helper.c | 29 +++++++++++++----------------\n 4 files changed, 68 insertions(+), 22 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"487172","messageId":"0994bc2d642501d0f9874773996cbb7ce26ef488.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 1/6] transport-helper: no connection restriction in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:33Z","receivedAt":"2024-01-21T13:15:44Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWhen commit b236752a (Support remote archive from all smart transports,\n2009-12-09) added \"remote archive\" support for \"smart transports\", it\nwas for transport that supports the \".connect\" method. The\n\"connect_helper()\" function protected itself from getting called for a\ntransport without the method before calling process_connect_service(),\nwhich only worked with the \".connect\" method.\n\nLater, commit edc9caf7 (transport-helper: introduce stateless-connect,\n2018-03-15) added a way for a transport without the \".connect\" method\nto establish a \"stateless\" connection in protocol v2, where\nprocess_connect_service() was taught to handle the \".stateless_connect\"\nmethod, making the old protection too strict. But commit edc9caf7 forgot\nto adjust this protection accordingly. Even at the time of commit\nb236752a, this protection seemed redundant, since\nprocess_connect_service() would return 0 if the connection could not be\nestablished, and connect_helper() would still die() early.\n\nRemove the restriction in connect_helper() and give the function\nprocess_connect_service() the opportunity to establish a connection\nusing \".connect\" or \".stateless_connect\" for protocol v2. So we can\nconnect with a stateless-rpc and do something useful. E.g., in a later\ncommit, implements remote archive for a repository over HTTP protocol.\n\nHelped-by: Junio C Hamano \nHelped-by: Linus Arver \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 49811ef176..2e127d24a5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \t/* Get_helper so connect is inited. */\n \tget_helper(transport);\n-\tif (!data->connect)\n-\t\tdie(_(\"operation not supported by protocol\"));\n \n \tif (!process_connect_service(transport, name, exec))\n \t\tdie(_(\"can't connect to subservice %s\"), name);\n-- \n2.43.0\n\n"},{"id":"487173","messageId":"b63b014a22a69ffdd680543ecb4c49ac2c83bb4c.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 2/6] remote-curl: supports git-upload-archive service","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:34Z","receivedAt":"2024-01-21T13:15:45Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAdd new service (git-upload-archive) support in remote-curl, so we can\nsupport remote archive over HTTP/HTTPS protocols. Differences between\ngit-upload-archive and other services:\n\n 1. The git-archive program does not expect to see protocol version and\n capabilities when connecting to remote-helper, so do not send them\n in remote-curl for the git-upload-archive service.\n\n 2. We need to detect protocol version by calling discover_refs().\n Fallback to use the git-upload-pack service (which, like\n git-upload-archive, is a read-only operation) to discover protocol\n version.\n\nSigned-off-by: Jiang Xin \n---\n remote-curl.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..ce6cb8ac05 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)\n \t * establish a stateless connection, otherwise we need to tell the\n \t * client to fallback to using other transport helper functions to\n \t * complete their request.\n+\t *\n+\t * The \"git-upload-archive\" service is a read-only operation. Fallback\n+\t * to use \"git-upload-pack\" service to discover protocol version.\n \t */\n-\tdiscover = discover_refs(service_name, 0);\n+\tif (!strcmp(service_name, \"git-upload-archive\"))\n+\t\tdiscover = discover_refs(\"git-upload-pack\", 0);\n+\telse\n+\t\tdiscover = discover_refs(service_name, 0);\n \tif (discover->version != protocol_v2) {\n \t\tprintf(\"fallback\\n\");\n \t\tfflush(stdout);\n@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)\n \n \t/*\n \t * Dump the capability listing that we got from the server earlier\n-\t * during the info/refs request.\n+\t * during the info/refs request. This does not work with the\n+\t * \"git-upload-archive\" service.\n \t */\n-\twrite_or_die(rpc.in, discover->buf, discover->len);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\twrite_or_die(rpc.in, discover->buf, discover->len);\n \n \t/* Until we see EOF keep sending POSTs */\n \twhile (1) {\n-- \n2.43.0\n\n"},{"id":"487174","messageId":"e7f63482606c23e31f9a26fd890dd28c1952a599.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 3/6] transport-helper: protocol v2 supports upload-archive","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:35Z","receivedAt":"2024-01-21T13:15:45Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nWe used to support only git-upload-pack service for protocol v2. In\norder to support remote archive over HTTP/HTTPS protocols, add new\nservice support for git-upload-archive in protocol v2.\n\nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2e127d24a5..6fe9f4f208 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,\n \t\tret = run_connect(transport, &cmdbuf);\n \t} else if (data->stateless_connect &&\n \t\t (get_protocol_version_config() == protocol_v2) &&\n-\t\t !strcmp(\"git-upload-pack\", name)) {\n+\t\t (!strcmp(\"git-upload-pack\", name) ||\n+\t\t !strcmp(\"git-upload-archive\", name))) {\n \t\tstrbuf_addf(&cmdbuf, \"stateless-connect %s\\n\", name);\n \t\tret = run_connect(transport, &cmdbuf);\n \t\tif (ret)\n-- \n2.43.0\n\n"},{"id":"487175","messageId":"4a5d48859324b21092b95865d2d02f6fe83fa0ea.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 4/6] http-backend: new rpc-service for git-upload-archive","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:36Z","receivedAt":"2024-01-21T13:15:46Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAdd new rpc-service \"upload-archive\" in http-backend to add server side\nsupport for remote archive over HTTP/HTTPS protocols.\n\nAlso add new test cases in t5003. In the test case \"archive remote http\nrepository\", git-archive exits with a non-0 exit code even though we\ncreate the archive correctly. It will be fixed in a later commit.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n http-backend.c | 13 ++++++++++---\n t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ff07b87e64..1ed1e29d07 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -38,6 +38,7 @@ struct rpc_service {\n static struct rpc_service rpc_service[] = {\n \t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n \t{ \"receive-pack\", \"receivepack\", 0, -1 },\n+\t{ \"upload-archive\", \"uploadarchive\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)\n \n static void service_rpc(struct strbuf *hdr, char *service_name)\n {\n-\tconst char *argv[] = {NULL, \"--stateless-rpc\", \".\", NULL};\n+\tstruct strvec argv = STRVEC_INIT;\n \tstruct rpc_service *svc = select_service(hdr, service_name);\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tstrvec_push(&argv, svc->name);\n+\tif (strcmp(service_name, \"git-upload-archive\"))\n+\t\tstrvec_push(&argv, \"--stateless-rpc\");\n+\tstrvec_push(&argv, \".\");\n+\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"application/x-git-%s-request\", svc->name);\n \tcheck_content_type(hdr, buf.buf);\n@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)\n \n \tend_headers(hdr);\n \n-\targv[0] = svc->name;\n-\trun_service(argv, svc->buffer_input);\n+\trun_service(argv.v, svc->buffer_input);\n \tstrbuf_release(&buf);\n+\tstrvec_clear(&argv);\n }\n \n static int dead;\n@@ -723,6 +729,7 @@ static struct service_cmd {\n \t{\"GET\", \"/objects/pack/pack-[0-9a-f]{64}\\\\.idx$\", get_idx_file},\n \n \t{\"POST\", \"/git-upload-pack$\", service_rpc},\n+\t{\"POST\", \"/git-upload-archive$\", service_rpc},\n \t{\"POST\", \"/git-receive-pack$\", service_rpc}\n };\n \ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff0..6f85bd3463 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -239,4 +239,38 @@ check_zip with_untracked2\n check_added with_untracked2 untracked one/untracked\n check_added with_untracked2 untracked two/untracked\n \n+# Test remote archive over HTTP protocol.\n+#\n+# Note: this should be the last part of this test suite, because\n+# by including lib-httpd.sh, the test may end early if httpd tests\n+# should not be run.\n+#\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success \"setup for HTTP protocol\" '\n+\tcp -R bare.git \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/bare.git\" \\\n+\t\tconfig http.uploadpack true &&\n+\tset_askpass user@host pass@host\n+'\n+\n+setup_askpass_helper\n+\n+test_expect_success 'remote archive does not work with protocol v1' '\n+\ttest_must_fail git -c protocol.version=1 archive \\\n+\t\t--remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD >actual 2>&1 &&\n+\tcat >expect <<-EOF &&\n+\tfatal: can${SQ}t connect to subservice git-upload-archive\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'archive remote http repository' '\n+\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\t\t--output=remote-http.zip HEAD &&\n+\ttest_cmp_bin d.zip remote-http.zip\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"487176","messageId":"12a5b5e532f46558a0b4abaf80b45a670a0e7f88.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 5/6] transport-helper: call do_take_over() in connect_helper","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:37Z","receivedAt":"2024-01-21T13:15:47Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nAfter successfully connecting to the smart transport by calling\nprocess_connect_service() in connect_helper(), run do_take_over() to\nreplace the old vtable with a new one which has methods ready for the\nsmart transport connection. This fixes the exit code of git-archive\nin test case \"archive remote http repository\" of t5003.\n\nThe connect_helper() function is used as the connect method of the\nvtable in \"transport-helper.c\", and it is called by transport_connect()\nin \"transport.c\" to setup a connection. The only place that we call\ntransport_connect() so far is in \"builtin/archive.c\". Without running\ndo_take_over(), it may fail to call transport_disconnect() in\nrun_remote_archiver() of \"builtin/archive.c\". This is because for a\nstateless connection and a service like \"git-upload-archive\", the\nremote helper may receive a SIGPIPE signal and exit early. Call\ndo_take_over() to have a graceful disconnect method, so that we still\ncall transport_disconnect() even if the remote helper exits early.\n\nHelped-by: Linus Arver \nSigned-off-by: Jiang Xin \n---\n t/t5003-archive-zip.sh | 2 +-\n transport-helper.c | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 6f85bd3463..961c6aac25 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '\n '\n \n test_expect_success 'archive remote http repository' '\n-\ttest_must_fail git archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n+\tgit archive --remote=\"$HTTPD_URL/auth/smart/bare.git\" \\\n \t\t--output=remote-http.zip HEAD &&\n \ttest_cmp_bin d.zip remote-http.zip\n '\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6fe9f4f208..91381be622 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,\n \n \tfd[0] = data->helper->out;\n \tfd[1] = data->helper->in;\n+\n+\tdo_take_over(transport);\n \treturn 0;\n }\n \n-- \n2.43.0\n\n"},{"id":"487177","messageId":"ed765da3433b372e75b7f44b925550fbf119d315.1705841443.git.zhiyou.jx@alibaba-inc.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"[PATCH v6 6/6] transport-helper: call do_take_over() in process_connect","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2024-01-21T13:15:38Z","receivedAt":"2024-01-21T13:15:48Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe existing pattern among all callers of process_connect() seems to be\n\n if (process_connect(...)) {\n do_take_over();\n ... dispatch to the underlying method ...\n }\n ... otherwise implement the fallback ...\n\nwhere the return value from process_connect() is the return value of the\ncall it makes to process_connect_service().\n\nMove the call of do_take_over() inside process_connect(), so that\ncalling the process_connect() function is more concise and will not\nmiss do_take_over().\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n transport-helper.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91381be622..566f7473df 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tconst char *name;\n \tconst char *exec;\n+\tint ret;\n \n \tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n \tif (for_push)\n@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,\n \telse\n \t\texec = data->transport_options.uploadpack;\n \n-\treturn process_connect_service(transport, name, exec);\n+\tret = process_connect_service(transport, name, exec);\n+\tif (ret)\n+\t\tdo_take_over(transport);\n+\treturn ret;\n }\n \n static int connect_helper(struct transport *transport, const char *name,\n@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,\n \n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->fetch_refs(transport, nr_heads, to_fetch);\n-\t}\n \n \t/*\n \t * If we reach here, then the server, the client, and/or the transport\n@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \n-\tif (process_connect(transport, 1)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 1))\n \t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n-\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr,\n@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, for_push)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, for_push))\n \t\treturn transport->vtable->get_refs_list(transport, for_push,\n \t\t\t\t\t\t\ttransport_options);\n-\t}\n \n \treturn get_refs_list_using_list(transport, for_push);\n }\n@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)\n {\n \tget_helper(transport);\n \n-\tif (process_connect(transport, 0)) {\n-\t\tdo_take_over(transport);\n+\tif (process_connect(transport, 0))\n \t\treturn transport->vtable->get_bundle_uri(transport);\n-\t}\n \n \treturn -1;\n }\n-- \n2.43.0\n\n"},{"id":"487179","messageId":"owlyy1cifwur.fsf@fine.c.googlers.com","threadId":"60243","inReplyTo":"cover.1705841443.git.zhiyou.jx@alibaba-inc.com","subject":"Re: [PATCH v6 0/6] support remote archive via stateless transport","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-21T16:57:16Z","receivedAt":"2024-01-21T16:57:18Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"\nThis v6 version LGTM. Thanks!\n"},{"id":"487208","messageId":"xmqqwms1cqi8.fsf@gitster.g","threadId":"60243","inReplyTo":"owlyy1cifwur.fsf@fine.c.googlers.com","subject":"Re: [PATCH v6 0/6] support remote archive via stateless transport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-22T15:54:55Z","receivedAt":"2024-01-22T15:55:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Arver writes:\n\n> This v6 version LGTM. Thanks!\n\nThanks, both of you. Will queue.\n\n"}]}