{"thread":{"id":"40398","subject":"[PATCHv3 01/13] Sending \"Fetching submodule \" output to stderr","startedAt":"2015-09-21T22:39:06Z","lastAt":"2015-09-23T01:51:27Z","messageCount":48,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine","Torsten Bögershausen","Jacob Keller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"270431","messageId":"1442875159-13027-1-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":null,"subject":"[PATCHv3 00/13] fetch submodules in parallel and a preview on parallel \"submodule update\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:06Z","receivedAt":"2015-09-21T22:39:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This build on top of origin/sb/submodule-helper\n\nPatches 1-8 are parallelising fetching submodules.\nThanks Eric, Jeff and Junio for the discussion and review!\nBased on your feedback:\n\nPatch 2 is optional and may be spun out as its own mini series.\nInstead of spinning we can poll until data are ready on the fd.\n\nPatch 3,4 have been revamped to include your suggestions, having \nxread_nonblock and strbuf_read_once as their own functions not\nto be integrated into other existing functions.\n\nPatch 5 was fixed to return 0 or -1 in the error case.\nPatch 6 has major syntactically changes such as no slots any more,\nbut in_use flags. The return code was flipped (1 means there is a child\nto be executed, while 0 means we're done).\n\nPatch 7,8 convert git fetch --recuse-submodules to use the fruit of patch 6.\n\nPatches 9 an onwards are RFC, so I could spin them out as their\nown series as they convert parts of \"submodule update\" to C using\nthe new asynchronous run command code.\nPatch 9 adds the update strategy to the submodule cache\nPatches 10,11,12 are just moving code around to make cmd_update leaner.\nPatch 13 shows how I plan to parallelize updating the submodules for now.\n\nAny feedback welcome,\nThanks,\nStefan\n\nJonathan Nieder (1):\n Sending \"Fetching submodule \" output to stderr\n\nStefan Beller (13):\n xread: poll on non blocking fds\n xread_nonblock: add functionality to read from fds nonblockingly\n strbuf: add strbuf_read_once to read without blocking\n run-command: factor out return value computation\n run-command: add an asynchronous parallel child processor\n fetch_populated_submodules: use new parallel job processing\n submodules: allow parallel fetching, add tests and documentation\n submodule config: keep update strategy around\n git submodule update: pass --prefix only with a non empty prefix\n git submodule update: cmd_update_recursive\n git submodule update: cmd_update_clone\n git submodule update: cmd_update_fetch\n Rewrite submodule update in C\n\n Documentation/fetch-options.txt | 7 +\n builtin/fetch.c | 6 +-\n builtin/pull.c | 6 +\n builtin/submodule--helper.c | 247 +++++++++++++++++++++++++++++\n git-compat-util.h | 1 +\n git-submodule.sh | 339 ++++++++++++++--------------------------\n run-command.c | 329 +++++++++++++++++++++++++++++++++++---\n run-command.h | 38 +++++\n strbuf.c | 11 ++\n strbuf.h | 6 +\n submodule-config.c | 16 ++\n submodule-config.h | 1 +\n submodule.c | 119 +++++++++-----\n submodule.h | 2 +-\n t/t0061-run-command.sh | 20 +++\n t/t5526-fetch-submodules.sh | 70 ++++++---\n test-run-command.c | 24 +++\n wrapper.c | 41 ++++-\n 18 files changed, 972 insertions(+), 311 deletions(-)\n\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270428","messageId":"1442875159-13027-2-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 01/13] Sending \"Fetching submodule \" output to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:07Z","receivedAt":"2015-09-21T22:39:07Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Jonathan Nieder \n\nThe \"Pushing submodule \" progress output correctly goes to\nstderr, but \"Fetching submodule \" is going to stdout by mistake.\nFix it to write to stderr.\n\nNoticed while trying to implement a parallel submodule fetch. When\nthis particular output line went to a different file descriptor, it\nwas buffered separately, resulting in wrongly interleaved output if\nwe copied it to the terminal naively.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 2 +-\n t/t5526-fetch-submodules.sh | 51 +++++++++++++++++++++++----------------------\n 2 files changed, 27 insertions(+), 26 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9fcc86f..1d64e57 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -694,7 +694,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n \t\t\tif (!quiet)\n-\t\t\t\tprintf(\"Fetching submodule %s%s\\n\", prefix, ce->name);\n+\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n \t\t\tcp.dir = submodule_path.buf;\n \t\t\targv_array_push(&argv, default_argv);\n \t\t\targv_array_push(&argv, \"--submodule-prefix\");\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex a4532b0..17759b1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -16,7 +16,8 @@ add_upstream_commit() {\n \t\tgit add subfile &&\n \t\tgit commit -m new subfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err\n \t) &&\n \t(\n@@ -27,6 +28,7 @@ add_upstream_commit() {\n \t\tgit add deepsubfile &&\n \t\tgit commit -m new deepsubfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n+\t\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> ../expect.err\n \t\techo \"From $pwd/deepsubmodule\" >> ../expect.err &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err\n \t)\n@@ -56,9 +58,7 @@ test_expect_success setup '\n \t(\n \t\tcd downstream &&\n \t\tgit submodule update --init --recursive\n-\t) &&\n-\techo \"Fetching submodule submodule\" > expect.out &&\n-\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> expect.out\n+\t)\n '\n \n test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n@@ -67,7 +67,7 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -96,7 +96,7 @@ test_expect_success \"using fetchRecurseSubmodules=true in .gitmodules recurses i\n \t\tgit config -f .gitmodules submodule.submodule.fetchRecurseSubmodules true &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -127,7 +127,7 @@ test_expect_success \"--recurse-submodules overrides fetchRecurseSubmodules setti\n \t\tgit config --unset -f .gitmodules submodule.submodule.fetchRecurseSubmodules &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -146,7 +146,7 @@ test_expect_success \"--dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules --dry-run >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -155,7 +155,7 @@ test_expect_success \"Without --dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -166,7 +166,7 @@ test_expect_success \"recurseSubmodules=true propagates into submodules\" '\n \t\tgit config fetch.recurseSubmodules true\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -180,7 +180,7 @@ test_expect_success \"--recurse-submodules overrides config in submodule\" '\n \t\t) &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -214,16 +214,15 @@ test_expect_success \"Recursion stops when no new submodule commits are fetched\"\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\techo \"Fetching submodule submodule\" > expect.out.sub &&\n \techo \"From $pwd/.\" > expect.err.sub &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.sub &&\n-\thead -2 expect.err >> expect.err.sub &&\n+\thead -3 expect.err >> expect.err.sub &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out.sub actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion doesn't happen when new superproject commits don't change any submodules\" '\n@@ -269,7 +268,7 @@ test_expect_success \"Recursion picks up config in submodule\" '\n \t\t)\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion picks up all submodules when necessary\" '\n@@ -285,7 +284,8 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\"\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err.sub\n \t) &&\n \thead1=$(git rev-parse --short HEAD) &&\n@@ -295,13 +295,13 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >> expect.err.2 &&\n \tcat expect.err.sub >> expect.err.2 &&\n-\ttail -2 expect.err >> expect.err.2 &&\n+\ttail -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.2 actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no new commits are fetched in the superproject (and ignores config)\" '\n@@ -317,7 +317,8 @@ test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no ne\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\" &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo Fetching submodule submodule > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err.sub\n \t) &&\n \t(\n@@ -335,7 +336,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\ttail -2 expect.err > expect.err.deepsub &&\n+\ttail -3 expect.err > expect.err.deepsub &&\n \techo \"From $pwd/.\" > expect.err &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err &&\n \tcat expect.err.sub >> expect.err &&\n@@ -354,7 +355,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \t\t\tgit config --unset -f .gitmodules submodule.subdir/deepsubmodule.fetchRecursive\n \t\t)\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -388,7 +389,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config fetch.recurseSubmodules on-demand &&\n@@ -399,7 +400,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \t\tcd downstream &&\n \t\tgit config --unset fetch.recurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n@@ -416,7 +417,7 @@ test_expect_success \"'submodule..fetchRecurseSubmodules=on-demand' override\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config submodule.submodule.fetchRecurseSubmodules on-demand &&\n@@ -427,7 +428,7 @@ test_expect_success \"'submodule..fetchRecurseSubmodules=on-demand' override\n \t\tcd downstream &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n-- \n2.5.0.275.ge015d2a\n"},{"id":"270429","messageId":"1442875159-13027-3-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:08Z","receivedAt":"2015-09-21T22:39:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">From the man page:\nEAGAIN The file descriptor fd refers to a file other than a socket\n and has been marked nonblocking (O_NONBLOCK), and the read\n would block.\n\nEAGAIN or EWOULDBLOCK\n The file descriptor fd refers to a socket and has been marked\n nonblocking (O_NONBLOCK), and the read would block. POSIX.1-2001\n allows either error to be returned for this case, and does not\n require these constants to have the same value, so a portable\n application should check for both possibilities.\n\nSo if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.\nAs the intend of xread is to read as much as possible either until the\nfd is EOF or an actual error occurs, we can ease the feeder of the fd\nby not spinning the whole time, but rather wait for it politely by not\nbusy waiting.\n\nSigned-off-by: Stefan Beller \n---\n wrapper.c | 19 +++++++++++++++++--\n 1 file changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex ff49807..50267a4 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -201,8 +201,23 @@ ssize_t xread(int fd, void *buf, size_t len)\n \t len = MAX_IO_SIZE;\n \twhile (1) {\n \t\tnr = read(fd, buf, len);\n-\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n-\t\t\tcontinue;\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK) {\n+\t\t\t\tstruct pollfd pfd;\n+\t\t\t\tint i;\n+\t\t\t\tpfd.events = POLLIN;\n+\t\t\t\tpfd.fd = fd;\n+\t\t\t\ti = poll(&pfd, 1, 100);\n+\t\t\t\tif (i < 0) {\n+\t\t\t\t\tif (errno == EINTR || errno == ENOMEM)\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\telse\n+\t\t\t\t\t\tdie_errno(\"poll\");\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n \t\treturn nr;\n \t}\n }\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270430","messageId":"1442875159-13027-4-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:09Z","receivedAt":"2015-09-21T22:39:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This wrapper just restarts on EINTR, but not on EAGAIN like xread\ndoes. This gives less guarantees on the actual reading rather than\non returning fast.\n\nSigned-off-by: Stefan Beller \n---\n git-compat-util.h | 1 +\n wrapper.c | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6d391f..9ccea85 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\ndiff --git a/wrapper.c b/wrapper.c\nindex 50267a4..54ce231 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -223,6 +223,28 @@ ssize_t xread(int fd, void *buf, size_t len)\n }\n \n /*\n+ * xread_nonblock() is the same a read(), but it automatically restarts read()\n+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that\n+ * \"len\" bytes is read even if the data is available.\n+ */\n+ssize_t xread_nonblock(int fd, void *buf, size_t len)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t len = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = read(fd, buf, len);\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EWOULDBLOCK)\n+\t\t\t\terrno = EAGAIN;\n+\t\t}\n+\t\treturn nr;\n+\t}\n+}\n+\n+/*\n * xwrite() is the same a write(), but it automatically restarts write()\n * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270433","messageId":"1442875159-13027-5-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:10Z","receivedAt":"2015-09-21T22:39:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new call will read a fd into a strbuf once. The underlying call\nxread_nonblock is meant to execute non blockingly if the fd is set to\nO_NONBLOCK.\n\nSigned-off-by: Stefan Beller \n---\n strbuf.c | 11 +++++++++++\n strbuf.h | 6 ++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex cce5eed..35e71b8 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \treturn sb->len - oldlen;\n }\n \n+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n+{\n+\tssize_t cnt;\n+\n+\tstrbuf_grow(sb, hint ? hint : 8192);\n+\tcnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\tif (cnt > 0)\n+\t\tstrbuf_setlen(sb, sb->len + cnt);\n+\treturn cnt;\n+}\n+\n #define STRBUF_MAXLINK (2*PATH_MAX)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\ndiff --git a/strbuf.h b/strbuf.h\nindex aef2794..4d4e5b1 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,6 +367,12 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n+ * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.\n+ * The fd must have set O_NONBLOCK.\n+ */\n+extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n+\n+/**\n * Read the contents of a file, specified by its path. The third argument\n * can be used to give a hint about the file size, to avoid reallocs.\n */\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270432","messageId":"1442875159-13027-6-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 05/13] run-command: factor out return value computation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:11Z","receivedAt":"2015-09-21T22:39:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We will need computing the return value in a later patch without the\nwait.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 28e1d55..674e348 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n+static int determine_return_value(int wait_status,\n+\t\t\t\t int *result,\n+\t\t\t\t int *error_code,\n+\t\t\t\t const char *argv0)\n+{\n+\tif (WIFSIGNALED(wait_status)) {\n+\t\t*result = WTERMSIG(wait_status);\n+\t\tif (*result != SIGINT && *result != SIGQUIT)\n+\t\t\terror(\"%s died of signal %d\", argv0, *result);\n+\t\t/*\n+\t\t * This return value is chosen so that code & 0xff\n+\t\t * mimics the exit code that a POSIX shell would report for\n+\t\t * a program that died from this signal.\n+\t\t */\n+\t\t*result += 128;\n+\t} else if (WIFEXITED(wait_status)) {\n+\t\t*result = WEXITSTATUS(wait_status);\n+\t\t/*\n+\t\t * Convert special exit code when execvp failed.\n+\t\t */\n+\t\tif (*result == 127) {\n+\t\t\t*result = -1;\n+\t\t\t*error_code = ENOENT;\n+\t\t}\n+\t} else\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int wait_or_whine(pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n@@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n-\t} else if (waiting != pid) {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n-\t} else if (WIFSIGNALED(status)) {\n-\t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n-\t\t\terror(\"%s died of signal %d\", argv0, code);\n-\t\t/*\n-\t\t * This return value is chosen so that code & 0xff\n-\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t * a program that died from this signal.\n-\t\t */\n-\t\tcode += 128;\n-\t} else if (WIFEXITED(status)) {\n-\t\tcode = WEXITSTATUS(status);\n-\t\t/*\n-\t\t * Convert special exit code when execvp failed.\n-\t\t */\n-\t\tif (code == 127) {\n-\t\t\tcode = -1;\n-\t\t\tfailed_errno = ENOENT;\n-\t\t}\n \t} else {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t\tif (waiting != pid\n+\t\t || (determine_return_value(status, &code, &failed_errno, argv0) < 0))\n+\t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n \n \tclear_child_for_cleanup(pid);\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270434","messageId":"1442875159-13027-7-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:12Z","receivedAt":"2015-09-21T22:39:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to run external commands in parallel with ordered output\non stderr.\n\nIf we run external commands in parallel we cannot pipe the output directly\nto the our stdout/err as it would mix up. So each process's output will\nflow through a pipe, which we buffer. One subprocess can be directly\npiped to out stdout/err for a low latency feedback to the user.\n\nExample:\nLet's assume we have 5 submodules A,B,C,D,E and each fetch takes a\ndifferent amount of time as the different submodules vary in size, then\nthe output of fetches in sequential order might look like this:\n\n time -->\n output: |---A---| |-B-| |-------C-------| |-D-| |-E-|\n\nWhen we schedule these submodules into maximal two parallel processes,\na schedule and sample output over time may look like this:\n\nprocess 1: |---A---| |-D-| |-E-|\n\nprocess 2: |-B-| |-------C-------|\n\noutput: |---A---|B|---C-------|DE\n\nSo A will be perceived as it would run normally in the single child\nversion. As B has finished by the time A is done, we can dump its whole\nprogress buffer on stderr, such that it looks like it finished in no\ntime. Once that is done, C is determined to be the visible child and\nits progress will be reported in real time.\n\nSo this way of output is really good for human consumption, as it only\nchanges the timing, not the actual output.\n\nFor machine consumption the output needs to be prepared in the tasks,\nby either having a prefix per line or per block to indicate whose tasks\noutput is displayed, because the output order may not follow the\noriginal sequential ordering:\n\n |----A----| |--B--| |-C-|\n\nwill be scheduled to be all parallel:\n\nprocess 1: |----A----|\nprocess 2: |--B--|\nprocess 3: |-C-|\noutput: |----A----|CB\n\nThis happens because C finished before B did, so it will be queued for\noutput before B.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 275 +++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 36 +++++++\n t/t0061-run-command.sh | 20 ++++\n test-run-command.c | 24 +++++\n 4 files changed, 355 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 674e348..06d5a5d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -3,6 +3,8 @@\n #include \"exec_cmd.h\"\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n+#include \"thread-utils.h\"\n+#include \"strbuf.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -862,3 +864,276 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n \tclose(cmd->out);\n \treturn finish_command(cmd);\n }\n+\n+struct parallel_processes {\n+\tvoid *data;\n+\n+\tint max_processes;\n+\tint nr_processes;\n+\tunsigned all_tasks_started : 1;\n+\n+\tget_next_task_fn get_next_task;\n+\tstart_failure_fn start_failure;\n+\treturn_value_fn return_value;\n+\n+\tstruct {\n+\t\tunsigned in_use : 1;\n+\t\tstruct child_process process;\n+\t\tstruct strbuf err;\n+\t} *children;\n+\t/*\n+\t * The struct pollfd is logically part of *children,\n+\t * but the system call expects it as its own array.\n+\t */\n+\tstruct pollfd *pfd;\n+\n+\tint output_owner;\n+\tstruct strbuf buffered_output; /* of finished children */\n+};\n+\n+void default_start_failure(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err)\n+{\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(&sb, \"%s \", cp->argv[i]);\n+\n+\tdie_errno(\"Starting a child failed:\\n%s\", sb.buf);\n+}\n+\n+void default_return_value(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t int result)\n+{\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!result)\n+\t\treturn;\n+\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(&sb, \"%s \", cp->argv[i]);\n+\n+\tdie_errno(\"A child failed with return code:\\n%s\\n%d\", sb.buf, result);\n+}\n+\n+static void run_processes_parallel_init(struct parallel_processes *pp,\n+\t\t\t\t\tint n, void *data,\n+\t\t\t\t\tget_next_task_fn get_next_task,\n+\t\t\t\t\tstart_failure_fn start_failure,\n+\t\t\t\t\treturn_value_fn return_value)\n+{\n+\tint i;\n+\n+\tif (n < 1)\n+\t\tn = online_cpus();\n+\n+\tpp->max_processes = n;\n+\tpp->data = data;\n+\tif (!get_next_task)\n+\t\tdie(\"BUG: you need to specify a get_next_task function\");\n+\tpp->get_next_task = get_next_task;\n+\n+\tpp->start_failure = start_failure ? start_failure : default_start_failure;\n+\tpp->return_value = return_value ? return_value : default_return_value;\n+\n+\tpp->nr_processes = 0;\n+\tpp->all_tasks_started = 0;\n+\tpp->output_owner = 0;\n+\tpp->children = xcalloc(n, sizeof(*pp->children));\n+\tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n+\tstrbuf_init(&pp->buffered_output, 0);\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tstrbuf_init(&pp->children[i].err, 0);\n+\t\tpp->pfd[i].events = POLLIN;\n+\t\tpp->pfd[i].fd = -1;\n+\t}\n+}\n+\n+static void run_processes_parallel_cleanup(struct parallel_processes *pp)\n+{\n+\tint i;\n+\tfor (i = 0; i < pp->max_processes; i++)\n+\t\tstrbuf_release(&pp->children[i].err);\n+\n+\tfree(pp->children);\n+\tfree(pp->pfd);\n+\tstrbuf_release(&pp->buffered_output);\n+}\n+\n+static void set_nonblocking(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFL);\n+\tif (flags < 0)\n+\t\twarning(\"Could not get file status flags, \"\n+\t\t\t\"output will be degraded\");\n+\telse if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n+\t\twarning(\"Could not set file status flags, \"\n+\t\t\t\"output will be degraded\");\n+}\n+\n+static void run_processes_parallel_start_one(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < pp->max_processes; i++)\n+\t\tif (!pp->children[i].in_use)\n+\t\t\tbreak;\n+\tif (i == pp->max_processes)\n+\t\tdie(\"BUG: bookkeeping is hard\");\n+\n+\tif (!pp->get_next_task(pp->data,\n+\t\t\t &pp->children[i].process,\n+\t\t\t &pp->children[i].err)) {\n+\t\tpp->all_tasks_started = 1;\n+\t\treturn;\n+\t}\n+\tif (start_command(&pp->children[i].process))\n+\t\tpp->start_failure(pp->data,\n+\t\t\t\t &pp->children[i].process,\n+\t\t\t\t &pp->children[i].err);\n+\n+\tset_nonblocking(pp->children[i].process.err);\n+\n+\tpp->nr_processes++;\n+\tpp->children[i].in_use = 1;\n+\tpp->pfd[i].fd = pp->children[i].process.err;\n+}\n+\n+static void run_processes_parallel_start_as_needed(struct parallel_processes *pp)\n+{\n+\twhile (pp->nr_processes < pp->max_processes &&\n+\t !pp->all_tasks_started)\n+\t\trun_processes_parallel_start_one(pp);\n+}\n+\n+static void run_processes_parallel_buffer_stderr(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\trun_processes_parallel_cleanup(pp);\n+\t\tdie_errno(\"poll\");\n+\t}\n+\n+\t/* Buffer output from all pipes. */\n+\tfor (i = 0; i < pp->max_processes; i++) {\n+\t\tif (pp->children[i].in_use &&\n+\t\t pp->pfd[i].revents & POLLIN)\n+\t\t\tif (strbuf_read_once(&pp->children[i].err,\n+\t\t\t\t\t pp->children[i].process.err, 0) < 0)\n+\t\t\t\tif (errno != EAGAIN)\n+\t\t\t\t\tdie_errno(\"read\");\n+\t}\n+}\n+\n+static void run_processes_parallel_output(struct parallel_processes *pp)\n+{\n+\tint i = pp->output_owner;\n+\tif (pp->children[i].in_use &&\n+\t pp->children[i].err.len) {\n+\t\tfputs(pp->children[i].err.buf, stderr);\n+\t\tstrbuf_reset(&pp->children[i].err);\n+\t}\n+}\n+\n+static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n+{\n+\tint i = 0;\n+\tpid_t pid;\n+\tint wait_status, code;\n+\tint n = pp->max_processes;\n+\n+\twhile (pp->nr_processes > 0) {\n+\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n+\t\tif (pid == 0)\n+\t\t\treturn;\n+\n+\t\tif (pid < 0)\n+\t\t\tdie_errno(\"wait\");\n+\n+\t\tfor (i = 0; i < pp->max_processes; i++)\n+\t\t\tif (pp->children[i].in_use &&\n+\t\t\t pid == pp->children[i].process.pid)\n+\t\t\t\tbreak;\n+\t\tif (i == pp->max_processes)\n+\t\t\t/*\n+\t\t\t * waitpid returned another process id\n+\t\t\t * which we are not waiting for.\n+\t\t\t */\n+\t\t\treturn;\n+\n+\t\tif (strbuf_read_once(&pp->children[i].err,\n+\t\t\t\t pp->children[i].process.err, 0) < 0 &&\n+\t\t errno != EAGAIN)\n+\t\t\tdie_errno(\"strbuf_read_once\");\n+\n+\n+\t\tif (determine_return_value(wait_status, &code, &errno,\n+\t\t\t\t\t pp->children[i].process.argv[0]) < 0)\n+\t\t\terror(\"waitpid is confused (%s)\",\n+\t\t\t pp->children[i].process.argv[0]);\n+\n+\t\tpp->return_value(pp->data, &pp->children[i].process, code);\n+\n+\t\targv_array_clear(&pp->children[i].process.args);\n+\t\targv_array_clear(&pp->children[i].process.env_array);\n+\n+\t\tpp->nr_processes--;\n+\t\tpp->children[i].in_use = 0;\n+\t\tpp->pfd[i].fd = -1;\n+\n+\t\tif (i != pp->output_owner) {\n+\t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\t\t} else {\n+\t\t\tfputs(pp->children[i].err.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\n+\t\t\t/* Output all other finished child processes */\n+\t\t\tfputs(pp->buffered_output.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->buffered_output);\n+\n+\t\t\t/*\n+\t\t\t * Pick next process to output live.\n+\t\t\t * NEEDSWORK:\n+\t\t\t * For now we pick it randomly by doing a round\n+\t\t\t * robin. Later we may want to pick the one with\n+\t\t\t * the most output or the longest or shortest\n+\t\t\t * running process time.\n+\t\t\t */\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tif (pp->children[(pp->output_owner + i) % n].in_use)\n+\t\t\t\t\tbreak;\n+\t\t\tpp->output_owner = (pp->output_owner + i) % n;\n+\t\t}\n+\t}\n+}\n+\n+int run_processes_parallel(int n, void *data,\n+\t\t\t get_next_task_fn get_next_task,\n+\t\t\t start_failure_fn start_failure,\n+\t\t\t return_value_fn return_value)\n+{\n+\tstruct parallel_processes pp;\n+\trun_processes_parallel_init(&pp, n, data,\n+\t\t\t\t get_next_task,\n+\t\t\t\t start_failure,\n+\t\t\t\t return_value);\n+\n+\twhile (!pp.all_tasks_started || pp.nr_processes > 0) {\n+\t\trun_processes_parallel_start_as_needed(&pp);\n+\t\trun_processes_parallel_buffer_stderr(&pp);\n+\t\trun_processes_parallel_output(&pp);\n+\t\trun_processes_parallel_collect_finished(&pp);\n+\t}\n+\trun_processes_parallel_cleanup(&pp);\n+\n+\treturn 0;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a..3807fd1 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -119,4 +119,40 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+/**\n+ * This callback should initialize the child process and preload the\n+ * error channel. The preloading of is useful if you want to have a message\n+ * printed directly before the output of the child process.\n+ * You MUST set stdout_to_stderr.\n+ *\n+ * Return 1 if the next child is ready to run.\n+ * Return 0 if there are no more tasks to be processed.\n+ */\n+typedef int (*get_next_task_fn)(void *data,\n+\t\t\t\tstruct child_process *cp,\n+\t\t\t\tstruct strbuf *err);\n+\n+typedef void (*start_failure_fn)(void *data,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err);\n+\n+typedef void (*return_value_fn)(void *data,\n+\t\t\t\tstruct child_process *cp,\n+\t\t\t\tint result);\n+\n+/**\n+ * Runs up to n processes at the same time. Whenever a process can be\n+ * started, the callback `get_next_task` is called to obtain the data\n+ * fed to the child process.\n+ *\n+ * The children started via this function run in parallel and their output\n+ * to stderr is buffered, while one of the children will directly output\n+ * to stderr.\n+ */\n+\n+int run_processes_parallel(int n, void *data,\n+\t\t\t get_next_task_fn,\n+\t\t\t start_failure_fn,\n+\t\t\t return_value_fn);\n+\n #endif\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 9acf628..49aa3db 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -47,4 +47,24 @@ test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<-EOF\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+EOF\n+\n+test_expect_success 'run_command runs in parallel' '\n+\ttest-run-command run-command-parallel-4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 89c7de2..94c6eee 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -10,9 +10,29 @@\n \n #include \"git-compat-util.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n+#include \"strbuf.h\"\n #include \n #include \n \n+static int number_callbacks;\n+int parallel_next(void *data,\n+\t\t struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tstruct child_process *d = data;\n+\tif (number_callbacks >= 4)\n+\t\treturn 0;\n+\n+\targv_array_pushv(&cp->args, d->argv);\n+\tcp->stdout_to_stderr = 1;\n+\tcp->no_stdin = 1;\n+\tcp->err = -1;\n+\tstrbuf_addf(err, \"preloaded output of a child\\n\");\n+\tnumber_callbacks++;\n+\treturn 1;\n+}\n+\n int main(int argc, char **argv)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n@@ -30,6 +50,10 @@ int main(int argc, char **argv)\n \tif (!strcmp(argv[1], \"run-command\"))\n \t\texit(run_command(&proc));\n \n+\tif (!strcmp(argv[1], \"run-command-parallel-4\"))\n+\t\texit(run_processes_parallel(4, &proc, parallel_next,\n+\t\t\t\t\t NULL, NULL));\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270440","messageId":"1442875159-13027-8-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 07/13] fetch_populated_submodules: use new parallel job processing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:13Z","receivedAt":"2015-09-21T22:39:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we enable parallel processing of submodules, this\nonly adds the possibility for it. So this change should not change\nany user facing behavior.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 3 +-\n submodule.c | 119 +++++++++++++++++++++++++++++++++++++++-----------------\n submodule.h | 2 +-\n 3 files changed, 87 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ee1f1a9..6620ed0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_populated_submodules(&options,\n \t\t\t\t\t\t submodule_prefix,\n \t\t\t\t\t\t recurse_submodules,\n-\t\t\t\t\t\t verbosity < 0);\n+\t\t\t\t\t\t verbosity < 0,\n+\t\t\t\t\t\t 0);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/submodule.c b/submodule.c\nindex 1d64e57..a0e06e8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -12,6 +12,7 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n #include \"blob.h\"\n+#include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n@@ -615,37 +616,79 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tconst char *work_tree;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint quiet;\n+\tint result;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err);\n+\n+void handle_submodule_fetch_start_err(void *data, struct child_process *cp, struct strbuf *err)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\tspf->result = 1;\n+}\n+\n+void handle_submodule_fetch_finish( void *data, struct child_process *cp, int retvalue)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\n+\tif (retvalue)\n+\t\tspf->result = 1;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet)\n+\t\t\t int quiet, int max_parallel_jobs)\n {\n-\tint i, result = 0;\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *work_tree = get_git_work_tree();\n-\tif (!work_tree)\n+\tint i;\n+\tstruct submodule_parallel_fetch spf = SPF_INIT;\n+\tspf.work_tree = get_git_work_tree();\n+\tspf.command_line_option = command_line_option;\n+\tspf.quiet = quiet;\n+\tspf.prefix = prefix;\n+\tif (!spf.work_tree)\n \t\tgoto out;\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \n-\targv_array_push(&argv, \"fetch\");\n+\targv_array_push(&spf.args, \"fetch\");\n \tfor (i = 0; i < options->argc; i++)\n-\t\targv_array_push(&argv, options->argv[i]);\n-\targv_array_push(&argv, \"--recurse-submodules-default\");\n+\t\targv_array_push(&spf.args, options->argv[i]);\n+\targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcp.env = local_repo_env;\n-\tcp.git_cmd = 1;\n-\tcp.no_stdin = 1;\n-\n \tcalculate_changed_submodule_paths();\n+\trun_processes_parallel(max_parallel_jobs, &spf,\n+\t\t\t get_next_submodule,\n+\t\t\t handle_submodule_fetch_start_err,\n+\t\t\t handle_submodule_fetch_finish);\n+\n+\targv_array_clear(&spf.args);\n+out:\n+\tstring_list_clear(&changed_submodule_paths, 1);\n+\treturn spf.result;\n+}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tint ret = 0;\n+\tstruct submodule_parallel_fetch *spf = data;\n \n-\tfor (i = 0; i < active_nr; i++) {\n+\tfor ( ; spf->count < active_nr; spf->count++) {\n \t\tstruct strbuf submodule_path = STRBUF_INIT;\n \t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n-\t\tconst struct cache_entry *ce = active_cache[i];\n+\t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n \n@@ -657,7 +700,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tsubmodule = submodule_from_name(null_sha1, ce->name);\n \n \t\tdefault_argv = \"yes\";\n-\t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n+\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n \t\t\tif (submodule &&\n \t\t\t submodule->fetch_recurse !=\n \t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n@@ -680,40 +723,46 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n-\t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_path, \"%s/%s\", work_tree, ce->name);\n+\t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n \t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", prefix, ce->name);\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n \t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n \t\tif (!git_dir)\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n-\t\t\tif (!quiet)\n-\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n-\t\t\tcp.dir = submodule_path.buf;\n-\t\t\targv_array_push(&argv, default_argv);\n-\t\t\targv_array_push(&argv, \"--submodule-prefix\");\n-\t\t\targv_array_push(&argv, submodule_prefix.buf);\n-\t\t\tcp.argv = argv.argv;\n-\t\t\tif (run_command(&cp))\n-\t\t\t\tresult = 1;\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n+\t\t\tchild_process_init(cp);\n+\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n+\t\t\tcp->git_cmd = 1;\n+\t\t\tcp->no_stdout = 1;\n+\t\t\tcp->no_stdin = 1;\n+\t\t\tcp->stdout_to_stderr = 1;\n+\t\t\tcp->err = -1;\n+\t\t\tcp->env = local_repo_env;\n+\t\t\tif (!spf->quiet)\n+\t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n+\t\t\t\t\t spf->prefix, ce->name);\n+\t\t\targv_array_init(&cp->args);\n+\t\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\t\targv_array_push(&cp->args, default_argv);\n+\t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\t\t\tret = 1;\n \t\t}\n \t\tstrbuf_release(&submodule_path);\n \t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n+\t\tif (ret) {\n+\t\t\tspf->count++;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n-\targv_array_clear(&argv);\n-out:\n-\tstring_list_clear(&changed_submodule_paths, 1);\n-\treturn result;\n+\treturn 1;\n }\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\ndiff --git a/submodule.h b/submodule.h\nindex 5507c3d..cbc0003 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);\n void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet);\n+\t\t\t int quiet, int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270441","messageId":"1442875159-13027-9-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 08/13] submodules: allow parallel fetching, add tests and documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:14Z","receivedAt":"2015-09-21T22:39:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This enables the work of the previous patches.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/fetch-options.txt | 7 +++++++\n builtin/fetch.c | 5 ++++-\n builtin/pull.c | 6 ++++++\n submodule.c | 4 ++--\n t/t5526-fetch-submodules.sh | 19 +++++++++++++++++++\n 5 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 45583d8..6b109f6 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -100,6 +100,13 @@ ifndef::git-pull[]\n \treference to a commit that isn't already in the local submodule\n \tclone.\n \n+-j::\n+--jobs=::\n+\tNumber of parallel children to be used for fetching submodules.\n+\tEach will fetch from different submodules, such that fetching many\n+\tsubmodules will be faster. By default submodules will be fetched\n+\tone at a time.\n+\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the '--recurse-submodules=no' option).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6620ed0..f28eac6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n+static int max_children = 1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n+\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\t\t N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n@@ -1218,7 +1221,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t submodule_prefix,\n \t\t\t\t\t\t recurse_submodules,\n \t\t\t\t\t\t verbosity < 0,\n-\t\t\t\t\t\t 0);\n+\t\t\t\t\t\t max_children);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 722a83c..f0af196 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -94,6 +94,7 @@ static int opt_force;\n static char *opt_tags;\n static char *opt_prune;\n static char *opt_recurse_submodules;\n+static char *max_children;\n static int opt_dry_run;\n static char *opt_keep;\n static char *opt_depth;\n@@ -177,6 +178,9 @@ static struct option pull_options[] = {\n \t\tN_(\"on-demand\"),\n \t\tN_(\"control recursive fetching of submodules\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU('j', \"jobs\", &max_children, N_(\"n\"),\n+\t\tN_(\"number of submodules pulled in parallel\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_BOOL(0, \"dry-run\", &opt_dry_run,\n \t\tN_(\"dry run\")),\n \tOPT_PASSTHRU('k', \"keep\", &opt_keep, NULL,\n@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\targv_array_push(&args, opt_prune);\n \tif (opt_recurse_submodules)\n \t\targv_array_push(&args, opt_recurse_submodules);\n+\tif (max_children)\n+\t\targv_array_push(&args, max_children);\n \tif (opt_dry_run)\n \t\targv_array_push(&args, \"--dry-run\");\n \tif (opt_keep)\ndiff --git a/submodule.c b/submodule.c\nindex a0e06e8..d15364f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -759,10 +759,10 @@ int get_next_submodule(void *data, struct child_process *cp,\n \t\tstrbuf_release(&submodule_prefix);\n \t\tif (ret) {\n \t\t\tspf->count++;\n-\t\t\treturn 0;\n+\t\t\treturn 1;\n \t\t}\n \t}\n-\treturn 1;\n+\treturn 0;\n }\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 17759b1..1b4ce69 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -71,6 +71,16 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n+\tadd_upstream_commit &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules -j2 2>../actual.err\n+\t) &&\n+\ttest_must_be_empty actual.out &&\n+\ttest_i18ncmp expect.err actual.err\n+'\n+\n test_expect_success \"fetch alone only fetches superproject\" '\n \tadd_upstream_commit &&\n \t(\n@@ -140,6 +150,15 @@ test_expect_success \"--quiet propagates to submodules\" '\n \t! test -s actual.err\n '\n \n+test_expect_success \"--quiet propagates to parallel submodules\" '\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules -j 2 --quiet >../actual.out 2>../actual.err\n+\t) &&\n+\t! test -s actual.out &&\n+\t! test -s actual.err\n+'\n+\n test_expect_success \"--dry-run propagates to submodules\" '\n \tadd_upstream_commit &&\n \t(\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270436","messageId":"1442875159-13027-10-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 09/13] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:15Z","receivedAt":"2015-09-21T22:39:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 16 ++++++++++++++++\n submodule-config.h | 1 +\n 2 files changed, 17 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..0298a60 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tfree((void *) submodule->url);\n \t\tstrbuf_addstr(&url, value);\n \t\tsubmodule->url = strbuf_detach(&url, NULL);\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tstruct strbuf update = STRBUF_INIT;\n+\t\tif (!value) {\n+\t\t\tret = config_error_nonbool(var);\n+\t\t\tgoto release_return;\n+\t\t}\n+\t\tif (!me->overwrite && submodule->update != NULL) {\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t\"update\");\n+\t\t\tgoto release_return;\n+\t\t}\n+\n+\t\tfree((void *) submodule->update);\n+\t\tstrbuf_addstr(&update, value);\n+\t\tsubmodule->update = strbuf_detach(&update, NULL);\n \t}\n \n release_return:\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..f9e2a29 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -14,6 +14,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *update;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270438","messageId":"1442875159-13027-11-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 10/13] git submodule update: cmd_update_recursive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:16Z","receivedAt":"2015-09-21T22:39:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"split the recursion part out to its own function\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 47 ++++++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8964b1d..71385cb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -582,6 +582,31 @@ cmd_deinit()\n \tdone\n }\n \n+\n+cmd_update_recursive()\n+{\n+\tif test -n \"$recursive\"\n+\tthen\n+\t\t(\n+\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\tclear_local_git_env\n+\t\t\tcd \"$sm_path\" &&\n+\t\t\teval cmd_update\n+\t\t)\n+\t\tres=$?\n+\t\tif test $res -gt 0\n+\t\tthen\n+\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n+\t\t\tif test $res -eq 1\n+\t\t\tthen\n+\t\t\t\terr=\"${err};$die_msg\"\n+\t\t\telse\n+\t\t\t\tdie_with_status $res \"$die_msg\"\n+\t\t\tfi\n+\t\tfi\n+\tfi\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -790,27 +815,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tfi\n \t\tfi\n \n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tclear_local_git_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_update\n-\t\t\t)\n-\t\t\tres=$?\n-\t\t\tif test $res -gt 0\n-\t\t\tthen\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n-\t\t\t\tif test $res -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\t\tcontinue\n-\t\t\t\telse\n-\t\t\t\t\tdie_with_status $res \"$die_msg\"\n-\t\t\t\tfi\n-\t\t\tfi\n-\t\tfi\n+\t\tcmd_update_recursive\n \tdone\n \n \tif test -n \"$err\"\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270437","messageId":"1442875159-13027-12-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 11/13] git submodule update: cmd_update_clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:17Z","receivedAt":"2015-09-21T22:39:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n git-submodule.sh | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 71385cb..7f11158 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -607,6 +607,24 @@ cmd_update_recursive()\n \tfi\n }\n \n+cmd_update_clone()\n+{\n+\tcommand=\"git checkout $subforce -q\"\n+\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n+\n+\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\n+\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\tthen\n+\t\tsay \"$say_msg\"\n+\telse\n+\t\terr=\"${err};$die_msg\"\n+\t\treturn\n+\tfi\n+\tcmd_update_recursive\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -680,7 +698,6 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n-\t\t\tsubsha1=\n+\t\t\tcmd_update_clone\n+\t\t\tcontinue\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270435","messageId":"1442875159-13027-13-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 12/13] git submodule update: cmd_update_fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:18Z","receivedAt":"2015-09-21T22:39:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n git-submodule.sh | 164 ++++++++++++++++++++++++++++---------------------------\n 1 file changed, 84 insertions(+), 80 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 7f11158..a1bc8d5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -625,6 +625,89 @@ cmd_update_clone()\n \tcmd_update_recursive\n }\n \n+cmd_update_fetch()\n+{\n+\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\tgit rev-parse --verify HEAD) ||\n+\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\n+\tif test -n \"$remote\"\n+\tthen\n+\t\tif test -z \"$nofetch\"\n+\t\tthen\n+\t\t\t# Fetch remote before determining tracking $sha1\n+\t\t\t(clear_local_git_env; cd \"$sm_path\" && git-fetch) ||\n+\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n+\t\tfi\n+\t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n+\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n+\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n+\tfi\n+\n+\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n+\tthen\n+\t\tsubforce=$force\n+\t\t# If we don't already have a -f flag and the submodule has never been checked out\n+\t\tif test -z \"$subsha1\" && test -z \"$force\"\n+\t\tthen\n+\t\t\tsubforce=\"-f\"\n+\t\tfi\n+\n+\t\tif test -z \"$nofetch\"\n+\t\tthen\n+\t\t\t# Run fetch only if $sha1 isn't present or it\n+\t\t\t# is not reachable from a ref.\n+\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n+\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n+\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\t\tfi\n+\n+\t\tmust_die_on_failure=\n+\t\tcase \"$update_module\" in\n+\t\tcheckout)\n+\t\t\tcommand=\"git checkout $subforce -q\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n+\t\t\t;;\n+\t\trebase)\n+\t\t\tcommand=\"git rebase\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\tmerge)\n+\t\t\tcommand=\"git merge\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\t!*)\n+\t\t\tcommand=\"${update_module#!}\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$prefix\\$sm_path'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$prefix\\$sm_path': '\\$command \\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\t*)\n+\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n+\t\tesac\n+\n+\t\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\t\tthen\n+\t\t\tsay \"$say_msg\"\n+\t\telif test -n \"$must_die_on_failure\"\n+\t\tthen\n+\t\t\tdie_with_status 2 \"$die_msg\"\n+\t\telse\n+\t\t\terr=\"${err};$die_msg\"\n+\t\t\treturn\n+\t\tfi\n+\tfi\n+\n+\tcmd_update_recursive\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -743,88 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n \t\t\tcmd_update_clone\n-\t\t\tcontinue\n \t\telse\n-\t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\tgit rev-parse --verify HEAD) ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\t\tcmd_update_fetch\n \t\tfi\n-\n-\t\tif test -n \"$remote\"\n-\t\tthen\n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" && git-fetch) ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n-\t\t\tfi\n-\t\t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n-\t\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n-\n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n-\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$prefix\\$sm_path'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$prefix\\$sm_path': '\\$command \\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n-\t\t\tesac\n-\n-\t\t\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n-\n-\t\tcmd_update_recursive\n \tdone\n \n \tif test -n \"$err\"\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270439","messageId":"1442875159-13027-14-git-send-email-sbeller@google.com","threadId":"40398","inReplyTo":"1442875159-13027-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 13/13] Rewrite submodule update in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-21T22:39:19Z","receivedAt":"2015-09-21T22:39:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This will make parallelisation easier in a followup patch. This is just\na translation from shell to C, hopefully not introducing any bugs.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 247 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 135 +-----------------------\n run-command.h | 2 +\n 3 files changed, 252 insertions(+), 132 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..baa7563 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,252 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct module_update_data {\n+\tstruct module_list list;\n+\tint count;\n+\n+\tstruct pathspec pathspec;\n+\n+\tint no_fetch;\n+\tint force;\n+\tint update;\n+\tint quiet;\n+\tint recursive;\n+\tint remote;\n+\tchar *prefix;\n+\tchar *reference;\n+\tchar *depth;\n+\n+\tstruct argv_array args;\n+\n+\tint result;\n+};\n+\n+static void module_update_data_init(struct module_update_data *mud)\n+{\n+\tmud->list.entries = NULL;\n+\tmud->list.alloc = 0;\n+\tmud->list.nr = 0;\n+\n+\tmemset(&mud->pathspec, 0, sizeof(mud->pathspec));\n+\n+\tmud->count = 0;\n+\tmud->no_fetch = 0;\n+\tmud->force = 0;\n+\tmud->update = 0;\n+\tmud->quiet = 0;\n+\tmud->remote = 0;\n+\tmud->recursive = 0;\n+\tmud->result = 0;\n+\n+\tmud->prefix = NULL;\n+\tmud->reference = NULL;\n+\tmud->depth = NULL;\n+\n+\targv_array_init(&mud->args);\n+}\n+\n+static int update_next_task(void *data,\n+\t\t struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tint i;\n+\tstruct module_update_data *mud = data;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *displaypath;\n+\n+\tfor (; mud->count < mud->list.nr; mud->count++) {\n+\t\tconst char *update_module;\n+\t\tconst char *sm_gitdir;\n+\t\tconst struct submodule *sub;\n+\t\tconst struct cache_entry *ce = mud->list.entries[mud->count];\n+\n+\t\tdisplaypath = relative_path(ce->name, mud->prefix, &sb);\n+\t\tstrbuf_reset(&sb);\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\",\n+\t\t\t\t displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (!sub) {\n+\t\t\tmud->result = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\tswitch (mud->update) {\n+\t\tcase 'r':\n+\t\t\tupdate_module = \"rebase\";\n+\t\t\tbreak;\n+\t\tcase 'c':\n+\t\t\tupdate_module = \"checkout\";\n+\t\t\tbreak;\n+\t\tcase 'm':\n+\t\t\tupdate_module = \"merge\";\n+\t\t\tbreak;\n+\t\tcase 0:\n+\t\t\t/* not specified by command line */\n+\t\t\tif (sub->update)\n+\t\t\t\tupdate_module = sub->update;\n+\t\t\telse\n+\t\t\t\tupdate_module = \"checkout\";\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"BUG: update mode not covered\");\n+\t\t}\n+\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!sub->url) {\n+\t\t\t/*\n+\t\t\t * Only mention uninitialized submodules when its\n+\t\t\t * path have been specified\n+\t\t\t */\n+\t\t\tif (!mud->pathspec.nr)\n+\t\t\t\tcontinue;\n+\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t _(\"Submodule path '%s' not initialized \\n\"\n+\t\t\t\t \"Maybe you want to use 'update --init'?\"),\n+\t\t\t\t displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n+\n+\t\tchild_process_init(cp);\n+\t\tfor (i = 0; local_repo_env[i]; i++)\n+\t\t\targv_array_pushf(&cp->env_array, \"%s\", local_repo_env[i]);\n+\n+\t\targv_array_pushf(&cp->env_array, \"displaypath=%s\", displaypath);\n+\t\targv_array_pushf(&cp->env_array, \"sm_path=%s\", sub->path);\n+\t\targv_array_pushf(&cp->env_array, \"name=%s\", sub->name);\n+\t\targv_array_pushf(&cp->env_array, \"url=%s\", sub->url);\n+\t\targv_array_pushf(&cp->env_array, \"update_module=%s\", update_module);\n+\n+\t\tcp->git_cmd = 1;\n+\t\tcp->stdout_to_stderr = 1;\n+\t\targv_array_push(&cp->args, \"submodule\");\n+\t\tif (!file_exists(sm_gitdir))\n+\t\t\targv_array_push(&cp->args, \"update_clone\");\n+\t\telse\n+\t\t\targv_array_push(&cp->args, \"update_fetch\");\n+\n+\t\targv_array_pushf(&cp->args, \"%s\", ce->name);\n+\t\tmud->count++;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void update_subcommand_failure(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct module_update_data *mud = data;\n+\tstrbuf_addf(err, _(\"Could not start child process\"));\n+\tmud->result = 1;\n+}\n+\n+void update_child_return(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t int result)\n+{\n+\tstruct module_update_data *mud = data;\n+\tmud->result = 1;\n+}\n+\n+static int module_update(int argc, const char **argv, const char *prefix)\n+{\n+\tint init;\n+\tstruct module_update_data mud;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_BOOL('i', \"init\", &init,\n+\t\t\tN_(\"Initialize the submodule if not yet done\")),\n+\t\tOPT_BOOL(0, \"remote\", &mud.remote,\n+\t\t\tN_(\"Update the submodule to the remote branch instead \"\n+\t\t\t \"of the superprojects specification\")),\n+\t\tOPT_BOOL('N', \"no-fetch\", &mud.no_fetch,\n+\t\t\tN_(\"Don’t fetch new objects from the remote site.\")),\n+\t\tOPT_BOOL('f', \"force\", &mud.force,\n+\t\t\tN_(\"Ignore local changes in submodules\")),\n+\t\tOPT_CMDMODE('r', \"rebase\", &mud.update,\n+\t\t\tN_(\"Rebase local changes in submodules\"), 'r'),\n+\t\tOPT_CMDMODE('m', \"merge\", &mud.update,\n+\t\t\tN_(\"Merge local changes in submodules\"), 'm'),\n+\t\tOPT_CMDMODE(0, \"checkout\", &mud.update,\n+\t\t\tN_(\"Checkout to a detached HEAD in submodules\"), 'c'),\n+\t\tOPT_BOOL(0, \"recursive\", &mud.recursive,\n+\t\t\tN_(\"Update nested submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &mud.reference, \"\",\n+\t\t\tN_(\"Use the local reference repository \"\n+\t\t\t \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &mud.depth, \"\",\n+\t\t\tN_(\"Create a shallow clone truncated to the \"\n+\t\t\t \"specified number of revisions\")),\n+\t\tOPT__QUIET(&mud.quiet, N_(\"be quiet\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper list [--prefix=] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\tmodule_update_data_init(&mud);\n+\tgitmodules_config();\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (mud.force)\n+\t\targv_array_push(&mud.args, \"force=1\");\n+\tif (mud.quiet)\n+\t\targv_array_push(&mud.args, \"GIT_QUIET=1\");\n+\tif (mud.recursive)\n+\t\targv_array_push(&mud.args, \"recursive=1\");\n+\tif (mud.prefix)\n+\t\targv_array_pushf(&mud.args, \"prefix=%s\", mud.prefix);\n+\tif (mud.reference)\n+\t\targv_array_pushf(&mud.args, \"reference=%s\", mud.reference);\n+\tif (mud.depth)\n+\t\targv_array_pushf(&mud.args, \"depth=%s\", mud.depth);\n+\n+\tif (module_list_compute(argc, argv, prefix, &mud.pathspec, &mud.list) < 0)\n+\t\treturn 1;\n+\n+\tif (init) {\n+\t\tconst char **argv_init = xmalloc((2 + mud.list.nr) * sizeof(char*));\n+\t\tint argc = 0, i, code;\n+\t\targv_init[argc++] = \"submodule\";\n+\t\targv_init[argc++] = \"init\";\n+\n+\t\tfor (i = 0; i < mud.list.nr; i++) {\n+\t\t\tconst struct cache_entry *ce = mud.list.entries[i];\n+\t\t\targv_init[argc++] = ce->name;\n+\t\t}\n+\t\tcode = run_command_v_opt(argv_init, RUN_GIT_CMD);\n+\t\t\tif (code)\n+\t\t\t\treturn code;\n+\t}\n+\n+\trun_processes_parallel(1, &mud,\n+\t\t\t update_next_task,\n+\t\t\t update_subcommand_failure,\n+\t\t\t update_child_return);\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +510,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update\", module_update}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a1bc8d5..63e9b3b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -640,6 +640,7 @@ cmd_update_fetch()\n \t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\tfi\n \t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n+\t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n \t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n@@ -715,137 +716,7 @@ cmd_update_fetch()\n #\n cmd_update()\n {\n-\t# parse $args after \"submodule ... update\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n-\t\t\t;;\n-\t\t-i|--init)\n-\t\t\tinit=1\n-\t\t\t;;\n-\t\t--remote)\n-\t\t\tremote=1\n-\t\t\t;;\n-\t\t-N|--no-fetch)\n-\t\t\tnofetch=1\n-\t\t\t;;\n-\t\t-f|--force)\n-\t\t\tforce=$1\n-\t\t\t;;\n-\t\t-r|--rebase)\n-\t\t\tupdate=\"rebase\"\n-\t\t\t;;\n-\t\t--reference)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\treference=\"--reference=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--reference=*)\n-\t\t\treference=\"$1\"\n-\t\t\t;;\n-\t\t-m|--merge)\n-\t\t\tupdate=\"merge\"\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\t;;\n-\t\t--checkout)\n-\t\t\tupdate=\"checkout\"\n-\t\t\t;;\n-\t\t--depth)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tdepth=\"--depth=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--depth=*)\n-\t\t\tdepth=$1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tif test -n \"$init\"\n-\tthen\n-\t\tcmd_init \"--\" \"$@\" || return\n-\tfi\n-\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n-\terr=\n-\twhile read mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\turl=$(git config submodule.\"$name\".url)\n-\t\tbranch=$(get_submodule_config \"$name\" branch master)\n-\t\tif ! test -z \"$update\"\n-\t\tthen\n-\t\t\tupdate_module=$update\n-\t\telse\n-\t\t\tupdate_module=$(git config submodule.\"$name\".update)\n-\t\t\tif test -z \"$update_module\"\n-\t\t\tthen\n-\t\t\t\tupdate_module=\"checkout\"\n-\t\t\tfi\n-\t\tfi\n-\n-\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n-\n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tcmd_update_clone\n-\t\telse\n-\t\t\tcmd_update_fetch\n-\t\tfi\n-\tdone\n-\n-\tif test -n \"$err\"\n-\tthen\n-\t\tOIFS=$IFS\n-\t\tIFS=';'\n-\t\tfor e in $err\n-\t\tdo\n-\t\t\tif test -n \"$e\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$e\"\n-\t\t\tfi\n-\t\tdone\n-\t\tIFS=$OIFS\n-\t\texit 1\n-\tfi\n-\t}\n+\tgit submodule--helper update ${prefix:+--prefix \"$prefix\"} \"$@\"\n }\n \n set_name_rev () {\n@@ -1243,7 +1114,7 @@ cmd_sync()\n while test $# != 0 && test -z \"$command\"\n do\n \tcase \"$1\" in\n-\tadd | foreach | init | deinit | update | status | summary | sync)\n+\tadd | foreach | init | deinit | update | update_fetch | update_clone | status | summary | sync)\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\ndiff --git a/run-command.h b/run-command.h\nindex 3807fd1..0c1b363 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -155,4 +155,6 @@ int run_processes_parallel(int n, void *data,\n \t\t\t start_failure_fn,\n \t\t\t return_value_fn);\n \n+void run_processes_parallel_schedule_error(struct strbuf *err);\n+\n #endif\n-- \n2.5.0.275.ge015d2a\n"},{"id":"270453","messageId":"xmqq7fnj8hck.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 01/13] Sending \"Fetching submodule \" output to stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T23:47:55Z","receivedAt":"2015-09-21T23:47:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Subject: Re: [PATCHv3 01/13] Sending \"Fetching submodule \" output to stderr\n\nSubject: submodule: send \"....\" to standard error\n\nor something?\n\n> \t\tif (is_directory(git_dir)) {\n> \t\t\tif (!quiet)\n> -\t\t\t\tprintf(\"Fetching submodule %s%s\\n\", prefix, ce->name);\n> +\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n> \t\t\tcp.dir = submodule_path.buf;\n> \t\t\targv_array_push(&argv, default_argv);\n> \t\t\targv_array_push(&argv, \"--submodule-prefix\");\n\nThe change itself (above) and updates to the tests (omitted) looked\nsensible.\n\nThanks.\n"},{"id":"270455","messageId":"xmqq37y78gzt.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T23:55:34Z","receivedAt":"2015-09-21T23:55:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.\n> As the intend of xread is to read as much as possible either until the\n> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n> by not spinning the whole time, but rather wait for it politely by not\n> busy waiting.\n\nAs you said in the cover letter, this does look questionable. It is\nsweeping the problem under the rug (the hard-coded 100ms is a good\nclue to tell that). If the caller does want us to read thru to the\nend, then we would need to make it easier for such a caller to stop\nmarking the file descriptor to be non-blocking, but this does not do\nanything to help that. An alternative might be to automatically\nturn nonblocking off temporarily once we get EAGAIN (and turn it on\nagain before leaving); that would be an approach to make it\nunnecessary to fix the caller (which has its own set of problems,\nthough).\n\n> +\t\t\t\tif (i < 0) {\n> +\t\t\t\t\tif (errno == EINTR || errno == ENOMEM)\n> +\t\t\t\t\t\tcontinue;\n\nI can sort of see EINTR but why is ENOMEM any special than other\nerrors?\n\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\tdie_errno(\"poll\");\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> \t\treturn nr;\n> \t}\n> }\n"},{"id":"270456","messageId":"CAPig+cQKOEYYR3j-uEeFzF3-qAfqq4SdQrH8LPmSP0VmAOCtzw@mail.gmail.com","threadId":"40398","inReplyTo":"1442875159-13027-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-21T23:56:56Z","receivedAt":"2015-09-21T23:56:56Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller wrote:\n> From the man page:\n> [...]\n> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.\n\ns/So/&,/\ns/error/&,/\n\n> As the intend of xread is to read as much as possible either until the\n\ns/intend/intent/\n\n> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n> by not spinning the whole time, but rather wait for it politely by not\n> busy waiting.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -201,8 +201,23 @@ ssize_t xread(int fd, void *buf, size_t len)\n> len = MAX_IO_SIZE;\n> while (1) {\n> nr = read(fd, buf, len);\n> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n> - continue;\n> + if (nr < 0) {\n> + if (errno == EINTR)\n> + continue;\n> + if (errno == EAGAIN || errno == EWOULDBLOCK) {\n> + struct pollfd pfd;\n> + int i;\n> + pfd.events = POLLIN;\n> + pfd.fd = fd;\n> + i = poll(&pfd, 1, 100);\n\nWhy is this poll() using a timeout? Isn't that still a busy wait of\nsorts (even if less aggressive)?\n\n> + if (i < 0) {\n> + if (errno == EINTR || errno == ENOMEM)\n> + continue;\n> + else\n> + die_errno(\"poll\");\n> + }\n> + }\n> + }\n> return nr;\n> }\n> }\n> --\n> 2.5.0.275.ge015d2a\n"},{"id":"270457","messageId":"xmqqwpvj7240.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T00:02:23Z","receivedAt":"2015-09-22T00:02:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This wrapper just restarts on EINTR, but not on EAGAIN like xread\n> does. This gives less guarantees on the actual reading rather than\n> on returning fast.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> git-compat-util.h | 1 +\n> wrapper.c | 22 ++++++++++++++++++++++\n> 2 files changed, 23 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index c6d391f..9ccea85 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n> extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n> extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n> extern ssize_t xread(int fd, void *buf, size_t len);\n> +extern ssize_t xread_nonblock(int fd, void *buf, size_t len);\n> extern ssize_t xwrite(int fd, const void *buf, size_t len);\n> extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n> extern int xdup(int fd);\n> diff --git a/wrapper.c b/wrapper.c\n> index 50267a4..54ce231 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -223,6 +223,28 @@ ssize_t xread(int fd, void *buf, size_t len)\n> }\n> \n> /*\n> + * xread_nonblock() is the same a read(), but it automatically restarts read()\n> + * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that\n> + * \"len\" bytes is read even if the data is available.\n> + */\n\nThis comment is somewhat misleading to readers who knew there was\nxread() and this new one is a variant of it. xread() does not say\nanything about \"if the data is available\" (it just blocks and\ninsists reading that much if the data is not ready yet). If we drop\nthe \"even if ...\" from the end, that confusion would be avoided.\n\nTurning EWOULDBLOCK to EAGAIN is a very useful thing to help the\ncallers, and it deserves to be described in the above comment.\n\n> +ssize_t xread_nonblock(int fd, void *buf, size_t len)\n> +{\n> +\tssize_t nr;\n> +\tif (len > MAX_IO_SIZE)\n> +\t len = MAX_IO_SIZE;\n\nThis line is under-indented?\n\n> +\twhile (1) {\n> +\t\tnr = read(fd, buf, len);\n> +\t\tif (nr < 0) {\n> +\t\t\tif (errno == EINTR)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (errno == EWOULDBLOCK)\n> +\t\t\t\terrno = EAGAIN;\n> +\t\t}\n> +\t\treturn nr;\n> +\t}\n> +}\n> +\n> +/*\n> * xwrite() is the same a write(), but it automatically restarts write()\n> * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n> * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n"},{"id":"270459","messageId":"xmqqsi6771r2.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T00:10:09Z","receivedAt":"2015-09-22T00:10:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This wrapper just restarts on EINTR, but not on EAGAIN like xread\n> does. This gives less guarantees on the actual reading rather than\n> on returning fast.\n\nThe last sentence is a bit hard to parse, by the way.\n\nIt lets caller make some progress without stalling by marking the\nfile descriptor as nonblocking and calling this function, compared\nto calling xread() which will not give control back until reading\nthe asked size (or EOF).\n\nIt is not like \"reading in full is always the desireable thing to\ndo, and returning early is something that needs compromise on that\ndesirable behaviour to achieve\", which was the way I read your\nversion. These two kinds of callers want two different things, but\nthe phrase \"less guarantees\" conveys a misguided value judgement (in\nthat \"wanting to read in full always trumps other considerations\" is\nthe stance the person who is making the judgement makes) more than\nuseful information (e.g. \"the caller may want the control back\nwithout stalling, in which case this is a useful thing to use\").\n\nThe implementation itself is exactly like we discussed, and looks\ngood.\n\nThanks.\n"},{"id":"270460","messageId":"xmqqoagv71es.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-5-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T00:17:31Z","receivedAt":"2015-09-22T00:17:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The new call will read a fd into a strbuf once. The underlying call\n\n\"read from a fd\"\n\n> xread_nonblock is meant to execute non blockingly if the fd is set to\n> O_NONBLOCK.\n\nThe latter sentence adds more questions than it answers. If the\nfile descriptor is not set to non-blocking, what happens? Is it a\nbug in the caller, and if so do we give help to diagnose such a bug?\n\n\n\n> Signed-off-by: Stefan Beller \n> ---\n> strbuf.c | 11 +++++++++++\n> strbuf.h | 6 ++++++\n> 2 files changed, 17 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index cce5eed..35e71b8 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> \treturn sb->len - oldlen;\n> }\n> \n> +ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n> +{\n> +\tssize_t cnt;\n> +\n> +\tstrbuf_grow(sb, hint ? hint : 8192);\n> +\tcnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> +\tif (cnt > 0)\n> +\t\tstrbuf_setlen(sb, sb->len + cnt);\n> +\treturn cnt;\n\nOK. So the caller that receives a negative value can check errno to\nsee if we got EAGAIN. How would the caller tell when it got an EOF?\n\n> /**\n> + * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.\n\nI do not think you want to say \"same as\" for this one.\nstrbuf_read() is about reading thru to the end, but this is about\nmaking some progress without blocking.\n\n Read from a file descriptor that is marked as O_NONBLOCK without\n blocking. Returns the number of new bytes appended to the sb.\n Negative return value signals there was an error returned from\n underlying read(2), in which case the caller should check errno.\n e.g. errno == EAGAIN when the read may have blocked.\n\nor something? Again, how would a caller tell when it got an EOF?\n\n> + * The fd must have set O_NONBLOCK.\n> + */\n> +extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n> +\n> +/**\n> * Read the contents of a file, specified by its path. The third argument\n> * can be used to give a hint about the file size, to avoid reallocs.\n> */\n"},{"id":"270461","messageId":"xmqqk2rj70fm.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 05/13] run-command: factor out return value computation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T00:38:37Z","receivedAt":"2015-09-22T00:38:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> We will need computing the return value in a later patch without the\n> wait.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------\n> 1 file changed, 32 insertions(+), 22 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 28e1d55..674e348 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)\n> \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n> }\n> \n> +static int determine_return_value(int wait_status,\n> +\t\t\t\t int *result,\n> +\t\t\t\t int *error_code,\n> +\t\t\t\t const char *argv0)\n> +{\n> +\tif (WIFSIGNALED(wait_status)) {\n> +\t\t*result = WTERMSIG(wait_status);\n> +\t\tif (*result != SIGINT && *result != SIGQUIT)\n> +\t\t\terror(\"%s died of signal %d\", argv0, *result);\n> +\t\t/*\n> +\t\t * This return value is chosen so that code & 0xff\n> +\t\t * mimics the exit code that a POSIX shell would report for\n> +\t\t * a program that died from this signal.\n> +\t\t */\n\nNot a new problem but \"from these signals\", I think.\n\n> @@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> \tif (waiting < 0) {\n> \t\tfailed_errno = errno;\n> \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n> -...\n> \t} else {\n> -\t\terror(\"waitpid is confused (%s)\", argv0);\n> +\t\tif (waiting != pid\n> +\t\t || (determine_return_value(status, &code, &failed_errno, argv0) < 0))\n\nMove \"||\" to the end of the previous line?\n\n> +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n> \t}\n> \n> \tclear_child_for_cleanup(pid);\n"},{"id":"270462","messageId":"CAPig+cRGBSYbt6FsRwNR2TYz7vD650f3=KH_c87h27SaVCUTcQ@mail.gmail.com","threadId":"40398","inReplyTo":"1442875159-13027-10-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 09/13] submodule config: keep update strategy around","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-22T00:56:49Z","receivedAt":"2015-09-22T00:56:49Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller wrote:\n> We need the submodule update strategies in a later patch.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/submodule-config.c b/submodule-config.c\n> @@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)\n> free((void *) submodule->url);\n> strbuf_addstr(&url, value);\n> submodule->url = strbuf_detach(&url, NULL);\n> + } else if (!strcmp(item.buf, \"update\")) {\n> + struct strbuf update = STRBUF_INIT;\n> + if (!value) {\n> + ret = config_error_nonbool(var);\n> + goto release_return;\n> + }\n> + if (!me->overwrite && submodule->update != NULL) {\n> + warn_multiple_config(me->commit_sha1, submodule->name,\n> + \"update\");\n> + goto release_return;\n> + }\n> +\n> + free((void *) submodule->update);\n> + strbuf_addstr(&update, value);\n> + submodule->update = strbuf_detach(&update, NULL);\n> }\n>\n> release_return:\n\nWhy the complicated logic flow? Also, why is strbuf 'update' needed?\n\nI'd have expected to see something simpler, such as:\n\n } else if (!strcmp(item.buf, \"update\")) {\n if (!value)\n ret = config_error_nonbool(var);\n else if (!me->overwrite && ...)\n warn_multiple_config(...);\n else {\n free((void *)submodule->update);\n submodule->update = xstrdup(value);\n }\n }\n"},{"id":"270465","messageId":"xmqqfv276z1q.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T01:08:33Z","receivedAt":"2015-09-22T01:08:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +void default_start_failure(void *data,\n> +\t\t\t struct child_process *cp,\n> +\t\t\t struct strbuf *err)\n> +{\n> +\tint i;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tfor (i = 0; cp->argv[i]; i++)\n> +\t\tstrbuf_addf(&sb, \"%s \", cp->argv[i]);\n> +\tdie_errno(\"Starting a child failed:\\n%s\", sb.buf);\n\nDo we want that trailing SP after the last element of argv[]?\nSame question applies to the one in \"return-value\".\n\n> +static void run_processes_parallel_init(struct parallel_processes *pp,\n> +\t\t\t\t\tint n, void *data,\n> +\t\t\t\t\tget_next_task_fn get_next_task,\n> +\t\t\t\t\tstart_failure_fn start_failure,\n> +\t\t\t\t\treturn_value_fn return_value)\n> +{\n> +\tint i;\n> +\n> +\tif (n < 1)\n> +\t\tn = online_cpus();\n> +\n> +\tpp->max_processes = n;\n> +\tpp->data = data;\n> +\tif (!get_next_task)\n> +\t\tdie(\"BUG: you need to specify a get_next_task function\");\n> +\tpp->get_next_task = get_next_task;\n> +\n> +\tpp->start_failure = start_failure ? start_failure : default_start_failure;\n> +\tpp->return_value = return_value ? return_value : default_return_value;\n\nI would actually have expected that leaving these to NULL will just\nskip pp->fn calls, instead of a \"default implementation\", but a pair\nof very simple default implementation would not hrtut.\n\n> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)\n> +{\n> +\tint i;\n\nHave a blank between the decl block and the first stmt here (and\nelsewhere, too---which you got correct in the function above)?\n\n> +\tfor (i = 0; i < pp->max_processes; i++)\n> +\t\tstrbuf_release(&pp->children[i].err);\n\n> +static void run_processes_parallel_start_one(struct parallel_processes *pp)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < pp->max_processes; i++)\n> +\t\tif (!pp->children[i].in_use)\n> +\t\t\tbreak;\n> +\tif (i == pp->max_processes)\n> +\t\tdie(\"BUG: bookkeeping is hard\");\n\nMental note: the caller is responsible for not calling this when all\nslots are taken.\n\n> +\tif (!pp->get_next_task(pp->data,\n> +\t\t\t &pp->children[i].process,\n> +\t\t\t &pp->children[i].err)) {\n> +\t\tpp->all_tasks_started = 1;\n> +\t\treturn;\n> +\t}\n\nMental note: but it is OK to call this if get_next_task() previously\nsaid \"no more task\".\n\nThe above two shows a slight discrepancy (nothing earth-breaking).\n\nI have this suspicion that the all-tasks-started bit may turn out to\nbe a big mistake that we may later regret. Don't we want to allow\npp->more_task() to say \"no more task to run at this moment\" implying\n\"but please do ask me later, because I may have found more to do by\nthe time you ask me again\"?\n\nThat is one of the reasons why I do not think the \"very top level is\na bulleted list\" organization is a good idea in general. A good\nscheduling decision can seldom be made in isolation without taking\nglobal picture into account.\n\n> +static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n> +{\n> +\tint i = 0;\n> +\tpid_t pid;\n> +\tint wait_status, code;\n> +\tint n = pp->max_processes;\n> +\n> +\twhile (pp->nr_processes > 0) {\n> +\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n> +\t\tif (pid == 0)\n> +\t\t\treturn;\n> +\n> +\t\tif (pid < 0)\n> +\t\t\tdie_errno(\"wait\");\n> +\n> +\t\tfor (i = 0; i < pp->max_processes; i++)\n> +\t\t\tif (pp->children[i].in_use &&\n> +\t\t\t pid == pp->children[i].process.pid)\n> +\t\t\t\tbreak;\n> +\t\tif (i == pp->max_processes)\n> +\t\t\t/*\n> +\t\t\t * waitpid returned another process id\n> +\t\t\t * which we are not waiting for.\n> +\t\t\t */\n> +\t\t\treturn;\n\nIf we culled a child process that this machinery is not in charge\nof, waitpid() in other places that wants to see that child will not\nsee it. Perhaps such a situation might even warrant an error() or\nBUG()? Do we want a \"NEEDSWORK: Is this a bug?\" comment here at\nleast?\n\n> +\t\tif (strbuf_read_once(&pp->children[i].err,\n> +\t\t\t\t pp->children[i].process.err, 0) < 0 &&\n> +\t\t errno != EAGAIN)\n> +\t\t\tdie_errno(\"strbuf_read_once\");\n\nDon't we want to read thru to the end here? The reason read_once()\ndid not read thru to the end may not have anything to do with\nNONBLOCK (e.g. xread_nonblock() caps len, and it does not loop).\n"},{"id":"270468","messageId":"5600DF2D.9010202@web.de","threadId":"40398","inReplyTo":"xmqq37y78gzt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-09-22T04:55:09Z","receivedAt":"2015-09-22T04:55:09Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 09/22/2015 01:55 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.\n>> As the intend of xread is to read as much as possible either until the\n>> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n>> by not spinning the whole time, but rather wait for it politely by not\n>> busy waiting.\n> As you said in the cover letter, this does look questionable. It is\n> sweeping the problem under the rug (the hard-coded 100ms is a good\n> clue to tell that). If the caller does want us to read thru to the\n> end, then we would need to make it easier for such a caller to stop\n> marking the file descriptor to be non-blocking, but this does not do\n> anything to help that. An alternative might be to automatically\n> turn nonblocking off temporarily once we get EAGAIN (and turn it on\n> again before leaving); that would be an approach to make it\n> unnecessary to fix the caller (which has its own set of problems,\n> though).\nWouldn' that function be somewhat mis-named and/or mis-behaved?\nread_in_full_with_hard_coded_timeout_and_fix_O_NONBLOCK()\ncould make sure that the user of this function knows what's going on\nunder the hood.\n\nMore seriously, if someone calls xread() with a non-blocking socket,\nthe caller wants to return and does want to his own timeout handling.\n\nIf we want to have a timeouted read(), we can call it\n\nxread_timout(int fd, voxread(int fd, void *buf, size_t len, int timeout)\n\n(Or something similar) to make clear that there is an underlying\ntimeout handling.\nAnother option could be to name the function\n\nread_in_full_timeout().\n\nBut in any case I suggest to xread() as it is, and not to change the \nfunctionality\nbehind the back of the users.\n"},{"id":"270471","messageId":"CA+P7+xozmsT4mZsBdvSQkGx4Wk-bOE1h-vAbRG_LAL5A-MAwTw@mail.gmail.com","threadId":"40398","inReplyTo":"5600DF2D.9010202@web.de","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T06:23:38Z","receivedAt":"2015-09-22T06:23:38Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Sep 21, 2015 at 9:55 PM, Torsten Bögershausen wrote:\n> But in any case I suggest to xread() as it is, and not to change the\n> functionality\n> behind the back of the users.\n>\n>\n\nI don't think this patch actually changes behavior as it stands now. I\nthink Junio's suggestion does. Personally, I'd prefer some sort of\nwarning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather\nsee it somehow warn so that we can find the bug (since we really\nreally shouldn't be calling xread with a blocking socket, especially\nif we have xread_noblock or similar as in this series.\n\nNot sure if we really want to handle that, but I know we don't want to\nchange external behavior of xread... I think that polling is better\nthan the current \"spinning\" behavior.\n\nRegards,\nJake\n"},{"id":"270472","messageId":"CA+P7+xpWPShFWokrKYsJ13dOnugaJAg9FhfEoxwi_wwscik=CQ@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqsi6771r2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T06:26:48Z","receivedAt":"2015-09-22T06:26:48Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Sep 21, 2015 at 5:10 PM, Junio C Hamano wrote:\n>> This wrapper just restarts on EINTR, but not on EAGAIN like xread\n>> does. This gives less guarantees on the actual reading rather than\n>> on returning fast.\n>\n> The last sentence is a bit hard to parse, by the way.\n>\n\nI'd imagine something more like:\n\nProvide a wrapper to read(), similar to xread(), that restarts on\nEINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to\nhandle polling itself, possibly polling multiple sockets or performing\nsome other action.\n\nRegards,\nJake\n"},{"id":"270473","messageId":"CA+P7+xp-D-N8W7GL6Rp9F-GHJ5O5jS6_icqB4DQGsiU5Zn5HEg@mail.gmail.com","threadId":"40398","inReplyTo":"1442875159-13027-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T06:27:45Z","receivedAt":"2015-09-22T06:27:45Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller wrote:\n\nMaybe change the title to \"without blocking\" instead of \"nonblockingly\"?\n\nRegards,\nJake\n"},{"id":"270474","messageId":"CA+P7+xpC+EiNTBqgwd0We+f-_xqfY0xx2BzvTGC7Owk-ay10fw@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqoagv71es.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-22T06:29:14Z","receivedAt":"2015-09-22T06:29:14Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Sep 21, 2015 at 5:17 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The new call will read a fd into a strbuf once. The underlying call\n>\n> \"read from a fd\"\n>\n>> xread_nonblock is meant to execute non blockingly if the fd is set to\n>> O_NONBLOCK.\n>\n> The latter sentence adds more questions than it answers. If the\n> file descriptor is not set to non-blocking, what happens? Is it a\n> bug in the caller, and if so do we give help to diagnose such a bug?\n>\n\nI would vote for adding debugging for calling xread with O_NONBLOCK\nand for calling xread_nonblock without O_NONBLOCK.. I'm not sure the\nbest way to do this, but it certainly feels like a bug to call xread\nwith O_NONBLOCK set... And the same here...\n\nRegards,\nJake\n"},{"id":"270487","messageId":"CAGZ79kYmz9+V8CUQhr3QsRdUkzi7ofMPK=qdXkW=o-+CToU1kw@mail.gmail.com","threadId":"40398","inReplyTo":"CAPig+cRGBSYbt6FsRwNR2TYz7vD650f3=KH_c87h27SaVCUTcQ@mail.gmail.com","subject":"Re: [PATCHv3 09/13] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T15:50:12Z","receivedAt":"2015-09-22T15:50:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 21, 2015 at 5:56 PM, Eric Sunshine wrote:\n> On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller wrote:\n>> We need the submodule update strategies in a later patch.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/submodule-config.c b/submodule-config.c\n>> @@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)\n>> free((void *) submodule->url);\n>> strbuf_addstr(&url, value);\n>> submodule->url = strbuf_detach(&url, NULL);\n>> + } else if (!strcmp(item.buf, \"update\")) {\n>> + struct strbuf update = STRBUF_INIT;\n>> + if (!value) {\n>> + ret = config_error_nonbool(var);\n>> + goto release_return;\n>> + }\n>> + if (!me->overwrite && submodule->update != NULL) {\n>> + warn_multiple_config(me->commit_sha1, submodule->name,\n>> + \"update\");\n>> + goto release_return;\n>> + }\n>> +\n>> + free((void *) submodule->update);\n>> + strbuf_addstr(&update, value);\n>> + submodule->update = strbuf_detach(&update, NULL);\n>> }\n>>\n>> release_return:\n>\n> Why the complicated logic flow? Also, why is strbuf 'update' needed?\n\nTo be honest, I just copied it from above, where the url is read using\nthe exact same workflow. In the reroll I'll fix both.\n\n>\n> I'd have expected to see something simpler, such as:\n>\n> } else if (!strcmp(item.buf, \"update\")) {\n> if (!value)\n> ret = config_error_nonbool(var);\n> else if (!me->overwrite && ...)\n> warn_multiple_config(...);\n> else {\n> free((void *)submodule->update);\n> submodule->update = xstrdup(value);\n> }\n> }\n"},{"id":"270489","messageId":"xmqqh9mm5tus.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAPig+cQKOEYYR3j-uEeFzF3-qAfqq4SdQrH8LPmSP0VmAOCtzw@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T15:58:19Z","receivedAt":"2015-09-22T15:58:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> while (1) {\n>> nr = read(fd, buf, len);\n>> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>> - continue;\n>> + if (nr < 0) {\n>> + if (errno == EINTR)\n>> + continue;\n>> + if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>> + struct pollfd pfd;\n>> + int i;\n>> + pfd.events = POLLIN;\n>> + pfd.fd = fd;\n>> + i = poll(&pfd, 1, 100);\n>\n> Why is this poll() using a timeout? Isn't that still a busy wait of\n> sorts (even if less aggressive)?\n\nGood point. If we _were_ to have this kind of \"hiding issues under\nthe rug and continuing without issues\" approach, I do not think we\nwould need timeout for this poll(2). The caller accepted that it is\nwilling to wait until we read up to len (which is capped, though) by\nnot calling the nonblocking variant.\n"},{"id":"270491","messageId":"xmqqd1xa5tsg.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CA+P7+xp-D-N8W7GL6Rp9F-GHJ5O5jS6_icqB4DQGsiU5Zn5HEg@mail.gmail.com","subject":"Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T15:59:43Z","receivedAt":"2015-09-22T15:59:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller wrote:\n>\n> Maybe change the title to \"without blocking\" instead of \"nonblockingly\"?\n\nBoth suggestions make sense ;-)\n"},{"id":"270495","messageId":"xmqqvbb24dvj.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"1442875159-13027-8-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 07/13] fetch_populated_submodules: use new parallel job processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T16:28:48Z","receivedAt":"2015-09-22T16:28:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In a later patch we enable parallel processing of submodules, this\n> only adds the possibility for it. So this change should not change\n> any user facing behavior.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/fetch.c | 3 +-\n> submodule.c | 119 +++++++++++++++++++++++++++++++++++++++-----------------\n> submodule.h | 2 +-\n> 3 files changed, 87 insertions(+), 37 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ee1f1a9..6620ed0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tresult = fetch_populated_submodules(&options,\n> \t\t\t\t\t\t submodule_prefix,\n> \t\t\t\t\t\t recurse_submodules,\n> -\t\t\t\t\t\t verbosity < 0);\n> +\t\t\t\t\t\t verbosity < 0,\n> +\t\t\t\t\t\t 0);\n\nThis one...\n\n> int fetch_populated_submodules(const struct argv_array *options,\n> \t\t\t const char *prefix, int command_line_option,\n> -\t\t\t int quiet)\n> +\t\t\t int quiet, int max_parallel_jobs)\n> {\n> -\tint i, result = 0;\n> -\tstruct child_process cp = CHILD_PROCESS_INIT;\n\n... together with this one, could have been made easier to follow if\nyou didn't add a new parameter to the function. Instead, you could\ndefine a local variable max_parallel_jobs initialized to 1 in this\nfunction without changing the function signature (and the caller) in\nthis step, and then did the above two changes in the same commit as\nthe one that actually enables the feature.\n\nThat would be more in line with the stated \"only adds the possiblity\nfor it\" goal.\n\nAs posted, I suspect that by passing 0 to max_parallel_jobs, you are\ntelling run_processes_parallel_init() to check online_cpus() and run\nthat many processes in parallel, no?\n\n> +int get_next_submodule(void *data, struct child_process *cp,\n> +\t\t struct strbuf *err)\n> +{\n> +\tint ret = 0;\n> +\tstruct submodule_parallel_fetch *spf = data;\n> ...\n> +\t\t\tchild_process_init(cp);\n> +\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n> +\t\t\tcp->git_cmd = 1;\n> +\t\t\tcp->no_stdout = 1;\n> +\t\t\tcp->no_stdin = 1;\n\nIn run-commands.c::start_command(), no_stdout triggers\ndup_devnull(1) and causes dup2(2, 1) that is triggered by\nstdout_to_stderrd to be bypassed. This looks wrong to me.\n\n> +\t\t\tcp->stdout_to_stderr = 1;\n> +\t\t\tcp->err = -1;\n\nOK, the original left the standard error stream connected to the\ninvoker's standard error, but now we are capturing it via a pipe.\n"},{"id":"270497","messageId":"CAGZ79kaJmy4EijFuLCXOzPbi3EoqSwarFj3_3r+ghUwVA+88Mg@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqh9mm5tus.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T17:38:54Z","receivedAt":"2015-09-22T17:38:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> while (1) {\n>>> nr = read(fd, buf, len);\n>>> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>>> - continue;\n>>> + if (nr < 0) {\n>>> + if (errno == EINTR)\n>>> + continue;\n>>> + if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>>> + struct pollfd pfd;\n>>> + int i;\n>>> + pfd.events = POLLIN;\n>>> + pfd.fd = fd;\n>>> + i = poll(&pfd, 1, 100);\n>>\n>> Why is this poll() using a timeout? Isn't that still a busy wait of\n>> sorts (even if less aggressive)?\n>\n\nTrue. Maybe we could have just a warning for now?\n\n if (errno == EAGAIN) {\n warning(\"Using xread with a non blocking fd\");\n continue; /* preserve previous behavior */\n }\n\nI think I am going to drop this patch off the main series and spin it out\nas an extra patch as the discussion is a bit unclear to me at the moment\nwhere we're heading.\n\n> Good point. If we _were_ to have this kind of \"hiding issues under\n> the rug and continuing without issues\" approach, I do not think we\n> would need timeout for this poll(2). The caller accepted that it is\n> willing to wait until we read up to len (which is capped, though) by\n> not calling the nonblocking variant.\n"},{"id":"270501","messageId":"xmqqeghq48nl.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAGZ79kaJmy4EijFuLCXOzPbi3EoqSwarFj3_3r+ghUwVA+88Mg@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T18:21:34Z","receivedAt":"2015-09-22T18:21:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>>> while (1) {\n>>>> nr = read(fd, buf, len);\n>>>> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>>>> - continue;\n>>>> + if (nr < 0) {\n>>>> + if (errno == EINTR)\n>>>> + continue;\n>>>> + if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>>>> + struct pollfd pfd;\n>>>> + int i;\n>>>> + pfd.events = POLLIN;\n>>>> + pfd.fd = fd;\n>>>> + i = poll(&pfd, 1, 100);\n>>>\n>>> Why is this poll() using a timeout? Isn't that still a busy wait of\n>>> sorts (even if less aggressive)?\n>\n> True. Maybe we could have just a warning for now?\n>\n> if (errno == EAGAIN) {\n> warning(\"Using xread with a non blocking fd\");\n> continue; /* preserve previous behavior */\n> }\n\nIt is very likely that you will hit the same EAGAIN immediately by\ncontinuing and end up spewing tons of the same warning, no?\n\nWe may want to slow down and think before sending a knee-jerk\nresponse (this applies to both of us).\n\nI actually think a blocking poll(2) here would not be such a bad\nidea. It would preserves the previous behaviour for callers who\nunknowingly inherited a O_NONBLOCK file descriptor from its\nenvironment without forcing them to waste CPU cycles.\n"},{"id":"270502","messageId":"CAGZ79kbUkUSAP+muhYxTwHZdD+ojJYXjogZfRXs0PemEdcqfbA@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqfv276z1q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T18:28:31Z","receivedAt":"2015-09-22T18:28:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 21, 2015 at 6:08 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +void default_start_failure(void *data,\n>> + struct child_process *cp,\n>> + struct strbuf *err)\n>> +{\n>> + int i;\n>> + struct strbuf sb = STRBUF_INIT;\n>> +\n>> + for (i = 0; cp->argv[i]; i++)\n>> + strbuf_addf(&sb, \"%s \", cp->argv[i]);\n>> + die_errno(\"Starting a child failed:\\n%s\", sb.buf);\n>\n> Do we want that trailing SP after the last element of argv[]?\n> Same question applies to the one in \"return-value\".\n\ndone\n\n>\n>> +static void run_processes_parallel_init(struct parallel_processes *pp,\n>> + int n, void *data,\n>> + get_next_task_fn get_next_task,\n>> + start_failure_fn start_failure,\n>> + return_value_fn return_value)\n>> +{\n>> + int i;\n>> +\n>> + if (n < 1)\n>> + n = online_cpus();\n>> +\n>> + pp->max_processes = n;\n>> + pp->data = data;\n>> + if (!get_next_task)\n>> + die(\"BUG: you need to specify a get_next_task function\");\n>> + pp->get_next_task = get_next_task;\n>> +\n>> + pp->start_failure = start_failure ? start_failure : default_start_failure;\n>> + pp->return_value = return_value ? return_value : default_return_value;\n>\n> I would actually have expected that leaving these to NULL will just\n> skip pp->fn calls, instead of a \"default implementation\", but a pair\n> of very simple default implementation would not hrtut.\n\nOk, I think the default implementation provided is a reasonable default, as\nit provides enough information in case of an error.\n\n>\n>> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)\n>> +{\n>> + int i;\n>\n> Have a blank between the decl block and the first stmt here (and\n> elsewhere, too---which you got correct in the function above)?\n\ndone\n\n>\n>> + for (i = 0; i < pp->max_processes; i++)\n>> + strbuf_release(&pp->children[i].err);\n>\n>> +static void run_processes_parallel_start_one(struct parallel_processes *pp)\n>> +{\n>> + int i;\n>> +\n>> + for (i = 0; i < pp->max_processes; i++)\n>> + if (!pp->children[i].in_use)\n>> + break;\n>> + if (i == pp->max_processes)\n>> + die(\"BUG: bookkeeping is hard\");\n>\n> Mental note: the caller is responsible for not calling this when all\n> slots are taken.\n>\n>> + if (!pp->get_next_task(pp->data,\n>> + &pp->children[i].process,\n>> + &pp->children[i].err)) {\n>> + pp->all_tasks_started = 1;\n>> + return;\n>> + }\n>\n> Mental note: but it is OK to call this if get_next_task() previously\n> said \"no more task\".\n>\n> The above two shows a slight discrepancy (nothing earth-breaking).\n\nI see. Maybe this can be improved by having the\nrun_processes_parallel_start_as_needed call get_next_task\nand pass the information into the run_processes_parallel_start_one\nor as we had it before, combine these two functions again.\n\n>\n> I have this suspicion that the all-tasks-started bit may turn out to\n> be a big mistake that we may later regret. Don't we want to allow\n> pp->more_task() to say \"no more task to run at this moment\" implying\n> \"but please do ask me later, because I may have found more to do by\n> the time you ask me again\"?\n\nAnd this task would arise because the current running children produce\nmore work to be done?\nSo you would have a\n more_tasks() question. If that returns true\n get_next_task() must provide that next task?\n\nIn case we had more work to do, which is based on the outcome of the\nchildren, we could just wait in get_next_task for a semaphore/condition\nvariable from the return_value. Though that would stop progress reporting\nend maybe lock up the whole program due to pipe clogging.\n\nIt seems to be a better design as we come back to the main loop fast\nwhich does the polling. Although I feel like it is over engineered for now.\n\nSo how would you find out when we are done?\n* more_tasks() could have different return values in an enum\n (YES_THERE_ARE, NO_BUT_ASK_LATER, NO_NEVER_ASK_AGAIN)\n* There could be yet another callback more_tasks_available() and\n parallel_processing_should_stop()\n* Hand back a callback ourselfs [Call signal_parallel_processing_done(void*)\n when more_tasks will never return true again, with a void* we provide to\n more_tasks()]\n* ...\n\n>\n> That is one of the reasons why I do not think the \"very top level is\n> a bulleted list\" organization is a good idea in general. A good\n> scheduling decision can seldom be made in isolation without taking\n> global picture into account.\n>\n>> +static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n>> +{\n>> + int i = 0;\n>> + pid_t pid;\n>> + int wait_status, code;\n>> + int n = pp->max_processes;\n>> +\n>> + while (pp->nr_processes > 0) {\n>> + pid = waitpid(-1, &wait_status, WNOHANG);\n>> + if (pid == 0)\n>> + return;\n>> +\n>> + if (pid < 0)\n>> + die_errno(\"wait\");\n>> +\n>> + for (i = 0; i < pp->max_processes; i++)\n>> + if (pp->children[i].in_use &&\n>> + pid == pp->children[i].process.pid)\n>> + break;\n>> + if (i == pp->max_processes)\n>> + /*\n>> + * waitpid returned another process id\n>> + * which we are not waiting for.\n>> + */\n>> + return;\n>\n> If we culled a child process that this machinery is not in charge\n> of, waitpid() in other places that wants to see that child will not\n> see it. Perhaps such a situation might even warrant an error() or\n> BUG()? Do we want a \"NEEDSWORK: Is this a bug?\" comment here at\n> least?\n>\n>> + if (strbuf_read_once(&pp->children[i].err,\n>> + pp->children[i].process.err, 0) < 0 &&\n>> + errno != EAGAIN)\n>> + die_errno(\"strbuf_read_once\");\n>\n> Don't we want to read thru to the end here? The reason read_once()\n> did not read thru to the end may not have anything to do with\n> NONBLOCK (e.g. xread_nonblock() caps len, and it does not loop).\n\nright.\n\n>\n"},{"id":"270505","messageId":"5601A086.5060104@web.de","threadId":"40398","inReplyTo":"CA+P7+xozmsT4mZsBdvSQkGx4Wk-bOE1h-vAbRG_LAL5A-MAwTw@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-09-22T18:40:06Z","receivedAt":"2015-09-22T18:40:06Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 22.09.15 08:23, Jacob Keller wrote:\n> On Mon, Sep 21, 2015 at 9:55 PM, Torsten Bögershausen wrote:\n>> But in any case I suggest to xread() as it is, and not to change the\n>> functionality\n>> behind the back of the users.\n>>\n>>\n> \n> I don't think this patch actually changes behavior as it stands now. I\n> think Junio's suggestion does. Personally, I'd prefer some sort of\n> warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather\n> see it somehow warn so that we can find the bug (since we really\n> really shouldn't be calling xread with a blocking socket, especially\n> if we have xread_noblock or similar as in this series.\n> \n> Not sure if we really want to handle that, but I know we don't want to\n> change external behavior of xread... I think that polling is better\n> than the current \"spinning\" behavior.\n> \n> Regards,\n> Jake\nOh sorry for my comment, I mis-read the whole thing completely.\n\nAnd yes, a warning would be better than a poll()\n"},{"id":"270507","messageId":"CAGZ79kbWVgWg54rTVSV3MeLHzqC2maJKz14y_tPTdkU5SWtF=w@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqeghq48nl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T18:41:33Z","receivedAt":"2015-09-22T18:41:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 11:21 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano wrote:\n>>> Eric Sunshine writes:\n>>>\n>>>>> while (1) {\n>>>>> nr = read(fd, buf, len);\n>>>>> - if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>>>>> - continue;\n>>>>> + if (nr < 0) {\n>>>>> + if (errno == EINTR)\n>>>>> + continue;\n>>>>> + if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>>>>> + struct pollfd pfd;\n>>>>> + int i;\n>>>>> + pfd.events = POLLIN;\n>>>>> + pfd.fd = fd;\n>>>>> + i = poll(&pfd, 1, 100);\n>>>>\n>>>> Why is this poll() using a timeout? Isn't that still a busy wait of\n>>>> sorts (even if less aggressive)?\n>>\n>> True. Maybe we could have just a warning for now?\n>>\n>> if (errno == EAGAIN) {\n>> warning(\"Using xread with a non blocking fd\");\n>> continue; /* preserve previous behavior */\n>> }\n>\n> It is very likely that you will hit the same EAGAIN immediately by\n> continuing and end up spewing tons of the same warning, no?\n\nRight.\n\n>\n> We may want to slow down and think before sending a knee-jerk\n> response (this applies to both of us).\n\nEarlier this year I was doing lots of work in Gerrit, which somehow\nprevented crossing emails even though I was knee-jerking all the time.\nMaybe I picked up habits in Gerrit land which are only positive in Gerrit\nland, while rather harming in a mailing list base workflow such as here\nin Git land.\n\n>\n> I actually think a blocking poll(2) here would not be such a bad\n> idea. It would preserves the previous behaviour for callers who\n> unknowingly inherited a O_NONBLOCK file descriptor from its\n> environment without forcing them to waste CPU cycles.\n\nSo rather a combination of both, with the warning only spewing every\n5 seconds or such?\n\nI mean we identified this as a potential bug which we want to fix eventually\nby having the caller make sure they only pass in blocking fds.\n\nI feel like this is similar to the && call chain detection Jeff made a few\nmonths back. At first there were bugreports coming in left and right about\nbroken test scripts, now it's all fixed hopefully. The difference is\nthis is not in\nthe test suite though, so maybe we can rollout this patch early next cycle\nand get all the bugs fixed before we get serious with a release again?\n\nIt's risky though. Maybe there is a legit use case for \"I have a non blocking fd\nfor $REASONS and I know it, but I still want to read it now until it's done\"\nActually I thought about doing that exactly myself as part of the cleanup in the\nasynchronous paralllel processing. We'd collect progress information using\nthis strbuf_read_once and polling. Once a process is done, we would need to get\nall its output (i.e. up to EOF), so then we could just call\nstrbuf_read as we know the\nchild has already terminated, so all we want is getting the last bytes\nout of the pipe.\n"},{"id":"270516","messageId":"xmqqy4fy2q6o.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CA+P7+xozmsT4mZsBdvSQkGx4Wk-bOE1h-vAbRG_LAL5A-MAwTw@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T19:45:51Z","receivedAt":"2015-09-22T19:45:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> I don't think this patch actually changes behavior as it stands now. I\n> think Junio's suggestion does. Personally, I'd prefer some sort of\n> warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather\n> see it somehow warn so that we can find the bug (since we really\n> really shouldn't be calling xread with a blocking socket, especially\n> if we have xread_noblock or similar as in this series.\n\nOne caveat is that the caller may not know in the first place.\n\nThe last time I checked the existing callers of xread(), there were\na few that read from a file descriptor they did not open themselves\n(e.g. unpack-objects that read from standard input). The invoker of\nthese processes is free to do O_NONBLOCK their input stream for\nwhatever reason.\n"},{"id":"270517","messageId":"20150922194927.GA622@sigill.intra.peff.net","threadId":"40398","inReplyTo":"xmqqy4fy2q6o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-22T19:49:28Z","receivedAt":"2015-09-22T19:49:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:\n\n> Jacob Keller writes:\n> \n> > I don't think this patch actually changes behavior as it stands now. I\n> > think Junio's suggestion does. Personally, I'd prefer some sort of\n> > warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather\n> > see it somehow warn so that we can find the bug (since we really\n> > really shouldn't be calling xread with a blocking socket, especially\n> > if we have xread_noblock or similar as in this series.\n> \n> One caveat is that the caller may not know in the first place.\n> \n> The last time I checked the existing callers of xread(), there were\n> a few that read from a file descriptor they did not open themselves\n> (e.g. unpack-objects that read from standard input). The invoker of\n> these processes is free to do O_NONBLOCK their input stream for\n> whatever reason.\n\nYeah. I do not think this is a bug at all; the user might have their\nreasons for handing off an O_NONBLOCK pipe. If we take xread() to mean\n\"try to read from fd until we get a real error, some data, or an EOF\",\nthen it is perfectly reasonable to replace spinning on read() (which we\ndo now) with a poll() for efficiency. The caller (and the user) does not\nhave to care, and should not notice; the outcome will be the same.\n\n-Peff\n"},{"id":"270518","messageId":"xmqqtwqm2puk.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAGZ79kbUkUSAP+muhYxTwHZdD+ojJYXjogZfRXs0PemEdcqfbA@mail.gmail.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T19:53:07Z","receivedAt":"2015-09-22T19:53:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So how would you find out when we are done?\n\n\twhile (1) {\n\t\tif (we have available slot)\n\t\t\task to start a new one;\n\t\tif (nobody is running anymore)\n \tbreak;\n\t\tcollect the output from running ones;\n drain the output from the foreground one;\n\t\tcull the finished process;\n }\n"},{"id":"270521","messageId":"xmqqlhby2pj0.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"20150922194927.GA622@sigill.intra.peff.net","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T20:00:03Z","receivedAt":"2015-09-22T20:00:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:\n>\n>> One caveat is that the caller may not know in the first place.\n>> \n>> The last time I checked the existing callers of xread(), there were\n>> a few that read from a file descriptor they did not open themselves\n>> (e.g. unpack-objects that read from standard input). The invoker of\n>> these processes is free to do O_NONBLOCK their input stream for\n>> whatever reason.\n>\n> Yeah. I do not think this is a bug at all; the user might have their\n> reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean\n> \"try to read from fd until we get a real error, some data, or an EOF\",\n> then it is perfectly reasonable to replace spinning on read() (which we\n> do now) with a poll() for efficiency. The caller (and the user) does not\n> have to care, and should not notice; the outcome will be the same.\n\nI think we are in agreement, and that answers the question/guidance\nStefan asked earlier in $gmane/278414, which was:\n\n> So rather a combination of both, with the warning only spewing every\n> 5 seconds or such?\n\nand the answer obviously is \"No warning, do a poll() without timeout\nto block\".\n"},{"id":"270532","messageId":"CAGZ79kYwMrpU-gW6FsbH1W4TRPisZH9GJoyZ6hoimXdxz4HCYQ@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqtwqm2puk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T21:31:04Z","receivedAt":"2015-09-22T21:31:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So how would you find out when we are done?\n>\n> while (1) {\n> if (we have available slot)\n> ask to start a new one;\n> if (nobody is running anymore)\n> break;\n> collect the output from running ones;\n> drain the output from the foreground one;\n> cull the finished process;\n> }\n>\n\nPersonally I do not like the break; in the middle of\nthe loop, but that's personal preference, I'd guess.\nI'll also move the condition for (we have available slot)\nback inside the called function.\n\nSo I am thinking about using this in the reroll instead:\n\n run_processes_parallel_start_as_needed(&pp);\n while (pp.nr_processes > 0) {\n run_processes_parallel_buffer_stderr(&pp);\n run_processes_parallel_output(&pp);\n run_processes_parallel_collect_finished(&pp);\n run_processes_parallel_start_as_needed(&pp);\n }\n\n\nThis eliminates the need for the flag and also exits the loop just\nafter the possible startup of new processes.\n"},{"id":"270534","messageId":"xmqqh9mm1690.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAGZ79kYwMrpU-gW6FsbH1W4TRPisZH9GJoyZ6hoimXdxz4HCYQ@mail.gmail.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T21:41:47Z","receivedAt":"2015-09-22T21:41:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> So how would you find out when we are done?\n>>\n>> while (1) {\n>> if (we have available slot)\n>> ask to start a new one;\n>> if (nobody is running anymore)\n>> break;\n>> collect the output from running ones;\n>> drain the output from the foreground one;\n>> cull the finished process;\n>> }\n>>\n>\n> Personally I do not like the break; in the middle of\n> the loop, but that's personal preference, I'd guess.\n> I'll also move the condition for (we have available slot)\n> back inside the called function.\n>\n> So I am thinking about using this in the reroll instead:\n>\n> run_processes_parallel_start_as_needed(&pp);\n> while (pp.nr_processes > 0) {\n> run_processes_parallel_buffer_stderr(&pp);\n> run_processes_parallel_output(&pp);\n> run_processes_parallel_collect_finished(&pp);\n> run_processes_parallel_start_as_needed(&pp);\n> }\n\nIf you truly think the latter is easier to follow its logic (with\nthe duplicated call to the same function only to avoid break that\nmakes it clear why we are quitting the loop, and without the\nexplicit \"start only if we can afford to\"), then I have to say that\nour design tastes are fundamentally incompatible. I have nothing\nmore to add.\n"},{"id":"270536","messageId":"CAGZ79kZFoTUugFdOuikOBUbg0DG+TJ3tNTuZaCs7WSaa2r7=Bg@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqh9mm1690.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-22T21:54:16Z","receivedAt":"2015-09-22T21:54:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>>\n>>>> So how would you find out when we are done?\n>>>\n>>> while (1) {\n>>> if (we have available slot)\n>>> ask to start a new one;\n>>> if (nobody is running anymore)\n>>> break;\n>>> collect the output from running ones;\n>>> drain the output from the foreground one;\n>>> cull the finished process;\n>>> }\n>>>\n>>\n>> Personally I do not like the break; in the middle of\n>> the loop, but that's personal preference, I'd guess.\n>> I'll also move the condition for (we have available slot)\n>> back inside the called function.\n>>\n>> So I am thinking about using this in the reroll instead:\n>>\n>> run_processes_parallel_start_as_needed(&pp);\n>> while (pp.nr_processes > 0) {\n>> run_processes_parallel_buffer_stderr(&pp);\n>> run_processes_parallel_output(&pp);\n>> run_processes_parallel_collect_finished(&pp);\n>> run_processes_parallel_start_as_needed(&pp);\n>> }\n>\n> If you truly think the latter is easier to follow its logic (with\n> the duplicated call to the same function only to avoid break that\n> makes it clear why we are quitting the loop,\n\nI dislike having the call twice, too.\n\n> and without the\n> explicit \"start only if we can afford to\"), then I have to say that\n> our design tastes are fundamentally incompatible.\n\nWell the \"start only if we can afford to\" is not as easy as just\n\n> if (we have available slot)\n> ask to start a new one;\n\nbecause that's only half the condition. If we don't start a new one\nas the callback get_next_task returned without a new task.\nSo it becomes\n\n while (pp.nr_processes > 0) {\n while (pp->nr_processes < pp->max_processes &&\n start_one_process(&pp))\n ; /* nothing */\n if (!pp.nr_processes)\n break;\n buffer(..);\n output_live_child(..);\n cull_finished(..);\n }\n\nI'll think about that.\n"},{"id":"270539","messageId":"xmqq4mim14c7.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAGZ79kZFoTUugFdOuikOBUbg0DG+TJ3tNTuZaCs7WSaa2r7=Bg@mail.gmail.com","subject":"Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-22T22:23:04Z","receivedAt":"2015-09-22T22:23:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano wrote:\n>>>> Stefan Beller writes:\n>>>>\n>>>>> So how would you find out when we are done?\n>>>>\n>>>> while (1) {\n>>>> if (we have available slot)\n>>>> ask to start a new one;\n>>>> if (nobody is running anymore)\n>>>> break;\n>>>> collect the output from running ones;\n>>>> drain the output from the foreground one;\n>>>> cull the finished process;\n>>>> }\n>>>>\n>>>\n>>> Personally I do not like the break; in the middle of\n>>> the loop, but that's personal preference, I'd guess.\n>>> I'll also move the condition for (we have available slot)\n>>> back inside the called function.\n>>>\n>>> So I am thinking about using this in the reroll instead:\n>>>\n>>> run_processes_parallel_start_as_needed(&pp);\n>>> while (pp.nr_processes > 0) {\n>>> run_processes_parallel_buffer_stderr(&pp);\n>>> run_processes_parallel_output(&pp);\n>>> run_processes_parallel_collect_finished(&pp);\n>>> run_processes_parallel_start_as_needed(&pp);\n>>> }\n>>\n>> If you truly think the latter is easier to follow its logic (with\n>> the duplicated call to the same function only to avoid break that\n>> makes it clear why we are quitting the loop,\n>\n> I dislike having the call twice, too.\n> ...\n>> and without the\n>> explicit \"start only if we can afford to\"), then I have to say that\n>> our design tastes are fundamentally incompatible.\n> ...\n> I'll think about that.\n\nDon't waste too much time on it ;-) This is largely a taste thing,\nand taste is very hard to reason about, understand, teach and learn.\n\nHaving said that, if I can pick one thing that I foresee to be\nproblematic the most (aside from these overlong names of the\nfunctions that are private and do not need such long names), it is\nthat \"start as many without giving any control to the caller\"\ninterface. I wrote \"start *a* new one\" in the outline above for a\nreason.\n\nEven if you want to utilize a moderate number of processes, say 16,\nworking at the steady state, I suspect that we would find it\nsuboptimal from perceived latency point of view, if we spin up 16\nprocesses all at once at the very beginning before we start to\ncollect output from the designated foreground process and relay its\nfirst line of output. We may want to be able to tweak the caller to\nspin up just a few first and let the loop ramp up to the full blast\ngradually so that we can give an early feedback to the end user.\nThat is not something easy to arrange with \"start as many without\ngiving control to the caller\" interface. We probably will discover\nother kinds of scheduling issues once we get familiar with this\nmachinery and would find need for finer grained control.\n\nAnd I consider such a ramp-up logic shouldn't be hidden inside the\n\"start_as_needed()\" helper function---it is the way how the calling\nloop wants its processes started, and I'd prefer to have the logic\nvisible in the caller's loop.\n\nBut that is also largely a \"taste\" thing.\n"},{"id":"270542","messageId":"CAGZ79kYKxNAXhbmXEtMeBYeGqu8i4bcUW_hpjLaPSF4p5O=uBg@mail.gmail.com","threadId":"40398","inReplyTo":"xmqqlhby2pj0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T00:14:42Z","receivedAt":"2015-09-23T00:14:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 1:00 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:\n>>\n>>> One caveat is that the caller may not know in the first place.\n>>>\n>>> The last time I checked the existing callers of xread(), there were\n>>> a few that read from a file descriptor they did not open themselves\n>>> (e.g. unpack-objects that read from standard input). The invoker of\n>>> these processes is free to do O_NONBLOCK their input stream for\n>>> whatever reason.\n>>\n>> Yeah. I do not think this is a bug at all; the user might have their\n>> reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean\n>> \"try to read from fd until we get a real error, some data, or an EOF\",\n>> then it is perfectly reasonable to replace spinning on read() (which we\n>> do now) with a poll() for efficiency. The caller (and the user) does not\n>> have to care, and should not notice; the outcome will be the same.\n>\n> I think we are in agreement, and that answers the question/guidance\n> Stefan asked earlier in $gmane/278414, which was:\n>\n>> So rather a combination of both, with the warning only spewing every\n>> 5 seconds or such?\n>\n> and the answer obviously is \"No warning, do a poll() without timeout\n> to block\".\n\nOk. Expect that in a reroll.\n\nTo answer the first question upthread:\n\n> I can sort of see EINTR but why is ENOMEM any special than other errors?\n\nSo we have 4 kinds of additional errors, when adding the poll. When we want to\nkeep the behavior as close to the original as possible, we should not error out\nfor things we had under control previously.\n\n EFAULT The array given as argument was not contained in the\ncalling program's address space.\n\n EINTR A signal occurred before any requested event; see signal(7).\n\n EINVAL The nfds value exceeds the RLIMIT_NOFILE value.\n\n ENOMEM There was no space to allocate file descriptor tables.\n\nI don't see a way EFAULT can be returned as we have the array hard\ncoded on the stack.\n(No tricks with the heap or anything, we'll have it on the stack)\n\nEINTR is possible while waiting. Actually it doesn't matter if we\nretry the poll or read first\nand then poll again, so we can easily just continue and do the read.\n\nEINVAL is highly unlikely (as nfds == 1 in the case here), but can happen.\nENOMEM is similar to EINVAL.\n\nWe should not care if the call to poll failed, as we're in an infinite loop and\ncan only get out with the correct read(..). So maybe an implementation like this\nwould already suffice:\n\nssize_t xread(int fd, void *buf, size_t len)\n{\n ssize_t nr;\n if (len > MAX_IO_SIZE)\n len = MAX_IO_SIZE;\n while (1) {\n nr = read(fd, buf, len);\n if (nr < 0) {\n if (errno == EINTR)\n continue;\n if (errno == EAGAIN || errno == EWOULDBLOCK) {\n struct pollfd pfd;\n pfd.events = POLLIN;\n pfd.fd = fd;\n /* We deliberately ignore the return value of poll. */\n poll(&pfd, 1, -1);\n continue;\n }\n }\n return nr;\n }\n}\n\nIn the resend I'll only check for EINTR and in other cases of early poll\nfailure, we just die(..).\n"},{"id":"270544","messageId":"xmqqzj0eynh3.fsf@gitster.mtv.corp.google.com","threadId":"40398","inReplyTo":"CAGZ79kYKxNAXhbmXEtMeBYeGqu8i4bcUW_hpjLaPSF4p5O=uBg@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T00:43:20Z","receivedAt":"2015-09-23T00:43:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> We should not care if the call to poll failed, as we're in an infinite loop and\n> can only get out with the correct read(..).\n\nI think I agree with that reasoning. The only thing we want out of\nthis call to poll(2) is to delay calling read(2) again when we know\nwe would get EAGAIN again; whether the reason why poll(2) returned\nis because of some error or because some data has become ready on\nthe file descriptor, we no longer are in that \"we know we would get\nEAGAIN\" situation and do want to call read(2), which would return\neither the right error or perform a successful read to make some\nprogress.\n\nSounds sensible.\n"},{"id":"270560","messageId":"20150923015127.GC21294@sigill.intra.peff.net","threadId":"40398","inReplyTo":"CAGZ79kYKxNAXhbmXEtMeBYeGqu8i4bcUW_hpjLaPSF4p5O=uBg@mail.gmail.com","subject":"Re: [PATCHv3 02/13] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-23T01:51:27Z","receivedAt":"2015-09-23T01:51:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 22, 2015 at 05:14:42PM -0700, Stefan Beller wrote:\n\n> We should not care if the call to poll failed, as we're in an infinite loop and\n> can only get out with the correct read(..). So maybe an implementation like this\n> would already suffice:\n> \n> ssize_t xread(int fd, void *buf, size_t len)\n> {\n> ssize_t nr;\n> if (len > MAX_IO_SIZE)\n> len = MAX_IO_SIZE;\n> while (1) {\n> nr = read(fd, buf, len);\n> if (nr < 0) {\n> if (errno == EINTR)\n> continue;\n> if (errno == EAGAIN || errno == EWOULDBLOCK) {\n> struct pollfd pfd;\n> pfd.events = POLLIN;\n> pfd.fd = fd;\n> /* We deliberately ignore the return value of poll. */\n> poll(&pfd, 1, -1);\n> continue;\n> }\n> }\n> return nr;\n> }\n> }\n\nFWIW, that is what I had imagined.\n\n-Peff\n"}]}