{"thread":{"id":"40409","subject":"[PATCHv4 03/14] xread_nonblock: add functionality to read from fds without blocking","startedAt":"2015-09-23T01:45:18Z","lastAt":"2015-09-25T19:32:16Z","messageCount":39,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"270548","messageId":"1442972732-12118-1-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":null,"subject":"[PATCHv4 00/14] fetch submodules in parallel and a preview on parallel \"submodule update\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:18Z","receivedAt":"2015-09-23T01:45:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Today there was lots of discussion on the correct way of reading the\nstrbufs as well as some discussion on the structure of the\nasynchronous parallel process loop.\n\nPatches 1-8 bring parallel fetching of submodules, and have had some good exposure\nto review and feedback is incorporated.\n\nPatches 9-14 bring parallel submodule updates.\nPatch 14 is not ready yet (i.e. test suite failures), but the cleanups before\nin patch 9-13 can be reviewed without wasting time.\n\nAny feedback welcome,\nThanks,\nStefan\n\nDiff to v3 below. The patches can also be found at [1]\n[1] https://github.com/stefanbeller/git/tree/submodulec_nonthreaded_parallel_4\n\nJonathan Nieder (1):\n submodule: Send \"Fetching submodule \" to standard error\n\nStefan Beller (13):\n xread: poll on non blocking fds\n xread_nonblock: add functionality to read from fds without blocking\n strbuf: add strbuf_read_once to read without blocking\n run-command: factor out return value computation\n run-command: add an asynchronous parallel child processor\n fetch_populated_submodules: use new parallel job processing\n submodules: allow parallel fetching, add tests and documentation\n submodule-config: Untangle logic in parse_config\n submodule config: keep update strategy around\n git submodule update: cmd_update_recursive\n git submodule update: cmd_update_clone\n git submodule update: cmd_update_fetch\n Rewrite submodule update in C\n\n Documentation/fetch-options.txt | 7 +\n builtin/fetch.c | 6 +-\n builtin/pull.c | 6 +\n builtin/submodule--helper.c | 251 +++++++++++++++++++++++++++++\n git-compat-util.h | 1 +\n git-submodule.sh | 339 ++++++++++++++--------------------------\n run-command.c | 320 ++++++++++++++++++++++++++++++++++---\n run-command.h | 36 +++++\n strbuf.c | 11 ++\n strbuf.h | 9 ++\n submodule-config.c | 85 +++++-----\n submodule-config.h | 1 +\n submodule.c | 120 +++++++++-----\n submodule.h | 2 +-\n t/t0061-run-command.sh | 20 +++\n t/t5526-fetch-submodules.sh | 70 ++++++---\n test-run-command.c | 24 +++\n wrapper.c | 35 ++++-\n 18 files changed, 987 insertions(+), 356 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex baa7563..b79117a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -382,10 +382,14 @@ static int update_next_task(void *data,\n \t\targv_array_pushf(&cp->env_array, \"sm_path=%s\", sub->path);\n \t\targv_array_pushf(&cp->env_array, \"name=%s\", sub->name);\n \t\targv_array_pushf(&cp->env_array, \"url=%s\", sub->url);\n+\t\targv_array_pushf(&cp->env_array, \"sha1=%s\", sha1_to_hex(ce->sha1));\n \t\targv_array_pushf(&cp->env_array, \"update_module=%s\", update_module);\n \n \t\tcp->git_cmd = 1;\n+\t\tcp->no_stdin = 1;\n \t\tcp->stdout_to_stderr = 1;\n+\t\tcp->err = -1;\n+\t\targv_array_init(&cp->args);\n \t\targv_array_push(&cp->args, \"submodule\");\n \t\tif (!file_exists(sm_gitdir))\n \t\t\targv_array_push(&cp->args, \"update_clone\");\ndiff --git a/run-command.c b/run-command.c\nindex 06d5a5d..494e1f8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -276,8 +276,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n \t} else {\n-\t\tif (waiting != pid\n-\t\t || (determine_return_value(status, &code, &failed_errno, argv0) < 0))\n+\t\tif (waiting != pid || (determine_return_value(status,\n+\t\t\t\t\t\t\t &code,\n+\t\t\t\t\t\t\t &failed_errno,\n+\t\t\t\t\t\t\t argv0) < 0))\n \t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n \n@@ -870,7 +872,6 @@ struct parallel_processes {\n \n \tint max_processes;\n \tint nr_processes;\n-\tunsigned all_tasks_started : 1;\n \n \tget_next_task_fn get_next_task;\n \tstart_failure_fn start_failure;\n@@ -899,9 +900,9 @@ void default_start_failure(void *data,\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(&sb, \"%s \", cp->argv[i]);\n+\t\tstrbuf_addf(&sb, \" %s\", cp->argv[i]);\n \n-\tdie_errno(\"Starting a child failed:\\n%s\", sb.buf);\n+\tdie_errno(\"Starting a child failed:%s\", sb.buf);\n }\n \n void default_return_value(void *data,\n@@ -915,12 +916,12 @@ void default_return_value(void *data,\n \t\treturn;\n \n \tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(&sb, \"%s \", cp->argv[i]);\n+\t\tstrbuf_addf(&sb, \" %s\", cp->argv[i]);\n \n-\tdie_errno(\"A child failed with return code:\\n%s\\n%d\", sb.buf, result);\n+\tdie_errno(\"A child failed with return code %d:%s\", result, sb.buf);\n }\n \n-static void run_processes_parallel_init(struct parallel_processes *pp,\n+static void pp_init(struct parallel_processes *pp,\n \t\t\t\t\tint n, void *data,\n \t\t\t\t\tget_next_task_fn get_next_task,\n \t\t\t\t\tstart_failure_fn start_failure,\n@@ -941,7 +942,6 @@ static void run_processes_parallel_init(struct parallel_processes *pp,\n \tpp->return_value = return_value ? return_value : default_return_value;\n \n \tpp->nr_processes = 0;\n-\tpp->all_tasks_started = 0;\n \tpp->output_owner = 0;\n \tpp->children = xcalloc(n, sizeof(*pp->children));\n \tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n@@ -954,9 +954,10 @@ static void run_processes_parallel_init(struct parallel_processes *pp,\n \t}\n }\n \n-static void run_processes_parallel_cleanup(struct parallel_processes *pp)\n+static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n+\n \tfor (i = 0; i < pp->max_processes; i++)\n \t\tstrbuf_release(&pp->children[i].err);\n \n@@ -976,7 +977,8 @@ static void set_nonblocking(int fd)\n \t\t\t\"output will be degraded\");\n }\n \n-static void run_processes_parallel_start_one(struct parallel_processes *pp)\n+/* returns 1 if a process was started, 0 otherwise */\n+static int pp_start_one(struct parallel_processes *pp)\n {\n \tint i;\n \n@@ -988,10 +990,9 @@ static void run_processes_parallel_start_one(struct parallel_processes *pp)\n \n \tif (!pp->get_next_task(pp->data,\n \t\t\t &pp->children[i].process,\n-\t\t\t &pp->children[i].err)) {\n-\t\tpp->all_tasks_started = 1;\n-\t\treturn;\n-\t}\n+\t\t\t &pp->children[i].err))\n+\t\treturn 1;\n+\n \tif (start_command(&pp->children[i].process))\n \t\tpp->start_failure(pp->data,\n \t\t\t\t &pp->children[i].process,\n@@ -1002,23 +1003,17 @@ static void run_processes_parallel_start_one(struct parallel_processes *pp)\n \tpp->nr_processes++;\n \tpp->children[i].in_use = 1;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n+\treturn 0;\n }\n \n-static void run_processes_parallel_start_as_needed(struct parallel_processes *pp)\n-{\n-\twhile (pp->nr_processes < pp->max_processes &&\n-\t !pp->all_tasks_started)\n-\t\trun_processes_parallel_start_one(pp);\n-}\n-\n-static void run_processes_parallel_buffer_stderr(struct parallel_processes *pp)\n+static void pp_buffer_stderr(struct parallel_processes *pp)\n {\n \tint i;\n \n \twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n \t\tif (errno == EINTR)\n \t\t\tcontinue;\n-\t\trun_processes_parallel_cleanup(pp);\n+\t\tpp_cleanup(pp);\n \t\tdie_errno(\"poll\");\n \t}\n \n@@ -1033,7 +1028,7 @@ static void run_processes_parallel_buffer_stderr(struct parallel_processes *pp)\n \t}\n }\n \n-static void run_processes_parallel_output(struct parallel_processes *pp)\n+static void pp_output(struct parallel_processes *pp)\n {\n \tint i = pp->output_owner;\n \tif (pp->children[i].in_use &&\n@@ -1043,7 +1038,7 @@ static void run_processes_parallel_output(struct parallel_processes *pp)\n \t}\n }\n \n-static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n+static void pp_collect_finished(struct parallel_processes *pp)\n {\n \tint i = 0;\n \tpid_t pid;\n@@ -1063,17 +1058,11 @@ static void run_processes_parallel_collect_finished(struct parallel_processes *p\n \t\t\t pid == pp->children[i].process.pid)\n \t\t\t\tbreak;\n \t\tif (i == pp->max_processes)\n-\t\t\t/*\n-\t\t\t * waitpid returned another process id\n-\t\t\t * which we are not waiting for.\n-\t\t\t */\n-\t\t\treturn;\n-\n-\t\tif (strbuf_read_once(&pp->children[i].err,\n-\t\t\t\t pp->children[i].process.err, 0) < 0 &&\n-\t\t errno != EAGAIN)\n-\t\t\tdie_errno(\"strbuf_read_once\");\n+\t\t\tdie(\"BUG: found a child process we were not aware of\");\n \n+\t\tif (strbuf_read(&pp->children[i].err,\n+\t\t\t\tpp->children[i].process.err, 0) < 0)\n+\t\t\tdie_errno(\"strbuf_read\");\n \n \t\tif (determine_return_value(wait_status, &code, &errno,\n \t\t\t\t\t pp->children[i].process.argv[0]) < 0)\n@@ -1122,18 +1111,20 @@ int run_processes_parallel(int n, void *data,\n \t\t\t return_value_fn return_value)\n {\n \tstruct parallel_processes pp;\n-\trun_processes_parallel_init(&pp, n, data,\n-\t\t\t\t get_next_task,\n-\t\t\t\t start_failure,\n-\t\t\t\t return_value);\n-\n-\twhile (!pp.all_tasks_started || pp.nr_processes > 0) {\n-\t\trun_processes_parallel_start_as_needed(&pp);\n-\t\trun_processes_parallel_buffer_stderr(&pp);\n-\t\trun_processes_parallel_output(&pp);\n-\t\trun_processes_parallel_collect_finished(&pp);\n+\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n+\n+\twhile (1) {\n+\t\twhile (pp.nr_processes < pp.max_processes &&\n+\t\t !pp_start_one(&pp))\n+\t\t\t; /* nothing */\n+\t\tif (!pp.nr_processes)\n+\t\t\tbreak;\n+\t\tpp_buffer_stderr(&pp);\n+\t\tpp_output(&pp);\n+\t\tpp_collect_finished(&pp);\n \t}\n-\trun_processes_parallel_cleanup(&pp);\n+\n+\tpp_cleanup(&pp);\n \n \treturn 0;\n }\ndiff --git a/run-command.h b/run-command.h\nindex 0c1b363..3807fd1 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -155,6 +155,4 @@ int run_processes_parallel(int n, void *data,\n \t\t\t start_failure_fn,\n \t\t\t return_value_fn);\n \n-void run_processes_parallel_schedule_error(struct strbuf *err);\n-\n #endif\ndiff --git a/strbuf.h b/strbuf.h\nindex 4d4e5b1..ea69665 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,8 +367,11 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n- * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.\n- * The fd must have set O_NONBLOCK.\n+ * Read from a file descriptor that is marked as O_NONBLOCK without\n+ * blocking. Returns the number of new bytes appended to the sb.\n+ * Negative return value signals there was an error returned from\n+ * underlying read(2), in which case the caller should check errno.\n+ * e.g. errno == EAGAIN when the read may have blocked.\n */\n extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n \ndiff --git a/submodule-config.c b/submodule-config.c\nindex 0298a60..8b8c7d1 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -258,93 +258,72 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!name_and_item_from_var(var, &name, &item))\n \t\treturn 0;\n \n-\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n-\t\t\tname.buf);\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 \n \tif (!strcmp(item.buf, \"path\")) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->path != NULL) {\n+\t\telse if (!me->overwrite && submodule->path != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tif (submodule->path)\n+\t\t\t\tcache_remove_path(me->cache, submodule);\n+\t\t\tfree((void *) submodule->path);\n+\t\t\tsubmodule->path = xstrdup(value);\n+\t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\n-\t\tif (submodule->path)\n-\t\t\tcache_remove_path(me->cache, submodule);\n-\t\tfree((void *) submodule->path);\n-\t\tstrbuf_addstr(&path, value);\n-\t\tsubmodule->path = strbuf_detach(&path, NULL);\n-\t\tcache_put_path(me->cache, submodule);\n \t} else if (!strcmp(item.buf, \"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-\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\n-\t\tsubmodule->fetch_recurse = parse_fetch_recurse(var, value,\n+\t\telse\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\tstruct strbuf ignore = STRBUF_INIT;\n-\t\tif (!me->overwrite && submodule->ignore != NULL) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->ignore != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!value) {\n-\t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n-\t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n+\t\telse if (strcmp(value, \"untracked\") &&\n+\t\t\t strcmp(value, \"dirty\") &&\n+\t\t\t strcmp(value, \"all\") &&\n+\t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tfree((void *) submodule->ignore);\n+\t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->ignore);\n-\t\tstrbuf_addstr(&ignore, value);\n-\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n \t} else if (!strcmp(item.buf, \"url\")) {\n-\t\tstruct strbuf url = STRBUF_INIT;\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url != NULL) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n-\t\t\tgoto release_return;\n+\t\t} else {\n+\t\t\tfree((void *) submodule->url);\n+\t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->url);\n-\t\tstrbuf_addstr(&url, value);\n-\t\tsubmodule->url = strbuf_detach(&url, NULL);\n \t} else if (!strcmp(item.buf, \"update\")) {\n-\t\tstruct strbuf update = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->update != NULL) {\n+\t\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\t\tgoto release_return;\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-\n-\t\tfree((void *) submodule->update);\n-\t\tstrbuf_addstr(&update, value);\n-\t\tsubmodule->update = strbuf_detach(&update, NULL);\n \t}\n \n-release_return:\n \tstrbuf_release(&name);\n \tstrbuf_release(&item);\n \ndiff --git a/submodule.c b/submodule.c\nindex d15364f..fdaf3e4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -650,10 +650,12 @@ int fetch_populated_submodules(const struct argv_array *options,\n {\n \tint i;\n \tstruct submodule_parallel_fetch spf = SPF_INIT;\n+\n \tspf.work_tree = get_git_work_tree();\n \tspf.command_line_option = command_line_option;\n \tspf.quiet = quiet;\n \tspf.prefix = prefix;\n+\n \tif (!spf.work_tree)\n \t\tgoto out;\n \n@@ -738,12 +740,11 @@ int get_next_submodule(void *data, struct child_process *cp,\n \t\tif (is_directory(git_dir)) {\n \t\t\tchild_process_init(cp);\n \t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n+\t\t\tcp->env = local_repo_env;\n \t\t\tcp->git_cmd = 1;\n-\t\t\tcp->no_stdout = 1;\n \t\t\tcp->no_stdin = 1;\n \t\t\tcp->stdout_to_stderr = 1;\n \t\t\tcp->err = -1;\n-\t\t\tcp->env = local_repo_env;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n \t\t\t\t\t spf->prefix, ce->name);\ndiff --git a/wrapper.c b/wrapper.c\nindex 54ce231..41a21e1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -206,16 +206,10 @@ ssize_t xread(int fd, void *buf, size_t len)\n \t\t\t\tcontinue;\n \t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK) {\n \t\t\t\tstruct pollfd pfd;\n-\t\t\t\tint i;\n \t\t\t\tpfd.events = POLLIN;\n \t\t\t\tpfd.fd = fd;\n-\t\t\t\ti = poll(&pfd, 1, 100);\n-\t\t\t\tif (i < 0) {\n-\t\t\t\t\tif (errno == EINTR || errno == ENOMEM)\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\telse\n-\t\t\t\t\t\tdie_errno(\"poll\");\n-\t\t\t\t}\n+\t\t\t\t/* We deliberately ignore the return value */\n+\t\t\t\tpoll(&pfd, 1, -1);\n \t\t\t}\n \t\t}\n \t\treturn nr;\n@@ -225,13 +219,13 @@ ssize_t xread(int fd, void *buf, size_t len)\n /*\n * xread_nonblock() is the same a read(), but it automatically restarts read()\n * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that\n- * \"len\" bytes is read even if the data is available.\n+ * \"len\" bytes is read. EWOULDBLOCK is turned into EAGAIN.\n */\n ssize_t xread_nonblock(int fd, void *buf, size_t len)\n {\n \tssize_t nr;\n \tif (len > MAX_IO_SIZE)\n-\t len = MAX_IO_SIZE;\n+\t\tlen = MAX_IO_SIZE;\n \twhile (1) {\n \t\tnr = read(fd, buf, len);\n \t\tif (nr < 0) {\n\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270546","messageId":"1442972732-12118-2-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 01/14] submodule: Send \"Fetching submodule \" to standard error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:19Z","receivedAt":"2015-09-23T01:45:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Jonathan Nieder \n\nThe \"Pushing submodule \" progress output correctly goes to\nstderr, but \"Fetching submodule \" is going to stdout by mistake.\nFix it to write to stderr.\n\nNoticed while trying to implement a parallel submodule fetch. When\nthis particular output line went to a different file descriptor, it\nwas buffered separately, resulting in wrongly interleaved output if\nwe copied it to the terminal naively.\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n submodule.c | 2 +-\n t/t5526-fetch-submodules.sh | 51 +++++++++++++++++++++++----------------------\n 2 files changed, 27 insertions(+), 26 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9fcc86f..1d64e57 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -694,7 +694,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n \t\t\tif (!quiet)\n-\t\t\t\tprintf(\"Fetching submodule %s%s\\n\", prefix, ce->name);\n+\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n \t\t\tcp.dir = submodule_path.buf;\n \t\t\targv_array_push(&argv, default_argv);\n \t\t\targv_array_push(&argv, \"--submodule-prefix\");\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex a4532b0..17759b1 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -16,7 +16,8 @@ add_upstream_commit() {\n \t\tgit add subfile &&\n \t\tgit commit -m new subfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err\n \t) &&\n \t(\n@@ -27,6 +28,7 @@ add_upstream_commit() {\n \t\tgit add deepsubfile &&\n \t\tgit commit -m new deepsubfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n+\t\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> ../expect.err\n \t\techo \"From $pwd/deepsubmodule\" >> ../expect.err &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err\n \t)\n@@ -56,9 +58,7 @@ test_expect_success setup '\n \t(\n \t\tcd downstream &&\n \t\tgit submodule update --init --recursive\n-\t) &&\n-\techo \"Fetching submodule submodule\" > expect.out &&\n-\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> expect.out\n+\t)\n '\n \n test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n@@ -67,7 +67,7 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -96,7 +96,7 @@ test_expect_success \"using fetchRecurseSubmodules=true in .gitmodules recurses i\n \t\tgit config -f .gitmodules submodule.submodule.fetchRecurseSubmodules true &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -127,7 +127,7 @@ test_expect_success \"--recurse-submodules overrides fetchRecurseSubmodules setti\n \t\tgit config --unset -f .gitmodules submodule.submodule.fetchRecurseSubmodules &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -146,7 +146,7 @@ test_expect_success \"--dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules --dry-run >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -155,7 +155,7 @@ test_expect_success \"Without --dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -166,7 +166,7 @@ test_expect_success \"recurseSubmodules=true propagates into submodules\" '\n \t\tgit config fetch.recurseSubmodules true\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -180,7 +180,7 @@ test_expect_success \"--recurse-submodules overrides config in submodule\" '\n \t\t) &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -214,16 +214,15 @@ test_expect_success \"Recursion stops when no new submodule commits are fetched\"\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\techo \"Fetching submodule submodule\" > expect.out.sub &&\n \techo \"From $pwd/.\" > expect.err.sub &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.sub &&\n-\thead -2 expect.err >> expect.err.sub &&\n+\thead -3 expect.err >> expect.err.sub &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out.sub actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion doesn't happen when new superproject commits don't change any submodules\" '\n@@ -269,7 +268,7 @@ test_expect_success \"Recursion picks up config in submodule\" '\n \t\t)\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion picks up all submodules when necessary\" '\n@@ -285,7 +284,8 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\"\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err.sub\n \t) &&\n \thead1=$(git rev-parse --short HEAD) &&\n@@ -295,13 +295,13 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >> expect.err.2 &&\n \tcat expect.err.sub >> expect.err.2 &&\n-\ttail -2 expect.err >> expect.err.2 &&\n+\ttail -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.2 actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no new commits are fetched in the superproject (and ignores config)\" '\n@@ -317,7 +317,8 @@ test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no ne\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\" &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo Fetching submodule submodule > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \" $head1..$head2 master -> origin/master\" >> ../expect.err.sub\n \t) &&\n \t(\n@@ -335,7 +336,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\ttail -2 expect.err > expect.err.deepsub &&\n+\ttail -3 expect.err > expect.err.deepsub &&\n \techo \"From $pwd/.\" > expect.err &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err &&\n \tcat expect.err.sub >> expect.err &&\n@@ -354,7 +355,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \t\t\tgit config --unset -f .gitmodules submodule.subdir/deepsubmodule.fetchRecursive\n \t\t)\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -388,7 +389,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config fetch.recurseSubmodules on-demand &&\n@@ -399,7 +400,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \t\tcd downstream &&\n \t\tgit config --unset fetch.recurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n@@ -416,7 +417,7 @@ test_expect_success \"'submodule..fetchRecurseSubmodules=on-demand' override\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \" $head1..$head2 master -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config submodule.submodule.fetchRecurseSubmodules on-demand &&\n@@ -427,7 +428,7 @@ test_expect_success \"'submodule..fetchRecurseSubmodules=on-demand' override\n \t\tcd downstream &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270549","messageId":"1442972732-12118-3-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 02/14] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:20Z","receivedAt":"2015-09-23T01:45:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">From the man page:\nEAGAIN The file descriptor fd refers to a file other than a socket\n and has been marked nonblocking (O_NONBLOCK), and the read\n would block.\n\nEAGAIN or EWOULDBLOCK\n The file descriptor fd refers to a socket and has been marked\n nonblocking (O_NONBLOCK), and the read would block. POSIX.1-2001\n allows either error to be returned for this case, and does not\n require these constants to have the same value, so a portable\n application should check for both possibilities.\n\nIf we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\nAs the intent of xread is to read as much as possible either until the\nfd is EOF or an actual error occurs, we can ease the feeder of the fd\nby not spinning the whole time, but rather wait for it politely by not\nbusy waiting.\n\nWe should not care if the call to poll failed, as we're in an infinite\nloop and can only get out with the correct read().\n\nSigned-off-by: Stefan Beller \n---\n wrapper.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex ff49807..5517928 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -201,8 +201,17 @@ ssize_t xread(int fd, void *buf, size_t len)\n \t len = MAX_IO_SIZE;\n \twhile (1) {\n \t\tnr = read(fd, buf, len);\n-\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n-\t\t\tcontinue;\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK) {\n+\t\t\t\tstruct pollfd pfd;\n+\t\t\t\tpfd.events = POLLIN;\n+\t\t\t\tpfd.fd = fd;\n+\t\t\t\t/* We deliberately ignore the return value */\n+\t\t\t\tpoll(&pfd, 1, -1);\n+\t\t\t}\n+\t\t}\n \t\treturn nr;\n \t}\n }\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270545","messageId":"1442972732-12118-4-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 03/14] xread_nonblock: add functionality to read from fds without blocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:21Z","receivedAt":"2015-09-23T01:45:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Provide a wrapper to read(), similar to xread(), that restarts on\nEINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to\nhandle polling itself, possibly polling multiple sockets or performing\nsome other action.\n\nHelped-by: Jacob Keller \nHelped-by: Jeff King ,\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n git-compat-util.h | 1 +\n wrapper.c | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6d391f..9ccea85 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\ndiff --git a/wrapper.c b/wrapper.c\nindex 5517928..41a21e1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -217,6 +217,28 @@ ssize_t xread(int fd, void *buf, size_t len)\n }\n \n /*\n+ * xread_nonblock() is the same a read(), but it automatically restarts read()\n+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that\n+ * \"len\" bytes is read. EWOULDBLOCK is turned into EAGAIN.\n+ */\n+ssize_t xread_nonblock(int fd, void *buf, size_t len)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t\tlen = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = read(fd, buf, len);\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EWOULDBLOCK)\n+\t\t\t\terrno = EAGAIN;\n+\t\t}\n+\t\treturn nr;\n+\t}\n+}\n+\n+/*\n * xwrite() is the same a write(), but it automatically restarts write()\n * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270547","messageId":"1442972732-12118-5-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 04/14] strbuf: add strbuf_read_once to read without blocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:22Z","receivedAt":"2015-09-23T01:45:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new call will read from a file descriptor into a strbuf once. The\nunderlying call xread_nonblock is meant to execute without blocking if\nthe file descriptor is set to O_NONBLOCK. It is a bug to call\nstrbuf_read_once on a file descriptor which would block.\n\nSigned-off-by: Stefan Beller \n---\n strbuf.c | 11 +++++++++++\n strbuf.h | 9 +++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex cce5eed..35e71b8 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \treturn sb->len - oldlen;\n }\n \n+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n+{\n+\tssize_t cnt;\n+\n+\tstrbuf_grow(sb, hint ? hint : 8192);\n+\tcnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\tif (cnt > 0)\n+\t\tstrbuf_setlen(sb, sb->len + cnt);\n+\treturn cnt;\n+}\n+\n #define STRBUF_MAXLINK (2*PATH_MAX)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\ndiff --git a/strbuf.h b/strbuf.h\nindex aef2794..ea69665 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,6 +367,15 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n+ * Read from a file descriptor that is marked as O_NONBLOCK without\n+ * blocking. Returns the number of new bytes appended to the sb.\n+ * Negative return value signals there was an error returned from\n+ * underlying read(2), in which case the caller should check errno.\n+ * e.g. errno == EAGAIN when the read may have blocked.\n+ */\n+extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n+\n+/**\n * Read the contents of a file, specified by its path. The third argument\n * can be used to give a hint about the file size, to avoid reallocs.\n */\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270557","messageId":"1442972732-12118-6-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 05/14] run-command: factor out return value computation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:23Z","receivedAt":"2015-09-23T01:45:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We will need computing the return value in a later patch without the\nwait.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 56 ++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 34 insertions(+), 22 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 28e1d55..2bf1aba 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n+static int determine_return_value(int wait_status,\n+\t\t\t\t int *result,\n+\t\t\t\t int *error_code,\n+\t\t\t\t const char *argv0)\n+{\n+\tif (WIFSIGNALED(wait_status)) {\n+\t\t*result = WTERMSIG(wait_status);\n+\t\tif (*result != SIGINT && *result != SIGQUIT)\n+\t\t\terror(\"%s died of signal %d\", argv0, *result);\n+\t\t/*\n+\t\t * This return value is chosen so that code & 0xff\n+\t\t * mimics the exit code that a POSIX shell would report for\n+\t\t * a program that died from this signal.\n+\t\t */\n+\t\t*result += 128;\n+\t} else if (WIFEXITED(wait_status)) {\n+\t\t*result = WEXITSTATUS(wait_status);\n+\t\t/*\n+\t\t * Convert special exit code when execvp failed.\n+\t\t */\n+\t\tif (*result == 127) {\n+\t\t\t*result = -1;\n+\t\t\t*error_code = ENOENT;\n+\t\t}\n+\t} else\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int wait_or_whine(pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n@@ -244,29 +273,12 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n-\t} else if (waiting != pid) {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n-\t} else if (WIFSIGNALED(status)) {\n-\t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n-\t\t\terror(\"%s died of signal %d\", argv0, code);\n-\t\t/*\n-\t\t * This return value is chosen so that code & 0xff\n-\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t * a program that died from this signal.\n-\t\t */\n-\t\tcode += 128;\n-\t} else if (WIFEXITED(status)) {\n-\t\tcode = WEXITSTATUS(status);\n-\t\t/*\n-\t\t * Convert special exit code when execvp failed.\n-\t\t */\n-\t\tif (code == 127) {\n-\t\t\tcode = -1;\n-\t\t\tfailed_errno = ENOENT;\n-\t\t}\n \t} else {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t\tif (waiting != pid || (determine_return_value(status,\n+\t\t\t\t\t\t\t &code,\n+\t\t\t\t\t\t\t &failed_errno,\n+\t\t\t\t\t\t\t argv0) < 0))\n+\t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n \n \tclear_child_for_cleanup(pid);\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270558","messageId":"1442972732-12118-7-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:24Z","receivedAt":"2015-09-23T01:45:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to run external commands in parallel with ordered output\non stderr.\n\nIf we run external commands in parallel we cannot pipe the output directly\nto the our stdout/err as it would mix up. So each process's output will\nflow through a pipe, which we buffer. One subprocess can be directly\npiped to out stdout/err for a low latency feedback to the user.\n\nExample:\nLet's assume we have 5 submodules A,B,C,D,E and each fetch takes a\ndifferent amount of time as the different submodules vary in size, then\nthe output of fetches in sequential order might look like this:\n\n time -->\n output: |---A---| |-B-| |-------C-------| |-D-| |-E-|\n\nWhen we schedule these submodules into maximal two parallel processes,\na schedule and sample output over time may look like this:\n\nprocess 1: |---A---| |-D-| |-E-|\n\nprocess 2: |-B-| |-------C-------|\n\noutput: |---A---|B|---C-------|DE\n\nSo A will be perceived as it would run normally in the single child\nversion. As B has finished by the time A is done, we can dump its whole\nprogress buffer on stderr, such that it looks like it finished in no\ntime. Once that is done, C is determined to be the visible child and\nits progress will be reported in real time.\n\nSo this way of output is really good for human consumption, as it only\nchanges the timing, not the actual output.\n\nFor machine consumption the output needs to be prepared in the tasks,\nby either having a prefix per line or per block to indicate whose tasks\noutput is displayed, because the output order may not follow the\noriginal sequential ordering:\n\n |----A----| |--B--| |-C-|\n\nwill be scheduled to be all parallel:\n\nprocess 1: |----A----|\nprocess 2: |--B--|\nprocess 3: |-C-|\noutput: |----A----|CB\n\nThis happens because C finished before B did, so it will be queued for\noutput before B.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 264 +++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 36 +++++++\n t/t0061-run-command.sh | 20 ++++\n test-run-command.c | 24 +++++\n 4 files changed, 344 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 2bf1aba..494e1f8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -3,6 +3,8 @@\n #include \"exec_cmd.h\"\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n+#include \"thread-utils.h\"\n+#include \"strbuf.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -864,3 +866,265 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n \tclose(cmd->out);\n \treturn finish_command(cmd);\n }\n+\n+struct parallel_processes {\n+\tvoid *data;\n+\n+\tint max_processes;\n+\tint nr_processes;\n+\n+\tget_next_task_fn get_next_task;\n+\tstart_failure_fn start_failure;\n+\treturn_value_fn return_value;\n+\n+\tstruct {\n+\t\tunsigned in_use : 1;\n+\t\tstruct child_process process;\n+\t\tstruct strbuf err;\n+\t} *children;\n+\t/*\n+\t * The struct pollfd is logically part of *children,\n+\t * but the system call expects it as its own array.\n+\t */\n+\tstruct pollfd *pfd;\n+\n+\tint output_owner;\n+\tstruct strbuf buffered_output; /* of finished children */\n+};\n+\n+void default_start_failure(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err)\n+{\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(&sb, \" %s\", cp->argv[i]);\n+\n+\tdie_errno(\"Starting a child failed:%s\", sb.buf);\n+}\n+\n+void default_return_value(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t int result)\n+{\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (!result)\n+\t\treturn;\n+\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(&sb, \" %s\", cp->argv[i]);\n+\n+\tdie_errno(\"A child failed with return code %d:%s\", result, sb.buf);\n+}\n+\n+static void pp_init(struct parallel_processes *pp,\n+\t\t\t\t\tint n, void *data,\n+\t\t\t\t\tget_next_task_fn get_next_task,\n+\t\t\t\t\tstart_failure_fn start_failure,\n+\t\t\t\t\treturn_value_fn return_value)\n+{\n+\tint i;\n+\n+\tif (n < 1)\n+\t\tn = online_cpus();\n+\n+\tpp->max_processes = n;\n+\tpp->data = data;\n+\tif (!get_next_task)\n+\t\tdie(\"BUG: you need to specify a get_next_task function\");\n+\tpp->get_next_task = get_next_task;\n+\n+\tpp->start_failure = start_failure ? start_failure : default_start_failure;\n+\tpp->return_value = return_value ? return_value : default_return_value;\n+\n+\tpp->nr_processes = 0;\n+\tpp->output_owner = 0;\n+\tpp->children = xcalloc(n, sizeof(*pp->children));\n+\tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n+\tstrbuf_init(&pp->buffered_output, 0);\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tstrbuf_init(&pp->children[i].err, 0);\n+\t\tpp->pfd[i].events = POLLIN;\n+\t\tpp->pfd[i].fd = -1;\n+\t}\n+}\n+\n+static void pp_cleanup(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < pp->max_processes; i++)\n+\t\tstrbuf_release(&pp->children[i].err);\n+\n+\tfree(pp->children);\n+\tfree(pp->pfd);\n+\tstrbuf_release(&pp->buffered_output);\n+}\n+\n+static void set_nonblocking(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFL);\n+\tif (flags < 0)\n+\t\twarning(\"Could not get file status flags, \"\n+\t\t\t\"output will be degraded\");\n+\telse if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n+\t\twarning(\"Could not set file status flags, \"\n+\t\t\t\"output will be degraded\");\n+}\n+\n+/* returns 1 if a process was started, 0 otherwise */\n+static int pp_start_one(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < pp->max_processes; i++)\n+\t\tif (!pp->children[i].in_use)\n+\t\t\tbreak;\n+\tif (i == pp->max_processes)\n+\t\tdie(\"BUG: bookkeeping is hard\");\n+\n+\tif (!pp->get_next_task(pp->data,\n+\t\t\t &pp->children[i].process,\n+\t\t\t &pp->children[i].err))\n+\t\treturn 1;\n+\n+\tif (start_command(&pp->children[i].process))\n+\t\tpp->start_failure(pp->data,\n+\t\t\t\t &pp->children[i].process,\n+\t\t\t\t &pp->children[i].err);\n+\n+\tset_nonblocking(pp->children[i].process.err);\n+\n+\tpp->nr_processes++;\n+\tpp->children[i].in_use = 1;\n+\tpp->pfd[i].fd = pp->children[i].process.err;\n+\treturn 0;\n+}\n+\n+static void pp_buffer_stderr(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\tpp_cleanup(pp);\n+\t\tdie_errno(\"poll\");\n+\t}\n+\n+\t/* Buffer output from all pipes. */\n+\tfor (i = 0; i < pp->max_processes; i++) {\n+\t\tif (pp->children[i].in_use &&\n+\t\t pp->pfd[i].revents & POLLIN)\n+\t\t\tif (strbuf_read_once(&pp->children[i].err,\n+\t\t\t\t\t pp->children[i].process.err, 0) < 0)\n+\t\t\t\tif (errno != EAGAIN)\n+\t\t\t\t\tdie_errno(\"read\");\n+\t}\n+}\n+\n+static void pp_output(struct parallel_processes *pp)\n+{\n+\tint i = pp->output_owner;\n+\tif (pp->children[i].in_use &&\n+\t pp->children[i].err.len) {\n+\t\tfputs(pp->children[i].err.buf, stderr);\n+\t\tstrbuf_reset(&pp->children[i].err);\n+\t}\n+}\n+\n+static void pp_collect_finished(struct parallel_processes *pp)\n+{\n+\tint i = 0;\n+\tpid_t pid;\n+\tint wait_status, code;\n+\tint n = pp->max_processes;\n+\n+\twhile (pp->nr_processes > 0) {\n+\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n+\t\tif (pid == 0)\n+\t\t\treturn;\n+\n+\t\tif (pid < 0)\n+\t\t\tdie_errno(\"wait\");\n+\n+\t\tfor (i = 0; i < pp->max_processes; i++)\n+\t\t\tif (pp->children[i].in_use &&\n+\t\t\t pid == pp->children[i].process.pid)\n+\t\t\t\tbreak;\n+\t\tif (i == pp->max_processes)\n+\t\t\tdie(\"BUG: found a child process we were not aware of\");\n+\n+\t\tif (strbuf_read(&pp->children[i].err,\n+\t\t\t\tpp->children[i].process.err, 0) < 0)\n+\t\t\tdie_errno(\"strbuf_read\");\n+\n+\t\tif (determine_return_value(wait_status, &code, &errno,\n+\t\t\t\t\t pp->children[i].process.argv[0]) < 0)\n+\t\t\terror(\"waitpid is confused (%s)\",\n+\t\t\t pp->children[i].process.argv[0]);\n+\n+\t\tpp->return_value(pp->data, &pp->children[i].process, code);\n+\n+\t\targv_array_clear(&pp->children[i].process.args);\n+\t\targv_array_clear(&pp->children[i].process.env_array);\n+\n+\t\tpp->nr_processes--;\n+\t\tpp->children[i].in_use = 0;\n+\t\tpp->pfd[i].fd = -1;\n+\n+\t\tif (i != pp->output_owner) {\n+\t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\t\t} else {\n+\t\t\tfputs(pp->children[i].err.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\n+\t\t\t/* Output all other finished child processes */\n+\t\t\tfputs(pp->buffered_output.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->buffered_output);\n+\n+\t\t\t/*\n+\t\t\t * Pick next process to output live.\n+\t\t\t * NEEDSWORK:\n+\t\t\t * For now we pick it randomly by doing a round\n+\t\t\t * robin. Later we may want to pick the one with\n+\t\t\t * the most output or the longest or shortest\n+\t\t\t * running process time.\n+\t\t\t */\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tif (pp->children[(pp->output_owner + i) % n].in_use)\n+\t\t\t\t\tbreak;\n+\t\t\tpp->output_owner = (pp->output_owner + i) % n;\n+\t\t}\n+\t}\n+}\n+\n+int run_processes_parallel(int n, void *data,\n+\t\t\t get_next_task_fn get_next_task,\n+\t\t\t start_failure_fn start_failure,\n+\t\t\t return_value_fn return_value)\n+{\n+\tstruct parallel_processes pp;\n+\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n+\n+\twhile (1) {\n+\t\twhile (pp.nr_processes < pp.max_processes &&\n+\t\t !pp_start_one(&pp))\n+\t\t\t; /* nothing */\n+\t\tif (!pp.nr_processes)\n+\t\t\tbreak;\n+\t\tpp_buffer_stderr(&pp);\n+\t\tpp_output(&pp);\n+\t\tpp_collect_finished(&pp);\n+\t}\n+\n+\tpp_cleanup(&pp);\n+\n+\treturn 0;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a..3807fd1 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -119,4 +119,40 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+/**\n+ * This callback should initialize the child process and preload the\n+ * error channel. The preloading of is useful if you want to have a message\n+ * printed directly before the output of the child process.\n+ * You MUST set stdout_to_stderr.\n+ *\n+ * Return 1 if the next child is ready to run.\n+ * Return 0 if there are no more tasks to be processed.\n+ */\n+typedef int (*get_next_task_fn)(void *data,\n+\t\t\t\tstruct child_process *cp,\n+\t\t\t\tstruct strbuf *err);\n+\n+typedef void (*start_failure_fn)(void *data,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err);\n+\n+typedef void (*return_value_fn)(void *data,\n+\t\t\t\tstruct child_process *cp,\n+\t\t\t\tint result);\n+\n+/**\n+ * Runs up to n processes at the same time. Whenever a process can be\n+ * started, the callback `get_next_task` is called to obtain the data\n+ * fed to the child process.\n+ *\n+ * The children started via this function run in parallel and their output\n+ * to stderr is buffered, while one of the children will directly output\n+ * to stderr.\n+ */\n+\n+int run_processes_parallel(int n, void *data,\n+\t\t\t get_next_task_fn,\n+\t\t\t start_failure_fn,\n+\t\t\t return_value_fn);\n+\n #endif\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 9acf628..49aa3db 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -47,4 +47,24 @@ test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<-EOF\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+EOF\n+\n+test_expect_success 'run_command runs in parallel' '\n+\ttest-run-command run-command-parallel-4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 89c7de2..94c6eee 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -10,9 +10,29 @@\n \n #include \"git-compat-util.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n+#include \"strbuf.h\"\n #include \n #include \n \n+static int number_callbacks;\n+int parallel_next(void *data,\n+\t\t struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tstruct child_process *d = data;\n+\tif (number_callbacks >= 4)\n+\t\treturn 0;\n+\n+\targv_array_pushv(&cp->args, d->argv);\n+\tcp->stdout_to_stderr = 1;\n+\tcp->no_stdin = 1;\n+\tcp->err = -1;\n+\tstrbuf_addf(err, \"preloaded output of a child\\n\");\n+\tnumber_callbacks++;\n+\treturn 1;\n+}\n+\n int main(int argc, char **argv)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n@@ -30,6 +50,10 @@ int main(int argc, char **argv)\n \tif (!strcmp(argv[1], \"run-command\"))\n \t\texit(run_command(&proc));\n \n+\tif (!strcmp(argv[1], \"run-command-parallel-4\"))\n+\t\texit(run_processes_parallel(4, &proc, parallel_next,\n+\t\t\t\t\t NULL, NULL));\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270550","messageId":"1442972732-12118-8-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 07/14] fetch_populated_submodules: use new parallel job processing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:25Z","receivedAt":"2015-09-23T01:45:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we enable parallel processing of submodules, this\nonly adds the possibility for it. So this change should not change\nany user facing behavior.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 119 +++++++++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 85 insertions(+), 34 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 1d64e57..d7c7a6e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -12,6 +12,7 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n #include \"blob.h\"\n+#include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n@@ -615,37 +616,82 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tconst char *work_tree;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint quiet;\n+\tint result;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err);\n+\n+void handle_submodule_fetch_start_err(void *data, struct child_process *cp, struct strbuf *err)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\tspf->result = 1;\n+}\n+\n+void handle_submodule_fetch_finish( void *data, struct child_process *cp, int retvalue)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\n+\tif (retvalue)\n+\t\tspf->result = 1;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n \t\t\t int quiet)\n {\n-\tint i, result = 0;\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *work_tree = get_git_work_tree();\n-\tif (!work_tree)\n+\tint i;\n+\tint max_parallel_jobs = 1;\n+\tstruct submodule_parallel_fetch spf = SPF_INIT;\n+\n+\tspf.work_tree = get_git_work_tree();\n+\tspf.command_line_option = command_line_option;\n+\tspf.quiet = quiet;\n+\tspf.prefix = prefix;\n+\n+\tif (!spf.work_tree)\n \t\tgoto out;\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \n-\targv_array_push(&argv, \"fetch\");\n+\targv_array_push(&spf.args, \"fetch\");\n \tfor (i = 0; i < options->argc; i++)\n-\t\targv_array_push(&argv, options->argv[i]);\n-\targv_array_push(&argv, \"--recurse-submodules-default\");\n+\t\targv_array_push(&spf.args, options->argv[i]);\n+\targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcp.env = local_repo_env;\n-\tcp.git_cmd = 1;\n-\tcp.no_stdin = 1;\n-\n \tcalculate_changed_submodule_paths();\n+\trun_processes_parallel(max_parallel_jobs, &spf,\n+\t\t\t get_next_submodule,\n+\t\t\t handle_submodule_fetch_start_err,\n+\t\t\t handle_submodule_fetch_finish);\n+\n+\targv_array_clear(&spf.args);\n+out:\n+\tstring_list_clear(&changed_submodule_paths, 1);\n+\treturn spf.result;\n+}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tint ret = 0;\n+\tstruct submodule_parallel_fetch *spf = data;\n \n-\tfor (i = 0; i < active_nr; i++) {\n+\tfor ( ; spf->count < active_nr; spf->count++) {\n \t\tstruct strbuf submodule_path = STRBUF_INIT;\n \t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n-\t\tconst struct cache_entry *ce = active_cache[i];\n+\t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n \n@@ -657,7 +703,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tsubmodule = submodule_from_name(null_sha1, ce->name);\n \n \t\tdefault_argv = \"yes\";\n-\t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n+\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n \t\t\tif (submodule &&\n \t\t\t submodule->fetch_recurse !=\n \t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n@@ -680,40 +726,45 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n-\t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_path, \"%s/%s\", work_tree, ce->name);\n+\t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n \t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", prefix, ce->name);\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n \t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n \t\tif (!git_dir)\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n-\t\t\tif (!quiet)\n-\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n-\t\t\tcp.dir = submodule_path.buf;\n-\t\t\targv_array_push(&argv, default_argv);\n-\t\t\targv_array_push(&argv, \"--submodule-prefix\");\n-\t\t\targv_array_push(&argv, submodule_prefix.buf);\n-\t\t\tcp.argv = argv.argv;\n-\t\t\tif (run_command(&cp))\n-\t\t\t\tresult = 1;\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n+\t\t\tchild_process_init(cp);\n+\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n+\t\t\tcp->env = local_repo_env;\n+\t\t\tcp->git_cmd = 1;\n+\t\t\tcp->no_stdin = 1;\n+\t\t\tcp->stdout_to_stderr = 1;\n+\t\t\tcp->err = -1;\n+\t\t\tif (!spf->quiet)\n+\t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n+\t\t\t\t\t spf->prefix, ce->name);\n+\t\t\targv_array_init(&cp->args);\n+\t\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\t\targv_array_push(&cp->args, default_argv);\n+\t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\t\t\tret = 1;\n \t\t}\n \t\tstrbuf_release(&submodule_path);\n \t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n+\t\tif (ret) {\n+\t\t\tspf->count++;\n+\t\t\treturn 1;\n+\t\t}\n \t}\n-\targv_array_clear(&argv);\n-out:\n-\tstring_list_clear(&changed_submodule_paths, 1);\n-\treturn result;\n+\treturn 0;\n }\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270553","messageId":"1442972732-12118-9-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 08/14] submodules: allow parallel fetching, add tests and documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:26Z","receivedAt":"2015-09-23T01:45:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This enables the work of the previous patches.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/fetch-options.txt | 7 +++++++\n builtin/fetch.c | 6 +++++-\n builtin/pull.c | 6 ++++++\n submodule.c | 3 +--\n submodule.h | 2 +-\n t/t5526-fetch-submodules.sh | 19 +++++++++++++++++++\n 6 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 45583d8..6b109f6 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -100,6 +100,13 @@ ifndef::git-pull[]\n \treference to a commit that isn't already in the local submodule\n \tclone.\n \n+-j::\n+--jobs=::\n+\tNumber of parallel children to be used for fetching submodules.\n+\tEach will fetch from different submodules, such that fetching many\n+\tsubmodules will be faster. By default submodules will be fetched\n+\tone at a time.\n+\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the '--recurse-submodules=no' option).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ee1f1a9..f28eac6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n+static int max_children = 1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n+\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\t\t N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n@@ -1217,7 +1220,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_populated_submodules(&options,\n \t\t\t\t\t\t submodule_prefix,\n \t\t\t\t\t\t recurse_submodules,\n-\t\t\t\t\t\t verbosity < 0);\n+\t\t\t\t\t\t verbosity < 0,\n+\t\t\t\t\t\t max_children);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 722a83c..f0af196 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -94,6 +94,7 @@ static int opt_force;\n static char *opt_tags;\n static char *opt_prune;\n static char *opt_recurse_submodules;\n+static char *max_children;\n static int opt_dry_run;\n static char *opt_keep;\n static char *opt_depth;\n@@ -177,6 +178,9 @@ static struct option pull_options[] = {\n \t\tN_(\"on-demand\"),\n \t\tN_(\"control recursive fetching of submodules\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU('j', \"jobs\", &max_children, N_(\"n\"),\n+\t\tN_(\"number of submodules pulled in parallel\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_BOOL(0, \"dry-run\", &opt_dry_run,\n \t\tN_(\"dry run\")),\n \tOPT_PASSTHRU('k', \"keep\", &opt_keep, NULL,\n@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\targv_array_push(&args, opt_prune);\n \tif (opt_recurse_submodules)\n \t\targv_array_push(&args, opt_recurse_submodules);\n+\tif (max_children)\n+\t\targv_array_push(&args, max_children);\n \tif (opt_dry_run)\n \t\targv_array_push(&args, \"--dry-run\");\n \tif (opt_keep)\ndiff --git a/submodule.c b/submodule.c\nindex d7c7a6e..fdaf3e4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -646,10 +646,9 @@ void handle_submodule_fetch_finish( void *data, struct child_process *cp, int re\n \n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet)\n+\t\t\t int quiet, int max_parallel_jobs)\n {\n \tint i;\n-\tint max_parallel_jobs = 1;\n \tstruct submodule_parallel_fetch spf = SPF_INIT;\n \n \tspf.work_tree = get_git_work_tree();\ndiff --git a/submodule.h b/submodule.h\nindex 5507c3d..cbc0003 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);\n void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet);\n+\t\t\t int quiet, int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 17759b1..1b4ce69 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -71,6 +71,16 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n+\tadd_upstream_commit &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules -j2 2>../actual.err\n+\t) &&\n+\ttest_must_be_empty actual.out &&\n+\ttest_i18ncmp expect.err actual.err\n+'\n+\n test_expect_success \"fetch alone only fetches superproject\" '\n \tadd_upstream_commit &&\n \t(\n@@ -140,6 +150,15 @@ test_expect_success \"--quiet propagates to submodules\" '\n \t! test -s actual.err\n '\n \n+test_expect_success \"--quiet propagates to parallel submodules\" '\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules -j 2 --quiet >../actual.out 2>../actual.err\n+\t) &&\n+\t! test -s actual.out &&\n+\t! test -s actual.err\n+'\n+\n test_expect_success \"--dry-run propagates to submodules\" '\n \tadd_upstream_commit &&\n \t(\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270551","messageId":"1442972732-12118-10-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 09/14] submodule-config: Untangle logic in parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:27Z","receivedAt":"2015-09-23T01:45:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"CC: Eric Sunshine \nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 74 +++++++++++++++++++++---------------------------------\n 1 file changed, 29 insertions(+), 45 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..afe0ea8 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!name_and_item_from_var(var, &name, &item))\n \t\treturn 0;\n \n-\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n-\t\t\tname.buf);\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 \n \tif (!strcmp(item.buf, \"path\")) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->path != NULL) {\n+\t\telse if (!me->overwrite && submodule->path != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tif (submodule->path)\n+\t\t\t\tcache_remove_path(me->cache, submodule);\n+\t\t\tfree((void *) submodule->path);\n+\t\t\tsubmodule->path = xstrdup(value);\n+\t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\n-\t\tif (submodule->path)\n-\t\t\tcache_remove_path(me->cache, submodule);\n-\t\tfree((void *) submodule->path);\n-\t\tstrbuf_addstr(&path, value);\n-\t\tsubmodule->path = strbuf_detach(&path, NULL);\n-\t\tcache_put_path(me->cache, submodule);\n \t} else if (!strcmp(item.buf, \"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-\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\n-\t\tsubmodule->fetch_recurse = parse_fetch_recurse(var, value,\n+\t\telse\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\tstruct strbuf ignore = STRBUF_INIT;\n-\t\tif (!me->overwrite && submodule->ignore != NULL) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->ignore != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!value) {\n-\t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n-\t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n+\t\telse if (strcmp(value, \"untracked\") &&\n+\t\t\t strcmp(value, \"dirty\") &&\n+\t\t\t strcmp(value, \"all\") &&\n+\t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tfree((void *) submodule->ignore);\n+\t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->ignore);\n-\t\tstrbuf_addstr(&ignore, value);\n-\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n \t} else if (!strcmp(item.buf, \"url\")) {\n-\t\tstruct strbuf url = STRBUF_INIT;\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url != NULL) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n-\t\t\tgoto release_return;\n+\t\t} else {\n+\t\t\tfree((void *) submodule->url);\n+\t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->url);\n-\t\tstrbuf_addstr(&url, value);\n-\t\tsubmodule->url = strbuf_detach(&url, NULL);\n \t}\n \n-release_return:\n \tstrbuf_release(&name);\n \tstrbuf_release(&item);\n \n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270559","messageId":"1442972732-12118-11-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 10/14] submodule config: keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:28Z","receivedAt":"2015-09-23T01:45:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 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..8b8c7d1 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.5.0.272.ga84127c.dirty\n"},{"id":"270554","messageId":"1442972732-12118-12-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 11/14] git submodule update: cmd_update_recursive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:29Z","receivedAt":"2015-09-23T01:45:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Split the recursion part out to its own function, this allow us\nin a later patch to convert cmd_update in C.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 47 ++++++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8b0eb9a..ea3260e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -582,6 +582,31 @@ cmd_deinit()\n \tdone\n }\n \n+\n+cmd_update_recursive()\n+{\n+\tif test -n \"$recursive\"\n+\tthen\n+\t\t(\n+\t\t\tprefix=\"$prefix$sm_path/\"\n+\t\t\tclear_local_git_env\n+\t\t\tcd \"$sm_path\" &&\n+\t\t\teval cmd_update\n+\t\t)\n+\t\tres=$?\n+\t\tif test $res -gt 0\n+\t\tthen\n+\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n+\t\t\tif test $res -eq 1\n+\t\t\tthen\n+\t\t\t\terr=\"${err};$die_msg\"\n+\t\t\telse\n+\t\t\t\tdie_with_status $res \"$die_msg\"\n+\t\t\tfi\n+\t\tfi\n+\tfi\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -790,27 +815,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tfi\n \t\tfi\n \n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tclear_local_git_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_update\n-\t\t\t)\n-\t\t\tres=$?\n-\t\t\tif test $res -gt 0\n-\t\t\tthen\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n-\t\t\t\tif test $res -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\t\tcontinue\n-\t\t\t\telse\n-\t\t\t\t\tdie_with_status $res \"$die_msg\"\n-\t\t\t\tfi\n-\t\t\tfi\n-\t\tfi\n+\t\tcmd_update_recursive\n \tdone\n \n \tif test -n \"$err\"\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270552","messageId":"1442972732-12118-13-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 12/14] git submodule update: cmd_update_clone","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:30Z","receivedAt":"2015-09-23T01:45:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Split the cloning part out to its own function,\nthis allow us in a later patch to convert cmd_update in C.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ea3260e..7f11158 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -607,6 +607,24 @@ cmd_update_recursive()\n \tfi\n }\n \n+cmd_update_clone()\n+{\n+\tcommand=\"git checkout $subforce -q\"\n+\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n+\n+\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\n+\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\tthen\n+\t\tsay \"$say_msg\"\n+\telse\n+\t\terr=\"${err};$die_msg\"\n+\t\treturn\n+\tfi\n+\tcmd_update_recursive\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -680,7 +698,6 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n-\t\t\tsubsha1=\n+\t\t\tcmd_update_clone\n+\t\t\tcontinue\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270556","messageId":"1442972732-12118-14-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 13/14] git submodule update: cmd_update_fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:31Z","receivedAt":"2015-09-23T01:45:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Split the fetching part out to its own function,\nthis allow us in a later patch to convert cmd_update in C.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 164 ++++++++++++++++++++++++++++---------------------------\n 1 file changed, 84 insertions(+), 80 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 7f11158..a1bc8d5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -625,6 +625,89 @@ cmd_update_clone()\n \tcmd_update_recursive\n }\n \n+cmd_update_fetch()\n+{\n+\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\tgit rev-parse --verify HEAD) ||\n+\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\n+\tif test -n \"$remote\"\n+\tthen\n+\t\tif test -z \"$nofetch\"\n+\t\tthen\n+\t\t\t# Fetch remote before determining tracking $sha1\n+\t\t\t(clear_local_git_env; cd \"$sm_path\" && git-fetch) ||\n+\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n+\t\tfi\n+\t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n+\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n+\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n+\tfi\n+\n+\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n+\tthen\n+\t\tsubforce=$force\n+\t\t# If we don't already have a -f flag and the submodule has never been checked out\n+\t\tif test -z \"$subsha1\" && test -z \"$force\"\n+\t\tthen\n+\t\t\tsubforce=\"-f\"\n+\t\tfi\n+\n+\t\tif test -z \"$nofetch\"\n+\t\tthen\n+\t\t\t# Run fetch only if $sha1 isn't present or it\n+\t\t\t# is not reachable from a ref.\n+\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n+\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n+\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\t\tfi\n+\n+\t\tmust_die_on_failure=\n+\t\tcase \"$update_module\" in\n+\t\tcheckout)\n+\t\t\tcommand=\"git checkout $subforce -q\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n+\t\t\t;;\n+\t\trebase)\n+\t\t\tcommand=\"git rebase\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\tmerge)\n+\t\t\tcommand=\"git merge\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\t!*)\n+\t\t\tcommand=\"${update_module#!}\"\n+\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$prefix\\$sm_path'\")\"\n+\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$prefix\\$sm_path': '\\$command \\$sha1'\")\"\n+\t\t\tmust_die_on_failure=yes\n+\t\t\t;;\n+\t\t*)\n+\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n+\t\tesac\n+\n+\t\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\t\tthen\n+\t\t\tsay \"$say_msg\"\n+\t\telif test -n \"$must_die_on_failure\"\n+\t\tthen\n+\t\t\tdie_with_status 2 \"$die_msg\"\n+\t\telse\n+\t\t\terr=\"${err};$die_msg\"\n+\t\t\treturn\n+\t\tfi\n+\tfi\n+\n+\tcmd_update_recursive\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -743,88 +826,9 @@ Maybe you want to use 'update --init'?\")\"\n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n \t\t\tcmd_update_clone\n-\t\t\tcontinue\n \t\telse\n-\t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\tgit rev-parse --verify HEAD) ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\t\tcmd_update_fetch\n \t\tfi\n-\n-\t\tif test -n \"$remote\"\n-\t\tthen\n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Fetch remote before determining tracking $sha1\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" && git-fetch) ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n-\t\t\tfi\n-\t\t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n-\t\t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n-\n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n-\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$prefix\\$sm_path'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$prefix\\$sm_path': '\\$command \\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n-\t\t\tesac\n-\n-\t\t\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n-\n-\t\tcmd_update_recursive\n \tdone\n \n \tif test -n \"$err\"\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270555","messageId":"1442972732-12118-15-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1442972732-12118-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 14/14] Rewrite submodule update in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T01:45:32Z","receivedAt":"2015-09-23T01:45:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This will make parallelisation easier in a followup patch. This is just\na translation from shell to C, hopefully not introducing any bugs.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 251 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 135 +-----------------------\n 2 files changed, 254 insertions(+), 132 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..b79117a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,256 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct module_update_data {\n+\tstruct module_list list;\n+\tint count;\n+\n+\tstruct pathspec pathspec;\n+\n+\tint no_fetch;\n+\tint force;\n+\tint update;\n+\tint quiet;\n+\tint recursive;\n+\tint remote;\n+\tchar *prefix;\n+\tchar *reference;\n+\tchar *depth;\n+\n+\tstruct argv_array args;\n+\n+\tint result;\n+};\n+\n+static void module_update_data_init(struct module_update_data *mud)\n+{\n+\tmud->list.entries = NULL;\n+\tmud->list.alloc = 0;\n+\tmud->list.nr = 0;\n+\n+\tmemset(&mud->pathspec, 0, sizeof(mud->pathspec));\n+\n+\tmud->count = 0;\n+\tmud->no_fetch = 0;\n+\tmud->force = 0;\n+\tmud->update = 0;\n+\tmud->quiet = 0;\n+\tmud->remote = 0;\n+\tmud->recursive = 0;\n+\tmud->result = 0;\n+\n+\tmud->prefix = NULL;\n+\tmud->reference = NULL;\n+\tmud->depth = NULL;\n+\n+\targv_array_init(&mud->args);\n+}\n+\n+static int update_next_task(void *data,\n+\t\t struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tint i;\n+\tstruct module_update_data *mud = data;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *displaypath;\n+\n+\tfor (; mud->count < mud->list.nr; mud->count++) {\n+\t\tconst char *update_module;\n+\t\tconst char *sm_gitdir;\n+\t\tconst struct submodule *sub;\n+\t\tconst struct cache_entry *ce = mud->list.entries[mud->count];\n+\n+\t\tdisplaypath = relative_path(ce->name, mud->prefix, &sb);\n+\t\tstrbuf_reset(&sb);\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\",\n+\t\t\t\t displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (!sub) {\n+\t\t\tmud->result = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\tswitch (mud->update) {\n+\t\tcase 'r':\n+\t\t\tupdate_module = \"rebase\";\n+\t\t\tbreak;\n+\t\tcase 'c':\n+\t\t\tupdate_module = \"checkout\";\n+\t\t\tbreak;\n+\t\tcase 'm':\n+\t\t\tupdate_module = \"merge\";\n+\t\t\tbreak;\n+\t\tcase 0:\n+\t\t\t/* not specified by command line */\n+\t\t\tif (sub->update)\n+\t\t\t\tupdate_module = sub->update;\n+\t\t\telse\n+\t\t\t\tupdate_module = \"checkout\";\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"BUG: update mode not covered\");\n+\t\t}\n+\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!sub->url) {\n+\t\t\t/*\n+\t\t\t * Only mention uninitialized submodules when its\n+\t\t\t * path have been specified\n+\t\t\t */\n+\t\t\tif (!mud->pathspec.nr)\n+\t\t\t\tcontinue;\n+\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t _(\"Submodule path '%s' not initialized \\n\"\n+\t\t\t\t \"Maybe you want to use 'update --init'?\"),\n+\t\t\t\t displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n+\n+\t\tchild_process_init(cp);\n+\t\tfor (i = 0; local_repo_env[i]; i++)\n+\t\t\targv_array_pushf(&cp->env_array, \"%s\", local_repo_env[i]);\n+\n+\t\targv_array_pushf(&cp->env_array, \"displaypath=%s\", displaypath);\n+\t\targv_array_pushf(&cp->env_array, \"sm_path=%s\", sub->path);\n+\t\targv_array_pushf(&cp->env_array, \"name=%s\", sub->name);\n+\t\targv_array_pushf(&cp->env_array, \"url=%s\", sub->url);\n+\t\targv_array_pushf(&cp->env_array, \"sha1=%s\", sha1_to_hex(ce->sha1));\n+\t\targv_array_pushf(&cp->env_array, \"update_module=%s\", update_module);\n+\n+\t\tcp->git_cmd = 1;\n+\t\tcp->no_stdin = 1;\n+\t\tcp->stdout_to_stderr = 1;\n+\t\tcp->err = -1;\n+\t\targv_array_init(&cp->args);\n+\t\targv_array_push(&cp->args, \"submodule\");\n+\t\tif (!file_exists(sm_gitdir))\n+\t\t\targv_array_push(&cp->args, \"update_clone\");\n+\t\telse\n+\t\t\targv_array_push(&cp->args, \"update_fetch\");\n+\n+\t\targv_array_pushf(&cp->args, \"%s\", ce->name);\n+\t\tmud->count++;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+void update_subcommand_failure(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct module_update_data *mud = data;\n+\tstrbuf_addf(err, _(\"Could not start child process\"));\n+\tmud->result = 1;\n+}\n+\n+void update_child_return(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t int result)\n+{\n+\tstruct module_update_data *mud = data;\n+\tmud->result = 1;\n+}\n+\n+static int module_update(int argc, const char **argv, const char *prefix)\n+{\n+\tint init;\n+\tstruct module_update_data mud;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_BOOL('i', \"init\", &init,\n+\t\t\tN_(\"Initialize the submodule if not yet done\")),\n+\t\tOPT_BOOL(0, \"remote\", &mud.remote,\n+\t\t\tN_(\"Update the submodule to the remote branch instead \"\n+\t\t\t \"of the superprojects specification\")),\n+\t\tOPT_BOOL('N', \"no-fetch\", &mud.no_fetch,\n+\t\t\tN_(\"Don’t fetch new objects from the remote site.\")),\n+\t\tOPT_BOOL('f', \"force\", &mud.force,\n+\t\t\tN_(\"Ignore local changes in submodules\")),\n+\t\tOPT_CMDMODE('r', \"rebase\", &mud.update,\n+\t\t\tN_(\"Rebase local changes in submodules\"), 'r'),\n+\t\tOPT_CMDMODE('m', \"merge\", &mud.update,\n+\t\t\tN_(\"Merge local changes in submodules\"), 'm'),\n+\t\tOPT_CMDMODE(0, \"checkout\", &mud.update,\n+\t\t\tN_(\"Checkout to a detached HEAD in submodules\"), 'c'),\n+\t\tOPT_BOOL(0, \"recursive\", &mud.recursive,\n+\t\t\tN_(\"Update nested submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &mud.reference, \"\",\n+\t\t\tN_(\"Use the local reference repository \"\n+\t\t\t \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &mud.depth, \"\",\n+\t\t\tN_(\"Create a shallow clone truncated to the \"\n+\t\t\t \"specified number of revisions\")),\n+\t\tOPT__QUIET(&mud.quiet, N_(\"be quiet\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper list [--prefix=] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\tmodule_update_data_init(&mud);\n+\tgitmodules_config();\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (mud.force)\n+\t\targv_array_push(&mud.args, \"force=1\");\n+\tif (mud.quiet)\n+\t\targv_array_push(&mud.args, \"GIT_QUIET=1\");\n+\tif (mud.recursive)\n+\t\targv_array_push(&mud.args, \"recursive=1\");\n+\tif (mud.prefix)\n+\t\targv_array_pushf(&mud.args, \"prefix=%s\", mud.prefix);\n+\tif (mud.reference)\n+\t\targv_array_pushf(&mud.args, \"reference=%s\", mud.reference);\n+\tif (mud.depth)\n+\t\targv_array_pushf(&mud.args, \"depth=%s\", mud.depth);\n+\n+\tif (module_list_compute(argc, argv, prefix, &mud.pathspec, &mud.list) < 0)\n+\t\treturn 1;\n+\n+\tif (init) {\n+\t\tconst char **argv_init = xmalloc((2 + mud.list.nr) * sizeof(char*));\n+\t\tint argc = 0, i, code;\n+\t\targv_init[argc++] = \"submodule\";\n+\t\targv_init[argc++] = \"init\";\n+\n+\t\tfor (i = 0; i < mud.list.nr; i++) {\n+\t\t\tconst struct cache_entry *ce = mud.list.entries[i];\n+\t\t\targv_init[argc++] = ce->name;\n+\t\t}\n+\t\tcode = run_command_v_opt(argv_init, RUN_GIT_CMD);\n+\t\t\tif (code)\n+\t\t\t\treturn code;\n+\t}\n+\n+\trun_processes_parallel(1, &mud,\n+\t\t\t update_next_task,\n+\t\t\t update_subcommand_failure,\n+\t\t\t update_child_return);\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +514,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update\", module_update}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a1bc8d5..63e9b3b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -640,6 +640,7 @@ cmd_update_fetch()\n \t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\tfi\n \t\tremote_name=$(clear_local_git_env; cd \"$sm_path\" && get_default_remote)\n+\t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n \t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n@@ -715,137 +716,7 @@ cmd_update_fetch()\n #\n cmd_update()\n {\n-\t# parse $args after \"submodule ... update\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n-\t\t\t;;\n-\t\t-i|--init)\n-\t\t\tinit=1\n-\t\t\t;;\n-\t\t--remote)\n-\t\t\tremote=1\n-\t\t\t;;\n-\t\t-N|--no-fetch)\n-\t\t\tnofetch=1\n-\t\t\t;;\n-\t\t-f|--force)\n-\t\t\tforce=$1\n-\t\t\t;;\n-\t\t-r|--rebase)\n-\t\t\tupdate=\"rebase\"\n-\t\t\t;;\n-\t\t--reference)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\treference=\"--reference=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--reference=*)\n-\t\t\treference=\"$1\"\n-\t\t\t;;\n-\t\t-m|--merge)\n-\t\t\tupdate=\"merge\"\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\t;;\n-\t\t--checkout)\n-\t\t\tupdate=\"checkout\"\n-\t\t\t;;\n-\t\t--depth)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tdepth=\"--depth=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--depth=*)\n-\t\t\tdepth=$1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tif test -n \"$init\"\n-\tthen\n-\t\tcmd_init \"--\" \"$@\" || return\n-\tfi\n-\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n-\terr=\n-\twhile read mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\turl=$(git config submodule.\"$name\".url)\n-\t\tbranch=$(get_submodule_config \"$name\" branch master)\n-\t\tif ! test -z \"$update\"\n-\t\tthen\n-\t\t\tupdate_module=$update\n-\t\telse\n-\t\t\tupdate_module=$(git config submodule.\"$name\".update)\n-\t\t\tif test -z \"$update_module\"\n-\t\t\tthen\n-\t\t\t\tupdate_module=\"checkout\"\n-\t\t\tfi\n-\t\tfi\n-\n-\t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n-\n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tcmd_update_clone\n-\t\telse\n-\t\t\tcmd_update_fetch\n-\t\tfi\n-\tdone\n-\n-\tif test -n \"$err\"\n-\tthen\n-\t\tOIFS=$IFS\n-\t\tIFS=';'\n-\t\tfor e in $err\n-\t\tdo\n-\t\t\tif test -n \"$e\"\n-\t\t\tthen\n-\t\t\t\techo >&2 \"$e\"\n-\t\t\tfi\n-\t\tdone\n-\t\tIFS=$OIFS\n-\t\texit 1\n-\tfi\n-\t}\n+\tgit submodule--helper update ${prefix:+--prefix \"$prefix\"} \"$@\"\n }\n \n set_name_rev () {\n@@ -1243,7 +1114,7 @@ cmd_sync()\n while test $# != 0 && test -z \"$command\"\n do\n \tcase \"$1\" in\n-\tadd | foreach | init | deinit | update | status | summary | sync)\n+\tadd | foreach | init | deinit | update | update_fetch | update_clone | status | summary | sync)\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\n-- \n2.5.0.272.ga84127c.dirty\n"},{"id":"270561","messageId":"xmqqeghpzm0y.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1442972732-12118-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T06:29:17Z","receivedAt":"2015-09-23T06:29:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> run-command.c | 264 +++++++++++++++++++++++++++++++++++++++++++++++++\n> run-command.h | 36 +++++++\n> t/t0061-run-command.sh | 20 ++++\n> test-run-command.c | 24 +++++\n> 4 files changed, 344 insertions(+)\n\nI think we are almost there, but there were still a few more \"huh?\"\nin this patch.\n\n> +/* returns 1 if a process was started, 0 otherwise */\n\nThis claim is dubious.\n\nThe last four lines of this function marks the i-th slot as in_use,\nand increments nr_processes, so that exit codepath clearly is about\n\"a process was started\" and should be returning 1, but the code\nreturns 0, which looks incorrect.\n\n> +static int pp_start_one(struct parallel_processes *pp)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < pp->max_processes; i++)\n> +\t\tif (!pp->children[i].in_use)\n> +\t\t\tbreak;\n> +\tif (i == pp->max_processes)\n> +\t\tdie(\"BUG: bookkeeping is hard\");\n> +\n> +\tif (!pp->get_next_task(pp->data,\n> +\t\t\t &pp->children[i].process,\n> +\t\t\t &pp->children[i].err))\n> +\t\treturn 1;\n\nAnd this one, when get_next_task() says \"nothing more to do\", is\nclearly \"we returned without starting anything\", so according to the\ncomment it should be returning 0, but the code returns 1, which\nlooks incorrect.\n\n> +\tif (start_command(&pp->children[i].process))\n> +\t\tpp->start_failure(pp->data,\n> +\t\t\t\t &pp->children[i].process,\n> +\t\t\t\t &pp->children[i].err);\n\nWhat should happen if start_failure returns without dying?\nShouldn't this function return something, without doing the\nremainder of it? i.e.\n\n\tif (start_command(...)) {\n\t\tpp->start_failur(...);\n return SOMETHING;\n\t}\n\nAccording to the comment at the beginning, that SOMETHING ought to\nbe 0, because we did not spawn anything, i.e. we did not increment\nnr_processes.\n\nBut don't make that change yet; I do not think it is a great\ninterface to say \"did we or did we not add a new process?\" anyway\n(see below).\n\n> +\tset_nonblocking(pp->children[i].process.err);\n> +\n> +\tpp->nr_processes++;\n> +\tpp->children[i].in_use = 1;\n> +\tpp->pfd[i].fd = pp->children[i].process.err;\n> +\treturn 0;\n\nAnd this is \"we spawned\" that ought to have returned 1.\n\nPerhaps the comment at the beginning is wrong, as the code\nconsistently does the opposite of what the comment says.\n\nBut it does not really matter, as I do not think this should be\nreturning \"did we or did we not start a new process?\" (see below).\n\n> +}\n\n> +int run_processes_parallel(int n, void *data,\n> +\t\t\t get_next_task_fn get_next_task,\n> +\t\t\t start_failure_fn start_failure,\n> +\t\t\t return_value_fn return_value)\n> +{\n> +\tstruct parallel_processes pp;\n> +\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n> +\n> +\twhile (1) {\n> +\t\twhile (pp.nr_processes < pp.max_processes &&\n> +\t\t !pp_start_one(&pp))\n> +\t\t\t; /* nothing */\n> +\t\tif (!pp.nr_processes)\n> +\t\t\tbreak;\n\nThis inner loop is why I think \"did we or did we not spawn a new\nprocess?\" is not a great interface.\n\nThe reason why it is not a great interface is because there are two\npossible reasons why pp_start_one() does not spawn a new process,\nand this caller wants to behave differently depending on why it did\nnot spawn a new process. They are:\n\n * get_next_task() truly ran out of things to do.\n\n * get_next_task() gave us a task, but it did not start, and\n start_failure was set not to die (e.g. the function can be used\n to tell the next_task machinery that it needs to return a\n replacement task for the one that failed to run. That way, upon\n next call to get_next_task, a replacement task can run instead of\n the old one that failed).\n\nFor the former, we want to stop looping, for the latter, we\ndefinitely do want to keep looping, as we want to make another call\nto get_next_task() to grab the replacement task for the one that\njust failed.\n\nSo I think it makes more sense to define the meaning of the return\nvalue from pp_start_one() differently from the way this patch\ndefines. \"Return 0 when we truly ran out of things to do, otherwise\nreturn non-zero\", for example, would make more sense. The return\nvalue does not tell you if the call resulted in one more process,\nbut that is not any loss, as you can look at pp.nr_processes\nyourself if you really cared.\n\nWith that, the above caller could be updated, with optional gradual\nramp_up, like so:\n\n\t#define RAMP_UP_LIMIT 2\n\n\twhile (1) {\n\t\tint ramp_up;\n\t\tint no_more_task;\n\n\t\tfor (no_more_task = 0, ramp_up = RAMP_UP_LIMIT;\n !no_more_task && ramp_up && pp.nr_processes < pp.max_processes;\n\t\t ramp_up--)\n\t\t\tif (!pp_start_one(&pp))\n\t\t\t\tno_more_task = 1;\n\n\t\tif (!pp.nr_processes && no_more_task)\n\t\t\tbreak;\n\nIf you prefer to swamp the system with a thundering herd at the\nbeginning, you can define RAMP_UP_LIMIT to really a high value\ninstead, e.g. \"#define RAMPUP_LIMIT pp.max_processes\". I however\nwould not recommend it because doing so would hurt the perceived\nlatency at the beginning.\n\nAfter the system goes into a steady state, how you set RAMP_UP_LIMIT\nwould not make that much difference, as your slots should be almost\nalways full and you will be replenishing an open slot with a single\ntask as each running task finishes, and you would not be running\nmore than one pp_start_one() at a time anyway.\n\n> +\t\tpp_buffer_stderr(&pp);\n> +\t\tpp_output(&pp);\n> +\t\tpp_collect_finished(&pp);\n> +\t}\n> +\n> +\tpp_cleanup(&pp);\n> +\n> +\treturn 0;\n> +}\n"},{"id":"270562","messageId":"xmqqa8sdzl6a.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1442972732-12118-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T06:47:41Z","receivedAt":"2015-09-23T06:47:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +static void pp_buffer_stderr(struct parallel_processes *pp)\n> +{\n> +\tint i;\n> +\n> +\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n> +\t\tif (errno == EINTR)\n> +\t\t\tcontinue;\n> +\t\tpp_cleanup(pp);\n> +\t\tdie_errno(\"poll\");\n> +\t}\n> +\n> +\t/* Buffer output from all pipes. */\n> +\tfor (i = 0; i < pp->max_processes; i++) {\n> +\t\tif (pp->children[i].in_use &&\n> +\t\t pp->pfd[i].revents & POLLIN)\n> +\t\t\tif (strbuf_read_once(&pp->children[i].err,\n> +\t\t\t\t\t pp->children[i].process.err, 0) < 0)\n> +\t\t\t\tif (errno != EAGAIN)\n> +\t\t\t\t\tdie_errno(\"read\");\n> +\t}\n> +}\n\nI think it is a good thing that the caller is passing the whole pp\nto this function. One thing you may want to consider is to adjust\nthe poll(2) timeout longer when the process slots are full.\n\nThere is nothing you can gain by returning early due to timeout\nwithout doing anything from this function when you know you cannot\nstart a new process (here, I am assuming that your poll(2) would be\nunblocked for a disconnect when one of the processes exits, letting\nyou return and letting the caller call collect_finished(), which in\nturn would allow us to make some progress).\n\nOn the other hand, during the early ramp-up period, you may want to\nuse a shorter poll(2) timeout to give the caller a chance to spawn\nmore processes sooner. But that falls into performance tuning that\ncan and should be left to a later follow-up patch after we get the\nbasic machinery right.\n"},{"id":"270577","messageId":"xmqq6131yyef.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"xmqqa8sdzl6a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T14:59:36Z","receivedAt":"2015-09-23T14:59:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> +static void pp_buffer_stderr(struct parallel_processes *pp)\n>> +{\n>> +\tint i;\n>> +\n>> +\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n>> +\t\tif (errno == EINTR)\n>> +\t\t\tcontinue;\n>> +\t\tpp_cleanup(pp);\n>> +\t\tdie_errno(\"poll\");\n>> +\t}\n>> +\n>> +\t/* Buffer output from all pipes. */\n>> +\tfor (i = 0; i < pp->max_processes; i++) {\n>> +\t\tif (pp->children[i].in_use &&\n>> +\t\t pp->pfd[i].revents & POLLIN)\n>> +\t\t\tif (strbuf_read_once(&pp->children[i].err,\n>> +\t\t\t\t\t pp->children[i].process.err, 0) < 0)\n>> +\t\t\t\tif (errno != EAGAIN)\n>> +\t\t\t\t\tdie_errno(\"read\");\n>> +\t}\n>> +}\n>\n> I think it is a good thing that the caller is passing the whole pp\n> to this function. One thing you may want to consider is to adjust\n> the poll(2) timeout longer when the process slots are full.\n> ...\n> But that falls into performance tuning that\n> can and should be left to a later follow-up patch after we get the\n> basic machinery right.\n\nJust to make sure there is no misunderstanding, just like I prefer\n\"start one\" over \"start as many as possible\" in order to give\nscheduling decision to the calling loop, I would expect that such a\nfollow-up performance tuning would be done by adding another field\nto pp tht lets the caller of this function, the overall scheduling\nloop, to tweak the timeout used, without this function making the\ndecision locallly.\n\nThanks.\n"},{"id":"270581","messageId":"CAGZ79kYaqFRPfRORbknTyez5u0d6_BD0d5wBtAnjSkb4sUBUBA@mail.gmail.com","threadId":"40409","inReplyTo":"xmqqeghpzm0y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T17:53:07Z","receivedAt":"2015-09-23T17:53:07Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 22, 2015 at 11:29 PM, Junio C Hamano wrote:\n\n>\n> And this one, when get_next_task() says \"nothing more to do\", is\n> clearly \"we returned without starting anything\", so according to the\n> comment it should be returning 0, but the code returns 1, which\n> looks incorrect.\n>\n>> + if (start_command(&pp->children[i].process))\n>> + pp->start_failure(pp->data,\n>> + &pp->children[i].process,\n>> + &pp->children[i].err);\n>\n> What should happen if start_failure returns without dying?\n> Shouldn't this function return something, without doing the\n> remainder of it? i.e.\n>\n> if (start_command(...)) {\n> pp->start_failur(...);\n> return SOMETHING;\n> }\n\nRight, I forgot about that code path as I was blinded by the obviousness\n(\"If you cannot start a process, of course we'll die\").\n\nSo for this SOMETHING we need to decide if it should signal\nthat an immediate retry can be done. But then we could perform the\nimmediate retry ourselves:\n\nstartnewtask:\n if (!pp->get_next_task(pp->data,\n &pp->children[i].process,\n &pp->children[i].err))\n return 0;\n\n if (start_command(&pp->children[i].process)) {\n pp->start_failure(pp->data,\n &pp->children[i].process,\n &pp->children[i].err);\n goto startnewtask;\n }\n\nBut this could result in an endless loop.\nEven if we would decide to return to the caller run_processes_parallel\nand let them decide to try again, this version of the patch may produce an\ninfinite loop there.\n\nThe other alternative would be to make SOMETHING signal to not\nimmediately try again. (\"We failed to start a child process, give it some time\nby doing the poll/output and try again\")\n\nThis however could not finish all workloads reliably as we may fail to start\nthe first child, such that there are 0 children processes running and the\ncontrol loop in run_processes_parallel shuts down the whole parallel processor.\n\nSo for now I'd lean on having the SOMETHING be the same boolean as\na successful start (failure -1, successful start 1, no more pending work 0)\nand the difference between -1 and 1 can be sorted out in a later patch, which\nintroduces workloads with failing children.\n\n>> +\n>> + while (1) {\n>> + while (pp.nr_processes < pp.max_processes &&\n>> + !pp_start_one(&pp))\n>> + ; /* nothing */\n>> + if (!pp.nr_processes)\n>> + break;\n>\n> This inner loop is why I think \"did we or did we not spawn a new\n> process?\" is not a great interface.\n\nRight, we actually need to return whether we have nothing more to do\n(\"Don't even try to call me again\") or if we did something useful and expect\nto do more useful things in the next call. (Either starting anew command or\nfinding out it failed).\n\nThis would be indicated by the -1/1/0 return signals.\n\n>\n> The reason why it is not a great interface is because there are two\n> possible reasons why pp_start_one() does not spawn a new process,\n> and this caller wants to behave differently depending on why it did\n> not spawn a new process. They are:\n>\n> * get_next_task() truly ran out of things to do.\n>\n> * get_next_task() gave us a task, but it did not start, and\n> start_failure was set not to die (e.g. the function can be used\n> to tell the next_task machinery that it needs to return a\n> replacement task for the one that failed to run. That way, upon\n> next call to get_next_task, a replacement task can run instead of\n> the old one that failed).\n>\n> For the former, we want to stop looping, for the latter, we\n> definitely do want to keep looping, as we want to make another call\n> to get_next_task() to grab the replacement task for the one that\n> just failed.\n>\n> So I think it makes more sense to define the meaning of the return\n> value from pp_start_one() differently from the way this patch\n> defines. \"Return 0 when we truly ran out of things to do, otherwise\n> return non-zero\", for example, would make more sense.\n\nok, we have the same opinion. I just documented poorly.\n\n> The return\n> value does not tell you if the call resulted in one more process,\n> but that is not any loss, as you can look at pp.nr_processes\n> yourself if you really cared.\n>\n> With that, the above caller could be updated, with optional gradual\n> ramp_up, like so:\n>\n> #define RAMP_UP_LIMIT 2\n>\n> while (1) {\n> int ramp_up;\n> int no_more_task;\n>\n> for (no_more_task = 0, ramp_up = RAMP_UP_LIMIT;\n> !no_more_task && ramp_up && pp.nr_processes < pp.max_processes;\n> ramp_up--)\n> if (!pp_start_one(&pp))\n> no_more_task = 1;\n\nI would not have the no_more_task variable, but just reuse\nramp_up and set it to zero in case of !pp_start_one(&pp).\n\nI am not sure if the ramp up machinery is really needed.\nI modified the test-run-command test function to start up to 400 processes.\n(Most people will use less than 400 processes in the next 5 years), and run\njust as in t0061:\n\n ./test-run-command run-command-parallel-400 sh -c \"printf\n\\\"%s\\n%s\\n\\\" Hello World\"\n\nThe output felt immediate (not slowed down or anything). The numbers seem to\nsupport that\n\n real 0m0.110s\n user 0m0.045s\n sys 0m0.366s\n\nAny delay below 0.1 second cannot really be perceived by a human. You\ncan sure tell\na difference of 0.1 second in say 2 acoustic signals or light flashes,\nbut you cannot\ntell that the output \"was slow\". So IMHO the ramp up machinery doesn't\nhave a high\npriority for now.\n\n\n>\n> if (!pp.nr_processes && no_more_task)\n> break;\n>\n> If you prefer to swamp the system with a thundering herd at the\n> beginning, you can define RAMP_UP_LIMIT to really a high value\n> instead, e.g. \"#define RAMPUP_LIMIT pp.max_processes\". I however\n> would not recommend it because doing so would hurt the perceived\n> latency at the beginning.\n>\n> After the system goes into a steady state, how you set RAMP_UP_LIMIT\n> would not make that much difference, as your slots should be almost\n> always full and you will be replenishing an open slot with a single\n> task as each running task finishes, and you would not be running\n> more than one pp_start_one() at a time anyway.\n\nYeah there we could have a simple\n\n if (pp->nr_processes == pp->max_processes)\n poll_timeout = 5 seconds\n else\n poll_timeout = 10..100 milliseconds\n\n>\n>> + pp_buffer_stderr(&pp);\n>> + pp_output(&pp);\n>> + pp_collect_finished(&pp);\n>> + }\n>> +\n>> + pp_cleanup(&pp);\n>> +\n>> + return 0;\n>> +}\n"},{"id":"270582","messageId":"xmqqpp19xbpz.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"xmqq6131yyef.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T17:54:48Z","receivedAt":"2015-09-23T17:54:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Just to make sure there is no misunderstanding, just like I prefer\n> \"start one\" over \"start as many as possible\" in order to give\n> scheduling decision to the calling loop, I would expect that...\n\nTo sum up, what I anticipate would happen over time on top of 06/14\nis something like this:\n\n * pp_start_one() stuffed unborn child to the list of children when\n start_command() failed and start_failure() did not die(); return\n to the caller without corrupting children[] list in this case.\n\n * update the semantics of the value returned from pp_start_one()\n and adjust the scheduling loop for it.\n\n * allow slow-start of the whole process, so that we do not spawn\n tons of processes before starting to read from any of them, to\n give a better first-byte latency. This is parameterized and\n in this squash, it is set not to do a slow-start.\n \n * make poll(2) timeout used in pp_buffer_stderr() configurable by\n the scheduing loop. Raise the timeout when we are already running\n as many children as we would at the same time, and lower it when\n we are still ramping up.\n\nThis squash itself is a mixed bag of things. The first point is to\nfix a real issue, while the rest are merely envisioning possible\nfuture. The \"envisioning possible future\" parts may be illustrative\nwhen deciding what design we want in the basic structure.\n\nAn imaginary alternative version would have the top-level loop that\nis a mere \"these are the things we do\" bullet-list that runs \"spawn\nprocesses\", \"slurp in their output with some timeout\", \"output if\nforeground process is ready\", and \"cull finished children\" in order,\nwhich was your original. I would imagine that we would teach \"spawn\nprocesses\" part to slow-start, \"slurp\" part to adjust timeout\ndepending on the fullness of children[] and if the slow-start logic\nis still ramping up, etc., in such a code structure by keeping\nfields in *pp that corresponds to 'cnt' and 'no_more_task' variables\nand have these four steps in the bullet-list communiate and\ncoordinate among themselves using those fields.\n\nCompared to code that is structured that way, I think the top-level\nloop that owns 'cnt' and 'child_timeout' variables to make the\nscheduling decisions on its own, and drives \"dumb\" helper functions\nto drive the whole system, which is what this squash attempts to\ncreate, makes the overall logic and structure much clearer to see.\n\nThanks.\n\ndiff --git a/run-command.c b/run-command.c\nindex 494e1f8..b6d8b39 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -977,7 +977,7 @@ static void set_nonblocking(int fd)\n \t\t\t\"output will be degraded\");\n }\n \n-/* returns 1 if a process was started, 0 otherwise */\n+/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */\n static int pp_start_one(struct parallel_processes *pp)\n {\n \tint i;\n@@ -991,26 +991,28 @@ static int pp_start_one(struct parallel_processes *pp)\n \tif (!pp->get_next_task(pp->data,\n \t\t\t &pp->children[i].process,\n \t\t\t &pp->children[i].err))\n-\t\treturn 1;\n+\t\treturn 0;\n \n-\tif (start_command(&pp->children[i].process))\n+\tif (start_command(&pp->children[i].process)) {\n \t\tpp->start_failure(pp->data,\n \t\t\t\t &pp->children[i].process,\n \t\t\t\t &pp->children[i].err);\n+\t\treturn -1;\n+\t}\n \n \tset_nonblocking(pp->children[i].process.err);\n \n \tpp->nr_processes++;\n \tpp->children[i].in_use = 1;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n-\treturn 0;\n+\treturn 1;\n }\n \n-static void pp_buffer_stderr(struct parallel_processes *pp)\n+static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n {\n \tint i;\n \n-\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n+\twhile ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n \t\tif (errno == EINTR)\n \t\t\tcontinue;\n \t\tpp_cleanup(pp);\n@@ -1105,6 +1107,9 @@ static void pp_collect_finished(struct parallel_processes *pp)\n \t}\n }\n \n+\n+#define SPAWN_CAP (pp.max_processes + 1) /* spawn as many as possible */\n+\n int run_processes_parallel(int n, void *data,\n \t\t\t get_next_task_fn get_next_task,\n \t\t\t start_failure_fn start_failure,\n@@ -1114,12 +1119,27 @@ int run_processes_parallel(int n, void *data,\n \tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n \n \twhile (1) {\n-\t\twhile (pp.nr_processes < pp.max_processes &&\n-\t\t !pp_start_one(&pp))\n-\t\t\t; /* nothing */\n-\t\tif (!pp.nr_processes)\n+\t\tint no_more_task, cnt, output_timeout;\n+\n+\t\tfor (cnt = SPAWN_CAP, no_more_task = 0;\n+\t\t cnt && pp.nr_processes < pp.max_processes;\n+\t\t cnt--) {\n+\t\t\tif (!pp_start_one(&pp)) {\n+\t\t\t\tno_more_task = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (no_more_task && !pp.nr_processes)\n \t\t\tbreak;\n-\t\tpp_buffer_stderr(&pp);\n+\t\tif (!cnt)\n+\t\t\toutput_timeout = 50;\n+\t\telse if (pp.nr_processes < pp.max_processes)\n+\t\t\toutput_timeout = 100;\n+\t\telse\n+\t\t\toutput_timeout = 1000;\n+\t\tpp_buffer_stderr(&pp, output_timeout);\n+\n \t\tpp_output(&pp);\n \t\tpp_collect_finished(&pp);\n \t}\n"},{"id":"270583","messageId":"xmqqlhbxxbai.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"CAGZ79kYaqFRPfRORbknTyez5u0d6_BD0d5wBtAnjSkb4sUBUBA@mail.gmail.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T18:04:05Z","receivedAt":"2015-09-23T18:04:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I modified the test-run-command test function to start up to 400 processes.\n> (Most people will use less than 400 processes in the next 5 years), and run\n> just as in t0061:\n>\n> ./test-run-command run-command-parallel-400 sh -c \"printf\n> \\\"%s\\n%s\\n\\\" Hello World\"\n>\n> The output felt immediate (not slowed down or anything).\n\nI doubt that such an experiment has any value. You are not driving\nprintf in real life.\n\nYou are running \"git fetch\" that are is a lot more heavy-weight.\nBecause once each of them started fully they will be network bound,\nit is likely that you would want to run more processes than you have\ncore.\n"},{"id":"270598","messageId":"xmqq7fnhx74i.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"xmqqlhbxxbai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T19:34:05Z","receivedAt":"2015-09-23T19:34:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> You are running \"git fetch\" that are is a lot more heavy-weight.\n> Because once each of them started fully they will be network bound,\n> it is likely that you would want to run more processes than you have\n> core.\n\nI thought the conclusion would be obvious, but just in case the\nreaders need the flow of thought completed, from the above it\nfollows that on a N-core box (say 8-core) you may want to run the\nfetch with -j16 (or more). If we start everything at once, the time\nbefore one process starts to produce first meaningful response (I am\nnot counting the \"starting command for submodule-$i\" message as\n\"meaningful\") would take twice as long under such condition even if\nyou have infinite network bandwidth and talking to an infinitely\nfast server.\n\nI agree that a full slow-start ramping-up is not necessary in order\nto hide the start-up lag. All you need to do is to start just one\nand make sure it becomes the foreground, and give it a time alone to\nmake enough progress to produce an early output without getting\nslowed down by system activities caused by the other background\nprocesses in the group that start all at the same time. And once\nthe foreground process starts showing its output, you can unleash\nthe remainder of the herd to thunder and nobody would notice as they\nrun all in the background. Again, I am assuming something like \"git\nfetch\" that takes reasonably large amount of resource and time to\nstart-up and tear-down.\n"},{"id":"270599","messageId":"CAGZ79ka8MAKKSzXQbA+ARFynkSzB36C-4v_a5JBKooUoWsgqRA@mail.gmail.com","threadId":"40409","inReplyTo":"xmqq7fnhx74i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T19:39:53Z","receivedAt":"2015-09-23T19:39:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 23, 2015 at 12:34 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> You are running \"git fetch\" that are is a lot more heavy-weight.\n>> Because once each of them started fully they will be network bound,\n>> it is likely that you would want to run more processes than you have\n>> core.\n>\n> I thought the conclusion would be obvious, but just in case the\n> readers need the flow of thought completed, from the above it\n> follows that on a N-core box (say 8-core) you may want to run the\n> fetch with -j16 (or more). If we start everything at once, the time\n> before one process starts to produce first meaningful response (I am\n> not counting the \"starting command for submodule-$i\" message as\n> \"meaningful\") would take twice as long under such condition even if\n> you have infinite network bandwidth and talking to an infinitely\n> fast server.\n>\n> I agree that a full slow-start ramping-up is not necessary in order\n> to hide the start-up lag. All you need to do is to start just one\n> and make sure it becomes the foreground, and give it a time alone to\n> make enough progress to produce an early output without getting\n> slowed down by system activities caused by the other background\n> processes in the group that start all at the same time. And once\n> the foreground process starts showing its output, you can unleash\n> the remainder of the herd to thunder and nobody would notice as they\n> run all in the background. Again, I am assuming something like \"git\n> fetch\" that takes reasonably large amount of resource and time to\n> start-up and tear-down.\n>\n\nI can confirm this now.\n\n git fetch --recurse-submodules=yes -j 400\n\nin an submodule-ified Android tree takes very long to start putting out useful\ninformation, but if I hardcode the SPAWN_CAP to 4 it looks pretty amazing\nfast.\n\nI'd guess we could have a reasonable default for SPAWN_CAP by\n\n #define SPAWN_CAP min(pp.max_processes + 1, online_cpus)\n\n>\n"},{"id":"270600","messageId":"xmqq37y4yl2f.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"CAGZ79ka8MAKKSzXQbA+ARFynkSzB36C-4v_a5JBKooUoWsgqRA@mail.gmail.com","subject":"Re: [PATCHv4 06/14] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T19:47:36Z","receivedAt":"2015-09-23T19:47:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I can confirm this now.\n>\n> git fetch --recurse-submodules=yes -j 400\n>\n> in an submodule-ified Android tree takes very long to start putting out useful\n> information, but if I hardcode the SPAWN_CAP to 4 it looks pretty amazing\n> fast.\n\nNice to hear that parallel fetching does work ;-)\n"},{"id":"270602","messageId":"xmqqy4fwx5bl.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1442972732-12118-13-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 12/14] git submodule update: cmd_update_clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-23T20:13:02Z","receivedAt":"2015-09-23T20:13:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Split the cloning part out to its own function,\n> this allow us in a later patch to convert cmd_update in C.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nIt appears that this is not just a refactor and loses the logic\naround $cloned_modules variable that seems to avoid doing the same\nthing twice. An explanation on the reason why it no longer is\nnecessary in the proposed log message would be nice.\n\nThanks.\n\n\n> git-submodule.sh | 31 ++++++++++++++++++++-----------\n> 1 file changed, 20 insertions(+), 11 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index ea3260e..7f11158 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -607,6 +607,24 @@ cmd_update_recursive()\n> \tfi\n> }\n> \n> +cmd_update_clone()\n> +{\n> +\tcommand=\"git checkout $subforce -q\"\n> +\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> +\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n> +\n> +\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> +\n> +\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n> +\tthen\n> +\t\tsay \"$say_msg\"\n> +\telse\n> +\t\terr=\"${err};$die_msg\"\n> +\t\treturn\n> +\tfi\n> +\tcmd_update_recursive\n> +}\n> +\n> #\n> # Update each submodule path to correct revision, using clone and checkout as needed\n> #\n> @@ -680,7 +698,6 @@ cmd_update()\n> \t\tcmd_init \"--\" \"$@\" || return\n> \tfi\n> \n> -\tcloned_modules=\n> \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n> \terr=\n> \twhile read mode sha1 stage sm_path\n> @@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?\")\"\n> \n> \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n> \t\tthen\n> -\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> -\t\t\tcloned_modules=\"$cloned_modules;$name\"\n> -\t\t\tsubsha1=\n> +\t\t\tcmd_update_clone\n> +\t\t\tcontinue\n> \t\telse\n> \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n> \t\t\t\tgit rev-parse --verify HEAD) ||\n> @@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?\")\"\n> \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n> \t\t\tfi\n> \n> -\t\t\t# Is this something we just cloned?\n> -\t\t\tcase \";$cloned_modules;\" in\n> -\t\t\t*\";$name;\"*)\n> -\t\t\t\t# then there is no local change to integrate\n> -\t\t\t\tupdate_module=checkout ;;\n> -\t\t\tesac\n> -\n> \t\t\tmust_die_on_failure=\n> \t\t\tcase \"$update_module\" in\n> \t\t\tcheckout)\n"},{"id":"270613","messageId":"1443051711-15322-1-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"xmqqpp19xbpz.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv5] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-23T23:41:51Z","receivedAt":"2015-09-23T23:41:51Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nI agree the commit 7087c5f3 (SQUASH??? on top of run-command: add an\nasynchronous parallel child processor) makes sense; I arrived at the same\npatch after adding in the feedback.\n\nHowever I have the difference below, which I think could also be squashed in.\nwhen we operate under the assumption that the CPUs are the bottleneck in\nthe case of parallelism.\n\n run-command.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b6d8b39..e9722ff 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1107,8 +1107,13 @@ static void pp_collect_finished(struct parallel_processes *pp)\n \t}\n }\n \n-\n-#define SPAWN_CAP (pp.max_processes + 1) /* spawn as many as possible */\n+/*\n+ * Throttle spawning children when starting the asynchronous processing,\n+ * such that the first process to produce output is not slowed down to\n+ * enable the fastest early feedback to the user.\n+ */\n+#define SPAWN_CAP (pp.max_processes + 1 < online_cpus() - 1 ? \\\n+\t\t pp.max_processes + 1 : online_cpus() - 1)\n \n int run_processes_parallel(int n, void *data,\n \t\t\t get_next_task_fn get_next_task,\n-- \n2.5.0.273.g6fa2560.dirty\n"},{"id":"270619","messageId":"xmqqzj0cv9v8.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1443051711-15322-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv5] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-24T02:17:47Z","receivedAt":"2015-09-24T02:17:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I agree the commit 7087c5f3 (SQUASH??? on top of run-command: add an\n> asynchronous parallel child processor) makes sense; I arrived at the same\n> patch after adding in the feedback.\n\nI dunno. As I said, while a small part of that commit is necessary\nto be squashed into [6/14] (i.e. the \"failed to start\" bugfix and\nredefinition of the return value of start_one()), the remainder of\nthe commit is primarily to illustrate possible future enhancements\nthat the basic structure of your code must support, so that we can\nget the fundamentals right. Each of the actual \"possible future\nenhancements\" may or may not be a good idea and there is nothing\nthat back it up. In such a vacuum, I'd prefer to leave them simple\nand avoid starting performance tuning prematurely.\n\nOne thing that is sort-of backed up already by your \"cap to 4\"\nexperiment is that some sort of slow-start ramping up is far better\nthan letting thundering herd stampede, so I am OK if we kept that\nSPAWN_CAP part of the commit.\n\nBut even then, we do not know if tying that cap to online_cpu() is a\ngood idea. Neither of us have a good argument backed by data on it.\n\nIt is tempting to imagine, when you have N cores on an otherwise\nidle box, the setting of SPAWN_CAP shouldn't make much difference to\nhow well the first child process makes its initial progress as long\nas it does not exceed the number of idle cores N-1 you have at hand.\n\nBut that assumes that the task is CPU bound and you have infinite\nmemory bandwidth. Once the task needs a lot of disk bandwidth to\nmake its initial progress, which certainly is the case for fetch,\nthe first child that is spawned together with (SPAWN_CAP-1) other\nprocesses would be competing for the shared resource, and having\nmore online_cpus() would not help you.\n\nIf we are not doing analysis that takes into such factors (and it is\nway too premature for us to be tuning), even \"online_cpu() - 1\" is\nunnecessarily too complex than a hardcoded small number (say, \"2\",\nor even \"1\").\n\nThe same thing can be said for the output_timeout selection. \"Do\nnot get stuck for too long until we have fully ramped up. Do not\nspin too frequently when there is no more room for a new child\" was\nsomething I came up out of thin air as an example of something we\nmight want to do, and I did write such a code in that commit, but\nthat was primarily done so that you can clearly see that a better\ndesign would be to allow the caller, i.e. the scheduling loop,\nspecify output_timeout to buffer_stderr(), and to keep the latter a\n\"dumb\" helper that can be controlled by a more smart caller (as\nopposed to hiding such a logic in buffer_stderr() and have a \"dumb\"\ndriver call it). The actual output_timeout computation logic is not\nwell thought out---it may even turn out to be that we are better off\nif we lengthened the timeout before we have fully ramped up, to\nencourage the first process to produce some output before we give\nchance to other new processes to be spawned in the later round.\n\nSo for that change, while I think adding that parameter to\nbuffer_stderr() is something we would want to keep, I'd prefer to\nkeep the caller simpler by always passing a hardcoded 100 in the\ninitial version, before we start tuning. And I do not think we want\nto start tuning before building a solid foundation to tune.\n\nIn short, if I were amending that SQUASH??? commit, I'd probably be\nmaking it do less, not more, than what it does, something along the\nline of the attached.\n\n run-command.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b6d8b39..829b6fe 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1108,20 +1108,19 @@ static void pp_collect_finished(struct parallel_processes *pp)\n }\n \n \n-#define SPAWN_CAP (pp.max_processes + 1) /* spawn as many as possible */\n-\n int run_processes_parallel(int n, void *data,\n \t\t\t get_next_task_fn get_next_task,\n \t\t\t start_failure_fn start_failure,\n \t\t\t return_value_fn return_value)\n {\n \tstruct parallel_processes pp;\n-\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n \n+\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n \twhile (1) {\n-\t\tint no_more_task, cnt, output_timeout;\n+\t\tint no_more_task, cnt, output_timeout = 100;\n+\t\tint spawn_cap = 2;\n \n-\t\tfor (cnt = SPAWN_CAP, no_more_task = 0;\n+\t\tfor (cnt = spawn_cap, no_more_task = 0;\n \t\t cnt && pp.nr_processes < pp.max_processes;\n \t\t cnt--) {\n \t\t\tif (!pp_start_one(&pp)) {\n@@ -1132,12 +1131,6 @@ int run_processes_parallel(int n, void *data,\n \n \t\tif (no_more_task && !pp.nr_processes)\n \t\t\tbreak;\n-\t\tif (!cnt)\n-\t\t\toutput_timeout = 50;\n-\t\telse if (pp.nr_processes < pp.max_processes)\n-\t\t\toutput_timeout = 100;\n-\t\telse\n-\t\t\toutput_timeout = 1000;\n \t\tpp_buffer_stderr(&pp, output_timeout);\n \n \t\tpp_output(&pp);\n"},{"id":"270721","messageId":"1443129187-18572-1-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"xmqqzj0cv9v8.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-24T21:13:05Z","receivedAt":"2015-09-24T21:13:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Here is another proposal for squashing.\nIt's basically both squash proposals by Junio, plus another issues I think\nshould be done right from the beginning.\n\n [added by sb:]\n * If you do not die() in start_failure_fn or return_value_fn, you\n don't want to write to stderr directly as you would destroy the fine\n ordering of the processes output. So make the err strbuf available in\n both these functions, and make sure the strbuf is appended to the\n buffered output in both cases.\n\nStefan Beller (2):\n SQUASH???\n SQUASH for \"fetch_populated_submodules: use new parallel job\n processing\"\n\n-- \n2.5.0.273.g6fa2560.dirty\n"},{"id":"270720","messageId":"1443129187-18572-2-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1443129187-18572-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] SQUASH for \"fetch_populated_submodules: use new parallel job processing\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-24T21:13:06Z","receivedAt":"2015-09-24T21:13:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This fixes the function signature in the first user of the async run processor.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d7c7a6e..2c4396b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -630,13 +630,15 @@ struct submodule_parallel_fetch {\n int get_next_submodule(void *data, struct child_process *cp,\n \t\t struct strbuf *err);\n \n-void handle_submodule_fetch_start_err(void *data, struct child_process *cp, struct strbuf *err)\n+void handle_submodule_fetch_start_err(void *data, struct child_process *cp,\n+\t\t\t\t struct strbuf *err)\n {\n \tstruct submodule_parallel_fetch *spf = data;\n \tspf->result = 1;\n }\n \n-void handle_submodule_fetch_finish( void *data, struct child_process *cp, int retvalue)\n+void handle_submodule_fetch_finish(void *data, struct child_process *cp,\n+\t\t\t\t struct strbuf *err, int retvalue)\n {\n \tstruct submodule_parallel_fetch *spf = data;\n \n-- \n2.5.0.273.g6fa2560.dirty\n"},{"id":"270722","messageId":"1443129187-18572-3-git-send-email-sbeller@google.com","threadId":"40409","inReplyTo":"1443129187-18572-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] SQUASH???","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-24T21:13:07Z","receivedAt":"2015-09-24T21:13:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a mixed bag of squashes.\n\n * pp_start_one() stuffed unborn child to the list of children when\n start_command() failed and start_failure() did not die(); return\n to the caller without corrupting children[] list in this case.\n\n * make poll(2) timeout used in pp_buffer_stderr() configurable by\n the scheduling loop.\n\n * allow slow-start of the whole process, so that we do not spawn\n tons of processes before starting to read from any of them, to\n give a better first-byte latency.\n\n * fix the semantics of the value returned from pp_start_one() and\n adjust the scheduling loop for it.\n\n[added by sb:]\n * If you do not die() in start_failure_fn or return_value_fn, you\n don't want to write to stderr directly as you would destroy the fine\n ordering of the processes output. So make the err strbuf available in\n both these functions, and make sure the strbuf is appended to the\n buffered output in both cases\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n run-command.c | 43 ++++++++++++++++++++++++++++++-------------\n run-command.h | 1 +\n 2 files changed, 31 insertions(+), 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 494e1f8..0d22291 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -907,6 +907,7 @@ void default_start_failure(void *data,\n \n void default_return_value(void *data,\n \t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err,\n \t\t\t int result)\n {\n \tint i;\n@@ -977,7 +978,7 @@ static void set_nonblocking(int fd)\n \t\t\t\"output will be degraded\");\n }\n \n-/* returns 1 if a process was started, 0 otherwise */\n+/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */\n static int pp_start_one(struct parallel_processes *pp)\n {\n \tint i;\n@@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)\n \tif (!pp->get_next_task(pp->data,\n \t\t\t &pp->children[i].process,\n \t\t\t &pp->children[i].err))\n-\t\treturn 1;\n+\t\treturn 0;\n \n-\tif (start_command(&pp->children[i].process))\n+\tif (start_command(&pp->children[i].process)) {\n \t\tpp->start_failure(pp->data,\n \t\t\t\t &pp->children[i].process,\n \t\t\t\t &pp->children[i].err);\n+\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\tstrbuf_reset(&pp->children[i].err);\n+\t\treturn -1;\n+\t}\n \n \tset_nonblocking(pp->children[i].process.err);\n \n \tpp->nr_processes++;\n \tpp->children[i].in_use = 1;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n-\treturn 0;\n+\treturn 1;\n }\n \n-static void pp_buffer_stderr(struct parallel_processes *pp)\n+static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n {\n \tint i;\n \n-\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n+\twhile ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n \t\tif (errno == EINTR)\n \t\t\tcontinue;\n \t\tpp_cleanup(pp);\n@@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)\n \t\t\terror(\"waitpid is confused (%s)\",\n \t\t\t pp->children[i].process.argv[0]);\n \n-\t\tpp->return_value(pp->data, &pp->children[i].process, code);\n+\t\tpp->return_value(pp->data, &pp->children[i].process,\n+\t\t\t\t &pp->children[i].err, code);\n \n \t\targv_array_clear(&pp->children[i].process.args);\n \t\targv_array_clear(&pp->children[i].process.env_array);\n@@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,\n \t\t\t return_value_fn return_value)\n {\n \tstruct parallel_processes pp;\n-\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n \n+\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n \twhile (1) {\n-\t\twhile (pp.nr_processes < pp.max_processes &&\n-\t\t !pp_start_one(&pp))\n-\t\t\t; /* nothing */\n-\t\tif (!pp.nr_processes)\n+\t\tint no_more_task, cnt;\n+\t\tint output_timeout = 100;\n+\t\tint spawn_cap = 4;\n+\n+\t\tfor (cnt = spawn_cap, no_more_task = 0;\n+\t\t cnt && pp.nr_processes < pp.max_processes;\n+\t\t cnt--) {\n+\t\t\tif (!pp_start_one(&pp)) {\n+\t\t\t\tno_more_task = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (no_more_task && !pp.nr_processes)\n \t\t\tbreak;\n-\t\tpp_buffer_stderr(&pp);\n+\t\tpp_buffer_stderr(&pp, output_timeout);\n+\n \t\tpp_output(&pp);\n \t\tpp_collect_finished(&pp);\n \t}\ndiff --git a/run-command.h b/run-command.h\nindex 3807fd1..f7035cb 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,\n \n typedef void (*return_value_fn)(void *data,\n \t\t\t\tstruct child_process *cp,\n+\t\t\t\tstruct strbuf *err,\n \t\t\t\tint result);\n \n /**\n-- \n2.5.0.273.g6fa2560.dirty\n"},{"id":"270732","messageId":"xmqqa8sbuxu0.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1443129187-18572-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] SQUASH???","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T00:49:59Z","receivedAt":"2015-09-25T00:49:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> * If you do not die() in start_failure_fn or return_value_fn, you\n> don't want to write to stderr directly as you would destroy the fine\n> ordering of the processes output. So make the err strbuf available in\n> both these functions, and make sure the strbuf is appended to the\n> buffered output in both cases\n\nI think that is a sensible change. Don't we want the same for the\nother failure handler, though. Capture any message from it and\nappend it to the output of the process that just finished, or\nsomething?\n\nBy the way, I understand that these two are solely for early review\nand I'll be getting them as either new patches or part of updated\npatches in the next reroll (i.e. you are not expecting me to split\nthese apart and do \"rebase -i\" for you to the last-posted version).\nAsking only to make sure we are on the same wavelength.\n\nThanks.\n\n>\n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n> run-command.c | 43 ++++++++++++++++++++++++++++++-------------\n> run-command.h | 1 +\n> 2 files changed, 31 insertions(+), 13 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 494e1f8..0d22291 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -907,6 +907,7 @@ void default_start_failure(void *data,\n> \n> void default_return_value(void *data,\n> \t\t\t struct child_process *cp,\n> +\t\t\t struct strbuf *err,\n> \t\t\t int result)\n> {\n> \tint i;\n> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)\n> \t\t\t\"output will be degraded\");\n> }\n> \n> -/* returns 1 if a process was started, 0 otherwise */\n> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */\n> static int pp_start_one(struct parallel_processes *pp)\n> {\n> \tint i;\n> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)\n> \tif (!pp->get_next_task(pp->data,\n> \t\t\t &pp->children[i].process,\n> \t\t\t &pp->children[i].err))\n> -\t\treturn 1;\n> +\t\treturn 0;\n> \n> -\tif (start_command(&pp->children[i].process))\n> +\tif (start_command(&pp->children[i].process)) {\n> \t\tpp->start_failure(pp->data,\n> \t\t\t\t &pp->children[i].process,\n> \t\t\t\t &pp->children[i].err);\n> +\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n> +\t\tstrbuf_reset(&pp->children[i].err);\n> +\t\treturn -1;\n> +\t}\n> \n> \tset_nonblocking(pp->children[i].process.err);\n> \n> \tpp->nr_processes++;\n> \tpp->children[i].in_use = 1;\n> \tpp->pfd[i].fd = pp->children[i].process.err;\n> -\treturn 0;\n> +\treturn 1;\n> }\n> \n> -static void pp_buffer_stderr(struct parallel_processes *pp)\n> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n> {\n> \tint i;\n> \n> -\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n> +\twhile ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n> \t\tif (errno == EINTR)\n> \t\t\tcontinue;\n> \t\tpp_cleanup(pp);\n> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)\n> \t\t\terror(\"waitpid is confused (%s)\",\n> \t\t\t pp->children[i].process.argv[0]);\n> \n> -\t\tpp->return_value(pp->data, &pp->children[i].process, code);\n> +\t\tpp->return_value(pp->data, &pp->children[i].process,\n> +\t\t\t\t &pp->children[i].err, code);\n> \n> \t\targv_array_clear(&pp->children[i].process.args);\n> \t\targv_array_clear(&pp->children[i].process.env_array);\n> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,\n> \t\t\t return_value_fn return_value)\n> {\n> \tstruct parallel_processes pp;\n> -\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n> \n> +\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n> \twhile (1) {\n> -\t\twhile (pp.nr_processes < pp.max_processes &&\n> -\t\t !pp_start_one(&pp))\n> -\t\t\t; /* nothing */\n> -\t\tif (!pp.nr_processes)\n> +\t\tint no_more_task, cnt;\n> +\t\tint output_timeout = 100;\n> +\t\tint spawn_cap = 4;\n> +\n> +\t\tfor (cnt = spawn_cap, no_more_task = 0;\n> +\t\t cnt && pp.nr_processes < pp.max_processes;\n> +\t\t cnt--) {\n> +\t\t\tif (!pp_start_one(&pp)) {\n> +\t\t\t\tno_more_task = 1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (no_more_task && !pp.nr_processes)\n> \t\t\tbreak;\n> -\t\tpp_buffer_stderr(&pp);\n> +\t\tpp_buffer_stderr(&pp, output_timeout);\n> +\n> \t\tpp_output(&pp);\n> \t\tpp_collect_finished(&pp);\n> \t}\n> diff --git a/run-command.h b/run-command.h\n> index 3807fd1..f7035cb 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,\n> \n> typedef void (*return_value_fn)(void *data,\n> \t\t\t\tstruct child_process *cp,\n> +\t\t\t\tstruct strbuf *err,\n> \t\t\t\tint result);\n> \n> /**\n"},{"id":"270733","messageId":"xmqq4mijuwyz.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"1443129187-18572-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T01:08:36Z","receivedAt":"2015-09-25T01:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> * If you do not die() in start_failure_fn or return_value_fn, you\n> don't want to write to stderr directly as you would destroy the fine\n> ordering of the processes output. So make the err strbuf available in\n> both these functions, and make sure the strbuf is appended to the\n> buffered output in both cases.\n\nAnother thing I noticed after re-reading the above is that we shared\nthe thinking that dying in these is _the_ normal thing to do and\ncontinuing is an advanced and/or wierd setting.\n\nAnd I think it is wrong. Suppose after spawning 15 tasks and while\nthey are still running, you start the 16th one and it fails to stop.\nIf your start-failure called die() to kill the controller, what\nhappens to the 15 tasks that are already running?\n\nI think two sensible choices that start-failure and return-value can\nmake are\n\n (1) This one task failed, but that is OK. Please let the other\n tasks run [*1*].\n\n (2) There is something seriously wrong with the whole world and I\n declare an emergency. Please kill the other ones and exit.\n\nDying in these callbacks do not achieve neither. Perhaps make these\ntwo functions return bool (or enum if you already know a third\nsensible option, but otherwise bool is fine and the person who\ndiscovers the need for the third will turn it into enum) to signal\nwhich one of these two behaviours it wants?\n\nAnd the default handlers should stop dying, of course.\n\n\n[Footnote]\n\n*1* Because start-failure gets pp, it can even leave a note in it to\n ask the next invocation of get-next to retry it if it chooses\n to. At this point in the design cycle, all we need to do is to\n make sure that kind of advanced usage is possible with this\n parallel-run-command API.\n"},{"id":"270734","messageId":"xmqqwpvfticn.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"xmqqa8sbuxu0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] SQUASH???","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T01:09:44Z","receivedAt":"2015-09-25T01:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think that is a sensible change. Don't we want the same for the\n> other failure handler, though. Capture any message from it and\n> append it to the output of the process that just finished, or\n> something?\n\nAh, that is already done. Scratch that \"don't we want\" part.\n\n>\n> By the way, I understand that these two are solely for early review\n> and I'll be getting them as either new patches or part of updated\n> patches in the next reroll (i.e. you are not expecting me to split\n> these apart and do \"rebase -i\" for you to the last-posted version).\n> Asking only to make sure we are on the same wavelength.\n>\n> Thanks.\n>\n>>\n>> Signed-off-by: Junio C Hamano \n>> Signed-off-by: Stefan Beller \n>> ---\n>> run-command.c | 43 ++++++++++++++++++++++++++++++-------------\n>> run-command.h | 1 +\n>> 2 files changed, 31 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 494e1f8..0d22291 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -907,6 +907,7 @@ void default_start_failure(void *data,\n>> \n>> void default_return_value(void *data,\n>> \t\t\t struct child_process *cp,\n>> +\t\t\t struct strbuf *err,\n>> \t\t\t int result)\n>> {\n>> \tint i;\n>> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)\n>> \t\t\t\"output will be degraded\");\n>> }\n>> \n>> -/* returns 1 if a process was started, 0 otherwise */\n>> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */\n>> static int pp_start_one(struct parallel_processes *pp)\n>> {\n>> \tint i;\n>> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)\n>> \tif (!pp->get_next_task(pp->data,\n>> \t\t\t &pp->children[i].process,\n>> \t\t\t &pp->children[i].err))\n>> -\t\treturn 1;\n>> +\t\treturn 0;\n>> \n>> -\tif (start_command(&pp->children[i].process))\n>> +\tif (start_command(&pp->children[i].process)) {\n>> \t\tpp->start_failure(pp->data,\n>> \t\t\t\t &pp->children[i].process,\n>> \t\t\t\t &pp->children[i].err);\n>> +\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n>> +\t\tstrbuf_reset(&pp->children[i].err);\n>> +\t\treturn -1;\n>> +\t}\n>> \n>> \tset_nonblocking(pp->children[i].process.err);\n>> \n>> \tpp->nr_processes++;\n>> \tpp->children[i].in_use = 1;\n>> \tpp->pfd[i].fd = pp->children[i].process.err;\n>> -\treturn 0;\n>> +\treturn 1;\n>> }\n>> \n>> -static void pp_buffer_stderr(struct parallel_processes *pp)\n>> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n>> {\n>> \tint i;\n>> \n>> -\twhile ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n>> +\twhile ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n>> \t\tif (errno == EINTR)\n>> \t\t\tcontinue;\n>> \t\tpp_cleanup(pp);\n>> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)\n>> \t\t\terror(\"waitpid is confused (%s)\",\n>> \t\t\t pp->children[i].process.argv[0]);\n>> \n>> -\t\tpp->return_value(pp->data, &pp->children[i].process, code);\n>> +\t\tpp->return_value(pp->data, &pp->children[i].process,\n>> +\t\t\t\t &pp->children[i].err, code);\n>> \n>> \t\targv_array_clear(&pp->children[i].process.args);\n>> \t\targv_array_clear(&pp->children[i].process.env_array);\n>> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,\n>> \t\t\t return_value_fn return_value)\n>> {\n>> \tstruct parallel_processes pp;\n>> -\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n>> \n>> +\tpp_init(&pp, n, data, get_next_task, start_failure, return_value);\n>> \twhile (1) {\n>> -\t\twhile (pp.nr_processes < pp.max_processes &&\n>> -\t\t !pp_start_one(&pp))\n>> -\t\t\t; /* nothing */\n>> -\t\tif (!pp.nr_processes)\n>> +\t\tint no_more_task, cnt;\n>> +\t\tint output_timeout = 100;\n>> +\t\tint spawn_cap = 4;\n>> +\n>> +\t\tfor (cnt = spawn_cap, no_more_task = 0;\n>> +\t\t cnt && pp.nr_processes < pp.max_processes;\n>> +\t\t cnt--) {\n>> +\t\t\tif (!pp_start_one(&pp)) {\n>> +\t\t\t\tno_more_task = 1;\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\tif (no_more_task && !pp.nr_processes)\n>> \t\t\tbreak;\n>> -\t\tpp_buffer_stderr(&pp);\n>> +\t\tpp_buffer_stderr(&pp, output_timeout);\n>> +\n>> \t\tpp_output(&pp);\n>> \t\tpp_collect_finished(&pp);\n>> \t}\n>> diff --git a/run-command.h b/run-command.h\n>> index 3807fd1..f7035cb 100644\n>> --- a/run-command.h\n>> +++ b/run-command.h\n>> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,\n>> \n>> typedef void (*return_value_fn)(void *data,\n>> \t\t\t\tstruct child_process *cp,\n>> +\t\t\t\tstruct strbuf *err,\n>> \t\t\t\tint result);\n>> \n>> /**\n"},{"id":"270760","messageId":"CAGZ79katew0UnG2nMrXpnp3AEW+6JgjX9pKaXf6Jb-FfEgeFkw@mail.gmail.com","threadId":"40409","inReplyTo":"xmqqa8sbuxu0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] SQUASH???","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-25T17:52:07Z","receivedAt":"2015-09-25T17:52:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 24, 2015 at 5:49 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> * If you do not die() in start_failure_fn or return_value_fn, you\n>> don't want to write to stderr directly as you would destroy the fine\n>> ordering of the processes output. So make the err strbuf available in\n>> both these functions, and make sure the strbuf is appended to the\n>> buffered output in both cases\n>\n> I think that is a sensible change. Don't we want the same for the\n> other failure handler, though. Capture any message from it and\n> append it to the output of the process that just finished, or\n> something?\n>\n> By the way, I understand that these two are solely for early review\n> and I'll be getting them as either new patches or part of updated\n> patches in the next reroll (i.e. you are not expecting me to split\n> these apart and do \"rebase -i\" for you to the last-posted version).\n> Asking only to make sure we are on the same wavelength.\n\nSure. I just wanted to point out the details instead of resending the series.\nI'll do a resend later today, hoping to get all issues addressed.\n\nThanks,\nStefan\n\n\n>\n> Thanks.\n>\n>>\n>> Signed-off-by: Junio C Hamano \n>> Signed-off-by: Stefan Beller \n>> ---\n>> run-command.c | 43 ++++++++++++++++++++++++++++++-------------\n>> run-command.h | 1 +\n>> 2 files changed, 31 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 494e1f8..0d22291 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -907,6 +907,7 @@ void default_start_failure(void *data,\n>>\n>> void default_return_value(void *data,\n>> struct child_process *cp,\n>> + struct strbuf *err,\n>> int result)\n>> {\n>> int i;\n>> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)\n>> \"output will be degraded\");\n>> }\n>>\n>> -/* returns 1 if a process was started, 0 otherwise */\n>> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */\n>> static int pp_start_one(struct parallel_processes *pp)\n>> {\n>> int i;\n>> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)\n>> if (!pp->get_next_task(pp->data,\n>> &pp->children[i].process,\n>> &pp->children[i].err))\n>> - return 1;\n>> + return 0;\n>>\n>> - if (start_command(&pp->children[i].process))\n>> + if (start_command(&pp->children[i].process)) {\n>> pp->start_failure(pp->data,\n>> &pp->children[i].process,\n>> &pp->children[i].err);\n>> + strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n>> + strbuf_reset(&pp->children[i].err);\n>> + return -1;\n>> + }\n>>\n>> set_nonblocking(pp->children[i].process.err);\n>>\n>> pp->nr_processes++;\n>> pp->children[i].in_use = 1;\n>> pp->pfd[i].fd = pp->children[i].process.err;\n>> - return 0;\n>> + return 1;\n>> }\n>>\n>> -static void pp_buffer_stderr(struct parallel_processes *pp)\n>> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n>> {\n>> int i;\n>>\n>> - while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {\n>> + while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n>> if (errno == EINTR)\n>> continue;\n>> pp_cleanup(pp);\n>> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)\n>> error(\"waitpid is confused (%s)\",\n>> pp->children[i].process.argv[0]);\n>>\n>> - pp->return_value(pp->data, &pp->children[i].process, code);\n>> + pp->return_value(pp->data, &pp->children[i].process,\n>> + &pp->children[i].err, code);\n>>\n>> argv_array_clear(&pp->children[i].process.args);\n>> argv_array_clear(&pp->children[i].process.env_array);\n>> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,\n>> return_value_fn return_value)\n>> {\n>> struct parallel_processes pp;\n>> - pp_init(&pp, n, data, get_next_task, start_failure, return_value);\n>>\n>> + pp_init(&pp, n, data, get_next_task, start_failure, return_value);\n>> while (1) {\n>> - while (pp.nr_processes < pp.max_processes &&\n>> - !pp_start_one(&pp))\n>> - ; /* nothing */\n>> - if (!pp.nr_processes)\n>> + int no_more_task, cnt;\n>> + int output_timeout = 100;\n>> + int spawn_cap = 4;\n>> +\n>> + for (cnt = spawn_cap, no_more_task = 0;\n>> + cnt && pp.nr_processes < pp.max_processes;\n>> + cnt--) {\n>> + if (!pp_start_one(&pp)) {\n>> + no_more_task = 1;\n>> + break;\n>> + }\n>> + }\n>> +\n>> + if (no_more_task && !pp.nr_processes)\n>> break;\n>> - pp_buffer_stderr(&pp);\n>> + pp_buffer_stderr(&pp, output_timeout);\n>> +\n>> pp_output(&pp);\n>> pp_collect_finished(&pp);\n>> }\n>> diff --git a/run-command.h b/run-command.h\n>> index 3807fd1..f7035cb 100644\n>> --- a/run-command.h\n>> +++ b/run-command.h\n>> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,\n>>\n>> typedef void (*return_value_fn)(void *data,\n>> struct child_process *cp,\n>> + struct strbuf *err,\n>> int result);\n>>\n>> /**\n"},{"id":"270761","messageId":"xmqqk2re4c3i.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"CAGZ79katew0UnG2nMrXpnp3AEW+6JgjX9pKaXf6Jb-FfEgeFkw@mail.gmail.com","subject":"Re: [PATCH 1/2] SQUASH???","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T17:56:17Z","receivedAt":"2015-09-25T17:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Sure. I just wanted to point out the details instead of resending the series.\n> I'll do a resend later today, hoping to get all issues addressed.\n\nThanks. For something this small, unless there are many small\npieces that need to be picked apart, I do not terribly mind to do\nthe squashing myself (after all that is what I do every day, prepare\na squash while queuing, do the integration testing and send the\nsquash out to make sure the original author agrees with it).\n\nI just wanted to make sure that we won't be in a situation where\nI am waiting forever expecting you to reroll while you are expecting\nme to do the squashing, resulting in no progress getting made until\neither one of us starts wondering what is going on.\n"},{"id":"270765","messageId":"CAGZ79kbpLwODzPrGcuvz2oY+XMMPAy9SRh1s_5wV8cvY-J79iw@mail.gmail.com","threadId":"40409","inReplyTo":"xmqq4mijuwyz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-25T18:56:11Z","receivedAt":"2015-09-25T18:56:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 24, 2015 at 6:08 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> * If you do not die() in start_failure_fn or return_value_fn, you\n>> don't want to write to stderr directly as you would destroy the fine\n>> ordering of the processes output. So make the err strbuf available in\n>> both these functions, and make sure the strbuf is appended to the\n>> buffered output in both cases.\n>\n> Another thing I noticed after re-reading the above is that we shared\n> the thinking that dying in these is _the_ normal thing to do and\n> continuing is an advanced and/or wierd setting.\n>\n> And I think it is wrong. Suppose after spawning 15 tasks and while\n> they are still running, you start the 16th one and it fails to stop.\n> If your start-failure called die() to kill the controller, what\n> happens to the 15 tasks that are already running?\n>\n> I think two sensible choices that start-failure and return-value can\n> make are\n>\n> (1) This one task failed, but that is OK. Please let the other\n> tasks run [*1*].\n>\n> (2) There is something seriously wrong with the whole world and I\n> declare an emergency. Please kill the other ones and exit.\n\n (3) There is something wrong, such that I cannot finish my\n job, but I know the other 15 processes help towards the goal,\n so I want to let them live on until they are done. E.g: fetch submodules\n may want to take this strategy if it fails to start another sub\nprocess fetching.\n\nBy having a return value indicating which strategy you want to pursue here,\nwe're making the design choice to have everything done monolithically\ninside the pp machinery.\n\nWe could also offer more access to the pp machinery and an implementation for\n(2) might look like this:\n\nstatic void fictious_start_failure(void *data,\n void *pp,\n struct child_process *cp,\n struct strbuf *err)\n{\n struct mydata *m = data;\n\n if (m->failstrategy == 1)\n ; /* nothing here */\n else if (m->failstrategy == 2)\n killall_children(pp);\n else if (m->failstrategy == 3) {\n m->stop_scheduling_new_tasks = 1;\n redirect_children_to_dev_null(pp);\n else\n ...\n}\n\nBy having the pointer to the pp struct passed around, we allow\nfor adding new callback functions to be added later to the\npp machinery, which may not be expressed via a return code.\n\n>\n> Dying in these callbacks do not achieve neither. Perhaps make these\n> two functions return bool (or enum if you already know a third\n> sensible option, but otherwise bool is fine and the person who\n> discovers the need for the third will turn it into enum) to signal\n> which one of these two behaviours it wants?\n>\n> And the default handlers should stop dying, of course.\n>\n>\n> [Footnote]\n>\n> *1* Because start-failure gets pp, it can even leave a note in it to\n> ask the next invocation of get-next to retry it if it chooses\n> to. At this point in the design cycle, all we need to do is to\n> make sure that kind of advanced usage is possible with this\n> parallel-run-command API.\n"},{"id":"270766","messageId":"xmqq612y48x5.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"CAGZ79kbpLwODzPrGcuvz2oY+XMMPAy9SRh1s_5wV8cvY-J79iw@mail.gmail.com","subject":"Re: [PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T19:04:54Z","receivedAt":"2015-09-25T19:04:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> I think two sensible choices that start-failure and return-value can\n>> make are\n>>\n>> (1) This one task failed, but that is OK. Please let the other\n>> tasks run [*1*].\n>>\n>> (2) There is something seriously wrong with the whole world and I\n>> declare an emergency. Please kill the other ones and exit.\n>\n> (3) There is something wrong, such that I cannot finish my\n> job, but I know the other 15 processes help towards the goal,\n> so I want to let them live on until they are done. E.g: fetch submodules\n> may want to take this strategy if it fails to start another sub\n> process fetching.\n\nHow is that different from (1)? Do you meann \"let other ones that\nare already running continue, but do not spawn any new ones?\"\n\n> We could also offer more access to the pp machinery and an implementation for\n> (2) might look like this:\n> ...\n> By having the pointer to the pp struct passed around, we allow\n> for adding new callback functions to be added later to the\n> pp machinery, which may not be expressed via a return code.\n\nWhat you are suggesting would lead to the same \"different smart\nparticipants making decisions locally, so you need to run around and\nfollow all the detailed codepaths to understand what is going on\"\ndesign.\n\nI was hoping that we have already passed discussing that stage.\n\nThe whole point of that \"SQUASH???\" commit was to correct the design\nof the overall structure so that we make the central dispatcher that\nuses bunch of \"dumb\" helpers (that do not make policy decisions\nlocally on their own) as the single place you need to read in order\nto understand the logic.\n"},{"id":"270767","messageId":"CAGZ79kYdrV2fyzWOCmrgEZBXDtEd+HMb9N3yrPquzCDfVogXCQ@mail.gmail.com","threadId":"40409","inReplyTo":"xmqq612y48x5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-25T19:19:26Z","receivedAt":"2015-09-25T19:19:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 25, 2015 at 12:04 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> I think two sensible choices that start-failure and return-value can\n>>> make are\n>>>\n>>> (1) This one task failed, but that is OK. Please let the other\n>>> tasks run [*1*].\n>>>\n>>> (2) There is something seriously wrong with the whole world and I\n>>> declare an emergency. Please kill the other ones and exit.\n>>\n>> (3) There is something wrong, such that I cannot finish my\n>> job, but I know the other 15 processes help towards the goal,\n>> so I want to let them live on until they are done. E.g: fetch submodules\n>> may want to take this strategy if it fails to start another sub\n>> process fetching.\n>\n> How is that different from (1)? Do you meann \"let other ones that\n> are already running continue, but do not spawn any new ones?\"\n\nRight. \"Phasing out\" before gracefully dying. Well to achieve this,\nwe don't need more options here. (Just set a flag to not return more\nwork in get_next_task).\n\n>\n>> We could also offer more access to the pp machinery and an implementation for\n>> (2) might look like this:\n>> ...\n>> By having the pointer to the pp struct passed around, we allow\n>> for adding new callback functions to be added later to the\n>> pp machinery, which may not be expressed via a return code.\n>\n> What you are suggesting would lead to the same \"different smart\n> participants making decisions locally, so you need to run around and\n> follow all the detailed codepaths to understand what is going on\"\n> design.\n>\n> I was hoping that we have already passed discussing that stage.\n>\n> The whole point of that \"SQUASH???\" commit was to correct the design\n> of the overall structure so that we make the central dispatcher that\n> uses bunch of \"dumb\" helpers (that do not make policy decisions\n> locally on their own) as the single place you need to read in order\n> to understand the logic.\n\nAnd I thought the discussion there was focused on the internals of the\ndispatcher. (You need to only read one place inside _that_ file now)\n\nThis discussion would have focused on the public API side (What do we\nwant to hide, how do we want to hide it?), which would be different enough\nfor me to warrant a new discussion.\n\nBut I guess I'll just go with your suggestion for now to have the return value\nindicate a full stop or keep going.\n"},{"id":"270768","messageId":"xmqq1tdm47nj.fsf@gitster.mtv.corp.google.com","threadId":"40409","inReplyTo":"CAGZ79kbpLwODzPrGcuvz2oY+XMMPAy9SRh1s_5wV8cvY-J79iw@mail.gmail.com","subject":"Re: [PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T19:32:16Z","receivedAt":"2015-09-25T19:32:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> We could also offer more access to the pp machinery and an implementation for\n> (2) might look like this:\n>\n> static void fictious_start_failure(void *data,\n> void *pp,\n> struct child_process *cp,\n> struct strbuf *err)\n> {\n> struct mydata *m = data;\n>\n> if (m->failstrategy == 1)\n> ; /* nothing here */\n> else if (m->failstrategy == 2)\n> killall_children(pp);\n\nThat looks nice and clean in theory, but I highly doubt that it is a\ngood idea to have killall_children(pp) in a client code of the API\nlike this, especially if you are envisioning that that function\nmerely goes through the list of children and does kill on the\nprocesses. If killall_children(pp) is supplied by the API, it makes\nthings less insane (now it can know and keep up with the evolution\nof the API implementation detail, such as how buffered output are\nkept and such), but still such an API constrains the structure of\nthe overall scheduling loop and the helper functions that live on\nthe API side. You need to make absolutely sure that calling\nkillall_children() is something that can be sanely done from inside\nstart_failure() callback, for example.\n\nIf you signal the \"emergency\" with a return value, the callchain on\nthe API side can choose to do the killing at a place it knows is\nsafe to do so in a more controlled way. For example, the main loop\nof the API side IIRC (I do not bother checking out 'pu' to read it\nas my working tree is busy on another topic right now) is\n\n while (1) {\n \tfor (cnt = 0; cnt < 4 && we still have slots; cnt++)\n\t\tstart_one();\n\n\tcollect_output();\n\tdrain_output_for_foreground();\n\twait_and_flush();\n }\n\nImagine that you have 0 or more processes running and start another\niteration of this loop. The first task started by start_one() fails\nand the above fictitious_start_failure() calls killall_children()\nitself. What happens to the other three iteration of the inner\nloop? After existing ones are killed, it adds three more?\n\nAnd to prevent that from happening, you also need to tell your\nfictitious_next_task() that no more processes are desired. The\nclient of the API is forced to coordinate across its multiple\ncallback functions.\n\nAnd you did that to gain what? One major thing I can think of is\nthat that way, the main scheduling loop does not have to know why\nafter attempting to spawn but failing, fictitious_next_task()\nstarted saying \"no more tasks\". For somebody who is coming from\n\"The main loop is a dumb bullet-list of things to do. All smarts\nare inside the callee\", that might appear to be a good thing.\n\nBut I do not think it is a good thing at all. That forces the\nreader of the code to not just follow the API layer but even down to\nits individual clients of the API to understand what is going on.\n\nIf you propagate the return from start_failure() callback upwards,\nthen the main scheduler would *know* that the client application\ndoes not want any more new tasks, and it does want to abort even the\nrunning tasks, so it will not call fictitious_next_task() in the\nfirst place after the client application declares an \"emergency\nexit\".\n\nAnd that would make the overall logic a lot easier to follow.\n\n\n\n\n\n\n\n\n\t\n"}]}