{"thread":{"id":"40721","subject":"[PATCHv3 03/11] run-command: omit setting file descriptors to non blocking in Windows","startedAt":"2015-11-04T00:37:03Z","lastAt":"2015-11-13T21:29:23Z","messageCount":34,"participants":["Stefan Beller","Junio C Hamano","Johannes Sixt","Jeff King","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"272867","messageId":"1446597434-1740-1-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":null,"subject":"[PATCHv3 00/11] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:03Z","receivedAt":"2015-11-04T00:37:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Where does it apply?\n---\nThis series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge\nbranch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces\nthe previous patches in sb/submodule-parallel-update\n\nWhat does it do?\n---\nThis series should finish the on going efforts of parallelizing\nsubmodule network traffic. The patches contain tests for clone,\nfetch and submodule update to use the actual parallelism both via\ncommand line as well as a configured option. I decided to go with\n\"submodule.jobs\" for all three for now.\n\nWhat's new in v3?\n---\n\n * 3 new patches (make it compile in Windows, better warnings in posix environment\n for setting fds to non blocking, drop check against NULL)\n * adressed reviews by Eric for readability. :) \n * addressed Junios comments for the new clone helper function\n\nStefan Beller (11):\n run_processes_parallel: delimit intermixed task output\n run-command: report failure for degraded output just once\n run-command: omit setting file descriptors to non blocking in Windows\n submodule-config: keep update strategy around\n submodule-config: drop check against NULL\n submodule-config: remove name_and_item_from_var\n submodule-config: introduce parse_generic_submodule_config\n fetching submodules: respect `submodule.jobs` config option\n git submodule update: have a dedicated helper for cloning\n submodule update: expose parallelism to the user\n clone: allow an explicit argument for parallel submodule clones\n\n Documentation/config.txt | 7 ++\n Documentation/git-clone.txt | 6 +-\n Documentation/git-submodule.txt | 7 +-\n builtin/clone.c | 19 +++-\n builtin/fetch.c | 2 +-\n builtin/submodule--helper.c | 239 ++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 ++++-----\n run-command.c | 26 ++++-\n submodule-config.c | 109 +++++++++++-------\n submodule-config.h | 3 +\n submodule.c | 5 +\n t/t5526-fetch-submodules.sh | 14 +++\n t/t7400-submodule-basic.sh | 4 +-\n t/t7406-submodule-update.sh | 27 +++++\n 14 files changed, 433 insertions(+), 89 deletions(-)\n\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272870","messageId":"1446597434-1740-2-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 01/11] run_processes_parallel: delimit intermixed task output","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:04Z","receivedAt":"2015-11-04T00:37:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This commit serves 2 purposes. First this may help the user who\ntries to diagnose intermixed process calls. Second this may be used\nin a later patch for testing. As the output of a command should not\nchange visibly except for going faster, grepping for the trace output\nseems like a viable testing strategy.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0a3c24e..7c00c21 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,\n \t\tn = online_cpus();\n \n \tpp->max_processes = n;\n+\n+\ttrace_printf(\"run_processes_parallel: preparing to run up to %d tasks\", n);\n+\n \tpp->data = data;\n \tif (!get_next_task)\n \t\tdie(\"BUG: you need to specify a get_next_task function\");\n@@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n \n+\ttrace_printf(\"run_processes_parallel: done\");\n \tfor (i = 0; i < pp->max_processes; i++) {\n \t\tstrbuf_release(&pp->children[i].err);\n \t\tchild_process_clear(&pp->children[i].process);\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272873","messageId":"1446597434-1740-3-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:05Z","receivedAt":"2015-11-04T00:37:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The warning message is cluttering the output itself,\nso just report it once.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 7c00c21..3ae563f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)\n \n static void set_nonblocking(int fd)\n {\n+\tstatic int reported_degrade = 0;\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+\tif (flags < 0) {\n+\t\tif (!reported_degrade) {\n+\t\t\twarning(\"Could not get file status flags, \"\n+\t\t\t\t\"output will be degraded\");\n+\t\t\treported_degrade = 1;\n+\t\t}\n+\t} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {\n+\t\tif (!reported_degrade) {\n+\t\t\twarning(\"Could not set file status flags, \"\n+\t\t\t\t\"output will be degraded\");\n+\t\t\treported_degrade = 1;\n+\t\t}\n+\t}\n }\n \n /* returns\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272865","messageId":"1446597434-1740-4-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 03/11] run-command: omit setting file descriptors to non blocking in Windows","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:06Z","receivedAt":"2015-11-04T00:37:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In Windows there is no fcntl apparently and as it only affects output\nslightly we can just run with degraded output in Windows.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3ae563f..8db7df8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1012,6 +1012,7 @@ static void pp_cleanup(struct parallel_processes *pp)\n \n static void set_nonblocking(int fd)\n {\n+#ifndef GIT_WINDOWS_NATIVE\n \tstatic int reported_degrade = 0;\n \tint flags = fcntl(fd, F_GETFL);\n \tif (flags < 0) {\n@@ -1027,6 +1028,7 @@ static void set_nonblocking(int fd)\n \t\t\treported_degrade = 1;\n \t\t}\n \t}\n+#endif\n }\n \n /* returns\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272875","messageId":"1446597434-1740-5-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 04/11] submodule-config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:07Z","receivedAt":"2015-11-04T00:37:07Z","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 \nSigned-off-by: Junio C Hamano \n---\n submodule-config.c | 11 +++++++++++\n submodule-config.h | 1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex afe0ea8..4239b0e 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@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!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\telse {\n+\t\t\tfree((void *) submodule->update);\n+\t\t\tsubmodule->update = xstrdup(value);\n+\t\t}\n \t}\n \n \tstrbuf_release(&name);\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.6.1.247.ge8f2a41.dirty\n"},{"id":"272876","messageId":"1446597434-1740-6-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 05/11] submodule-config: drop check against NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:08Z","receivedAt":"2015-11-04T00:37:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Adhere to the common coding style of Git and not check explicitly\nfor NULL throughout the file. There are still other occurrences in the\ncode base but that is usually inside of conditions with side effects.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4239b0e..6d01941 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -265,7 +265,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!strcmp(item.buf, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->path != NULL)\n+\t\telse if (!me->overwrite && submodule->path)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n \t\telse {\n@@ -289,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->ignore != NULL)\n+\t\telse if (!me->overwrite && submodule->ignore)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n \t\telse if (strcmp(value, \"untracked\") &&\n@@ -305,7 +305,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t} else if (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n \t\t} else {\n@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t} else if (!strcmp(item.buf, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\telse if (!me->overwrite && submodule->update)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t \"update\");\n \t\telse {\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272868","messageId":"1446597434-1740-7-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 06/11] submodule-config: remove name_and_item_from_var","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:09Z","receivedAt":"2015-11-04T00:37:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`name_and_item_from_var` does not provide the proper abstraction\nwe need here in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 48 ++++++++++++++++--------------------------------\n 1 file changed, 16 insertions(+), 32 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 6d01941..b826841 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -161,31 +161,17 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,\n \treturn NULL;\n }\n \n-static int name_and_item_from_var(const char *var, struct strbuf *name,\n-\t\t\t\t struct strbuf *item)\n-{\n-\tconst char *subsection, *key;\n-\tint subsection_len, parse;\n-\tparse = parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t&subsection_len, &key);\n-\tif (parse < 0 || !subsection)\n-\t\treturn 0;\n-\n-\tstrbuf_add(name, subsection, subsection_len);\n-\tstrbuf_addstr(item, key);\n-\n-\treturn 1;\n-}\n-\n static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n-\t\tconst unsigned char *gitmodules_sha1, const char *name)\n+\t\t\t\t\t\t const unsigned char *gitmodules_sha1,\n+\t\t\t\t\t\t const char *name_ptr, int name_len)\n {\n \tstruct submodule *submodule;\n \tstruct strbuf name_buf = STRBUF_INIT;\n+\tchar *name = xmemdupz(name_ptr, name_len);\n \n \tsubmodule = cache_lookup_name(cache, gitmodules_sha1, name);\n \tif (submodule)\n-\t\treturn submodule;\n+\t\tgoto out;\n \n \tsubmodule = xmalloc(sizeof(*submodule));\n \n@@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \thashcpy(submodule->gitmodules_sha1, gitmodules_sha1);\n \n \tcache_add(cache, submodule);\n-\n+out:\n+\tfree(name);\n \treturn submodule;\n }\n \n@@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)\n {\n \tstruct parse_config_parameter *me = data;\n \tstruct submodule *submodule;\n-\tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n-\tint ret = 0;\n+\tint subsection_len, ret = 0;\n+\tconst char *subsection, *key;\n \n-\t/* this also ensures that we only parse submodule entries */\n-\tif (!name_and_item_from_var(var, &name, &item))\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0 || !subsection_len)\n \t\treturn 0;\n \n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t me->gitmodules_sha1,\n-\t\t\t\t\t name.buf);\n+\t\t\t\t\t subsection, subsection_len);\n \n-\tif (!strcmp(item.buf, \"path\")) {\n+\tif (!strcmp(key, \"path\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path)\n@@ -275,7 +262,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->path = xstrdup(value);\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n@@ -286,7 +273,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n \t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n-\t} else if (!strcmp(item.buf, \"ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore)\n@@ -302,7 +289,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->ignore);\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"url\")) {\n+\t} else if (!strcmp(key, \"url\")) {\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url) {\n@@ -312,7 +299,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tfree((void *) submodule->url);\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\t} else if (!strcmp(item.buf, \"update\")) {\n+\t} else if (!strcmp(key, \"update\")) {\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->update)\n@@ -324,9 +311,6 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t}\n \t}\n \n-\tstrbuf_release(&name);\n-\tstrbuf_release(&item);\n-\n \treturn ret;\n }\n \n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272866","messageId":"1446597434-1740-8-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 07/11] submodule-config: introduce parse_generic_submodule_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:10Z","receivedAt":"2015-11-04T00:37:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This rewrites parse_config to distinguish between configs specific to\none submodule and configs which apply generically to all submodules.\nWe do not have generic submodule configs yet, but the next patch will\nintroduce \"submodule.jobs\".\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 41 ++++++++++++++++++++++++++++++++---------\n 1 file changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex b826841..29e21b2 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -234,17 +234,22 @@ struct parse_config_parameter {\n \tint overwrite;\n };\n \n-static int parse_config(const char *var, const char *value, void *data)\n+static int parse_generic_submodule_config(const char *key,\n+\t\t\t\t\t const char *var,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t struct parse_config_parameter *me)\n {\n-\tstruct parse_config_parameter *me = data;\n-\tstruct submodule *submodule;\n-\tint subsection_len, ret = 0;\n-\tconst char *subsection, *key;\n-\n-\tif (parse_config_key(var, \"submodule\", &subsection,\n-\t\t\t &subsection_len, &key) < 0 || !subsection_len)\n-\t\treturn 0;\n+\treturn 0;\n+}\n \n+static int parse_specific_submodule_config(const char *subsection, int subsection_len,\n+\t\t\t\t\t const char *key,\n+\t\t\t\t\t const char *var,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t struct parse_config_parameter *me)\n+{\n+\tint ret = 0;\n+\tstruct submodule *submodule;\n \tsubmodule = lookup_or_create_by_name(me->cache,\n \t\t\t\t\t me->gitmodules_sha1,\n \t\t\t\t\t subsection, subsection_len);\n@@ -314,6 +319,24 @@ static int parse_config(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n+static int parse_config(const char *var, const char *value, void *data)\n+{\n+\tstruct parse_config_parameter *me = data;\n+\tint subsection_len;\n+\tconst char *subsection, *key;\n+\n+\tif (parse_config_key(var, \"submodule\", &subsection,\n+\t\t\t &subsection_len, &key) < 0)\n+\t\treturn 0;\n+\n+\tif (!subsection_len)\n+\t\treturn parse_generic_submodule_config(key, var, value, me);\n+\telse\n+\t\treturn parse_specific_submodule_config(subsection,\n+\t\t\t\t\t\t subsection_len, key,\n+\t\t\t\t\t\t var, value, me);\n+}\n+\n static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,\n \t\t\t\t unsigned char *gitmodules_sha1)\n {\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272869","messageId":"1446597434-1740-9-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:11Z","receivedAt":"2015-11-04T00:37:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to configure fetching and updating in parallel\nwithout having the command line option.\n\nThis moved the responsibility to determine how many parallel processes\nto start from builtin/fetch to submodule.c as we need a way to communicate\n\"The user did not specify the number of parallel processes in the command\nline options\" in the builtin fetch. The submodule code takes care of\nthe precedence (CLI > config > default)\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 7 +++++++\n builtin/fetch.c | 2 +-\n submodule-config.c | 15 +++++++++++++++\n submodule-config.h | 2 ++\n submodule.c | 5 +++++\n t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n 6 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..70e1b88 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2643,6 +2643,13 @@ submodule..ignore::\n \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n \taffected by this setting.\n \n+submodule.jobs::\n+\tThis is used to determine how many submodules can be operated on in\n+\tparallel. Specifying a positive integer allows up to that number\n+\tof submodules being fetched in parallel. This is used in fetch\n+\tand clone operations only. A value of 0 will give some reasonable\n+\tconfiguration. It defaults to 1.\n+\n tag.sort::\n \tThis variable controls the sort ordering of tags when displayed by\n \tlinkgit:git-tag[1]. Without the \"--sort=\" option provided, the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9cc1c9d..60e6797 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,7 +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 int max_children = -1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 29e21b2..475551a 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -32,6 +32,7 @@ enum lookup_type {\n \n static struct submodule_cache cache;\n static int is_cache_init;\n+static int parallel_jobs = -1;\n \n static int config_path_cmp(const struct submodule_entry *a,\n \t\t\t const struct submodule_entry *b,\n@@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,\n \t\t\t\t\t const char *value,\n \t\t\t\t\t struct parse_config_parameter *me)\n {\n+\tif (!strcmp(key, \"jobs\")) {\n+\t\tparallel_jobs = strtol(value, NULL, 10);\n+\t\tif (parallel_jobs < 0) {\n+\t\t\twarning(\"submodule.jobs not allowed to be negative.\");\n+\t\t\tparallel_jobs = 1;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n@@ -482,3 +492,8 @@ void submodule_free(void)\n \tcache_free(&cache);\n \tis_cache_init = 0;\n }\n+\n+int config_parallel_submodules(void)\n+{\n+\treturn parallel_jobs;\n+}\ndiff --git a/submodule-config.h b/submodule-config.h\nindex f9e2a29..d9bbf9a 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n \t\tconst char *path);\n void submodule_free(void);\n \n+int config_parallel_submodules(void);\n+\n #endif /* SUBMODULE_CONFIG_H */\ndiff --git a/submodule.c b/submodule.c\nindex 0257ea3..188ba02 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = config_parallel_submodules();\n+\tif (max_parallel_jobs < 0)\n+\t\tmax_parallel_jobs = 1;\n+\n \tcalculate_changed_submodule_paths();\n \trun_processes_parallel(max_parallel_jobs,\n \t\t\t get_next_submodule,\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 1b4ce69..5c3579c 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -470,4 +470,18 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success 'fetching submodules respects parallel settings' '\n+\tgit config fetch.recurseSubmodules true &&\n+\t(\n+\t\tcd downstream &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n+\t\tgrep \"7 children\" trace.out &&\n+\t\tgit config submodule.jobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n+\t\tgrep \"8 children\" trace.out &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n+\t\tgrep \"9 children\" trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272874","messageId":"1446597434-1740-10-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 09/11] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:12Z","receivedAt":"2015-11-04T00:37:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces a new helper function in git submodule--helper\nwhich takes care of cloning all submodules, which we want to\nparallelize eventually.\n\nSome tests (such as empty URL, update_mode=none) are required in the\nhelper to make the decision for cloning. These checks have been\nmoved into the C function as well (no need to repeat them in the\nshell script).\n\nAs we can only access the stderr channel from within the parallel\nprocessing engine, we need to reroute the error message for\nspecified but initialized submodules to stderr. As it is an error\nmessage, this should have gone to stderr in the first place, so it\nis a bug fix along the way.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 229 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 45 +++------\n t/t7400-submodule-basic.sh | 4 +-\n 3 files changed, 242 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..95b45a2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_submodule_config(const char *var, const char *value, void *cb)\n+{\n+\treturn parse_submodule_config_option(var, value);\n+}\n+\n+struct submodule_update_clone {\n+\t/* states */\n+\tint count;\n+\tint print_unmatched;\n+\t/* configuration */\n+\tint quiet;\n+\tconst char *reference;\n+\tconst char *depth;\n+\tconst char *update;\n+\tconst char *recursive_prefix;\n+\tconst char *prefix;\n+\tstruct module_list list;\n+\tstruct string_list projectlines;\n+\tstruct pathspec pathspec;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}\n+\n+static void fill_clone_command(struct child_process *cp, int quiet,\n+\t\t\t const char *prefix, const char *path,\n+\t\t\t const char *name, const char *url,\n+\t\t\t const char *reference, const char *depth)\n+{\n+\tcp->git_cmd = 1;\n+\tcp->no_stdin = 1;\n+\tcp->stdout_to_stderr = 1;\n+\tcp->err = -1;\n+\targv_array_push(&cp->args, \"submodule--helper\");\n+\targv_array_push(&cp->args, \"clone\");\n+\tif (quiet)\n+\t\targv_array_push(&cp->args, \"--quiet\");\n+\n+\tif (prefix)\n+\t\targv_array_pushl(&cp->args, \"--prefix\", prefix, NULL);\n+\n+\targv_array_pushl(&cp->args, \"--path\", path, NULL);\n+\targv_array_pushl(&cp->args, \"--name\", name, NULL);\n+\targv_array_pushl(&cp->args, \"--url\", url, NULL);\n+\tif (reference)\n+\t\targv_array_push(&cp->args, reference);\n+\tif (depth)\n+\t\targv_array_push(&cp->args, depth);\n+}\n+\n+static int update_clone_get_next_task(void **pp_task_cb,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tfor (; pp->count < pp->list.nr; pp->count++) {\n+\t\tconst struct submodule *sub = NULL;\n+\t\tconst char *displaypath = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *update_module = NULL;\n+\t\tchar *url = NULL;\n+\t\tint needs_cloning = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t\tpp->recursive_prefix, ce->name);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\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\tstrbuf_addf(err, \"BUG: internal error managing submodules. \"\n+\t\t\t\t \"The cache could not locate '%s'\", ce->name);\n+\t\t\tpp->print_unmatched = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif (pp->update)\n+\t\t\tupdate_module = pp->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = sub->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = \"checkout\";\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We must not fall back to .gitmodules as we only want to process\n+\t\t * configured submodules.\n+\t\t */\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\t\tgit_config_get_string(sb.buf, &url);\n+\t\tif (!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 (pp->pathspec.nr)\n+\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tneeds_cloning = !file_exists(sb.buf);\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\t\tneeds_cloning, ce->name);\n+\t\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\t\tif (needs_cloning) {\n+\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix, ce->name,\n+\t\t\t\t\t sub->name, url, pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t\tfree(url);\n+\t\t\treturn 1;\n+\t\t} else\n+\t\t\tfree(url);\n+\t}\n+\treturn 0;\n+}\n+\n+static int update_clone_start_failure(struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tstrbuf_addf(err, \"error when starting a child process\");\n+\tpp->print_unmatched = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone_task_finished(int result,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tif (!result) {\n+\t\treturn 0;\n+\t} else {\n+\t\tstrbuf_addf(err, \"error in one child process\");\n+\t\tpp->print_unmatched = 1;\n+\t\treturn 1;\n+\t}\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\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_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive_prefix\", &pp.recursive_prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &pp.update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"update command for submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &pp.reference, \"\",\n+\t\t\t N_(\"Use the local reference repository \"\n+\t\t\t \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n+\t\t\t N_(\"Create a shallow clone truncated to the \"\n+\t\t\t \"specified number of revisions\")),\n+\t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\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+\tpp.prefix = prefix;\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tgitmodules_config();\n+\t/* Overlay the parsed .gitmodules file with .git/config */\n+\tgit_config(git_submodule_config, NULL);\n+\trun_processes_parallel(1, update_clone_get_next_task,\n+\t\t\t\t update_clone_start_failure,\n+\t\t\t\t update_clone_task_finished,\n+\t\t\t\t &pp);\n+\n+\tif (pp.print_unmatched) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor_each_string_list_item(item, &pp.projectlines) {\n+\t\tutf8_fprintf(stdout, \"%s\", item->string);\n+\t}\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +492,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update-clone\", update_clone}\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 9bc5c5f..9f554fb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -664,17 +664,18 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t${prefix:+--recursive_prefix \"$prefix\"} \\\n+\t\t${update:+--update \"$update\"} \\\n+\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${depth:+--depth \"$depth\"} \\\n+\t\t\"$@\" | {\n \terr=\n-\twhile read mode sha1 stage sm_path\n+\twhile read mode sha1 stage just_cloned 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+\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@@ -691,27 +692,10 @@ cmd_update()\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\tif test $just_cloned -eq 1\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --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\tupdate_module=checkout\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -751,13 +735,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)\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..5991e3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -462,7 +462,7 @@ test_expect_success 'update --init' '\n \tgit config --remove-section submodule.example &&\n \ttest_must_fail git config submodule.example.url &&\n \n-\tgit submodule update init > update.out &&\n+\tgit submodule update init 2> update.out &&\n \tcat update.out &&\n \ttest_i18ngrep \"not initialized\" update.out &&\n \ttest_must_fail git rev-parse --resolve-git-dir init/.git &&\n@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '\n \tmkdir -p sub &&\n \t(\n \t\tcd sub &&\n-\t\tgit submodule update ../init >update.out &&\n+\t\tgit submodule update ../init 2>update.out &&\n \t\tcat update.out &&\n \t\ttest_i18ngrep \"not initialized\" update.out &&\n \t\ttest_must_fail git rev-parse --resolve-git-dir ../init/.git &&\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272871","messageId":"1446597434-1740-11-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 10/11] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:13Z","receivedAt":"2015-11-04T00:37:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Expose possible parallelism either via the \"--jobs\" CLI parameter or\nthe \"submodule.jobs\" setting.\n\nBy having the variable initialized to -1, we make sure 0 can be passed\ninto the parallel processing machine, which will then pick as many parallel\nworkers as there are CPUs.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-submodule.txt | 7 ++++++-\n builtin/submodule--helper.c | 18 ++++++++++++++----\n git-submodule.sh | 9 +++++++++\n t/t7406-submodule-update.sh | 12 ++++++++++++\n 4 files changed, 41 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex f17687e..c70fafd 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n 'git submodule' [--quiet] deinit [-f|--force] [--] ...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t [-f|--force] [--rebase|--merge] [--reference ]\n-\t [--depth ] [--recursive] [--] [...]\n+\t [--depth ] [--recursive] [--jobs ] [--] [...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]\n \t [commit] [--] [...]\n 'git submodule' [--quiet] foreach [--recursive] \n@@ -374,6 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n \tclone with a history truncated to the specified number of revisions.\n \tSee linkgit:git-clone[1]\n \n+-j ::\n+--jobs ::\n+\tThis option is only valid for the update command.\n+\tClone new submodules in parallel with as many jobs.\n+\tDefaults to the `submodule.jobs` option.\n \n ...::\n \tPaths to submodule(s). When specified this will restrict the command\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 95b45a2..662d329 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -426,6 +426,7 @@ static int update_clone_task_finished(int result,\n \n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n+\tint max_jobs = -1;\n \tstruct string_list_item *item;\n \tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -446,6 +447,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &pp.depth, \"\",\n \t\t\t N_(\"Create a shallow clone truncated to the \"\n \t\t\t \"specified number of revisions\")),\n+\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t\t N_(\"parallel jobs\")),\n \t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n \t\tOPT_END()\n \t};\n@@ -467,10 +470,17 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(git_submodule_config, NULL);\n-\trun_processes_parallel(1, update_clone_get_next_task,\n-\t\t\t\t update_clone_start_failure,\n-\t\t\t\t update_clone_task_finished,\n-\t\t\t\t &pp);\n+\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = config_parallel_submodules();\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = 1;\n+\n+\trun_processes_parallel(max_jobs,\n+\t\t\t update_clone_get_next_task,\n+\t\t\t update_clone_start_failure,\n+\t\t\t update_clone_task_finished,\n+\t\t\t &pp);\n \n \tif (pp.print_unmatched) {\n \t\tprintf(\"#unmatched\\n\");\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9f554fb..10c5af9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -645,6 +645,14 @@ cmd_update()\n \t\t--depth=*)\n \t\t\tdepth=$1\n \t\t\t;;\n+\t\t-j|--jobs)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tjobs=\"--jobs=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--jobs=*)\n+\t\t\tjobs=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -670,6 +678,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+--reference \"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n+\t\t${jobs:+$jobs} \\\n \t\t\"$@\" | {\n \terr=\n \twhile read mode sha1 stage just_cloned sm_path\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex dda3929..05ea66f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n \t test_i18ngrep \"Submodule path .deeper/submodule/subsubmodule.: checked out\" actual\n \t)\n '\n+\n+test_expect_success 'submodule update can be run in parallel' '\n+\t(cd super2 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&\n+\t grep \"7 children\" trace.out &&\n+\t git config submodule.jobs 8 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update &&\n+\t grep \"8 children\" trace.out &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&\n+\t grep \"9 children\" trace.out\n+\t)\n+'\n test_done\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272872","messageId":"1446597434-1740-12-git-send-email-sbeller@google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 11/11] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T00:37:14Z","receivedAt":"2015-11-04T00:37:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just pass it along to \"git submodule update\", which may pick reasonable\ndefaults if you don't specify an explicit number.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-clone.txt | 6 +++++-\n builtin/clone.c | 19 +++++++++++++------\n t/t7406-submodule-update.sh | 15 +++++++++++++++\n 3 files changed, 33 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex f1f2a3f..01bd6b7 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t [-o ] [-b ] [-u ] [--reference ]\n \t [--dissociate] [--separate-git-dir ]\n \t [--depth ] [--[no-]single-branch]\n-\t [--recursive | --recurse-submodules] [--] \n+\t [--recursive | --recurse-submodules] [--jobs ] [--] \n \t []\n \n DESCRIPTION\n@@ -216,6 +216,10 @@ objects from the source repository into a pack in the cloned repository.\n \tThe result is Git repository can be separated from working\n \ttree.\n \n+-j ::\n+--jobs ::\n+\tThe number of submodules fetched at the same time.\n+\tDefaults to the `submodule.jobs` option.\n \n ::\n \tThe (possibly remote) repository to clone from. See the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9eaecd9..ce578d2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -50,6 +50,7 @@ static int option_progress = -1;\n static struct string_list option_config;\n static struct string_list option_reference;\n static int option_dissociate;\n+static int max_jobs = -1;\n \n static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"initialize submodules in the clone\")),\n \tOPT_BOOL(0, \"recurse-submodules\", &option_recursive,\n \t\t N_(\"initialize submodules in the clone\")),\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n \t\t N_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {\n \tOPT_END()\n };\n \n-static const char *argv_submodule[] = {\n-\t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n-};\n-\n static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n@@ -724,8 +723,16 @@ static int checkout(void)\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t sha1_to_hex(sha1), \"1\", NULL);\n \n-\tif (!err && option_recursive)\n-\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n+\tif (!err && option_recursive) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n+\n+\t\tif (max_jobs != -1)\n+\t\t\targv_array_pushf(&args, \"--jobs=%d\", max_jobs);\n+\n+\t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n+\t\targv_array_clear(&args);\n+\t}\n \n \treturn err;\n }\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 05ea66f..ade0524 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '\n \t grep \"9 children\" trace.out\n \t)\n '\n+\n+test_expect_success 'git clone passes the parallel jobs config on to submodules' '\n+\ttest_when_finished \"rm -rf super4\" &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&\n+\tgrep \"7 children\" trace.out &&\n+\trm -rf super4 &&\n+\tgit config --global submodule.jobs 8 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&\n+\tgrep \"8 children\" trace.out &&\n+\trm -rf super4 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&\n+\tgrep \"9 children\" trace.out &&\n+\trm -rf super4\n+'\n+\n test_done\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272895","messageId":"xmqqk2pxbqft.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"1446597434-1740-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 00/11] Expose the submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T17:54:46Z","receivedAt":"2015-11-04T17:54:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Where does it apply?\n> ---\n> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge\n> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces\n> the previous patches in sb/submodule-parallel-update\n>\n> What does it do?\n> ---\n> This series should finish the on going efforts of parallelizing\n> submodule network traffic. The patches contain tests for clone,\n> fetch and submodule update to use the actual parallelism both via\n> command line as well as a configured option. I decided to go with\n> \"submodule.jobs\" for all three for now.\n\nThe order of patches and where the series builds makes me suspect\nthat I have been expecting too much from the \"parallel-fetch\" topic.\n\nI've been hoping that it would be useful for the project as a whole\nto polish the other topic and make it available to wider audience\nsooner by itself (both from \"end users get improved Git early\"\naspect and from \"the core machinery to be reused in follow-up\nimprovements are made closer to perfection sooner\" perspective). So\nI've been expecting that \"Let's fix it on Windows\" change directly\non top of sb/submodule-parallel-fetch to make that topic usable\nbefore everything else. Other patches in this series may require\nthe child_process_cleanup() change, so they may be applied on top of\nthe merge between sb/submodule-parallel-fetch (updated for Windows)\nand rs/daemon-plug-child-leak topic.\n\nThat does not seem to be what's happening here (note: I am not\ncomplaining; I am just trying to make sure expectation matches\nreality). Am I reading you correctly?\n\nI think sb/submodule-parallel-fetch + sb/submodule-parallel-update\nas a single topic would need more time to mature to be in a tagged\nrelease than we have in the remainder of this cycle. It is likely\nthat the former topic has a chance to get rebased after 2.7 happens.\nAnd that would allow us to (1) use the child_process_cleanup() from\nget-go instead of _deinit and to (2) get the machinery right both\nfor UNIX and Windows from get-go. Which would make the result\neasier to understand. As this is one of the more important areas,\nit matters to keep the resulting code and the rationale behind it\nunderstandable by reading \"log --reverse -p\".\n\nThanks.\n"},{"id":"272896","messageId":"CAGZ79kZhgVThVrR-gOZj3zaicG43JNgv1FTxk2S5mr__7YB5Yg@mail.gmail.com","threadId":"40721","inReplyTo":"xmqqk2pxbqft.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 00/11] Expose the submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T18:08:36Z","receivedAt":"2015-11-04T18:08:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 4, 2015 at 9:54 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Where does it apply?\n>> ---\n>> This series applies on top of d075d2604c0f92045caa8d5bb6ab86cf4921a4ae (Merge\n>> branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update) and replaces\n>> the previous patches in sb/submodule-parallel-update\n>>\n>> What does it do?\n>> ---\n>> This series should finish the on going efforts of parallelizing\n>> submodule network traffic. The patches contain tests for clone,\n>> fetch and submodule update to use the actual parallelism both via\n>> command line as well as a configured option. I decided to go with\n>> \"submodule.jobs\" for all three for now.\n>\n> The order of patches and where the series builds makes me suspect\n> that I have been expecting too much from the \"parallel-fetch\" topic.\n>\n> I've been hoping that it would be useful for the project as a whole\n> to polish the other topic and make it available to wider audience\n> sooner by itself (both from \"end users get improved Git early\"\n> aspect and from \"the core machinery to be reused in follow-up\n> improvements are made closer to perfection sooner\" perspective). So\n> I've been expecting that \"Let's fix it on Windows\" change directly\n> on top of sb/submodule-parallel-fetch to make that topic usable\n> before everything else.\n\nI can resend the patches on top of sb/submodule-parallel-fetch\n(though looking at sb/submodule-parallel-fetch..d075d2604c0f920\n[Merge branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update]\nI don't expect conflicts, so it would be a verbatim resend)\n\n\n> Other patches in this series may require\n> the child_process_cleanup() change, so they may be applied on top of\n> the merge between sb/submodule-parallel-fetch (updated for Windows)\n> and rs/daemon-plug-child-leak topic.\n\nI assumed the rs/daemon-plug-child-leak topic is no feature, but cleanup.\nWhich is why I would have expected a sb/submodule-parallel-fetch-for-windows\npointing at maybe the third patch of the series on top of\nrs/daemon-plug-child-leak\n\n>\n> That does not seem to be what's happening here (note: I am not\n> complaining; I am just trying to make sure expectation matches\n> reality). Am I reading you correctly?\n\nI really wanted to send out just one series, my bad.\nThe ordering made sense to me (first the run-command related fixes\nand then the new features in later patches)\n\n>\n> I think sb/submodule-parallel-fetch + sb/submodule-parallel-update\n> as a single topic would need more time to mature to be in a tagged\n> release than we have in the remainder of this cycle.\n\nI agree on that.\n\n> It is likely\n> that the former topic has a chance to get rebased after 2.7 happens.\n> And that would allow us to (1) use the child_process_cleanup() from\n> get-go instead of _deinit and to (2) get the machinery right both\n> for UNIX and Windows from get-go. Which would make the result\n> easier to understand. As this is one of the more important areas,\n> it matters to keep the resulting code and the rationale behind it\n> understandable by reading \"log --reverse -p\".\n\nSo you are saying that reading the Windows cleanup patch\nbefore the s/deinit/clear/ Patch by Rene makes it way easier to understand?\nWhich is why you would prefer another history. (Merging an updated\nsb/submodule-parallel-fetch again to rs/daemon-plug-child-leak or even\nsb/submodule-parallel-update)\n\nThanks,\nStefan\n"},{"id":"272897","messageId":"xmqqd1vpbpik.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"1446597434-1740-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T18:14:43Z","receivedAt":"2015-11-04T18:14:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The warning message is cluttering the output itself,\n> so just report it once.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> run-command.c | 20 ++++++++++++++------\n> 1 file changed, 14 insertions(+), 6 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 7c00c21..3ae563f 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)\n> \n> static void set_nonblocking(int fd)\n> {\n> +\tstatic int reported_degrade = 0;\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> +\tif (flags < 0) {\n> +\t\tif (!reported_degrade) {\n> +\t\t\twarning(\"Could not get file status flags, \"\n> +\t\t\t\t\"output will be degraded\");\n> +\t\t\treported_degrade = 1;\n> +\t\t}\n> +\t} else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {\n> +\t\tif (!reported_degrade) {\n> +\t\t\twarning(\"Could not set file status flags, \"\n> +\t\t\t\t\"output will be degraded\");\n> +\t\t\treported_degrade = 1;\n> +\t\t}\n> +\t}\n> }\n\nImagine that we are running two things A and B at the same time. We\nask poll(2) and it says both A and B have some data ready to be\nread, and we try to read from A. strbuf_read_once() would try to\nread up to 8K, relying on the fact that you earlier set the IO to be\nnonblock. It will get stuck reading from A without allowing output\nfrom B to drain. B's write may get stuck because we are not reading\nfrom it, and would cause B to stop making progress.\n\nWhat if the other sides of the connection from A and B are talking\nwith each other, and B's non-progress caused the processing for A on\nthe other side of the connection to block, causing it not to produce\nmore output to allow us to make progress reading from A (so that\neventually we can give B a chance to drain its output)? Imagine A\nand B are pushes to the same remote, B may be pushing a change to a\nsubmodule while A may be pushing a matching change to its\nsuperproject, and the server may be trying to make sure that the\nsubmodule update completes and updates the ref before making the\nsuperproject's tree that binds that updated submodule's commit\navailble, for example? Can we make any progress from that point?\n\nI am not convinced that the failure to set nonblock IO is merely\n\"output will be degraded\". It feels more like a fatal error if we\nare driving more than one task at the same time.\n"},{"id":"272898","messageId":"xmqq8u6dbpdd.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"CAGZ79kZhgVThVrR-gOZj3zaicG43JNgv1FTxk2S5mr__7YB5Yg@mail.gmail.com","subject":"Re: [PATCHv3 00/11] Expose the submodule parallelism to the user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T18:17:50Z","receivedAt":"2015-11-04T18:17:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So you are saying that reading the Windows cleanup patch\n> before the s/deinit/clear/ Patch by Rene makes it way easier to understand?\n\nNo.\n\nThe run-parallel API added in parallel-fetch that needs to be fixed\nup (because the topic is in 'next', my bad merging prematurely) with\na separate \"oops that was not friendly to Windows\" is the primary\nconcern I have for those who later want to learn how it was designed\nby going through \"log --reverse -p\".\n"},{"id":"272913","messageId":"CAGZ79kaiRKHd2RS9eNeZt_VZqqBF0HS0D=x1HbOTPXYOphu8pg@mail.gmail.com","threadId":"40721","inReplyTo":"xmqqd1vpbpik.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T20:14:44Z","receivedAt":"2015-11-04T20:14:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The warning message is cluttering the output itself,\n>> so just report it once.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> run-command.c | 20 ++++++++++++++------\n>> 1 file changed, 14 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 7c00c21..3ae563f 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -1012,13 +1012,21 @@ static void pp_cleanup(struct parallel_processes *pp)\n>>\n>> static void set_nonblocking(int fd)\n>> {\n>> + static int reported_degrade = 0;\n>> int flags = fcntl(fd, F_GETFL);\n>> - if (flags < 0)\n>> - warning(\"Could not get file status flags, \"\n>> - \"output will be degraded\");\n>> - else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n>> - warning(\"Could not set file status flags, \"\n>> - \"output will be degraded\");\n>> + if (flags < 0) {\n>> + if (!reported_degrade) {\n>> + warning(\"Could not get file status flags, \"\n>> + \"output will be degraded\");\n>> + reported_degrade = 1;\n>> + }\n>> + } else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {\n>> + if (!reported_degrade) {\n>> + warning(\"Could not set file status flags, \"\n>> + \"output will be degraded\");\n>> + reported_degrade = 1;\n>> + }\n>> + }\n>> }\n>\n> Imagine that we are running two things A and B at the same time. We\n> ask poll(2) and it says both A and B have some data ready to be\n> read, and we try to read from A. strbuf_read_once() would try to\n> read up to 8K, relying on the fact that you earlier set the IO to be\n> nonblock. It will get stuck reading from A without allowing output\n> from B to drain. B's write may get stuck because we are not reading\n> from it, and would cause B to stop making progress.\n>\n> What if the other sides of the connection from A and B are talking\n> with each other,\n\nI am not sure if we want to allow this ever. How would that work with\njobs==1? How do we guarantee to have A and B running at the same time?\nIn a later version of the parallel processing we may have some other ramping\nup mechanisms, such as: \"First run only one process until it outputted at least\n250 bytes\", which would also produce such a lock. So instead a time based ramp\nup may be better. But my general concern is how much guarantees are we selling\nhere? Maybe the documentation needs to explicitly state that we cannot talk to\neach or at least should assume the blocking of stdout/err.\n\n> and B's non-progress caused the processing for A on\n> the other side of the connection to block, causing it not to produce\n> more output to allow us to make progress reading from A (so that\n> eventually we can give B a chance to drain its output)? Imagine A\n> and B are pushes to the same remote, B may be pushing a change to a\n> submodule while A may be pushing a matching change to its\n> superproject, and the server may be trying to make sure that the\n> submodule update completes and updates the ref before making the\n> superproject's tree that binds that updated submodule's commit\n> availble, for example? Can we make any progress from that point?\n>\n> I am not convinced that the failure to set nonblock IO is merely\n> \"output will be degraded\". It feels more like a fatal error if we\n> are driving more than one task at the same time.\n>\n\nAnother approach would be to test if we can set to non blocking and if\nthat is not possible, do not buffer it, but redirect the subcommand\ndirectly to stderr of the calling process.\n\n if (set_nonblocking(pp->children[i].process.err) < 0) {\n pp->children[i].process.err = 2;\n degraded_parallelism = 1;\n }\n\nand once we observe the degraded_parallelism flag, we can only\nschedule a maximum of one job at a time, having direct output?\n"},{"id":"272915","messageId":"563A6C3D.2050805@kdbg.org","threadId":"40721","inReplyTo":"CAGZ79kaiRKHd2RS9eNeZt_VZqqBF0HS0D=x1HbOTPXYOphu8pg@mail.gmail.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-04T20:36:13Z","receivedAt":"2015-11-04T20:36:13Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.11.2015 um 21:14 schrieb Stefan Beller:\n> On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano wrote:\n>> Imagine that we are running two things A and B at the same time. We\n>> ask poll(2) and it says both A and B have some data ready to be\n>> read, and we try to read from A. strbuf_read_once() would try to\n>> read up to 8K, relying on the fact that you earlier set the IO to be\n>> nonblock. It will get stuck reading from A without allowing output\n>> from B to drain. B's write may get stuck because we are not reading\n>> from it, and would cause B to stop making progress.\n>>\n>> What if the other sides of the connection from A and B are talking\n>> with each other,\n>\n> I am not sure if we want to allow this ever. How would that work with\n> jobs==1? How do we guarantee to have A and B running at the same time?\n\nI think that a scenario where A and B are communicating is rather \nfar-fetched. We are talking about parallelizing independent tasks. I \nwould not worry.\n\n-- Hannes\n"},{"id":"272916","messageId":"xmqq8u6da448.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"CAGZ79kaiRKHd2RS9eNeZt_VZqqBF0HS0D=x1HbOTPXYOphu8pg@mail.gmail.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T20:42:15Z","receivedAt":"2015-11-04T20:42:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Another approach would be to test if we can set to non blocking and if\n> that is not possible, do not buffer it, but redirect the subcommand\n> directly to stderr of the calling process.\n>\n> if (set_nonblocking(pp->children[i].process.err) < 0) {\n> pp->children[i].process.err = 2;\n> degraded_parallelism = 1;\n> }\n>\n> and once we observe the degraded_parallelism flag, we can only\n> schedule a maximum of one job at a time, having direct output?\n\nI would even say that on a platform that is _capable_ of setting fd\nnon-blocking, we should signal a grave error and die if an attempt\nto do so fails, period.\n\nOn the other hand, on a platform that is known to be incapable\n(e.g. lacks SETFL or NONBLOCK), we have two options.\n\n1. If we can arrange to omit the intermediary buffer processing\n without butchering the flow of the main logic with many\n #ifdef..#endif, then that would make a lot of sense to do so, and\n running the processes in parallel with mixed output might be OK.\n It may not be very nice, but should be an acceptable compromise.\n\n2. If we need to sprinkle conditional compilation all over the place\n to do so, then I do not think it is worth it. Instead, we should\n keep a single code structure, and forbid setting numtasks to more\n than one, which would also remove the need for nonblock IO.\n\nEither way, bringing \"parallelism with sequential output\" to\nplatforms without nonblock IO can be left for a later day, when we\nfind either (1) a good approach that does not require nonblock IO to\ndo this, or (2) a good approach to do a nonblock IO on these\nplatforms (we know about Windows, but there may be others; I dunno).\n"},{"id":"272918","messageId":"xmqq4mh1a37i.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"563A6C3D.2050805@kdbg.org","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T21:01:53Z","receivedAt":"2015-11-04T21:01:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I think that a scenario where A and B are communicating is rather\n> far-fetched. We are talking about parallelizing independent tasks. I\n> would not worry.\n\nI wouldn't worry too much if this were merely a hack that is only\napplicable to submodules, but I do not think it is a healthy\nattitude to dismiss potential problem as far-fetched without\nthinking things through, when you are designing what goes into\nthe run-command API.\n\nI'd grant you that a complete deadlock is unlikely to be a problem\non its own. Somewhere somebody will eventually time out and unblock\nthe deadlock anyway.\n\nBut the symptom does not have to be as severe as a total deadlock to\nbe problematic. If we block B (and other tasks) by not reading from\nthem quickly because we are blocked on reading from A, which may\ntake forever (in timescale of B and other tasks) to feed us enough\nto satisfy strbuf_read_once(), we are wasting resource by spawning B\n(and other tasks) early when we are not prepared to service them\nwell, on both our end and on the other side of the connection.\n\nBy the way, A and B do not have to be directly communicating to\ndeadlock. The underlying system, either the remote end or the local\nend or even a relaying system in between (think: network) can\nthrottle to cause the same symptom without A and B knowing (which\nwas the example I gave).\n"},{"id":"272919","messageId":"CAGZ79kbwJrQ9SrGkJsSx9oUcP98dn9wP=ZvgQLRjmPaZtOzanA@mail.gmail.com","threadId":"40721","inReplyTo":"xmqq8u6da448.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T21:04:31Z","receivedAt":"2015-11-04T21:04:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 4, 2015 at 12:42 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Another approach would be to test if we can set to non blocking and if\n>> that is not possible, do not buffer it, but redirect the subcommand\n>> directly to stderr of the calling process.\n>>\n>> if (set_nonblocking(pp->children[i].process.err) < 0) {\n>> pp->children[i].process.err = 2;\n>> degraded_parallelism = 1;\n>> }\n>>\n>> and once we observe the degraded_parallelism flag, we can only\n>> schedule a maximum of one job at a time, having direct output?\n>\n> I would even say that on a platform that is _capable_ of setting fd\n> non-blocking, we should signal a grave error and die if an attempt\n> to do so fails, period.\n\nSo more like:\n\n if (platform_capable_non_blocking_IO())\n set_nonblocking_or_die(&pp->children[i].process.err);\n else\n pp->children[i].process.err = 2; /* ugly intermixed output is possible*/\n\n>\n> On the other hand, on a platform that is known to be incapable\n> (e.g. lacks SETFL or NONBLOCK), we have two options.\n>\n> 1. If we can arrange to omit the intermediary buffer processing\n> without butchering the flow of the main logic with many\n> #ifdef..#endif, then that would make a lot of sense to do so, and\n> running the processes in parallel with mixed output might be OK.\n> It may not be very nice, but should be an acceptable compromise.\n\n>From what I hear this kind of output is very annoying. (One of the\nmain complaints of repo users beside missing atomic fetch transactions)\n\n>\n> 2. If we need to sprinkle conditional compilation all over the place\n> to do so, then I do not think it is worth it. Instead, we should\n> keep a single code structure, and forbid setting numtasks to more\n> than one, which would also remove the need for nonblock IO.\n\nSo additional to the code above, we can add the\nplatform_capable_non_blocking_IO() condition to either the ramp up process,\nor have a\n\n if (!platform_capable_non_blocking_IO())\n pp.max_processes = 1;\n\nin the init phase. Then we have only 2 places that deal with the\nproblem, no #ifdefs\nelsewhere.\n\n>\n> Either way, bringing \"parallelism with sequential output\" to\n> platforms without nonblock IO can be left for a later day, when we\n> find either (1) a good approach that does not require nonblock IO to\n> do this, or (2) a good approach to do a nonblock IO on these\n> platforms (we know about Windows, but there may be others; I dunno).\n>\n"},{"id":"272921","messageId":"xmqqziyt8nth.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"CAGZ79kbwJrQ9SrGkJsSx9oUcP98dn9wP=ZvgQLRjmPaZtOzanA@mail.gmail.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T21:19:38Z","receivedAt":"2015-11-04T21:19:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So more like:\n>\n> if (platform_capable_non_blocking_IO())\n> set_nonblocking_or_die(&pp->children[i].process.err);\n> else\n> pp->children[i].process.err = 2; /* ugly intermixed output is possible*/\n\nWhen I mentioned #if..#endif, I didn't mean it as a dogmatic\n\"conditional compilation is wrong\" sense. It was more about \"the\nhigh-level flow of logic should not have to know too much about\nplatform peculiarities\". As platform_capable_non_blocking_IO()\nfunction would be a constant function after you compile it for a\nsingle platform, if you add 10 instances of such if/else in a patch\nthat adds 250 lines, unless the change is to add a set of lowest\nlevel helpers to be called from the higher-level flow of logic so\nthat the callers do not have to know about the platform details,\nthat's just as bad as adding 10 instances of #if..#endif.\n\n>> On the other hand, on a platform that is known to be incapable\n>> (e.g. lacks SETFL or NONBLOCK), we have two options.\n>>\n>> 1. If we can arrange to omit the intermediary buffer processing\n>> without butchering the flow of the main logic with many\n>> #ifdef..#endif, then that would make a lot of sense to do so, and\n>> running the processes in parallel with mixed output might be OK.\n>> It may not be very nice, but should be an acceptable compromise.\n>\n> From what I hear this kind of output is very annoying. (One of the\n> main complaints of repo users beside missing atomic fetch transactions)\n\nWhen (1) \"parallelism with sequential output\" is the desired\noutcome, (2) on some platforms we haven't found a way to achieve\nboth, and (3) a non-sequential output is unacceptable, then\nparallelism has to give :-(.\n\nI was getting an impression from your \"not buffer\" suggestion that\n\"sequential output\" would be the one that can be sacrificed, but\nthat is OK. Until we find a way to achieve both at the same time,\nachieving only either one or the other is better than achieving\nnothing.\n\n>> Either way, bringing \"parallelism with sequential output\" to\n>> platforms without nonblock IO can be left for a later day, when we\n>> find either (1) a good approach that does not require nonblock IO to\n>> do this, or (2) a good approach to do a nonblock IO on these\n>> platforms (we know about Windows, but there may be others; I dunno).\n"},{"id":"272922","messageId":"CAGZ79kYYvgLyDhNO5j=t-RPmKV_KHHMoN3xOV4Ygt2MwQe2HRQ@mail.gmail.com","threadId":"40721","inReplyTo":"xmqqziyt8nth.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-04T21:41:04Z","receivedAt":"2015-11-04T21:41:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 4, 2015 at 1:19 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So more like:\n>>\n>> if (platform_capable_non_blocking_IO())\n>> set_nonblocking_or_die(&pp->children[i].process.err);\n>> else\n>> pp->children[i].process.err = 2; /* ugly intermixed output is possible*/\n>\n> When I mentioned #if..#endif, I didn't mean it as a dogmatic\n> \"conditional compilation is wrong\" sense. It was more about \"the\n> high-level flow of logic should not have to know too much about\n> platform peculiarities\". As platform_capable_non_blocking_IO()\n> function would be a constant function after you compile it for a\n> single platform, if you add 10 instances of such if/else in a patch\n> that adds 250 lines, unless the change is to add a set of lowest\n> level helpers to be called from the higher-level flow of logic so\n> that the callers do not have to know about the platform details,\n> that's just as bad as adding 10 instances of #if..#endif.\n\nRight. Yeah I was just outlining the program flow as I understood it.\n\nSpecially:\n * It's fine if the platform doesn't support non blocking IO\n * but if the platform claims to support it and fails to, this is a hard error.\n How is this worse than the platform claiming to not support it at all?\n\nI mean another solution there would be to try to set the non blocking IO\nand if that fails we put that process-to-be-started into a special queue,\nwhich will start the process once the stderr channel is not occupied any more\n(i.e. the process which is the current output owner has ended or there is\nno such process)\n\nThat way we would fall back to no parallelism in Windows and could\neven gracefully fallback in other systems, which suddenly have problems\nsetting non blocking IO. (Though I suspect that isn't required here).\n\n>\n>>> On the other hand, on a platform that is known to be incapable\n>>> (e.g. lacks SETFL or NONBLOCK), we have two options.\n>>>\n>>> 1. If we can arrange to omit the intermediary buffer processing\n>>> without butchering the flow of the main logic with many\n>>> #ifdef..#endif, then that would make a lot of sense to do so, and\n>>> running the processes in parallel with mixed output might be OK.\n>>> It may not be very nice, but should be an acceptable compromise.\n>>\n>> From what I hear this kind of output is very annoying. (One of the\n>> main complaints of repo users beside missing atomic fetch transactions)\n>\n> When (1) \"parallelism with sequential output\" is the desired\n> outcome, (2) on some platforms we haven't found a way to achieve\n> both, and (3) a non-sequential output is unacceptable, then\n> parallelism has to give :-(.\n\nOk, then I will go that route.\n\n>\n> I was getting an impression from your \"not buffer\" suggestion that\n> \"sequential output\" would be the one that can be sacrificed, but\n> that is OK. Until we find a way to achieve both at the same time,\n> achieving only either one or the other is better than achieving\n> nothing.\n\nI was not sure what is better to sacrifice. Scarifying parallelism sounds\nsafer to me (no apparent change compared to before).\n\n>\n>>> Either way, bringing \"parallelism with sequential output\" to\n>>> platforms without nonblock IO can be left for a later day, when we\n>>> find either (1) a good approach that does not require nonblock IO to\n>>> do this, or (2) a good approach to do a nonblock IO on these\n>>> platforms (we know about Windows, but there may be others; I dunno).\n"},{"id":"272926","messageId":"20151104225618.GA18805@sigill.intra.peff.net","threadId":"40721","inReplyTo":"xmqq4mh1a37i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-04T22:56:18Z","receivedAt":"2015-11-04T22:56:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 01:01:53PM -0800, Junio C Hamano wrote:\n\n> But the symptom does not have to be as severe as a total deadlock to\n> be problematic. If we block B (and other tasks) by not reading from\n> them quickly because we are blocked on reading from A, which may\n> take forever (in timescale of B and other tasks) to feed us enough\n> to satisfy strbuf_read_once(), we are wasting resource by spawning B\n> (and other tasks) early when we are not prepared to service them\n> well, on both our end and on the other side of the connection.\n\nI'm not sure I understand this line of reasoning. It is entirely\npossible that I have not been paying close enough attention and am\nmissing something subtle, so please feel free to hit me with the clue\nstick.\n\nBut why would we ever block reading from A? If poll() reported to us\nthat \"A\" is ready to read, and we call strbuf_read_once(), we will make\na _single_ read call (which was, after all, the point of adding\nstrbuf_read_once in the first place).\n\nSo even if descriptor \"A\" isn't non-blocking, why would we block? Only\nif the OS told us we are ready to read via poll(), but we are somehow\nnot (which, AFAIK, would be a bug in the OS).\n\nSo I'm not sure I see why we need to be non-blocking at all here, if we\nare correctly hitting poll() and doing a single read on anybody who\nclaims to be ready (rather than trying to soak up all of their available\ndata), then we should never block, and we should never starve one\nprocess (even without blocking, we could be in a busy loop slurping from\nA and starve B, but by hitting the descriptors in round-robin for each\npoll(), we make sure they all progress).\n\nWhat am I missing?\n\n-Peff\n"},{"id":"272928","messageId":"xmqqvb9h8ale.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"20151104225618.GA18805@sigill.intra.peff.net","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T02:05:17Z","receivedAt":"2015-11-05T02:05:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I'm not sure I see why we need to be non-blocking at all here, if we\n> are correctly hitting poll() and doing a single read on anybody who\n> claims to be ready (rather than trying to soak up all of their available\n> data), then we should never block, and we should never starve one\n> process (even without blocking, we could be in a busy loop slurping from\n> A and starve B, but by hitting the descriptors in round-robin for each\n> poll(), we make sure they all progress).\n>\n> What am I missing?\n\nI've always assumed that the original reason why we wanted to set\nthe fd to nonblock was because poll(2) only tells us there is\nsomething to read (even a single byte), and the xread_nonblock()\ncall strbuf_read_once() makes with the default size of 8KB is\nallowed to consume all available bytes and then get stuck waiting\nfor the remainder of 8KB before returning.\n\nIf the read(2) in xread_nonblock() always returns as soon as we\nreceive as much as there is data available without waiting for any\nmore, ignoring the size of the buffer (rather, taking the size of\nthe buffer only as the upper bound), then there is no need for\nnonblock anywhere.\n\nSo perhaps the original reasoning of doing nonblock was faulty, you\nare saying?\n"},{"id":"272942","messageId":"20151105065111.GA4725@sigill.intra.peff.net","threadId":"40721","inReplyTo":"xmqqvb9h8ale.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-05T06:51:11Z","receivedAt":"2015-11-05T06:51:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 06:05:17PM -0800, Junio C Hamano wrote:\n\n> I've always assumed that the original reason why we wanted to set\n> the fd to nonblock was because poll(2) only tells us there is\n> something to read (even a single byte), and the xread_nonblock()\n> call strbuf_read_once() makes with the default size of 8KB is\n> allowed to consume all available bytes and then get stuck waiting\n> for the remainder of 8KB before returning.\n> \n> If the read(2) in xread_nonblock() always returns as soon as we\n> receive as much as there is data available without waiting for any\n> more, ignoring the size of the buffer (rather, taking the size of\n> the buffer only as the upper bound), then there is no need for\n> nonblock anywhere.\n\nThis latter paragraph was my impression of how pipe reading generally\nworked, for blocking or non-blocking. That is, if there is data, both\ncases return what we have (up to the length specified by the user), and\nit is only when there is _no_ data that we might choose to block.\n\nIt's easy to verify experimentally. E.g.:\n\n perl -e 'while(1) { syswrite(STDOUT, \"a\", 1); sleep(1); }' |\n strace perl -e 'while(1) { sysread(STDIN, my $buf, 1024) }'\n\nshould show a series of 1-byte reads. But of course that only shows that\nit works on my system[1], not everywhere.\n\nPOSIX implies it is the case in the definition of read[2] in two ways:\n\n 1. The O_NONBLOCK behavior for pipes is mentioned only when dealing\n with empty pipes.\n\n 2. Later, it says:\n\n The value returned may be less than nbyte if the number of bytes\n left in the file is less than nbyte, if the read() request was\n interrupted by a signal, or if the file is a pipe or FIFO or\n special file and has fewer than nbyte bytes immediately available\n for reading.\n\n That is not explicit, but the \"immediately\" there seems to imply\n it.\n\n> So perhaps the original reasoning of doing nonblock was faulty, you\n> are saying?\n\nExactly. And therefore a convenient way to deal with the portability\nissue is to get rid of it. :)\n\n-Peff\n"},{"id":"272948","messageId":"xmqq4mh09a0q.fsf@gitster.mtv.corp.google.com","threadId":"40721","inReplyTo":"20151105065111.GA4725@sigill.intra.peff.net","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T07:32:21Z","receivedAt":"2015-11-05T07:32:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> POSIX implies it is the case in the definition of read[2] in two ways:\n>\n> 1. The O_NONBLOCK behavior for pipes is mentioned only when dealing\n> with empty pipes.\n>\n> 2. Later, it says:\n>\n> The value returned may be less than nbyte if the number of bytes\n> left in the file is less than nbyte, if the read() request was\n> interrupted by a signal, or if the file is a pipe or FIFO or\n> special file and has fewer than nbyte bytes immediately available\n> for reading.\n>\n> That is not explicit, but the \"immediately\" there seems to imply\n> it.\n\nWe were reading the same book, but I was more worried about that\n\"may\" there; it merely tells the caller of read(2) not to be alarmed\nwhen the call returned without filling the entire buffer, without\nmandating the implementation of read(2) never to block.\n\nHaving said that,...\n\n>> So perhaps the original reasoning of doing nonblock was faulty, you\n>> are saying?\n>\n> Exactly. And therefore a convenient way to deal with the portability\n> issue is to get rid of it. :)\n\n... I do like the simplification you alluded to in the other\nmessage. Not having to worry about the nonblock (at least until it\nis found problematic in the real world) is a very good first step,\nespecially because the approach allows us to collectively make\nprogress by letting all of us in various platforms build and\nexperiment with \"something that works\".\n"},{"id":"272960","messageId":"CAGZ79kaoWkJR+dg1fiKTiWHWi7N2Ni3ANi4n06_7OU=qt=X_KA@mail.gmail.com","threadId":"40721","inReplyTo":"xmqq4mh09a0q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 02/11] run-command: report failure for degraded output just once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T17:37:14Z","receivedAt":"2015-11-05T17:37:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 4, 2015 at 11:32 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> POSIX implies it is the case in the definition of read[2] in two ways:\n>>\n>> 1. The O_NONBLOCK behavior for pipes is mentioned only when dealing\n>> with empty pipes.\n>>\n>> 2. Later, it says:\n>>\n>> The value returned may be less than nbyte if the number of bytes\n>> left in the file is less than nbyte, if the read() request was\n>> interrupted by a signal, or if the file is a pipe or FIFO or\n>> special file and has fewer than nbyte bytes immediately available\n>> for reading.\n>>\n>> That is not explicit, but the \"immediately\" there seems to imply\n>> it.\n>\n> We were reading the same book, but I was more worried about that\n> \"may\" there; it merely tells the caller of read(2) not to be alarmed\n> when the call returned without filling the entire buffer, without\n> mandating the implementation of read(2) never to block.\n>\n> Having said that,...\n>\n>>> So perhaps the original reasoning of doing nonblock was faulty, you\n>>> are saying?\n\nI agree that the original reasoning was faulty. It happened in the first place,\nbecause of how I approached the problem. (strbuf_read should return immediately\nafter reading and to communicate that we had non blocking read and checked for\nEAGAIN).\n\nHaving read the man pages again, I agree with you that the non blocking is\nbogus to begin with.\n\n>>\n>> Exactly. And therefore a convenient way to deal with the portability\n>> issue is to get rid of it. :)\n>\n> ... I do like the simplification you alluded to in the other\n> message. Not having to worry about the nonblock (at least until it\n> is found problematic in the real world) is a very good first step,\n> especially because the approach allows us to collectively make\n> progress by letting all of us in various platforms build and\n> experiment with \"something that works\".\n\nI'll send a patch to just remove set_nonblocking which should fix the compile\nproblems on Windows and make it work regardless on all platforms.\n\nAfter that I continue with the update series.\n"},{"id":"273163","messageId":"56426DD1.6090904@web.de","threadId":"40721","inReplyTo":"1446597434-1740-9-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-11-10T22:21:05Z","receivedAt":"2015-11-10T22:21:05Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 04.11.2015 um 01:37 schrieb Stefan Beller:\n> This allows to configure fetching and updating in parallel\n> without having the command line option.\n>\n> This moved the responsibility to determine how many parallel processes\n> to start from builtin/fetch to submodule.c as we need a way to communicate\n> \"The user did not specify the number of parallel processes in the command\n> line options\" in the builtin fetch. The submodule code takes care of\n> the precedence (CLI > config > default)\n>\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/config.txt | 7 +++++++\n> builtin/fetch.c | 2 +-\n> submodule-config.c | 15 +++++++++++++++\n> submodule-config.h | 2 ++\n> submodule.c | 5 +++++\n> t/t5526-fetch-submodules.sh | 14 ++++++++++++++\n> 6 files changed, 44 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..70e1b88 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2643,6 +2643,13 @@ submodule..ignore::\n> \t\"--ignore-submodules\" option. The 'git submodule' commands are not\n> \taffected by this setting.\n>\n> +submodule.jobs::\n> +\tThis is used to determine how many submodules can be operated on in\n> +\tparallel. Specifying a positive integer allows up to that number\n> +\tof submodules being fetched in parallel. This is used in fetch\n> +\tand clone operations only. A value of 0 will give some reasonable\n> +\tconfiguration. It defaults to 1.\n> +\n\nJust curious (and sorry if this has already been discussed and I missed\nit, but the volume of your output is too much for my current git time\nbudget ;-): While this config is for fetching only, do I recall correctly\nthat you have plans to do submodule work tree updates in parallel too?\nIf so, would it make sense to have different settings for fetching and\nupdating?\n\n> tag.sort::\n> \tThis variable controls the sort ordering of tags when displayed by\n> \tlinkgit:git-tag[1]. Without the \"--sort=\" option provided, the\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 9cc1c9d..60e6797 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -37,7 +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 int max_children = -1;\n> static const char *depth;\n> static const char *upload_pack;\n> static struct strbuf default_rla = STRBUF_INIT;\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 29e21b2..475551a 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -32,6 +32,7 @@ enum lookup_type {\n>\n> static struct submodule_cache cache;\n> static int is_cache_init;\n> +static int parallel_jobs = -1;\n>\n> static int config_path_cmp(const struct submodule_entry *a,\n> \t\t\t const struct submodule_entry *b,\n> @@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,\n> \t\t\t\t\t const char *value,\n> \t\t\t\t\t struct parse_config_parameter *me)\n> {\n> +\tif (!strcmp(key, \"jobs\")) {\n> +\t\tparallel_jobs = strtol(value, NULL, 10);\n> +\t\tif (parallel_jobs < 0) {\n> +\t\t\twarning(\"submodule.jobs not allowed to be negative.\");\n> +\t\t\tparallel_jobs = 1;\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> @@ -482,3 +492,8 @@ void submodule_free(void)\n> \tcache_free(&cache);\n> \tis_cache_init = 0;\n> }\n> +\n> +int config_parallel_submodules(void)\n> +{\n> +\treturn parallel_jobs;\n> +}\n> diff --git a/submodule-config.h b/submodule-config.h\n> index f9e2a29..d9bbf9a 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n> \t\tconst char *path);\n> void submodule_free(void);\n>\n> +int config_parallel_submodules(void);\n> +\n> #endif /* SUBMODULE_CONFIG_H */\n> diff --git a/submodule.c b/submodule.c\n> index 0257ea3..188ba02 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array *options,\n> \targv_array_push(&spf.args, \"--recurse-submodules-default\");\n> \t/* default value, \"--submodule-prefix\" and its value are added later */\n>\n> +\tif (max_parallel_jobs < 0)\n> +\t\tmax_parallel_jobs = config_parallel_submodules();\n> +\tif (max_parallel_jobs < 0)\n> +\t\tmax_parallel_jobs = 1;\n> +\n> \tcalculate_changed_submodule_paths();\n> \trun_processes_parallel(max_parallel_jobs,\n> \t\t\t get_next_submodule,\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 1b4ce69..5c3579c 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -470,4 +470,18 @@ test_expect_success \"don't fetch submodule when newly recorded commits are alrea\n> \ttest_i18ncmp expect.err actual.err\n> '\n>\n> +test_expect_success 'fetching submodules respects parallel settings' '\n> +\tgit config fetch.recurseSubmodules true &&\n> +\t(\n> +\t\tcd downstream &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&\n> +\t\tgrep \"7 children\" trace.out &&\n> +\t\tgit config submodule.jobs 8 &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch &&\n> +\t\tgrep \"8 children\" trace.out &&\n> +\t\tGIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&\n> +\t\tgrep \"9 children\" trace.out\n> +\t)\n> +'\n> +\n> test_done\n>\n"},{"id":"273164","messageId":"CAGZ79kbqedWRDADChorvWhcmyjO4iZqt4WO8KSo917pxWgr4Rg@mail.gmail.com","threadId":"40721","inReplyTo":"56426DD1.6090904@web.de","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-10T22:29:48Z","receivedAt":"2015-11-10T22:29:48Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann wrote:\n>> +submodule.jobs::\n>> + This is used to determine how many submodules can be operated on in\n>> + parallel. Specifying a positive integer allows up to that number\n>> + of submodules being fetched in parallel. This is used in fetch\n>> + and clone operations only. A value of 0 will give some reasonable\n>> + configuration. It defaults to 1.\n>> +\n>\n>\n> Just curious (and sorry if this has already been discussed and I missed\n> it, but the volume of your output is too much for my current git time\n> budget ;-): While this config is for fetching only, do I recall correctly\n> that you have plans to do submodule work tree updates in parallel too?\n> If so, would it make sense to have different settings for fetching and\n> updating?\n\nTL;DR: checkout is serial, network-related stuff only will be using\nsubmodule.jobs\n\nIn the next series (origin/sb/submodule-parallel-update) this is reused for\nfetches, clones, so only the network stuff. The checkout (as all local\noperations)\nis still done serially, as then you don't run into problems in\nparallel at the same time.\n(checkouts may be parallelized but I haven't done that yet, and postpone that\nuntil it has settled a bit more)\n"},{"id":"273219","messageId":"56439D1E.8080102@web.de","threadId":"40721","inReplyTo":"CAGZ79kbqedWRDADChorvWhcmyjO4iZqt4WO8KSo917pxWgr4Rg@mail.gmail.com","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-11-11T19:55:10Z","receivedAt":"2015-11-11T19:55:10Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 10.11.2015 um 23:29 schrieb Stefan Beller:\n> On Tue, Nov 10, 2015 at 2:21 PM, Jens Lehmann wrote:\n>>> +submodule.jobs::\n>>> + This is used to determine how many submodules can be operated on in\n>>> + parallel. Specifying a positive integer allows up to that number\n>>> + of submodules being fetched in parallel. This is used in fetch\n>>> + and clone operations only. A value of 0 will give some reasonable\n>>> + configuration. It defaults to 1.\n>>> +\n>>\n>>\n>> Just curious (and sorry if this has already been discussed and I missed\n>> it, but the volume of your output is too much for my current git time\n>> budget ;-): While this config is for fetching only, do I recall correctly\n>> that you have plans to do submodule work tree updates in parallel too?\n>> If so, would it make sense to have different settings for fetching and\n>> updating?\n>\n> TL;DR: checkout is serial, network-related stuff only will be using\n> submodule.jobs\n\nMy point being: isn't \"jobs\" a bit too generic for a config option that\nis only relevant for network-related stuff? Maybe \"submodule.fetchJobs\"\nor similar would be better, as you are already thinking about adding\nother parallelisms with different constraints later?\n\n> In the next series (origin/sb/submodule-parallel-update) this is reused for\n> fetches, clones, so only the network stuff. The checkout (as all local\n> operations)\n> is still done serially, as then you don't run into problems in\n> parallel at the same time.\n> (checkouts may be parallelized but I haven't done that yet, and postpone that\n> until it has settled a bit more)\n\nMakes sense.\n"},{"id":"273234","messageId":"CAGZ79kYiPb-GJ37Zq-2ULpLD8Lh_3qAxa0W+u6+5fMrX6YzJdw@mail.gmail.com","threadId":"40721","inReplyTo":"56439D1E.8080102@web.de","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-11T23:34:27Z","receivedAt":"2015-11-11T23:34:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann wrote:\n>>\n>> TL;DR: checkout is serial, network-related stuff only will be using\n>> submodule.jobs\n>\n>\n> My point being: isn't \"jobs\" a bit too generic for a config option that\n> is only relevant for network-related stuff? Maybe \"submodule.fetchJobs\"\n> or similar would be better, as you are already thinking about adding\n> other parallelisms with different constraints later?\n\nActually I don't think that far ahead.\n\n(I assume network to be the bottleneck for clone/fetch operations)\nAll I want is a saturated network all the time, and as the native git protocol\ndoesn't provide that (tcp startup takes time until full band witdth is reached,\nlocal operations both on client and server) I added the parallel stuff\nto 'smear' different submodule network traffics along the timeline,\nsuch that we have a better approximation of an always fully saturated link\nfor the whole operation. So in the long term future, we maybe want to\nreuse an http/ssh session for a different submodule, possibly interleaving\nthe different submodules on the wire to make it even faster. Though that\nmay not be helping much.\n\nSo we're back at bike shedding about the name. submodule.fetchJobs\nsounds like it only applies to fetching, do you think it's sufficient for clone\nas well?\n\nOnce upon a time, Junio used 'submodule.fetchParallel' or 'submodule.paralle'\nin a discussion[1] for the distinction of the local and networked things.\n[1] Discussing \"[PATCH] Add fetch.recurseSubmoduleParallelism config option\"\n\nHow about submodules.parallelNetwork for the networking part and\nsubmodules.parallelLocal for the local part? (I don't implement parallelLocal in\nthe next few weeks I'd estimate).\n"},{"id":"273289","messageId":"56464C44.6090902@web.de","threadId":"40721","inReplyTo":"CAGZ79kYiPb-GJ37Zq-2ULpLD8Lh_3qAxa0W+u6+5fMrX6YzJdw@mail.gmail.com","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-11-13T20:47:00Z","receivedAt":"2015-11-13T20:47:00Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 12.11.2015 um 00:34 schrieb Stefan Beller:\n> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann wrote:\n>>>\n>>> TL;DR: checkout is serial, network-related stuff only will be using\n>>> submodule.jobs\n>>\n>>\n>> My point being: isn't \"jobs\" a bit too generic for a config option that\n>> is only relevant for network-related stuff? Maybe \"submodule.fetchJobs\"\n>> or similar would be better, as you are already thinking about adding\n>> other parallelisms with different constraints later?\n>\n> Actually I don't think that far ahead.\n\nMaybe I've been bitten once too often by too generic names that became\na problem later on ... ;-)\n\n> (I assume network to be the bottleneck for clone/fetch operations)\n> All I want is a saturated network all the time, and as the native git protocol\n> doesn't provide that (tcp startup takes time until full band witdth is reached,\n> local operations both on client and server) I added the parallel stuff\n> to 'smear' different submodule network traffics along the timeline,\n> such that we have a better approximation of an always fully saturated link\n> for the whole operation. So in the long term future, we maybe want to\n> reuse an http/ssh session for a different submodule, possibly interleaving\n> the different submodules on the wire to make it even faster. Though that\n> may not be helping much.\n>\n> So we're back at bike shedding about the name. submodule.fetchJobs\n> sounds like it only applies to fetching, do you think it's sufficient for clone\n> as well?\n\nHmm, to me fetching is a part of cloning, so I don't have a problem with\nthat. And documenting it accordingly should make it clear to everyone.\n\n> Once upon a time, Junio used 'submodule.fetchParallel' or 'submodule.paralle'\n> in a discussion[1] for the distinction of the local and networked things.\n> [1] Discussing \"[PATCH] Add fetch.recurseSubmoduleParallelism config option\"\n>\n> How about submodules.parallelNetwork for the networking part and\n> submodules.parallelLocal for the local part? (I don't implement parallelLocal in\n> the next few weeks I'd estimate).\n\nIf 'submodules.parallelNetwork' will be used for submodule push too as\nsoon as that learns parallel operation, I'm ok with that. But if we don't\nhave good reason to believe the number of jobs for fetch can simply be\nreused for push, me thinks we should have one config option containing the\nterm \"fetch\" now and another that contains \"push\" when we need it later,\njust to be on the safe side. Otherwise it might be hard to explain to\nusers why 'submodules.parallelNetwork' is only used for fetch and clone\nand why they have to set 'submodules.parallelPush' for pushing ...\n\nSo either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for\nme, and 'submodules.parallelNetwork' is ok too as long as we have reason\nto believe this value can be used for push later too.\n"},{"id":"273290","messageId":"CAGZ79kYjtpsVuTQt6Hg=GukFVyavLphH_i58v7ZNDF9ETh58Lw@mail.gmail.com","threadId":"40721","inReplyTo":"56464C44.6090902@web.de","subject":"Re: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-13T21:29:23Z","receivedAt":"2015-11-13T21:29:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 13, 2015 at 12:47 PM, Jens Lehmann wrote:\n> Am 12.11.2015 um 00:34 schrieb Stefan Beller:\n>>\n>> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann \n>> wrote:\n>>>>\n>>>>\n>>>> TL;DR: checkout is serial, network-related stuff only will be using\n>>>> submodule.jobs\n>>>\n>>>\n>>>\n>>> My point being: isn't \"jobs\" a bit too generic for a config option that\n>>> is only relevant for network-related stuff? Maybe \"submodule.fetchJobs\"\n>>> or similar would be better, as you are already thinking about adding\n>>> other parallelisms with different constraints later?\n>>\n>>\n>> Actually I don't think that far ahead.\n>\n>\n> Maybe I've been bitten once too often by too generic names that became\n> a problem later on ... ;-)\n>\n>> (I assume network to be the bottleneck for clone/fetch operations)\n>> All I want is a saturated network all the time, and as the native git\n>> protocol\n>> doesn't provide that (tcp startup takes time until full band witdth is\n>> reached,\n>> local operations both on client and server) I added the parallel stuff\n>> to 'smear' different submodule network traffics along the timeline,\n>> such that we have a better approximation of an always fully saturated link\n>> for the whole operation. So in the long term future, we maybe want to\n>> reuse an http/ssh session for a different submodule, possibly interleaving\n>> the different submodules on the wire to make it even faster. Though that\n>> may not be helping much.\n>>\n>> So we're back at bike shedding about the name. submodule.fetchJobs\n>> sounds like it only applies to fetching, do you think it's sufficient for\n>> clone\n>> as well?\n>\n>\n> Hmm, to me fetching is a part of cloning, so I don't have a problem with\n> that. And documenting it accordingly should make it clear to everyone.\n>\n>> Once upon a time, Junio used 'submodule.fetchParallel' or\n>> 'submodule.paralle'\n>> in a discussion[1] for the distinction of the local and networked things.\n>> [1] Discussing \"[PATCH] Add fetch.recurseSubmoduleParallelism config\n>> option\"\n>>\n>> How about submodules.parallelNetwork for the networking part and\n>> submodules.parallelLocal for the local part? (I don't implement\n>> parallelLocal in\n>> the next few weeks I'd estimate).\n>\n>\n> If 'submodules.parallelNetwork' will be used for submodule push too as\n> soon as that learns parallel operation, I'm ok with that. But if we don't\n> have good reason to believe the number of jobs for fetch can simply be\n> reused for push, me thinks we should have one config option containing the\n> term \"fetch\" now and another that contains \"push\" when we need it later,\n> just to be on the safe side. Otherwise it might be hard to explain to\n> users why 'submodules.parallelNetwork' is only used for fetch and clone\n> and why they have to set 'submodules.parallelPush' for pushing ...\n>\n> So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for\n> me, and 'submodules.parallelNetwork' is ok too as long as we have reason\n> to believe this value can be used for push later too.\n\nOk, got it. So fetchJobs is fine with me.\nMind the difference in the first part, submodule[s] in singular/plural.\nI thought submodule as a prefix for any individual submodule, but any\nsettings applying to all of the submodules, you'd take the plural submodules.*\nsettings.\n"}]}