{"thread":{"id":"37390","subject":"[PATCH] send-pack: take refspecs over stdin","startedAt":"2014-08-21T12:17:10Z","lastAt":"2014-08-21T12:21:20Z","messageCount":2,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"248051","messageId":"20140821121709.GA15299@peff.net","threadId":"37390","inReplyTo":null,"subject":"[PATCH] send-pack: take refspecs over stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-21T12:17:10Z","receivedAt":"2014-08-21T12:17:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Pushing a large number of refs works over most transports,\nbecause we implement send-pack as an internal function.\nHowever, it can sometimes fail when pushing over http,\nbecause we have to spawn \"git send-pack --stateless-rpc\" to\ndo the heavy lifting, and we pass each refspec on the\ncommand line. This can cause us to overflow the OS limits on\nthe size of the command line for a large push.\n\nWe can solve this by giving send-pack a --stdin option and\nusing it from remote-curl.  We already dealt with this on\nthe fetch-pack side in 078b895 (fetch-pack: new --stdin\noption to read refs from stdin, 2012-04-02). The stdin\noption (and in particular, its use of packet-lines for\nstateless-rpc input) is modeled after that solution.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI had to fiddle with the numbers in the http test. Linux gives up to 1/4\nof the configured stack ulimit as space for the cmdline, so I had to\npick a number big enough so that we had stack to run the actual\noperation but small enough to limit the cmdline. The test as it is there\nfails for me without this patch and succeeds with it. I suspect the\nnumbers are quite different on other systems, but I think it should at\nleast succeed everywhere with this patch. I'd also be fine with cutting\nthat test if it proves too flaky.\n\nI tried originally bumping it to 50,000 tags to match the fetch-pack\ntest. But besides needing to protect it behind an EXPENSIVE prereq\n(which means basically nobody is ever going to run it), it also seems to\ntrigger a nasty quadratic behavior in send-pack (6400 refs takes ~16s,\nwith the time quadrupling for each doubling of refs; the same operation\nover a pipe takes 140ms).  Oddly, the behavior doesn't seem to trigger\nwhen pushing over a local pipe, so it's presumably related to\nstateless-rpc. It looked like were deep in match_refs, but I haven't\nfigured it out beyond that.\n\n Documentation/git-send-pack.txt | 13 ++++++++++++-\n builtin/send-pack.c             | 27 +++++++++++++++++++++++++++\n remote-curl.c                   |  8 +++++++-\n t/t5541-http-push-smart.sh      | 15 +++++++++++++++\n 4 files changed, 61 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex dc3a568..2a0de42 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -35,6 +35,16 @@ OPTIONS\n \tInstead of explicitly specifying which refs to update,\n \tupdate all heads that locally exist.\n \n+--stdin::\n+\tTake the list of refs from stdin, one per line. If there\n+\tare refs specified on the command line in addition to this\n+\toption, then the refs from stdin are processed after those\n+\ton the command line.\n++\n+If '--stateless-rpc' is specified together with this option then\n+the list of refs must be in packet format (pkt-line). Each ref must\n+be in a separate packet, and the list must end with a flush packet.\n+\n --dry-run::\n \tDo everything except actually send the updates.\n \n@@ -77,7 +87,8 @@ this flag.\n Without '--all' and without any '<ref>', the heads that exist\n both on the local side and on the remote side are updated.\n \n-When one or more '<ref>' are specified explicitly, it can be either a\n+When one or more '<ref>' are specified explicitly (whether on the\n+command line or via `--stdin`), it can be either a\n single pattern, or a pair of such pattern separated by a colon\n \":\" (this means that a ref name cannot have a colon in it).  A\n single pattern '<name>' is just a shorthand for '<name>:<name>'.\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..4b1bc0f 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -110,6 +110,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint flags;\n \tunsigned int reject_reasons;\n \tint progress = -1;\n+\tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n \n \targv++;\n@@ -169,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\targs.stateless_rpc = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--stdin\")) {\n+\t\t\t\tfrom_stdin = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--helper-status\")) {\n \t\t\t\thelper_status = 1;\n \t\t\t\tcontinue;\n@@ -201,6 +206,28 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!dest)\n \t\tusage(send_pack_usage);\n+\n+\tif (from_stdin) {\n+\t\tstruct argv_array all_refspecs = ARGV_ARRAY_INIT;\n+\n+\t\tfor (i = 0; i < nr_refspecs; i++)\n+\t\t\targv_array_push(&all_refspecs, refspecs[i]);\n+\n+\t\tif (args.stateless_rpc) {\n+\t\t\tconst char *buf;\n+\t\t\twhile ((buf = packet_read_line(0, NULL)))\n+\t\t\t\targv_array_push(&all_refspecs, buf);\n+\t\t} else {\n+\t\t\tstruct strbuf line = STRBUF_INIT;\n+\t\t\twhile (strbuf_getline(&line, stdin, '\\n') != EOF)\n+\t\t\t\targv_array_push(&all_refspecs, line.buf);\n+\t\t\tstrbuf_release(&line);\n+\t\t}\n+\n+\t\trefspecs = all_refspecs.argv;\n+\t\tnr_refspecs = all_refspecs.argc;\n+\t}\n+\n \t/*\n \t * --all and --mirror are incompatible; neither makes sense\n \t * with any refspecs.\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0fcf2ce..558b9fe 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -863,6 +863,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tint i, err;\n \tstruct argv_array args;\n \tstruct string_list_item *cas_option;\n+\tstruct strbuf preamble = STRBUF_INIT;\n \n \targv_array_init(&args);\n \targv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\",\n@@ -880,17 +881,22 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\targv_array_push(&args, cas_option->string);\n \targv_array_push(&args, url.buf);\n+\n+\targv_array_push(&args, \"--stdin\");\n \tfor (i = 0; i < nr_spec; i++)\n-\t\targv_array_push(&args, specs[i]);\n+\t\tpacket_buf_write(&preamble, \"%s\\n\", specs[i]);\n+\tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n \trpc.argv = args.argv;\n+\trpc.stdin_preamble = &preamble;\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n+\tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n }\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 73af16f..db19988 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -323,5 +323,20 @@ test_expect_success 'push into half-auth-complete requires password' '\n \ttest_cmp expect actual\n '\n \n+run_with_limited_cmdline () {\n+\t(ulimit -s 128 && \"$@\")\n+}\n+\n+test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n+\n+test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '\n+\tsha1=$(git rev-parse HEAD) &&\n+\ttest_seq 2000 |\n+\t  sort |\n+\t  sed \"s|.*|$sha1 refs/tags/really-long-tag-name-&|\" \\\n+\t  >.git/packed-refs &&\n+\trun_with_limited_cmdline git push --mirror\n+'\n+\n stop_httpd\n test_done\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248052","messageId":"20140821122120.GA17600@peff.net","threadId":"37390","inReplyTo":"20140821121709.GA15299@peff.net","subject":"[PATCH v2] send-pack: take refspecs over stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-21T12:21:20Z","receivedAt":"2014-08-21T12:21:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 21, 2014 at 08:17:10AM -0400, Jeff King wrote:\n\n>  Documentation/git-send-pack.txt | 13 ++++++++++++-\n>  builtin/send-pack.c             | 27 +++++++++++++++++++++++++++\n>  remote-curl.c                   |  8 +++++++-\n>  t/t5541-http-push-smart.sh      | 15 +++++++++++++++\n>  4 files changed, 61 insertions(+), 2 deletions(-)\n\nWhoops. Forgot to actually add the battery of individual send-pack\ntests. Here's a re-send.\n\n-- >8 --\nSubject: send-pack: take refspecs over stdin\n\nPushing a large number of refs works over most transports,\nbecause we implement send-pack as an internal function.\nHowever, it can sometimes fail when pushing over http,\nbecause we have to spawn \"git send-pack --stateless-rpc\" to\ndo the heavy lifting, and we pass each refspec on the\ncommand line. This can cause us to overflow the OS limits on\nthe size of the command line for a large push.\n\nWe can solve this by giving send-pack a --stdin option and\nusing it from remote-curl.  We already dealt with this on\nthe fetch-pack side in 078b895 (fetch-pack: new --stdin\noption to read refs from stdin, 2012-04-02). The stdin\noption (and in particular, its use of packet-lines for\nstateless-rpc input) is modeled after that solution.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-send-pack.txt | 13 +++++-\n builtin/send-pack.c             | 27 ++++++++++++\n remote-curl.c                   |  8 +++-\n t/t5408-send-pack-stdin.sh      | 92 +++++++++++++++++++++++++++++++++++++++++\n t/t5541-http-push-smart.sh      | 15 +++++++\n 5 files changed, 153 insertions(+), 2 deletions(-)\n create mode 100755 t/t5408-send-pack-stdin.sh\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex dc3a568..2a0de42 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -35,6 +35,16 @@ OPTIONS\n \tInstead of explicitly specifying which refs to update,\n \tupdate all heads that locally exist.\n \n+--stdin::\n+\tTake the list of refs from stdin, one per line. If there\n+\tare refs specified on the command line in addition to this\n+\toption, then the refs from stdin are processed after those\n+\ton the command line.\n++\n+If '--stateless-rpc' is specified together with this option then\n+the list of refs must be in packet format (pkt-line). Each ref must\n+be in a separate packet, and the list must end with a flush packet.\n+\n --dry-run::\n \tDo everything except actually send the updates.\n \n@@ -77,7 +87,8 @@ this flag.\n Without '--all' and without any '<ref>', the heads that exist\n both on the local side and on the remote side are updated.\n \n-When one or more '<ref>' are specified explicitly, it can be either a\n+When one or more '<ref>' are specified explicitly (whether on the\n+command line or via `--stdin`), it can be either a\n single pattern, or a pair of such pattern separated by a colon\n \":\" (this means that a ref name cannot have a colon in it).  A\n single pattern '<name>' is just a shorthand for '<name>:<name>'.\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..4b1bc0f 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -110,6 +110,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint flags;\n \tunsigned int reject_reasons;\n \tint progress = -1;\n+\tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n \n \targv++;\n@@ -169,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\targs.stateless_rpc = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--stdin\")) {\n+\t\t\t\tfrom_stdin = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--helper-status\")) {\n \t\t\t\thelper_status = 1;\n \t\t\t\tcontinue;\n@@ -201,6 +206,28 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t}\n \tif (!dest)\n \t\tusage(send_pack_usage);\n+\n+\tif (from_stdin) {\n+\t\tstruct argv_array all_refspecs = ARGV_ARRAY_INIT;\n+\n+\t\tfor (i = 0; i < nr_refspecs; i++)\n+\t\t\targv_array_push(&all_refspecs, refspecs[i]);\n+\n+\t\tif (args.stateless_rpc) {\n+\t\t\tconst char *buf;\n+\t\t\twhile ((buf = packet_read_line(0, NULL)))\n+\t\t\t\targv_array_push(&all_refspecs, buf);\n+\t\t} else {\n+\t\t\tstruct strbuf line = STRBUF_INIT;\n+\t\t\twhile (strbuf_getline(&line, stdin, '\\n') != EOF)\n+\t\t\t\targv_array_push(&all_refspecs, line.buf);\n+\t\t\tstrbuf_release(&line);\n+\t\t}\n+\n+\t\trefspecs = all_refspecs.argv;\n+\t\tnr_refspecs = all_refspecs.argc;\n+\t}\n+\n \t/*\n \t * --all and --mirror are incompatible; neither makes sense\n \t * with any refspecs.\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0fcf2ce..558b9fe 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -863,6 +863,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tint i, err;\n \tstruct argv_array args;\n \tstruct string_list_item *cas_option;\n+\tstruct strbuf preamble = STRBUF_INIT;\n \n \targv_array_init(&args);\n \targv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\",\n@@ -880,17 +881,22 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\targv_array_push(&args, cas_option->string);\n \targv_array_push(&args, url.buf);\n+\n+\targv_array_push(&args, \"--stdin\");\n \tfor (i = 0; i < nr_spec; i++)\n-\t\targv_array_push(&args, specs[i]);\n+\t\tpacket_buf_write(&preamble, \"%s\\n\", specs[i]);\n+\tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n \trpc.argv = args.argv;\n+\trpc.stdin_preamble = &preamble;\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n+\tstrbuf_release(&preamble);\n \targv_array_clear(&args);\n \treturn err;\n }\ndiff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh\nnew file mode 100755\nindex 0000000..e8737df\n--- /dev/null\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -0,0 +1,92 @@\n+#!/bin/sh\n+\n+test_description='send-pack --stdin tests'\n+. ./test-lib.sh\n+\n+create_ref () {\n+\ttree=$(git write-tree) &&\n+\ttest_tick &&\n+\tcommit=$(echo \"$1\" | git commit-tree $tree) &&\n+\tgit update-ref \"$1\" $commit\n+}\n+\n+clear_remote () {\n+\trm -rf remote.git &&\n+\tgit init --bare remote.git\n+}\n+\n+verify_push () {\n+\tgit rev-parse \"$1\" >expect &&\n+\tgit --git-dir=remote.git rev-parse \"${2:-$1}\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'setup refs' '\n+\tcat >refs <<-\\EOF &&\n+\trefs/heads/A\n+\trefs/heads/C\n+\trefs/tags/D\n+\trefs/heads/B\n+\trefs/tags/E\n+\tEOF\n+\tfor i in $(cat refs); do\n+\t\tcreate_ref $i || return 1\n+\tdone\n+'\n+\n+# sanity check our setup\n+test_expect_success 'refs on cmdline' '\n+\tclear_remote &&\n+\tgit send-pack remote.git $(cat refs) &&\n+\tfor i in $(cat refs); do\n+\t\tverify_push $i || return 1\n+\tdone\n+'\n+\n+test_expect_success 'refs over stdin' '\n+\tclear_remote &&\n+\tgit send-pack remote.git --stdin <refs &&\n+\tfor i in $(cat refs); do\n+\t\tverify_push $i || return 1\n+\tdone\n+'\n+\n+test_expect_success 'stdin lines are full refspecs' '\n+\tclear_remote &&\n+\techo \"A:other\" >input &&\n+\tgit send-pack remote.git --stdin <input &&\n+\tverify_push refs/heads/A refs/heads/other\n+'\n+\n+test_expect_success 'stdin mixed with cmdline' '\n+\tclear_remote &&\n+\techo A >input &&\n+\tgit send-pack remote.git --stdin B <input &&\n+\tverify_push A &&\n+\tverify_push B\n+'\n+\n+test_expect_success 'cmdline refs written in order' '\n+\tclear_remote &&\n+\ttest_must_fail git send-pack remote.git A:foo B:foo &&\n+\tverify_push A foo\n+'\n+\n+test_expect_success '--stdin refs come after cmdline' '\n+\tclear_remote &&\n+\techo A:foo >input &&\n+\ttest_must_fail git send-pack remote.git --stdin B:foo <input &&\n+\tverify_push B foo\n+'\n+\n+test_expect_success 'refspecs and --mirror do not mix (cmdline)' '\n+\tclear_remote &&\n+\ttest_must_fail git send-pack remote.git --mirror $(cat refs)\n+'\n+\n+test_expect_success 'refspecs and --mirror do not mix (stdin)' '\n+\tclear_remote &&\n+\ttest_must_fail git send-pack remote.git --mirror --stdin <refs\n+'\n+\n+test_done\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 73af16f..db19988 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -323,5 +323,20 @@ test_expect_success 'push into half-auth-complete requires password' '\n \ttest_cmp expect actual\n '\n \n+run_with_limited_cmdline () {\n+\t(ulimit -s 128 && \"$@\")\n+}\n+\n+test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'\n+\n+test_expect_success CMDLINE_LIMIT 'push 2000 tags over http' '\n+\tsha1=$(git rev-parse HEAD) &&\n+\ttest_seq 2000 |\n+\t  sort |\n+\t  sed \"s|.*|$sha1 refs/tags/really-long-tag-name-&|\" \\\n+\t  >.git/packed-refs &&\n+\trun_with_limited_cmdline git push --mirror\n+'\n+\n stop_httpd\n test_done\n-- \n2.1.0.346.ga0367b9\n"}]}