{"thread":{"id":"40367","subject":"[PATCH 01/10] strbuf: Add strbuf_read_noblock","startedAt":"2015-09-17T01:38:58Z","lastAt":"2015-09-18T16:36:30Z","messageCount":37,"participants":["Stefan Beller","Jeff King","Junio C Hamano","Jacob Keller","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"270189","messageId":"1442453948-9885-1-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":null,"subject":"[PATCH 00/10] fetch submodules in parallel and a preview on parallel \"submodule update\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:38:58Z","receivedAt":"2015-09-17T01:38:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I didn't say this in the previous round because it smelled like an\n> RFC, but for a real submission, 2/2 may be doing too many things at\n> once. I suspect this is more or less \"taste\" thing, so I won't mind\n> too much as long as the reviewers are OK with it.\n\nThe patch 2/2 is now broken up into the first five patches.\n\nThere are only 2 minor changes:\n* If a number of tasks <= 0 is specified, use the number of cpus instead.\n* Renamed the command line option in test-run-command.c to \"run-command-parallel-4\"\n as the 4 is hardcoded there.\n \nThe patch 6,7 are small cleanups (6 should add a test in a reroll)\n\nPatches 7,8,9 are a preview of how I want to proceed in the near future:\nAfter these functions are split out, we can add another patch on top which\nrewrites the short main loop in cmd_update to be in C in submodule--helper\nrunning in parallel.\nIt took me a while to get the idea how to realize parallelism with the\nparallel run command structure now as opposed to the thread pool I proposed\nearlier, but I think it will be straightforward from here.\n\nStefan\n\nStefan Beller (10):\n strbuf: Add strbuf_read_noblock\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 git submodule update: Redirect any output to stderr\n git submodule update: pass --prefix only with a non empty prefix\n git submodule update: cmd_update_recursive\n git submodule update: cmd_update_recursive\n git submodule update: cmd_update_fetch\n\n Documentation/fetch-options.txt | 7 +\n builtin/fetch.c | 6 +-\n builtin/pull.c | 6 +\n git-submodule.sh | 242 ++++++++++++++++++----------------\n run-command.c | 281 ++++++++++++++++++++++++++++++++++++----\n run-command.h | 36 +++++\n strbuf.c | 25 +++-\n strbuf.h | 6 +\n submodule.c | 119 ++++++++++++-----\n submodule.h | 2 +-\n t/t0061-run-command.sh | 20 +++\n t/t5526-fetch-submodules.sh | 19 +++\n test-run-command.c | 24 ++++\n 13 files changed, 620 insertions(+), 173 deletions(-)\n\n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270187","messageId":"1442453948-9885-2-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:38:59Z","receivedAt":"2015-09-17T01:38:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need to read from pipes without blocking in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n strbuf.c | 25 +++++++++++++++++++++++--\n strbuf.h | 6 ++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex cce5eed..4130ee2 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -357,7 +357,10 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n \treturn res;\n }\n \n-ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n+#define IGNORE_EAGAIN (1)\n+\n+static ssize_t strbuf_read_internal(struct strbuf *sb, int fd,\n+\t\t\t\t size_t hint, int flags)\n {\n \tsize_t oldlen = sb->len;\n \tsize_t oldalloc = sb->alloc;\n@@ -366,8 +369,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \tfor (;;) {\n \t\tssize_t cnt;\n \n-\t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\t\tcnt = read(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \t\tif (cnt < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EAGAIN) {\n+\t\t\t\tif (flags & IGNORE_EAGAIN)\n+\t\t\t\t\tbreak;\n+\t\t\t\telse\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (oldalloc == 0)\n \t\t\t\tstrbuf_release(sb);\n \t\t\telse\n@@ -384,6 +395,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \treturn sb->len - oldlen;\n }\n \n+ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n+{\n+\treturn strbuf_read_internal(sb, fd, hint, 0);\n+}\n+\n+ssize_t strbuf_read_noblock(struct strbuf *sb, int fd, size_t hint)\n+{\n+\treturn strbuf_read_internal(sb, fd, hint, IGNORE_EAGAIN);\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..23ca7aa 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,6 +367,12 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n+ * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.\n+ * The fd must have set O_NONBLOCK.\n+ */\n+extern ssize_t strbuf_read_noblock(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.6.0.rc0.131.gf624c3d\n"},{"id":"270188","messageId":"1442453948-9885-3-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 02/10] run-command: factor out return value computation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:00Z","receivedAt":"2015-09-17T01:39:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We will need computing the return value in a later patch without the\nwait.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 28e1d55..c892e9a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n+static int determine_return_value(int wait_status,\n+\t\t\t\t int *result,\n+\t\t\t\t int *error_code,\n+\t\t\t\t const char *argv0)\n+{\n+\tif (WIFSIGNALED(wait_status)) {\n+\t\t*result = WTERMSIG(wait_status);\n+\t\tif (*result != SIGINT && *result != SIGQUIT)\n+\t\t\terror(\"%s died of signal %d\", argv0, *result);\n+\t\t/*\n+\t\t * This return value is chosen so that code & 0xff\n+\t\t * mimics the exit code that a POSIX shell would report for\n+\t\t * a program that died from this signal.\n+\t\t */\n+\t\t*result += 128;\n+\t} else if (WIFEXITED(wait_status)) {\n+\t\t*result = WEXITSTATUS(wait_status);\n+\t\t/*\n+\t\t * Convert special exit code when execvp failed.\n+\t\t */\n+\t\tif (*result == 127) {\n+\t\t\t*result = -1;\n+\t\t\t*error_code = ENOENT;\n+\t\t}\n+\t} else\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static int wait_or_whine(pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n@@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n-\t} else if (waiting != pid) {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n-\t} else if (WIFSIGNALED(status)) {\n-\t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n-\t\t\terror(\"%s died of signal %d\", argv0, code);\n-\t\t/*\n-\t\t * This return value is chosen so that code & 0xff\n-\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t * a program that died from this signal.\n-\t\t */\n-\t\tcode += 128;\n-\t} else if (WIFEXITED(status)) {\n-\t\tcode = WEXITSTATUS(status);\n-\t\t/*\n-\t\t * Convert special exit code when execvp failed.\n-\t\t */\n-\t\tif (code == 127) {\n-\t\t\tcode = -1;\n-\t\t\tfailed_errno = ENOENT;\n-\t\t}\n \t} else {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t\tif (waiting != pid\n+\t\t || (determine_return_value(status, &code, &failed_errno, argv0) < 0))\n+\t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n \n \tclear_child_for_cleanup(pid);\n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270194","messageId":"1442453948-9885-4-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 03/10] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:01Z","receivedAt":"2015-09-17T01:39:01Z","isPatch":true,"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\nthread 1: |---A---| |-D-| |-E-|\n\nthread 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 time.\nOnce that is done, C is determined to be the visible child and its progress\nwill be reported in real time.\n\nSo this way of output is really good for human consumption,\nas it only changes the timing, not the actual output.\n\nFor machine consumption the output needs to be prepared in\nthe tasks, by either having a prefix per line or per block\nto indicate whose tasks output is displayed.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 228 +++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 36 ++++++++\n t/t0061-run-command.sh | 20 +++++\n test-run-command.c | 24 ++++++\n 4 files changed, 308 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex c892e9a..3af97ab 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -3,6 +3,7 @@\n #include \"exec_cmd.h\"\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n+#include \"thread-utils.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -862,3 +863,230 @@ 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+\tint max_number_processes;\n+\tvoid *data;\n+\tget_next_task fn;\n+\thandle_child_starting_failure fn_err;\n+\thandle_child_return_value fn_exit;\n+\n+\tint nr_processes;\n+\tint all_tasks_started;\n+\tint foreground_child;\n+\tchar *slots;\n+\tstruct child_process *children;\n+\tstruct pollfd *pfd;\n+\tstruct strbuf *err;\n+\tstruct strbuf finished_children;\n+};\n+\n+static void run_processes_parallel_init(struct parallel_processes *pp,\n+\t\t\t\t\tint n, void *data,\n+\t\t\t\t\tget_next_task fn,\n+\t\t\t\t\thandle_child_starting_failure fn_err,\n+\t\t\t\t\thandle_child_return_value fn_exit)\n+{\n+\tint i;\n+\n+\tif (n < 1)\n+\t\tn = online_cpus();\n+\n+\tpp->max_number_processes = n;\n+\tpp->data = data;\n+\tpp->fn = fn;\n+\tpp->fn_err = fn_err;\n+\tpp->fn_exit = fn_exit;\n+\n+\tpp->nr_processes = 0;\n+\tpp->all_tasks_started = 0;\n+\tpp->foreground_child = 0;\n+\tpp->slots = xcalloc(n, sizeof(*pp->slots));\n+\tpp->children = xcalloc(n, sizeof(*pp->children));\n+\tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n+\tpp->err = xcalloc(n, sizeof(*pp->err));\n+\tstrbuf_init(&pp->finished_children, 0);\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tstrbuf_init(&pp->err[i], 0);\n+\t\tpp->pfd[i].events = POLLIN;\n+\t\tpp->pfd[i].fd = -1;\n+\t}\n+}\n+\n+static void run_processes_parallel_cleanup(struct parallel_processes *pp)\n+{\n+\tint i;\n+\tfor (i = 0; i < pp->max_number_processes; i++)\n+\t\tstrbuf_release(&pp->err[i]);\n+\n+\tfree(pp->children);\n+\tfree(pp->slots);\n+\tfree(pp->pfd);\n+\tfree(pp->err);\n+\tstrbuf_release(&pp->finished_children);\n+}\n+\n+static void unblock_fd(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+\t\treturn;\n+\t}\n+\tif (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {\n+\t\t\twarning(\"Could not set file status flags, \"\n+\t\t\t\"output will be degraded\");\n+\t\treturn;\n+\t}\n+}\n+\n+static void run_processes_parallel_start_new(struct parallel_processes *pp)\n+{\n+\tint i;\n+\t/* Start new processes. */\n+\twhile (!pp->all_tasks_started\n+\t && pp->nr_processes < pp->max_number_processes) {\n+\t\tfor (i = 0; i < pp->max_number_processes; i++)\n+\t\t\tif (!pp->slots[i])\n+\t\t\t\tbreak; /* found an empty slot */\n+\t\tif (i == pp->max_number_processes)\n+\t\t\tdie(\"BUG: bookkeeping is hard\");\n+\n+\t\tif (pp->fn(pp->data, &pp->children[i], &pp->err[i])) {\n+\t\t\tpp->all_tasks_started = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (start_command(&pp->children[i]))\n+\t\t\tpp->fn_err(pp->data, &pp->children[i], &pp->err[i]);\n+\n+\t\tunblock_fd(pp->children[i].err);\n+\n+\t\tpp->nr_processes++;\n+\t\tpp->slots[i] = 1;\n+\t\tpp->pfd[i].fd = pp->children[i].err;\n+\t}\n+}\n+\n+static int run_processes_parallel_buffer_stderr(struct parallel_processes *pp)\n+{\n+\tint i;\n+\ti = poll(pp->pfd, pp->max_number_processes, 100);\n+\tif (i < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\t/* A signal was caught; try again */\n+\t\t\treturn -1;\n+\t\telse {\n+\t\t\trun_processes_parallel_cleanup(pp);\n+\t\t\tdie_errno(\"poll\");\n+\t\t}\n+\t}\n+\n+\t/* Buffer output from all pipes. */\n+\tfor (i = 0; i < pp->max_number_processes; i++) {\n+\t\tif (!pp->slots[i])\n+\t\t\tcontinue;\n+\t\tif (pp->pfd[i].revents & POLLIN)\n+\t\t\tstrbuf_read_noblock(&pp->err[i], pp->children[i].err, 0);\n+\t\tif (pp->foreground_child == i) {\n+\t\t\tfputs(pp->err[i].buf, stderr);\n+\t\t\tstrbuf_reset(&pp->err[i]);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+\n+static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n+{\n+\tint i = 0;\n+\tpid_t pid;\n+\tint wait_status, code;\n+\tint n = pp->max_number_processes;\n+\t/* Collect finished child processes. */\n+\twhile (pp->nr_processes > 0) {\n+\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n+\t\tif (pid == 0)\n+\t\t\treturn; /* no child finished */\n+\n+\t\tif (pid < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\treturn; /* just try again next time */\n+\t\t\tif (errno == EINVAL || errno == ECHILD)\n+\t\t\t\tdie_errno(\"wait\");\n+\t\t} else {\n+\t\t\t/* Find the finished child. */\n+\t\t\tfor (i = 0; i < pp->max_number_processes; i++)\n+\t\t\t\tif (pp->slots[i] && pid == pp->children[i].pid)\n+\t\t\t\t\tbreak;\n+\t\t\tif (i == pp->max_number_processes)\n+\t\t\t\t/*\n+\t\t\t\t * waitpid returned another process id\n+\t\t\t\t * which we are not waiting for.\n+\t\t\t\t */\n+\t\t\t\treturn;\n+\t\t}\n+\t\tstrbuf_read_noblock(&pp->err[i], pp->children[i].err, 0);\n+\n+\t\tif (determine_return_value(wait_status, &code, &errno,\n+\t\t\t\t\t pp->children[i].argv[0]) < 0)\n+\t\t\terror(\"waitpid is confused (%s)\",\n+\t\t\t pp->children[i].argv[0]);\n+\n+\t\tpp->fn_exit(pp->data, &pp->children[i], code);\n+\n+\t\targv_array_clear(&pp->children[i].args);\n+\t\targv_array_clear(&pp->children[i].env_array);\n+\n+\t\tpp->nr_processes--;\n+\t\tpp->slots[i] = 0;\n+\t\tpp->pfd[i].fd = -1;\n+\n+\t\tif (i != pp->foreground_child) {\n+\t\t\tstrbuf_addbuf(&pp->finished_children, &pp->err[i]);\n+\t\t\tstrbuf_reset(&pp->err[i]);\n+\t\t} else {\n+\t\t\tfputs(pp->err[i].buf, stderr);\n+\t\t\tstrbuf_reset(&pp->err[i]);\n+\n+\t\t\t/* Output all other finished child processes */\n+\t\t\tfputs(pp->finished_children.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->finished_children);\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->slots[(pp->foreground_child + i) % n])\n+\t\t\t\t\tbreak;\n+\t\t\tpp->foreground_child = (pp->foreground_child + i) % n;\n+\t\t\tfputs(pp->err[pp->foreground_child].buf, stderr);\n+\t\t\tstrbuf_reset(&pp->err[pp->foreground_child]);\n+\t\t}\n+\t}\n+}\n+\n+int run_processes_parallel(int n, void *data,\n+\t\t\t get_next_task fn,\n+\t\t\t handle_child_starting_failure fn_err,\n+\t\t\t handle_child_return_value fn_exit)\n+{\n+\tstruct parallel_processes pp;\n+\trun_processes_parallel_init(&pp, n, data, fn, fn_err, fn_exit);\n+\n+\twhile (!pp.all_tasks_started || pp.nr_processes > 0) {\n+\t\trun_processes_parallel_start_new(&pp);\n+\t\tif (run_processes_parallel_buffer_stderr(&pp))\n+\t\t\tcontinue;\n+\t\trun_processes_parallel_collect_finished(&pp);\n+\t}\n+\trun_processes_parallel_cleanup(&pp);\n+\n+\treturn 0;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a..0487f71 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 0 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)(void *data,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err);\n+\n+typedef void (*handle_child_starting_failure)(void *data,\n+\t\t\t\t\t struct child_process *cp,\n+\t\t\t\t\t struct strbuf *err);\n+\n+typedef void (*handle_child_return_value)(void *data,\n+\t\t\t\t\t struct child_process *cp,\n+\t\t\t\t\t int 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 handle_child_starting_failure,\n+\t\t\t handle_child_return_value);\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..70b6c7a 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 1;\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 0;\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.6.0.rc0.131.gf624c3d\n"},{"id":"270191","messageId":"1442453948-9885-5-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 04/10] fetch_populated_submodules: use new parallel job processing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:02Z","receivedAt":"2015-09-17T01:39:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we enable parallel processing of submodules, this\nonly adds the possibility for it. So this change should not change\nany user facing behavior.\n\nSigned-off-by: Stefan Beller \n---\n builtin/fetch.c | 3 +-\n submodule.c | 119 +++++++++++++++++++++++++++++++++++++++-----------------\n submodule.h | 2 +-\n 3 files changed, 87 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ee1f1a9..6620ed0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_populated_submodules(&options,\n \t\t\t\t\t\t submodule_prefix,\n \t\t\t\t\t\t recurse_submodules,\n-\t\t\t\t\t\t verbosity < 0);\n+\t\t\t\t\t\t verbosity < 0,\n+\t\t\t\t\t\t 0);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/submodule.c b/submodule.c\nindex 1d64e57..a0e06e8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -12,6 +12,7 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n #include \"blob.h\"\n+#include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n@@ -615,37 +616,79 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tconst char *work_tree;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint quiet;\n+\tint result;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err);\n+\n+void handle_submodule_fetch_start_err(void *data, struct child_process *cp, struct strbuf *err)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\tspf->result = 1;\n+}\n+\n+void handle_submodule_fetch_finish( void *data, struct child_process *cp, int retvalue)\n+{\n+\tstruct submodule_parallel_fetch *spf = data;\n+\n+\tif (retvalue)\n+\t\tspf->result = 1;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet)\n+\t\t\t int quiet, int max_parallel_jobs)\n {\n-\tint i, result = 0;\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *work_tree = get_git_work_tree();\n-\tif (!work_tree)\n+\tint i;\n+\tstruct submodule_parallel_fetch spf = SPF_INIT;\n+\tspf.work_tree = get_git_work_tree();\n+\tspf.command_line_option = command_line_option;\n+\tspf.quiet = quiet;\n+\tspf.prefix = prefix;\n+\tif (!spf.work_tree)\n \t\tgoto out;\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \n-\targv_array_push(&argv, \"fetch\");\n+\targv_array_push(&spf.args, \"fetch\");\n \tfor (i = 0; i < options->argc; i++)\n-\t\targv_array_push(&argv, options->argv[i]);\n-\targv_array_push(&argv, \"--recurse-submodules-default\");\n+\t\targv_array_push(&spf.args, options->argv[i]);\n+\targv_array_push(&spf.args, \"--recurse-submodules-default\");\n \t/* default value, \"--submodule-prefix\" and its value are added later */\n \n-\tcp.env = local_repo_env;\n-\tcp.git_cmd = 1;\n-\tcp.no_stdin = 1;\n-\n \tcalculate_changed_submodule_paths();\n+\trun_processes_parallel(max_parallel_jobs, &spf,\n+\t\t\t get_next_submodule,\n+\t\t\t handle_submodule_fetch_start_err,\n+\t\t\t handle_submodule_fetch_finish);\n+\n+\targv_array_clear(&spf.args);\n+out:\n+\tstring_list_clear(&changed_submodule_paths, 1);\n+\treturn spf.result;\n+}\n+\n+int get_next_submodule(void *data, struct child_process *cp,\n+\t\t struct strbuf *err)\n+{\n+\tint ret = 0;\n+\tstruct submodule_parallel_fetch *spf = data;\n \n-\tfor (i = 0; i < active_nr; i++) {\n+\tfor ( ; spf->count < active_nr; spf->count++) {\n \t\tstruct strbuf submodule_path = STRBUF_INIT;\n \t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n-\t\tconst struct cache_entry *ce = active_cache[i];\n+\t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n \n@@ -657,7 +700,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tsubmodule = submodule_from_name(null_sha1, ce->name);\n \n \t\tdefault_argv = \"yes\";\n-\t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n+\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n \t\t\tif (submodule &&\n \t\t\t submodule->fetch_recurse !=\n \t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n@@ -680,40 +723,46 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n-\t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_path, \"%s/%s\", work_tree, ce->name);\n+\t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n \t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", prefix, ce->name);\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n \t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n \t\tif (!git_dir)\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n-\t\t\tif (!quiet)\n-\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n-\t\t\tcp.dir = submodule_path.buf;\n-\t\t\targv_array_push(&argv, default_argv);\n-\t\t\targv_array_push(&argv, \"--submodule-prefix\");\n-\t\t\targv_array_push(&argv, submodule_prefix.buf);\n-\t\t\tcp.argv = argv.argv;\n-\t\t\tif (run_command(&cp))\n-\t\t\t\tresult = 1;\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n+\t\t\tchild_process_init(cp);\n+\t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n+\t\t\tcp->git_cmd = 1;\n+\t\t\tcp->no_stdout = 1;\n+\t\t\tcp->no_stdin = 1;\n+\t\t\tcp->stdout_to_stderr = 1;\n+\t\t\tcp->err = -1;\n+\t\t\tcp->env = local_repo_env;\n+\t\t\tif (!spf->quiet)\n+\t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n+\t\t\t\t\t spf->prefix, ce->name);\n+\t\t\targv_array_init(&cp->args);\n+\t\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\t\targv_array_push(&cp->args, default_argv);\n+\t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\t\t\tret = 1;\n \t\t}\n \t\tstrbuf_release(&submodule_path);\n \t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n+\t\tif (ret) {\n+\t\t\tspf->count++;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n-\targv_array_clear(&argv);\n-out:\n-\tstring_list_clear(&changed_submodule_paths, 1);\n-\treturn result;\n+\treturn 1;\n }\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\ndiff --git a/submodule.h b/submodule.h\nindex 5507c3d..cbc0003 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);\n void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n-\t\t\t int quiet);\n+\t\t\t int quiet, int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270193","messageId":"1442453948-9885-6-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 05/10] submodules: Allow parallel fetching, add tests and documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:03Z","receivedAt":"2015-09-17T01:39:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n Documentation/fetch-options.txt | 7 +++++++\n builtin/fetch.c | 5 ++++-\n builtin/pull.c | 6 ++++++\n t/t5526-fetch-submodules.sh | 19 +++++++++++++++++++\n 4 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 45583d8..6b109f6 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -100,6 +100,13 @@ ifndef::git-pull[]\n \treference to a commit that isn't already in the local submodule\n \tclone.\n \n+-j::\n+--jobs=::\n+\tNumber of parallel children to be used for fetching submodules.\n+\tEach will fetch from different submodules, such that fetching many\n+\tsubmodules will be faster. By default submodules will be fetched\n+\tone at a time.\n+\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the '--recurse-submodules=no' option).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6620ed0..f28eac6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n+static int max_children = 1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n+\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\t\t N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n@@ -1218,7 +1221,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t submodule_prefix,\n \t\t\t\t\t\t recurse_submodules,\n \t\t\t\t\t\t verbosity < 0,\n-\t\t\t\t\t\t 0);\n+\t\t\t\t\t\t max_children);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 722a83c..f0af196 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -94,6 +94,7 @@ static int opt_force;\n static char *opt_tags;\n static char *opt_prune;\n static char *opt_recurse_submodules;\n+static char *max_children;\n static int opt_dry_run;\n static char *opt_keep;\n static char *opt_depth;\n@@ -177,6 +178,9 @@ static struct option pull_options[] = {\n \t\tN_(\"on-demand\"),\n \t\tN_(\"control recursive fetching of submodules\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU('j', \"jobs\", &max_children, N_(\"n\"),\n+\t\tN_(\"number of submodules pulled in parallel\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_BOOL(0, \"dry-run\", &opt_dry_run,\n \t\tN_(\"dry run\")),\n \tOPT_PASSTHRU('k', \"keep\", &opt_keep, NULL,\n@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\targv_array_push(&args, opt_prune);\n \tif (opt_recurse_submodules)\n \t\targv_array_push(&args, opt_recurse_submodules);\n+\tif (max_children)\n+\t\targv_array_push(&args, max_children);\n \tif (opt_dry_run)\n \t\targv_array_push(&args, \"--dry-run\");\n \tif (opt_keep)\ndiff --git a/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.6.0.rc0.131.gf624c3d\n"},{"id":"270196","messageId":"1442453948-9885-7-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 06/10] git submodule update: Redirect any output to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:04Z","receivedAt":"2015-09-17T01:39:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There are no tests, which fail by this.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8b0eb9a..7ef3247 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -663,7 +663,7 @@ cmd_update()\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\tsay >&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@@ -684,7 +684,7 @@ cmd_update()\n \n \t\tif test \"$update_module\" = \"none\"\n \t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n+\t\t\tsay >&2 \"Skipping submodule '$displaypath'\"\n \t\t\tcontinue\n \t\tfi\n \n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270197","messageId":"1442453948-9885-8-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 07/10] git submodule update: pass --prefix only with a non empty prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:05Z","receivedAt":"2015-09-17T01:39:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We should not pass --prefix NULL into the helper. Although the helper\ncan deal with it, it's just messy.\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 7ef3247..3ccb0b6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -700,7 +700,7 @@ 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\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270195","messageId":"1442453948-9885-9-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 08/10] git submodule update: cmd_update_recursive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:06Z","receivedAt":"2015-09-17T01:39:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"split the recursion part out to its own function\n\nSigned-off-by: Stefan Beller \n---\n git-submodule.sh | 47 ++++++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 3ccb0b6..52c2967 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.6.0.rc0.131.gf624c3d\n"},{"id":"270190","messageId":"1442453948-9885-10-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 09/10] git submodule update: cmd_update_recursive","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:07Z","receivedAt":"2015-09-17T01:39:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n git-submodule.sh | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 52c2967..c40d60f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -607,6 +607,24 @@ cmd_update_recursive()\n \tfi\n }\n \n+cmd_update_clone()\n+{\n+\tcommand=\"git checkout $subforce -q\"\n+\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n+\n+\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\n+\tif (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n+\tthen\n+\t\tsay \"$say_msg\"\n+\telse\n+\t\terr=\"${err};$die_msg\"\n+\t\treturn\n+\tfi\n+\tcmd_update_recursive\n+}\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -680,7 +698,6 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n \tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n-\t\t\tsubsha1=\n+\t\t\tcmd_update_clone\n+\t\t\tcontinue\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\n-- \n2.6.0.rc0.131.gf624c3d\n"},{"id":"270192","messageId":"1442453948-9885-11-git-send-email-sbeller@google.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"[PATCH 10/10] git submodule update: cmd_update_fetch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T01:39:08Z","receivedAt":"2015-09-17T01:39:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n git-submodule.sh | 164 ++++++++++++++++++++++++++++---------------------------\n 1 file changed, 84 insertions(+), 80 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c40d60f..2c9f1f2 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.6.0.rc0.131.gf624c3d\n"},{"id":"270202","messageId":"20150917103310.GC20086@sigill.intra.peff.net","threadId":"40367","inReplyTo":"1442453948-9885-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 02/10] run-command: factor out return value computation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T10:33:10Z","receivedAt":"2015-09-17T10:33:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 16, 2015 at 06:39:00PM -0700, Stefan Beller wrote:\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\nLooks like we can return \"0\" or \"1\" here, and the exit code goes into\n\"result\". But our caller:\n\n> static int wait_or_whine(pid_t pid, const char *argv0)\n> {\n> \tint status, code = -1;\n> @@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> \tif (waiting < 0) {\n> \t\tfailed_errno = errno;\n> \t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n> -\t} else if (waiting != pid) {\n> -\t\terror(\"waitpid is confused (%s)\", argv0);\n> -\t} else if (WIFSIGNALED(status)) {\n> -\t\tcode = WTERMSIG(status);\n> -\t\tif (code != SIGINT && code != SIGQUIT)\n> -\t\t\terror(\"%s died of signal %d\", argv0, code);\n> -\t\t/*\n> -\t\t * This return value is chosen so that code & 0xff\n> -\t\t * mimics the exit code that a POSIX shell would report for\n> -\t\t * a program that died from this signal.\n> -\t\t */\n> -\t\tcode += 128;\n> -\t} else if (WIFEXITED(status)) {\n> -\t\tcode = WEXITSTATUS(status);\n> -\t\t/*\n> -\t\t * Convert special exit code when execvp failed.\n> -\t\t */\n> -\t\tif (code == 127) {\n> -\t\t\tcode = -1;\n> -\t\t\tfailed_errno = ENOENT;\n> -\t\t}\n> \t} else {\n> -\t\terror(\"waitpid is confused (%s)\", argv0);\n> +\t\tif (waiting != pid\n> +\t\t || (determine_return_value(status, &code, &failed_errno, argv0) < 0))\n> +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n> \t}\n\n...is looking for \"< 0\", which will never happen. Should the \"1\" above\nhave been \"-1\"?\n\nI also wondered what happened to \"code\" and \"failed_errno\" in that case.\nThey are OK to access because wait_or_whine() has set them to defaults,\nbut I wonder if determine_return_value should do so in every branch (so\nit is is clear that the values are always defined when it returns).\n\n-Peff\n"},{"id":"270224","messageId":"xmqqtwqtja6j.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"1442453948-9885-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T16:13:40Z","receivedAt":"2015-09-17T16:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Subject: Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock\n\ns/Add/add/;\n\n> We need to read from pipes without blocking in a later patch.\n\nI am hoping that you are at least not spinning---i.e. do a poll \nfirst to make sure there is at least some progress to be made\nbefore calling this.\n\n> Signed-off-by: Stefan Beller \n> ---\n> strbuf.c | 25 +++++++++++++++++++++++--\n> strbuf.h | 6 ++++++\n> 2 files changed, 29 insertions(+), 2 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index cce5eed..4130ee2 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -357,7 +357,10 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n> \treturn res;\n> }\n> \n> -ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> +#define IGNORE_EAGAIN (1)\n> +\n> +static ssize_t strbuf_read_internal(struct strbuf *sb, int fd,\n> +\t\t\t\t size_t hint, int flags)\n> {\n> \tsize_t oldlen = sb->len;\n> \tsize_t oldalloc = sb->alloc;\n> @@ -366,8 +369,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> \tfor (;;) {\n> \t\tssize_t cnt;\n> \n> -\t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> +\t\tcnt = read(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> \t\tif (cnt < 0) {\n> +\t\t\tif (errno == EINTR)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (errno == EAGAIN) {\n> +\t\t\t\tif (flags & IGNORE_EAGAIN)\n> +\t\t\t\t\tbreak;\n> +\t\t\t\telse\n> +\t\t\t\t\tcontinue;\n> +\t\t\t}\n\nIn order to ensure that this is not breaking the normal case, I had\nto look at the implementation of xread() to see it behaves identically\nwhen the flags is not passed. That one-time review burden implies\nthat this is adding a maintenance burden to keep this copied function\nin sync.\n\nWe should also handle EWOULDBLOCK not just EAGAIN.\n\nMore importantly, I am not sure if this helper is even necessary.\n\nLooking at xread (reproduced in its full glory):\n\n/*\n * xread() is the same a read(), but it automatically restarts read()\n * operations with a recoverable error (EAGAIN and EINTR). xread()\n * DOES NOT GUARANTEE that \"len\" bytes is read even if the data is available.\n */\nssize_t xread(int fd, void *buf, size_t len)\n{\n\tssize_t nr;\n\tif (len > MAX_IO_SIZE)\n\t len = MAX_IO_SIZE;\n\twhile (1) {\n\t\tnr = read(fd, buf, len);\n\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n\t\t\tcontinue;\n\t\treturn nr;\n\t}\n}\n\nAre we doing the right thing for EAGAIN here?\n\nNow, man read(2) says this:\n\n EAGAIN \n\t\tThe file descriptor fd refers to a file other than a\n socket and has been marked nonblocking (O_NONBLOCK),\n and the read would block.\n\n EAGAIN or EWOULDBLOCK\n\t\tThe file descriptor fd refers to a socket and has\n been marked nonblocking (O_NONBLOCK), and the read\n would block. POSIX.1-2001 allows either error to be\n returned for this case, and does not require these\n con‐ stants to have the same value, so a portable\n application should check for both possibilities.\n\nIf the fd is not marked nonblocking, then we will not get EAGAIN (or\nEWOULDBLOCK).\n\nWhen fd is set to nonblocking, the current xread() spins if read()\nsays that the operation would block. What would we achieve by\nspinning ourselves here, though? The caller must have set the fd to\nbe nonblocking for a reason, and that reason cannot be \"if the data\nis not ready, we do not have anything better than just spinning for\nnew data to arrive\"---if so, the caller wouldn't have set the fd to\nbe nonblocking in the first place.\n\nEven if there is such a stupid caller that only wants us to loop,\nbecause we explicitly allow xread() to return a short read, all of\nour callers must call it in a loop if they know how much they want\nto read. We can just return from here and let them loop around us.\n\nAnd your new caller that does O_NONBLOCK wants to do more than\nlooping upon EWOULDBLOCK. It certainly would not want us to loop\nhere.\n\nSo I wonder if you can just O_NONBLOCK the fd and use the usual\nstrbuf_read(), i.e. without any change in this patch, and update\nxread() to _unconditionally_ return when read(2) says EAGAIN or\nEWOULDBLOCK.\n\nWhat would that break?\n\n> \t\t\tif (oldalloc == 0)\n> \t\t\t\tstrbuf_release(sb);\n> \t\t\telse\n> @@ -384,6 +395,16 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> \treturn sb->len - oldlen;\n> }\n> \n> +ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> +{\n> +\treturn strbuf_read_internal(sb, fd, hint, 0);\n> +}\n> +\n> +ssize_t strbuf_read_noblock(struct strbuf *sb, int fd, size_t hint)\n> +{\n> +\treturn strbuf_read_internal(sb, fd, hint, IGNORE_EAGAIN);\n> +}\n> +\n> #define STRBUF_MAXLINK (2*PATH_MAX)\n> \n> int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n> diff --git a/strbuf.h b/strbuf.h\n> index aef2794..23ca7aa 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -367,6 +367,12 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n> extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n> \n> /**\n> + * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.\n> + * The fd must have set O_NONBLOCK.\n> + */\n> +extern ssize_t strbuf_read_noblock(struct strbuf *, int fd, size_t hint);\n> +\n> +/**\n> * Read the contents of a file, specified by its path. The third argument\n> * can be used to give a hint about the file size, to avoid reallocs.\n> */\n"},{"id":"270228","messageId":"20150917163012.GB25837@sigill.intra.peff.net","threadId":"40367","inReplyTo":"xmqqtwqtja6j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T16:30:12Z","receivedAt":"2015-09-17T16:30:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 09:13:40AM -0700, Junio C Hamano wrote:\n\n> And your new caller that does O_NONBLOCK wants to do more than\n> looping upon EWOULDBLOCK. It certainly would not want us to loop\n> here.\n> \n> So I wonder if you can just O_NONBLOCK the fd and use the usual\n> strbuf_read(), i.e. without any change in this patch, and update\n> xread() to _unconditionally_ return when read(2) says EAGAIN or\n> EWOULDBLOCK.\n> \n> What would that break?\n\nCertainly anybody who does not realize their descriptor is O_NONBLOCK\nand is using the spinning for correctness. I tend to think that such\nsites are wrong, though, and would benefit from us realizing they are\nspinning.\n\nBut I think you can't quite get away with leaving strbuf_read untouched\nin this case. On error, it wants to restore the original value of the\nstrbuf before the strbuf_read call. Which means that we throw away\nanything read into the strbuf before we get EAGAIN, and the caller never\ngets to see it.\n\nSo I think we would probably want to treat EAGAIN specially: return -1\nto signal to the caller but _don't_ truncate the strbuf.\n\nArguably we should actually return the number of bytes we _did_ read,\nbut then caller cannot easily tell the difference between EOF and\nEAGAIN.\n\n-Peff\n"},{"id":"270231","messageId":"CAPc5daVY=t27wkX2x7DSfObwxZuv37VYxi1XD-RKiS1mv4=6KA@mail.gmail.com","threadId":"40367","inReplyTo":"20150917163012.GB25837@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T16:44:19Z","receivedAt":"2015-09-17T16:44:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Sep 17, 2015 at 9:30 AM, Jeff King wrote:\n>\n> So I think we would probably want to treat EAGAIN specially: return -1\n> to signal to the caller but _don't_ truncate the strbuf.\n\nYeah, \"don't truncate\" is needed.\n\n> Arguably we should actually return the number of bytes we _did_ read,\n> but then caller cannot easily tell the difference between EOF and\n> EAGAIN.\n\nWhy can't it check errno==EAGAIN/EWOULDBLOCK?\n"},{"id":"270233","messageId":"CAGZ79kb+GgPaHgVze+q3W99cJ+b2SzdX_DfjLSQCvvLjbDXmbA@mail.gmail.com","threadId":"40367","inReplyTo":"CAPc5daVY=t27wkX2x7DSfObwxZuv37VYxi1XD-RKiS1mv4=6KA@mail.gmail.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T16:51:46Z","receivedAt":"2015-09-17T16:51:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 9:44 AM, Junio C Hamano wrote:\n> On Thu, Sep 17, 2015 at 9:30 AM, Jeff King wrote:\n>>\n>> So I think we would probably want to treat EAGAIN specially: return -1\n>> to signal to the caller but _don't_ truncate the strbuf.\n>\n> Yeah, \"don't truncate\" is needed.\n>\n>> Arguably we should actually return the number of bytes we _did_ read,\n>> but then caller cannot easily tell the difference between EOF and\n>> EAGAIN.\n>\n> Why can't it check errno==EAGAIN/EWOULDBLOCK?\n\nGrepping through Gits sources, there are no occurrences of\nexplicitly setting O_NONBLOCK except for the newly introduced\nspot in the followup patch in run-command (yes we do poll there).\n\nSo how would I find out if a fd is blocking or not in the 82 cases\nof strbuf_read? (Now I naively assume they would all block.)\n\nWe could also expose the flags in the API. I think IGNORE_EAGAIN\nmight not be the best now, but rather a NO_TRUNCATE flag would do.\n"},{"id":"270236","messageId":"20150917165724.GA27102@sigill.intra.peff.net","threadId":"40367","inReplyTo":"CAPc5daVY=t27wkX2x7DSfObwxZuv37VYxi1XD-RKiS1mv4=6KA@mail.gmail.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T16:57:24Z","receivedAt":"2015-09-17T16:57:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 09:44:19AM -0700, Junio C Hamano wrote:\n\n> > Arguably we should actually return the number of bytes we _did_ read,\n> > but then caller cannot easily tell the difference between EOF and\n> > EAGAIN.\n> \n> Why can't it check errno==EAGAIN/EWOULDBLOCK?\n\nIs it trustworthy to check errno if read() has not actually ever\nreturned -1? E.g., consider this sequence:\n\n 1. somebody (maybe even us calling strbuf_read) calls read() and it\n returns -1, setting errno to EAGAIN\n\n 2. we call strbuf_read()\n\n 2a. it calls read(), which returns N bytes\n\n 2b. it calls read() again, which returns 0 for EOF\n\n 2c. it returns N, because that's how many bytes it read\n\n 3. we wonder if we hit EOF, or if we simply need to read again. We\n check errno == EAGAIN\n\nI don't think the read calls in step 2 are guaranteed to clear errno,\nand we might read the cruft from step 1.\n\n-Peff\n"},{"id":"270237","messageId":"xmqq6139j84n.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"20150917163012.GB25837@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T16:58:00Z","receivedAt":"2015-09-17T16:58:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 17, 2015 at 09:13:40AM -0700, Junio C Hamano wrote:\n>\n>> And your new caller that does O_NONBLOCK wants to do more than\n>> looping upon EWOULDBLOCK. It certainly would not want us to loop\n>> here.\n>> \n>> So I wonder if you can just O_NONBLOCK the fd and use the usual\n>> strbuf_read(), i.e. without any change in this patch, and update\n>> xread() to _unconditionally_ return when read(2) says EAGAIN or\n>> EWOULDBLOCK.\n>> \n>> What would that break?\n>\n> Certainly anybody who does not realize their descriptor is O_NONBLOCK\n> and is using the spinning for correctness. I tend to think that such\n> sites are wrong, though, and would benefit from us realizing they are\n> spinning.\n\nWith or without O_NONBLOCK, not looping around xread() _and_ relying\non the spinning for \"correctness\" means the caller is not getting\ncorrectness anyway, I think, because xread() does return a short\nread, and we deliberately and explicitly do so since 0b6806b9\n(xread, xwrite: limit size of IO to 8MB, 2013-08-20).\n\n> But I think you can't quite get away with leaving strbuf_read untouched\n> in this case. On error, it wants to restore the original value of the\n> strbuf before the strbuf_read call. Which means that we throw away\n> anything read into the strbuf before we get EAGAIN, and the caller never\n> gets to see it.\n\nI agree we need to teach strbuf_read() that xread() is now nicer on\nO_NONBLOCK; perhaps like this?\n\n strbuf.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex cce5eed..49104d7 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -368,6 +368,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \t\tif (cnt < 0) {\n+\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK)\n+\t\t\t\tbreak;\n \t\t\tif (oldalloc == 0)\n \t\t\t\tstrbuf_release(sb);\n \t\t\telse\n"},{"id":"270239","messageId":"CA+P7+xoE9OfSbyuFL-8WtditdnxHatF-fUSXDaU3na=LJHeV=g@mail.gmail.com","threadId":"40367","inReplyTo":"1442453948-9885-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 00/10] fetch submodules in parallel and a preview on parallel \"submodule update\"","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-09-17T17:06:40Z","receivedAt":"2015-09-17T17:06:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 16, 2015 at 6:38 PM, Stefan Beller wrote:\n> It took me a while to get the idea how to realize parallelism with the\n> parallel run command structure now as opposed to the thread pool I proposed\n> earlier, but I think it will be straightforward from here.\n>\n\nYea at least from a cursory review this seems significantly simpler.\nI'm still trying to give a better deep dive of the code, so hopefully\nI will have some more thoughts soon.\n\nRegards,\nJake\n"},{"id":"270241","messageId":"20150917171308.GA28046@sigill.intra.peff.net","threadId":"40367","inReplyTo":"xmqq6139j84n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T17:13:08Z","receivedAt":"2015-09-17T17:13:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 09:58:00AM -0700, Junio C Hamano wrote:\n\n> > Certainly anybody who does not realize their descriptor is O_NONBLOCK\n> > and is using the spinning for correctness. I tend to think that such\n> > sites are wrong, though, and would benefit from us realizing they are\n> > spinning.\n> \n> With or without O_NONBLOCK, not looping around xread() _and_ relying\n> on the spinning for \"correctness\" means the caller is not getting\n> correctness anyway, I think, because xread() does return a short\n> read, and we deliberately and explicitly do so since 0b6806b9\n> (xread, xwrite: limit size of IO to 8MB, 2013-08-20).\n\nI think they have to loop for correctness, but they may do this:\n\n if (xread(fd, buf, len) < 0)\n\tdie_errno(\"OMG, an error!\");\n\nwhich is not correct if \"fd\" is unknowingly non-blocking. As Stefan\nmentioned, we do not set O_NONBLOCK ourselves very much, but I wonder if\nwe could inherit it from the environment in some cases.\n\nThe spinning behavior is not great, but does mean that we spin and\ncontinue rather than bailing with an error.\n\n> > But I think you can't quite get away with leaving strbuf_read untouched\n> > in this case. On error, it wants to restore the original value of the\n> > strbuf before the strbuf_read call. Which means that we throw away\n> > anything read into the strbuf before we get EAGAIN, and the caller never\n> > gets to see it.\n> \n> I agree we need to teach strbuf_read() that xread() is now nicer on\n> O_NONBLOCK; perhaps like this?\n> \n> strbuf.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index cce5eed..49104d7 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -368,6 +368,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> \n> \t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> \t\tif (cnt < 0) {\n> +\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK)\n> +\t\t\t\tbreak;\n> \t\t\tif (oldalloc == 0)\n> \t\t\t\tstrbuf_release(sb);\n> \t\t\telse\n\nIf we get EAGAIN on the first read, this will return \"0\", and I think we\nend up in the \"was it EOF, or EAGAIN?\" situation I mentioned earlier.\nIf we reset errno to \"0\" at the top of the function, we could get around\none problem, but it still makes an annoying interface: the caller has to\ncheck errno for any 0-return to figure out if it was really EOF, or just\nEAGAIN.\n\nIf we return -1, though, we have a similar annoyance. If the caller\nnotices a -1 return value and finds EAGAIN, they still may need to check\nsb->len to see if they made forward progress and have data they should\nbe dealing with.\n\n-Peff\n"},{"id":"270242","messageId":"CAGZ79kY3ueSqVofLmo7jiRQv6R7rXmFvkoj5KouiXi+__h8_GQ@mail.gmail.com","threadId":"40367","inReplyTo":"xmqq6139j84n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T17:20:55Z","receivedAt":"2015-09-17T17:20:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 9:58 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Thu, Sep 17, 2015 at 09:13:40AM -0700, Junio C Hamano wrote:\n>>\n>>> And your new caller that does O_NONBLOCK wants to do more than\n>>> looping upon EWOULDBLOCK. It certainly would not want us to loop\n>>> here.\n>>>\n>>> So I wonder if you can just O_NONBLOCK the fd and use the usual\n>>> strbuf_read(), i.e. without any change in this patch, and update\n>>> xread() to _unconditionally_ return when read(2) says EAGAIN or\n>>> EWOULDBLOCK.\n>>>\n>>> What would that break?\n>>\n>> Certainly anybody who does not realize their descriptor is O_NONBLOCK\n>> and is using the spinning for correctness. I tend to think that such\n>> sites are wrong, though, and would benefit from us realizing they are\n>> spinning.\n>\n> With or without O_NONBLOCK, not looping around xread() _and_ relying\n> on the spinning for \"correctness\" means the caller is not getting\n> correctness anyway, I think, because xread() does return a short\n> read, and we deliberately and explicitly do so since 0b6806b9\n> (xread, xwrite: limit size of IO to 8MB, 2013-08-20).\n>\n>> But I think you can't quite get away with leaving strbuf_read untouched\n>> in this case. On error, it wants to restore the original value of the\n>> strbuf before the strbuf_read call. Which means that we throw away\n>> anything read into the strbuf before we get EAGAIN, and the caller never\n>> gets to see it.\n>\n> I agree we need to teach strbuf_read() that xread() is now nicer on\n> O_NONBLOCK; perhaps like this?\n>\n> strbuf.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index cce5eed..49104d7 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -368,6 +368,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>\n> cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> if (cnt < 0) {\n> + if (errno == EAGAIN || errno == EWOULDBLOCK)\n> + break;\n\nThis would need xread to behave differently too. (if EAGAIN, return)\n\nLooking at xread to answer: \"Why did we have a spinning loop in case of\nEAGAIN in the first place?\" I ended up with 1c15afb9343b (2005-12-19,\nxread/xwrite:\ndo not worry about EINTR at calling sites.)\n\nThere we had lots of EAGAIN checks sprinkled through the code base, so we had\nnon blocking IO back then?\n\nI think I chose to not use xread, as it contradicts everything we want\nin the non\nblocking case. We want to ignore any operations with a recoverable error (EAGAIN\nand EINTR) and keep going with the rest of the program. In the blocking case\n(as it is used currently) we can just have the checks from xread moved to\nstrbuf_read.\n\n> if (oldalloc == 0)\n> strbuf_release(sb);\n> else\n"},{"id":"270244","messageId":"CAGZ79kaZOyqwbf+BpG2oPBBt5zj3=q-abk+F-HdkZPL3GzTzsw@mail.gmail.com","threadId":"40367","inReplyTo":"20150917171308.GA28046@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T17:26:19Z","receivedAt":"2015-09-17T17:26:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 10:13 AM, Jeff King wrote:\n> On Thu, Sep 17, 2015 at 09:58:00AM -0700, Junio C Hamano wrote:\n>\n>> > Certainly anybody who does not realize their descriptor is O_NONBLOCK\n>> > and is using the spinning for correctness. I tend to think that such\n>> > sites are wrong, though, and would benefit from us realizing they are\n>> > spinning.\n>>\n>> With or without O_NONBLOCK, not looping around xread() _and_ relying\n>> on the spinning for \"correctness\" means the caller is not getting\n>> correctness anyway, I think, because xread() does return a short\n>> read, and we deliberately and explicitly do so since 0b6806b9\n>> (xread, xwrite: limit size of IO to 8MB, 2013-08-20).\n>\n> I think they have to loop for correctness, but they may do this:\n>\n> if (xread(fd, buf, len) < 0)\n> die_errno(\"OMG, an error!\");\n>\n> which is not correct if \"fd\" is unknowingly non-blocking. As Stefan\n> mentioned, we do not set O_NONBLOCK ourselves very much, but I wonder if\n> we could inherit it from the environment in some cases.\n>\n> The spinning behavior is not great, but does mean that we spin and\n> continue rather than bailing with an error.\n>\n>> > But I think you can't quite get away with leaving strbuf_read untouched\n>> > in this case. On error, it wants to restore the original value of the\n>> > strbuf before the strbuf_read call. Which means that we throw away\n>> > anything read into the strbuf before we get EAGAIN, and the caller never\n>> > gets to see it.\n>>\n>> I agree we need to teach strbuf_read() that xread() is now nicer on\n>> O_NONBLOCK; perhaps like this?\n>>\n>> strbuf.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/strbuf.c b/strbuf.c\n>> index cce5eed..49104d7 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -368,6 +368,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>>\n>> cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n>> if (cnt < 0) {\n>> + if (errno == EAGAIN || errno == EWOULDBLOCK)\n>> + break;\n>> if (oldalloc == 0)\n>> strbuf_release(sb);\n>> else\n>\n> If we get EAGAIN on the first read, this will return \"0\", and I think we\n> end up in the \"was it EOF, or EAGAIN?\" situation I mentioned earlier.\n> If we reset errno to \"0\" at the top of the function, we could get around\n> one problem, but it still makes an annoying interface: the caller has to\n> check errno for any 0-return to figure out if it was really EOF, or just\n> EAGAIN.\n>\n> If we return -1, though, we have a similar annoyance. If the caller\n> notices a -1 return value and finds EAGAIN, they still may need to check\n> sb->len to see if they made forward progress and have data they should\n> be dealing with.\n\nIf errno == EAGAIN, we know it is a non blocking fd, so we could encode\nthe length read as (- 2 - length), such that\n\n...-2 the length read if EAGAIN was ignored\n-1 for error, check errno!\n0 for EOF\n+1... length if we just read it or restarted it due to EINTR.\n\nThe call site should know if it is non blocking (i.e. if the <-2 case can\nhappen) and handle it appropriately.\n\nAny callsite of today which is unaware of the fd being non blocking would break\nby this (as we want to fix the spinning anyway eventually)\n\n\n\n\n>\n> -Peff\n"},{"id":"270245","messageId":"20150917173536.GA28987@sigill.intra.peff.net","threadId":"40367","inReplyTo":"CAGZ79kaZOyqwbf+BpG2oPBBt5zj3=q-abk+F-HdkZPL3GzTzsw@mail.gmail.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T17:35:37Z","receivedAt":"2015-09-17T17:35:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 10:26:19AM -0700, Stefan Beller wrote:\n\n> > If we return -1, though, we have a similar annoyance. If the caller\n> > notices a -1 return value and finds EAGAIN, they still may need to check\n> > sb->len to see if they made forward progress and have data they should\n> > be dealing with.\n> \n> If errno == EAGAIN, we know it is a non blocking fd, so we could encode\n> the length read as (- 2 - length), such that\n> \n> ...-2 the length read if EAGAIN was ignored\n> -1 for error, check errno!\n> 0 for EOF\n> +1... length if we just read it or restarted it due to EINTR.\n> \n> The call site should know if it is non blocking (i.e. if the <-2 case can\n> happen) and handle it appropriately.\n\nI actually wonder if callers who are _expecting_ non-blocking want to\nloop in strbuf_read() at all.\n\nstrbuf_read() is really about reading to EOF, and growing the buffer to\nfit all of the input. But that's not at all what you want to do with\nnon-blocking. There you believe for some reason that data may be\navailable (probably due to poll), and you want to read one chunk of it,\nmaybe act, and then go back to polling.\n\nYou _can_ loop on read until you hit EAGAIN, but I think in general you\nshouldn't; if you get a lot of input on this fd, you'll starve all of\nthe other descriptors you're polling. You're better off to read a\nfinite amount from each descriptor, and then check again who is ready to\nread.\n\nAnd then the return value becomes a no-brainer, because it's just the\nreturn value of read(). Either you got some data, you got EOF, or you\nget an error, which might be EAGAIN. You never have the case where you\ngot some data, but then you also got EOF and EAGAIN, and the caller has\nto figure out which.\n\nSo I think you really want something like:\n\n ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n {\n\tssize_t cnt;\n\n\tstrbuf_grow(hint ? hint : 8192);\n\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n\tif (cnt > 0)\n\t\tstrbuf_setlen(sb->len + cnt);\n\treturn cnt;\n }\n\n(where I'm assuming xread passes us back EAGAIN; we could also replace\nit with read and loop on EINTR ourselves).\n\n-Peff\n"},{"id":"270247","messageId":"CAGZ79kYnZr3nb_5n-5J0vCMi7xb91y-OkrAEq8-uH2PvzmkSmA@mail.gmail.com","threadId":"40367","inReplyTo":"20150917173536.GA28987@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T17:45:40Z","receivedAt":"2015-09-17T17:45:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 10:35 AM, Jeff King wrote:\n>\n\n>\n> You _can_ loop on read until you hit EAGAIN, but I think in general you\n> shouldn't; if you get a lot of input on this fd, you'll starve all of\n> the other descriptors you're polling. You're better off to read a\n> finite amount from each descriptor, and then check again who is ready to\n> read.\n\nThat's what I do with the current implementation. Except it's not as clear and\nconcise as I patched it into the strbuf_read.\n\n>\n> And then the return value becomes a no-brainer, because it's just the\n> return value of read(). Either you got some data, you got EOF, or you\n> get an error, which might be EAGAIN. You never have the case where you\n> got some data, but then you also got EOF and EAGAIN, and the caller has\n> to figure out which.\n>\n> So I think you really want something like:\n>\n> ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n> {\n> ssize_t cnt;\n>\n> strbuf_grow(hint ? hint : 8192);\n> cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> if (cnt > 0)\n> strbuf_setlen(sb->len + cnt);\n> return cnt;\n> }\n>\n> (where I'm assuming xread passes us back EAGAIN; we could also replace\n> it with read and loop on EINTR ourselves).\n\nYeah that's exactly what I am looking for (the hint may even be over\nengineered here, as I have no clue how much data comes back).\n\nSo I guess I could just use that new method now.\n\n\n> I actually wonder if callers who are _expecting_ non-blocking want to\n> loop in strbuf_read() at all.\n>\n> strbuf_read() is really about reading to EOF, and growing the buffer to\n> fit all of the input. But that's not at all what you want to do with\n> non-blocking. There you believe for some reason that data may be\n> available (probably due to poll), and you want to read one chunk of it,\n> maybe act, and then go back to polling.\n\nAs a micro project (leftover bit maybe?):\nWhen strbuf_read is reading data out from a non blocking pipe, we're currently\nspinning (with the EAGAIN/EWOULDBLOCK). Introduce a call to poll\nto reduce the spinning.\n\n>\n> -Peff\n"},{"id":"270248","messageId":"20150917175008.GA29601@sigill.intra.peff.net","threadId":"40367","inReplyTo":"CAGZ79kYnZr3nb_5n-5J0vCMi7xb91y-OkrAEq8-uH2PvzmkSmA@mail.gmail.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T17:50:08Z","receivedAt":"2015-09-17T17:50:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 10:45:40AM -0700, Stefan Beller wrote:\n\n> > You _can_ loop on read until you hit EAGAIN, but I think in general you\n> > shouldn't; if you get a lot of input on this fd, you'll starve all of\n> > the other descriptors you're polling. You're better off to read a\n> > finite amount from each descriptor, and then check again who is ready to\n> > read.\n> \n> That's what I do with the current implementation. Except it's not as clear and\n> concise as I patched it into the strbuf_read.\n\nIs it? I thought the implementation you posted bumped the existing\nstrbuf_read to strbuf_buf_internal, including the loop. So as long as we\nare not getting EAGAIN, it will keep reading forever. Actually not quite\ntrue, as any read shorter than 8192 bytes will cause us to jump out of\nthe loop, too, but if we assume that the caller is feeding us data\nfaster than we can read it, we'll never exit strbuf_read_nonblock() and\nserve any of the other descriptors.\n\n-Peff\n"},{"id":"270249","messageId":"CAGZ79kYKD6e=JmeFgbqHRmDAb1W3jraBayWHjA=-D+SXzubULw@mail.gmail.com","threadId":"40367","inReplyTo":"20150917175008.GA29601@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T17:53:06Z","receivedAt":"2015-09-17T17:53:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 10:50 AM, Jeff King wrote:\n> On Thu, Sep 17, 2015 at 10:45:40AM -0700, Stefan Beller wrote:\n>\n>> > You _can_ loop on read until you hit EAGAIN, but I think in general you\n>> > shouldn't; if you get a lot of input on this fd, you'll starve all of\n>> > the other descriptors you're polling. You're better off to read a\n>> > finite amount from each descriptor, and then check again who is ready to\n>> > read.\n>>\n>> That's what I do with the current implementation. Except it's not as clear and\n>> concise as I patched it into the strbuf_read.\n>\n> Is it? I thought the implementation you posted bumped the existing\n> strbuf_read to strbuf_buf_internal, including the loop. So as long as we\n> are not getting EAGAIN, it will keep reading forever.\n\nYou'll get EAGAIN pretty fast though, as all you do is reading as fast\nas you can.\n\n> Actually not quite\n> true, as any read shorter than 8192 bytes will cause us to jump out of\n> the loop, too, but if we assume that the caller is feeding us data\n> faster than we can read it, we'll never exit strbuf_read_nonblock() and\n> serve any of the other descriptors.\n\nI see the difference now. That makes sense.\n\n>\n> -Peff\n"},{"id":"270250","messageId":"xmqqoah1hqxs.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"20150917171308.GA28046@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T17:54:39Z","receivedAt":"2015-09-17T17:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think they have to loop for correctness, but they may do this:\n>\n> if (xread(fd, buf, len) < 0)\n> \tdie_errno(\"OMG, an error!\");\n>\n> which is not correct if \"fd\" is unknowingly non-blocking.\n> ...\n> The spinning behavior is not great, but does mean that we spin and\n> continue rather than bailing with an error.\n\nOK, that is a problem (I just read through \"git grep xread -- \\*.c\".\nThere aren't that many codepaths that read from a fd that we didn't\nopen ourselves, but there indeed are some.\n\nSo it seems that we would want a xread() that spins to help such\ncodepaths, and xread_nonblock() that gives a short-read upon\nEWOULDBLOCK. They can share a helper function, of course.\n\n> If we reset errno to \"0\" at the top of the function, we could get around\n> one problem, but it still makes an annoying interface: the caller has to\n> check errno for any 0-return to figure out if it was really EOF, or just\n> EAGAIN.\n>\n> If we return -1, though, we have a similar annoyance. If the caller\n> notices a -1 return value and finds EAGAIN, they still may need to check\n> sb->len to see if they made forward progress and have data they should\n> be dealing with.\n\nOK. Trying to repurpose strbuf_read() for non-blocking FD was a\nsilly idea to begin with, as it wants to read thru to the EOF, and\nsetting FD explicitly to O_NONBLOCK is a sign that the caller wants\nto grab as much as possible and does not want to wait for the EOF.\n\nSo assuming we want strbuf_read_nonblock(), what interface do we\nwant from it? We could:\n\n * Have it grab as much as possible into sb as long as it does not\n block?\n\n * Have it grab reasonably large amount into sb, and not blocking is\n an absolute requirement, but the function is not required to read\n everything that is available on the FD (i.e. the caller is\n expected to loop)?\n\nIf we choose the latter, then your \"EAGAIN? EOF?\" becomes easier,\nno? We only have to do a single call to xread(), and then we:\n\n - get EAGAIN or EWOULDBLOCK; leave sb as-is, set errno==EAGAIN and\n return -1.\n\n - get something (in which case that is not an EOF yet); append to\n sb, return the number of bytes.\n\n - get EOF; leave sb as-is, return 0.\n\n\nssize_t strbuf_read_nonblock(struct strbuf *sb, int fd, size_t hint)\n{\n\tstrbuf_grow(sb, hint ? hint : 8192);\n\tssize_t want = sb->alloc - sb->len - 1;\n\tssize_t got = xread_nonblock(fd, sb->buf + sb->len, want);\n\tif (got < 0) {\n\t\tif (errno == EWOULDBLOCK)\n\t\t\terrno = EAGAIN; /* make life easier for the caller */\n\t\treturn got;\n\t}\n\tsb->len += got;\n\tsb->buf[sb->len] = '\\0';\n\treturn got;\n}\n"},{"id":"270251","messageId":"xmqqk2rphqsd.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"20150917173536.GA28987@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T17:57:54Z","receivedAt":"2015-09-17T17:57:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I actually wonder if callers who are _expecting_ non-blocking want to\n> loop in strbuf_read() at all.\n>\n> strbuf_read() is really about reading to EOF, and growing the buffer to\n> fit all of the input. But that's not at all what you want to do with\n> non-blocking. There you believe for some reason that data may be\n> available (probably due to poll), and you want to read one chunk of it,\n> maybe act, and then go back to polling.\n\nI think I could have gone to grab a cup of coffee instead of typing\nanother message that essentially said the same thing ;-)\n"},{"id":"270252","messageId":"20150917180205.GA29985@sigill.intra.peff.net","threadId":"40367","inReplyTo":"xmqqoah1hqxs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/10] strbuf: Add strbuf_read_noblock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-17T18:02:06Z","receivedAt":"2015-09-17T18:02:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2015 at 10:54:39AM -0700, Junio C Hamano wrote:\n\n> OK. Trying to repurpose strbuf_read() for non-blocking FD was a\n> silly idea to begin with, as it wants to read thru to the EOF, and\n> setting FD explicitly to O_NONBLOCK is a sign that the caller wants\n> to grab as much as possible and does not want to wait for the EOF.\n> \n> So assuming we want strbuf_read_nonblock(), what interface do we\n> want from it? We could:\n> \n> * Have it grab as much as possible into sb as long as it does not\n> block?\n> \n> * Have it grab reasonably large amount into sb, and not blocking is\n> an absolute requirement, but the function is not required to read\n> everything that is available on the FD (i.e. the caller is\n> expected to loop)?\n\nI think we are crossing emails, but I would definitely argue for the\nlatter.\n\n> If we choose the latter, then your \"EAGAIN? EOF?\" becomes easier,\n> no? We only have to do a single call to xread(), and then we:\n> \n> - get EAGAIN or EWOULDBLOCK; leave sb as-is, set errno==EAGAIN and\n> return -1.\n> \n> - get something (in which case that is not an EOF yet); append to\n> sb, return the number of bytes.\n> \n> - get EOF; leave sb as-is, return 0.\n\nYes, exactly.\n\n> ssize_t strbuf_read_nonblock(struct strbuf *sb, int fd, size_t hint)\n> {\n> \tstrbuf_grow(sb, hint ? hint : 8192);\n> \tssize_t want = sb->alloc - sb->len - 1;\n> \tssize_t got = xread_nonblock(fd, sb->buf + sb->len, want);\n> \tif (got < 0) {\n> \t\tif (errno == EWOULDBLOCK)\n> \t\t\terrno = EAGAIN; /* make life easier for the caller */\n> \t\treturn got;\n> \t}\n\nI like the normalizing of errno, but that should probably be part of\nxread_nonblock(), I would think.\n\n> \tsb->len += got;\n> \tsb->buf[sb->len] = '\\0';\n\nUse strbuf_setlen() here?\n\n> \treturn got;\n\nIf \"got == 0\", we naturally do not change sb->len at all, and the strbuf\nis left unchanged. But do we want to de-allocate what we allocated in\nstrbuf_grow() above? That is what strbuf_read() does, but I think it is\neven less likely to help anybody here. With the original strbuf_read(),\nyou might do:\n\n if (strbuf_read(&buf, fd, hint) <= 0)\n\treturn; /* got nothing */\n\nbut because the nature of strbuf_read_nonblock() is to call it from a\nloop, you'd want to strbuf_release() when you leave the loop anyway.\n\n-Peff\n"},{"id":"270264","messageId":"CAPig+cT=vTJvwRkqdHw6w04HYbZM7dsUvZ0pzy9Ppe_2n-2bbg@mail.gmail.com","threadId":"40367","inReplyTo":"1442453948-9885-7-git-send-email-sbeller@google.com","subject":"Re: [PATCH 06/10] git submodule update: Redirect any output to stderr","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-17T20:31:48Z","receivedAt":"2015-09-17T20:31:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller wrote:\n> git submodule update: Redirect any output to stderr\n\nThis commit message seems to be lacking an explanation of why this is\nbeing done.\n\n> There are no tests, which fail by this.\n\nNot sure what this means. I suppose you're trying to say that this\npatch doesn't break any existing tests, but isn't that an implied goal\nof all patches posted to this list?\n\n> Signed-off-by: Stefan Beller \n> ---\n> git-submodule.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 8b0eb9a..7ef3247 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -663,7 +663,7 @@ cmd_update()\n> die_if_unmatched \"$mode\"\n> if test \"$stage\" = U\n> then\n> - echo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n> + say >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n> continue\n> fi\n> name=$(git submodule--helper name \"$sm_path\") || exit\n> @@ -684,7 +684,7 @@ cmd_update()\n>\n> if test \"$update_module\" = \"none\"\n> then\n> - echo \"Skipping submodule '$displaypath'\"\n> + say >&2 \"Skipping submodule '$displaypath'\"\n\nThese changes seem to be doing more than what the commit message\nclaims. The changed code isn't just redirecting to stderr, but is also\nnow respecting $GIT_QUIET.\n\n> continue\n> fi\n>\n> --\n> 2.6.0.rc0.131.gf624c3d\n"},{"id":"270261","messageId":"CAPig+cQtrL1jN+iePU8BBqQSrbDC78y_bPrb-MwnrqEHysmiGA@mail.gmail.com","threadId":"40367","inReplyTo":"1442453948-9885-8-git-send-email-sbeller@google.com","subject":"Re: [PATCH 07/10] git submodule update: pass --prefix only with a non empty prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-17T20:33:24Z","receivedAt":"2015-09-17T20:33:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller wrote:\n> We should not pass --prefix NULL into the helper. Although the helper\n> can deal with it, it's just messy.\n\nPerhaps the commit message can explain under what conditions $prefix\nwill be null...\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 7ef3247..3ccb0b6 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -700,7 +700,7 @@ Maybe you want to use 'update --init'?\")\"\n>\n> if ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n> then\n> - git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> + git submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> cloned_modules=\"$cloned_modules;$name\"\n> subsha1=\n> else\n> --\n> 2.6.0.rc0.131.gf624c3d\n"},{"id":"270262","messageId":"CAPig+cTbgTO_xOZ_c89n+iL6kRtpfwi7_Srcfr7YyV3as5YjZg@mail.gmail.com","threadId":"40367","inReplyTo":"1442453948-9885-10-git-send-email-sbeller@google.com","subject":"Re: [PATCH 09/10] git submodule update: cmd_update_recursive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-17T20:37:58Z","receivedAt":"2015-09-17T20:37:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller wrote:\n> git submodule update: cmd_update_recursive\n\nCommit message doesn't seem to match the patch...\n\n> Signed-off-by: Stefan Beller \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 52c2967..c40d60f 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -607,6 +607,24 @@ cmd_update_recursive()\n> fi\n> }\n>\n> +cmd_update_clone()\n> +{\n> + command=\"git checkout $subforce -q\"\n> + die_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> + say_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n> +\n> + git submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> +\n> + if (clear_local_git_env; cd \"$sm_path\" && $command \"$sha1\")\n> + then\n> + say \"$say_msg\"\n> + else\n> + err=\"${err};$die_msg\"\n> + return\n> + fi\n> + cmd_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> cmd_init \"--\" \"$@\" || return\n> fi\n>\n> - cloned_modules=\n> git submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n> err=\n> while read mode sha1 stage sm_path\n> @@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?\")\"\n>\n> if ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n> then\n> - git submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n> - cloned_modules=\"$cloned_modules;$name\"\n> - subsha1=\n> + cmd_update_clone\n> + continue\n> else\n> subsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n> git rev-parse --verify HEAD) ||\n> @@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?\")\"\n> die \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n> fi\n>\n> - # Is this something we just cloned?\n> - case \";$cloned_modules;\" in\n> - *\";$name;\"*)\n> - # then there is no local change to integrate\n> - update_module=checkout ;;\n> - esac\n> -\n> must_die_on_failure=\n> case \"$update_module\" in\n> checkout)\n> --\n> 2.6.0.rc0.131.gf624c3d\n"},{"id":"270263","messageId":"CAGZ79kbwOZRnD99UBsKiWZF5xdV8Eg4Q1U81_=2GP6bPLHverQ@mail.gmail.com","threadId":"40367","inReplyTo":"CAPig+cT=vTJvwRkqdHw6w04HYbZM7dsUvZ0pzy9Ppe_2n-2bbg@mail.gmail.com","subject":"Re: [PATCH 06/10] git submodule update: Redirect any output to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T20:38:53Z","receivedAt":"2015-09-17T20:38:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 1:31 PM, Eric Sunshine wrote:\n> On Wed, Sep 16, 2015 at 9:39 PM, Stefan Beller wrote:\n>> git submodule update: Redirect any output to stderr\n>\n> This commit message seems to be lacking an explanation of why this is\n> being done.\n>\n>> There are no tests, which fail by this.\n>\n> Not sure what this means. I suppose you're trying to say that this\n> patch doesn't break any existing tests, but isn't that an implied goal\n> of all patches posted to this list?\n\nYes they should (but they don't yet).\n\nWhat I was trying to say:\n\nIn a reroll I want to add tests to this as I was surprised\nof not breaking a test by this commit (so the behavior\nof this is untested which is bad)\n\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> git-submodule.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 8b0eb9a..7ef3247 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -663,7 +663,7 @@ cmd_update()\n>> die_if_unmatched \"$mode\"\n>> if test \"$stage\" = U\n>> then\n>> - echo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>> + say >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>> continue\n>> fi\n>> name=$(git submodule--helper name \"$sm_path\") || exit\n>> @@ -684,7 +684,7 @@ cmd_update()\n>>\n>> if test \"$update_module\" = \"none\"\n>> then\n>> - echo \"Skipping submodule '$displaypath'\"\n>> + say >&2 \"Skipping submodule '$displaypath'\"\n>\n> These changes seem to be doing more than what the commit message\n> claims. The changed code isn't just redirecting to stderr, but is also\n> now respecting $GIT_QUIET.\n\nRight, I need to redo the commit message anyways, so I'll mention that.\n\n>\n>> continue\n>> fi\n>>\n>> --\n>> 2.6.0.rc0.131.gf624c3d\n"},{"id":"270267","messageId":"xmqqbnd0iuv6.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"1442453948-9885-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 03/10] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T21:44:29Z","receivedAt":"2015-09-17T21:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> time -->\n> output: |---A---| |-B-| |----C-----------| |-D-| |-E-|\n\nBe nice and distribute the line evenly around \"C\". Same for thread\n2 below.\n\n> diff --git a/run-command.c b/run-command.c\n> index c892e9a..3af97ab 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -3,6 +3,7 @@\n> #include \"exec_cmd.h\"\n> #include \"sigchain.h\"\n> #include \"argv-array.h\"\n> +#include \"thread-utils.h\"\n> \n> void child_process_init(struct child_process *child)\n> {\n> @@ -862,3 +863,230 @@ 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> +\tint max_number_processes;\n> +\tvoid *data;\n\n> +\tget_next_task fn;\n> +\thandle_child_starting_failure fn_err;\n> +\thandle_child_return_value fn_exit;\n\nThe 'fn' feels really misnamed, especially when compared with the\nother fields. fn_task or something, perhaps.\n\nAlso I think we call a function type we define with a name that ends\nwith _fn, e.g.\n\n typedef void (*show_commit_fn)(struct commit *, void *);\n void traverse_commit_list(struct rev_info *revs,\n show_commit_fn show_commit,\n show_object_fn show_object,\n void *data)\n\nSo perhaps\n\n\tget_next_task_fn get_next_task;\n start_failure_fn start_failure;\n return_value_fn return_value;\n\nor something like that.\n\n> +\tint nr_processes;\n> +\tint all_tasks_started;\n> +\tint foreground_child;\n> +\tchar *slots;\n\nWhat does slots[i] mean? Whatever explanation you would use as an\nanswer to that question, I'd name the field after the key words used\nin the explanation. For example, if it means \"children[i] is in use\nwith a process\", then the code would be a lot happier if the field\nis called in_use[] or something.\n\nBut do not just rename the field yet...\n\n> +\tstruct child_process *children;\n> +\tstruct pollfd *pfd;\n> +\tstruct strbuf *err;\n\nstruct pollfd needs to be a contiguous array of nr_processes\nelements because that is what poll(2) takes, but other per-child\nfields would be easier to grasp if you did it like so:\n\n\tstruct parallel_processes {\n \t...\n struct {\n \tint in_use;\n struct child_process child;\n struct strbuf err;\n ... /* maybe other per-child field later */\n\t\t} *children;\n\t\t...\n\t}\n\n> +\tstruct strbuf finished_children;\n\nA strbuf that holds \"finished_children\"? Does it hold \"what\nfinished_children said\"?\n\nWe care about good naming because clearly named variables and fields\nmake the code easier to read.\n\n> +static void unblock_fd(int fd)\n\nI would probably have called this \"set_nonblocking()\".\n\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> +\t\treturn;\n> +\t}\n> +\tif (fcntl(fd, F_SETFL, flags | O_NONBLOCK)) {\n> +\t\t\twarning(\"Could not set file status flags, \"\n> +\t\t\t\"output will be degraded\");\n\nThe first line of \"warning(\" is indented one level too deep.\n\n> +\t\treturn;\n> +\t}\n\nWouldn't it be easier to follow if you did\n\n\tfn(...)\n\t{\n if (flags < 0)\n warn();\n else if (fcntrl() < 0)\n warn();\n\t}\n\n> +static void run_processes_parallel_start_new(struct parallel_processes *pp)\n> +{\n> +\tint i;\n> +\t/* Start new processes. */\n\nDrop this comment and replace it with a blank line.\n\n> +\twhile (!pp->all_tasks_started\n> +\t && pp->nr_processes < pp->max_number_processes) {\n\nRemove \"&& \" and add \" &&\" at the end of the previous line.\n\n> +\t\tfor (i = 0; i < pp->max_number_processes; i++)\n> +\t\t\tif (!pp->slots[i])\n> +\t\t\t\tbreak; /* found an empty slot */\n\nThe comment does not help us at all. if (...) break; tells as much,\nand it does not tell us what it means that slot[] being empty at all.\n\n\t\tfor (...)\n\t\t\tif (!pp->children[i].in_use)\n\t\t\t\tbreak;\n\nwould be a lot easier to follow without any comment.\n\n> +\t\tif (i == pp->max_number_processes)\n> +\t\t\tdie(\"BUG: bookkeeping is hard\");\n> +\n> +\t\tif (pp->fn(pp->data, &pp->children[i], &pp->err[i])) {\n\nThis use pattern (and the explanation in run-command.h) suggests\nthat the name of the field should be s/fn/more_task/; or something\nalong that line (and flip the return value, i.e. more_task() returns\nyes if it did grab another task, no if there is no more task).\n\n> +\t\t\tpp->all_tasks_started = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (start_command(&pp->children[i]))\n> +\t\t\tpp->fn_err(pp->data, &pp->children[i], &pp->err[i]);\n\nCan fn_err be NULL here? Shouldn't it (to give a default behaviour\nto lazy or bog-standard callers)?\n\n> +\t\tunblock_fd(pp->children[i].err);\n> +\n> +\t\tpp->nr_processes++;\n> +\t\tpp->slots[i] = 1;\n> +\t\tpp->pfd[i].fd = pp->children[i].err;\n> +\t}\n> +}\n> +\n> +static int run_processes_parallel_buffer_stderr(struct parallel_processes *pp)\n> +{\n> +\tint i;\n\nHave a blank line here between decls and the first statement.\n\n> +\ti = poll(pp->pfd, pp->max_number_processes, 100);\n\nGive a symbolic constant for this 100ms, e.g. OUTPUT_POLL_INTERVAL\nor something.\n\n> +\tif (i < 0) {\n> +\t\tif (errno == EINTR)\n> +\t\t\t/* A signal was caught; try again */\n> +\t\t\treturn -1;\n> +\t\telse {\n> +\t\t\trun_processes_parallel_cleanup(pp);\n> +\t\t\tdie_errno(\"poll\");\n> +\t\t}\n> +\t}\n\nShouldn't this be more like\n\n\twhile ((i = poll()) < 0) {\n \tif (errno == EINTR)\n \tcontinue;\n\t\tcleanup;\n\t\tdie;\n\t}\n\nThe caller after all was willing to wait for some time, and we were\ninterrupted before that time came.\n\n> +\t/* Buffer output from all pipes. */\n> +\tfor (i = 0; i < pp->max_number_processes; i++) {\n> +\t\tif (!pp->slots[i])\n> +\t\t\tcontinue;\n> +\t\tif (pp->pfd[i].revents & POLLIN)\n> +\t\t\tstrbuf_read_noblock(&pp->err[i], pp->children[i].err, 0);\n> +\t\tif (pp->foreground_child == i) {\n> +\t\t\tfputs(pp->err[i].buf, stderr);\n> +\t\t\tstrbuf_reset(&pp->err[i]);\n> +\t\t}\n\nEven if we own the output channel, we may not have read anything\nyet---poll() may have said that pfd[i] is not ready, or\nread_nonblock() may have returned EWOULDBLOCK. Perhaps check not\njust that i owns the output but err[i].len is not zero?\n\nI think output should be done outside the loop and make the comment\nbefore the loop match what the loop actually does.\n\n\t/* Buffer output from all pipes. */\n\tfor (i = 0; ...) {\n\t\tif (pp->children[i].in_use && (pp->pfd[i].revents & POLLIN))\n\t\t\tstrbuf_read_nonblock();\n\t}\n\n\t/* Drain the output from the owner of the output channel */\n\tif (pp->children[pp->output_owner].in_use &&\n\t pp->children[pp->output_owner].err.len) {\n\t \tfputs(...);\n strbuf_reset(...);\n\t}\n\n> +static void run_processes_parallel_collect_finished(struct parallel_processes *pp)\n> +{\n> +\tint i = 0;\n> +\tpid_t pid;\n> +\tint wait_status, code;\n> +\tint n = pp->max_number_processes;\n> +\t/* Collect finished child processes. */\n\nDrop this comment and replace it with a blank line.\n\n> +\twhile (pp->nr_processes > 0) {\n> +\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n> +\t\tif (pid == 0)\n> +\t\t\treturn; /* no child finished */\n\nDo we need that comment?\n\n> +\t\tif (pid < 0) {\n> +\t\t\tif (errno == EINTR)\n> +\t\t\t\treturn; /* just try again next time */\n\nCan we get EINTR here (we are passing WNOHANG above)?\n\n> +\t\t\tif (errno == EINVAL || errno == ECHILD)\n> +\t\t\t\tdie_errno(\"wait\");\n\nWhat should happen when we get an error not listed here? 'i' is\nleft as initialized to 0 and we do strbuf_read_nonblock() for the\nfirst child (which may not even be running)?\n\nYou can sweep this bug under the rug by returning here, but I\nsuspect that you would just want\n\n\tif (pid < 0)\n\t\tdie_errno();\n\nAnd that would allow you to dedent the else clause below.\n\n> +\t\t} else {\n> +\t\t\t/* Find the finished child. */\n> +\t\t\tfor (i = 0; i < pp->max_number_processes; i++)\n> +\t\t\t\tif (pp->slots[i] && pid == pp->children[i].pid)\n> +\t\t\t\t\tbreak;\n\nHmm, you are relying on the fact that a valid pid can never be 0, so\nyou can just use pp->children[i].child.pid to see if a \"slot\" is\noccupied without even using pp->slots[] (or pp->children[i].in_use).\n\n> +\t\t\tif (i == pp->max_number_processes)\n> +\t\t\t\t/*\n> +\t\t\t\t * waitpid returned another process id\n> +\t\t\t\t * which we are not waiting for.\n> +\t\t\t\t */\n> +\t\t\t\treturn;\n> +\t\t}\n> +\t\tstrbuf_read_noblock(&pp->err[i], pp->children[i].err, 0);\n\nThis is to read leftover output?\n\nAs discussed elsewhere, read_nonblock() will have to have \"read\nsome, not necessarily to the end\" semantics to serve the caller in\nrun_processes_parallel_buffer_stderr(), so you'd need a loop around\nit here to read until you see EOF.\n\nOr you may be able to just call strbuf_read() and the function may\ndo the right thing to read things through to the EOF. It depends on\nhow you redo the patch [2/10].\n\n> +\t\tif (determine_return_value(wait_status, &code, &errno,\n> +\t\t\t\t\t pp->children[i].argv[0]) < 0)\n> +\t\t\terror(\"waitpid is confused (%s)\",\n> +\t\t\t pp->children[i].argv[0]);\n> +\n> +\t\tpp->fn_exit(pp->data, &pp->children[i], code);\n\nYou are clobbering errno by calling determine_return_value() but you\ndo not use the returned value anywhere. Intended? Or should that\nbe given to fn_exit() for error reporting?\n\n> +\t\targv_array_clear(&pp->children[i].args);\n> +\t\targv_array_clear(&pp->children[i].env_array);\n> +\n> +\t\tpp->nr_processes--;\n> +\t\tpp->slots[i] = 0;\n> +\t\tpp->pfd[i].fd = -1;\n\nMental note: here the \"slot\" is cleared for the child 'i'.\n\n> +\t\tif (i != pp->foreground_child) {\n> +\t\t\tstrbuf_addbuf(&pp->finished_children, &pp->err[i]);\n> +\t\t\tstrbuf_reset(&pp->err[i]);\n\nOK, so the idea is that pp->child[i].err holds the entire output for\nany process that does not own the output channel until it dies, and\nthey are appended to pp->finished_children. That suggests that the\nname of the \"finished\" field should have \"output\" somewhere in it.\n\n> +\t\t} else {\n\nMental note: this side of if/else is what happens to the process\nthat used to own the output channel.\n\n> +\t\t\tfputs(pp->err[i].buf, stderr);\n> +\t\t\tstrbuf_reset(&pp->err[i]);\n\n... and it just flushes the final part of the output.\n\n> +\t\t\t/* Output all other finished child processes */\n> +\t\t\tfputs(pp->finished_children.buf, stderr);\n> +\t\t\tstrbuf_reset(&pp->finished_children);\n\nIf there is any, that is.\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->slots[(pp->foreground_child + i) % n])\n> +\t\t\t\t\tbreak;\n> +\t\t\tpp->foreground_child = (pp->foreground_child + i) % n;\n\n... and then picks a new owner of the output channel.\n\nUp to this point it looks sensible.\n\n> +\t\t\tfputs(pp->err[pp->foreground_child].buf, stderr);\n> +\t\t\tstrbuf_reset(&pp->err[pp->foreground_child]);\n\nI do not think these two lines need to be here, especially if you\nfollow the above advice of separating buffering and draining.\n\n> +int run_processes_parallel(int n, void *data,\n> +\t\t\t get_next_task fn,\n> +\t\t\t handle_child_starting_failure fn_err,\n> +\t\t\t handle_child_return_value fn_exit)\n> +{\n> +\tstruct parallel_processes pp;\n> +\trun_processes_parallel_init(&pp, n, data, fn, fn_err, fn_exit);\n> +\n> +\twhile (!pp.all_tasks_started || pp.nr_processes > 0) {\n\nThe former is true as long as more_task() says there may be more.\nThe latter is true as long as we have something already running.\n\nIn either case, we should keep collecting and spawning as needed.\n\n> +\t\trun_processes_parallel_start_new(&pp);\n\nBut calling start_new() unconditionally feels sloppy. It should at\nleast be something like\n\n\tif (pp.nr_processes < pp.max_processes &&\n\t !pp.all_task_started)\n\t\tstart_new_process()\n\nno?\n\n> diff --git a/test-run-command.c b/test-run-command.c\n> index 89c7de2..70b6c7a 100644\n> --- a/test-run-command.c\n> +++ b/test-run-command.c\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\nSo not only fn_err, but fn_exit is optional. You'd need to update\nthe code above to allow these.\n\n> +\n> \tfprintf(stderr, \"check usage\\n\");\n> \treturn 1;\n> }\n\nIt was a fun read. There were tons of niggles, but I didn't see\nanything fundamentally unsalvageable.\n\nThanks.\n"},{"id":"270272","messageId":"CAGZ79kbgBOkiii6B3BFwB778DM=mNcKyOoS-KQGuLf=At7hG0g@mail.gmail.com","threadId":"40367","inReplyTo":"xmqqbnd0iuv6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/10] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-17T23:19:35Z","receivedAt":"2015-09-17T23:19:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 2:44 PM, Junio C Hamano wrote:\n\n> Hmm, you are relying on the fact that a valid pid can never be 0, so\n> you can just use pp->children[i].child.pid to see if a \"slot\" is\n> occupied without even using pp->slots[] (or pp->children[i].in_use).\n\nWe could either use the pid as accessed via children[i].process.pid\nor we could rely on the children[i].process.err != -1 as start_process\nwill set it to an actual fd, and when it's done we reset it to -1.\n\nI am unsure if this make it less readable though (all your other\nsuggestions improve readability a lot!)\n\n>\n>> + if (i == pp->max_number_processes)\n>> + /*\n>> + * waitpid returned another process id\n>> + * which we are not waiting for.\n>> + */\n>> + return;\n>> + }\n>> + strbuf_read_noblock(&pp->err[i], pp->children[i].err, 0);\n>\n> This is to read leftover output?\n>\n> As discussed elsewhere, read_nonblock() will have to have \"read\n> some, not necessarily to the end\" semantics to serve the caller in\n> run_processes_parallel_buffer_stderr(), so you'd need a loop around\n> it here to read until you see EOF.\n>\n> Or you may be able to just call strbuf_read() and the function may\n> do the right thing to read things through to the EOF. It depends on\n> how you redo the patch [2/10].\n\nstrbuf_read sounds like the logical choice here (and the redo of 2/10\nsupports that).\n\n\n>> + * NEEDSWORK:\n>> + * For now we pick it randomly by doing a round\n>> + * robin. Later we may want to pick the one with\n>> + * the most output or the longest or shortest\n>> + * running process time.\n>> + */\n>> + for (i = 0; i < n; i++)\n>> + if (pp->slots[(pp->foreground_child + i) % n])\n>> + break;\n>> + pp->foreground_child = (pp->foreground_child + i) % n;\n>\n> ... and then picks a new owner of the output channel.\n>\n> Up to this point it looks sensible.\n>\n>> + fputs(pp->err[pp->foreground_child].buf, stderr);\n>> + strbuf_reset(&pp->err[pp->foreground_child]);\n>\n> I do not think these two lines need to be here, especially if you\n> follow the above advice of separating buffering and draining.\n\nThey are outputting the buffer, if the next selected child is still running.\nI mean it would output eventually anyway, but it would only output after\nthe next start of processes and poll() is done. Yeah maybe that's what we\nwant (starting new processes earlier is better than outputting earlier as we're\ntalking microseconds here).\n\n>\n>> +int run_processes_parallel(int n, void *data,\n>> + get_next_task fn,\n>> + handle_child_starting_failure fn_err,\n>> + handle_child_return_value fn_exit)\n>> +{\n>> + struct parallel_processes pp;\n>> + run_processes_parallel_init(&pp, n, data, fn, fn_err, fn_exit);\n>> +\n>> + while (!pp.all_tasks_started || pp.nr_processes > 0) {\n>\n> The former is true as long as more_task() says there may be more.\n> The latter is true as long as we have something already running.\n>\n> In either case, we should keep collecting and spawning as needed.\n>\n>> + run_processes_parallel_start_new(&pp);\n>\n> But calling start_new() unconditionally feels sloppy. It should at\n> least be something like\n>\n> if (pp.nr_processes < pp.max_processes &&\n> !pp.all_task_started)\n> start_new_process()\n>\n> no?\n\nThat's the exact condition we have in start_new_process\nso I don't want to repeat it here? We could move the while\nloop outside of it though. That looks better, done.\n"},{"id":"270273","messageId":"xmqqk2ro7czy.fsf@gitster.mtv.corp.google.com","threadId":"40367","inReplyTo":"CAGZ79kbgBOkiii6B3BFwB778DM=mNcKyOoS-KQGuLf=At7hG0g@mail.gmail.com","subject":"Re: [PATCH 03/10] run-command: add an asynchronous parallel child processor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-18T01:05:53Z","receivedAt":"2015-09-18T01:05:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Sep 17, 2015 at 2:44 PM, Junio C Hamano wrote:\n>\n>> Hmm, you are relying on the fact that a valid pid can never be 0, so\n>> you can just use pp->children[i].child.pid to see if a \"slot\" is\n>> occupied without even using pp->slots[] (or pp->children[i].in_use).\n>\n> We could either use the pid as accessed via children[i].process.pid\n> or we could rely on the children[i].process.err != -1 as start_process\n> will set it to an actual fd, and when it's done we reset it to -1.\n>\n> I am unsure if this make it less readable though (all your other\n> suggestions improve readability a lot!)\n\nSorry, the above was not a suggestion but merely an observation with\na bit of thinking aloud mixed in. I should have marked it as such\nmore clearly.\n\n>> ... and then picks a new owner of the output channel.\n>>\n>> Up to this point it looks sensible.\n>>\n>>> + fputs(pp->err[pp->foreground_child].buf, stderr);\n>>> + strbuf_reset(&pp->err[pp->foreground_child]);\n>>\n>> I do not think these two lines need to be here, especially if you\n>> follow the above advice of separating buffering and draining.\n>\n> They are outputting the buffer, if the next selected child is still running.\n> I mean it would output eventually anyway, but it would only output after\n> the next start of processes and poll() is done. Yeah maybe that's what we\n> want (starting new processes earlier is better than outputting earlier as we're\n> talking microseconds here).\n\nI am not talking about microseconds but refraining from doing the\nsame thing in multiple places.\n\n>> But calling start_new() unconditionally feels sloppy. It should at\n>> least be something like\n>>\n>> if (pp.nr_processes < pp.max_processes &&\n>> !pp.all_task_started)\n>> start_new_process()\n>>\n>> no?\n>\n> That's the exact condition we have in start_new_process\n> so I don't want to repeat it here?\n\nYou are advocating to have a function whose definition is \"this may\nor may not start a new process and the caller should not care\", and\nthen make the caller keep calling it, knowing/hoping that the caller\ndoes not care if we spawn a new thing or not.\n\nI somehow find it a highly questionable design taste to base the\ndecision on \"don't want to repeat it here\".\n\nStepping back and thinking about it, what is suggested is more\nexplicit in the caller. \"If we know we need a new worker and we can\nhave a new worker, then start it.\" is the logic in the caller in the\nsuggested structure, and we would have a well defined helper whose\nsole task is to start a new worker to be called from the caller.\nThe helper may want to check if the request to start a new one makes\nsense (e.g. if slots[] are all full, it may even want to return an\nerror), but the checks are primarily for error checking (i.e. \"we\ncan have max N processes, so make sure we do not exceed that limit\"),\nnot a semantic one (i.e. the caller _could_ choose to spawn less\nthan that max when there is a good reason to do so).\n"},{"id":"270303","messageId":"CAGZ79kZd8nZoj3Rk+ZeSfCxhH0vqp4Oaoh2+MYvFa98rzfaNxw@mail.gmail.com","threadId":"40367","inReplyTo":"xmqqk2ro7czy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/10] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-18T16:36:30Z","receivedAt":"2015-09-18T16:36:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 17, 2015 at 6:05 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Thu, Sep 17, 2015 at 2:44 PM, Junio C Hamano wrote:\n>>\n>>> Hmm, you are relying on the fact that a valid pid can never be 0, so\n>>> you can just use pp->children[i].child.pid to see if a \"slot\" is\n>>> occupied without even using pp->slots[] (or pp->children[i].in_use).\n>>\n>> We could either use the pid as accessed via children[i].process.pid\n>> or we could rely on the children[i].process.err != -1 as start_process\n>> will set it to an actual fd, and when it's done we reset it to -1.\n>>\n>> I am unsure if this make it less readable though (all your other\n>> suggestions improve readability a lot!)\n>\n> Sorry, the above was not a suggestion but merely an observation with\n> a bit of thinking aloud mixed in. I should have marked it as such\n> more clearly.\n\nOk, I see. no harm done, I did not take that as a suggestion.\n\n>\n>>> ... and then picks a new owner of the output channel.\n>>>\n>>> Up to this point it looks sensible.\n>>>\n>>>> + fputs(pp->err[pp->foreground_child].buf, stderr);\n>>>> + strbuf_reset(&pp->err[pp->foreground_child]);\n>>>\n>>> I do not think these two lines need to be here, especially if you\n>>> follow the above advice of separating buffering and draining.\n>>\n>> They are outputting the buffer, if the next selected child is still running.\n>> I mean it would output eventually anyway, but it would only output after\n>> the next start of processes and poll() is done. Yeah maybe that's what we\n>> want (starting new processes earlier is better than outputting earlier as we're\n>> talking microseconds here).\n>\n> I am not talking about microseconds but refraining from doing the\n> same thing in multiple places.\n\nok\n\n>\n>>> But calling start_new() unconditionally feels sloppy. It should at\n>>> least be something like\n>>>\n>>> if (pp.nr_processes < pp.max_processes &&\n>>> !pp.all_task_started)\n>>> start_new_process()\n>>>\n>>> no?\n>>\n>> That's the exact condition we have in start_new_process\n>> so I don't want to repeat it here?\n>\n> You are advocating to have a function whose definition is \"this may\n> or may not start a new process and the caller should not care\", and\n> then make the caller keep calling it, knowing/hoping that the caller\n> does not care if we spawn a new thing or not.\n>\n> I somehow find it a highly questionable design taste to base the\n> decision on \"don't want to repeat it here\".\n>\n> Stepping back and thinking about it, what is suggested is more\n> explicit in the caller. \"If we know we need a new worker and we can\n> have a new worker, then start it.\" is the logic in the caller in the\n> suggested structure, and we would have a well defined helper whose\n> sole task is to start a new worker to be called from the caller.\n> The helper may want to check if the request to start a new one makes\n> sense (e.g. if slots[] are all full, it may even want to return an\n> error), but the checks are primarily for error checking (i.e. \"we\n> can have max N processes, so make sure we do not exceed that limit\"),\n> not a semantic one (i.e. the caller _could_ choose to spawn less\n> than that max when there is a good reason to do so).\n\nI would not put the decision to spawn fewer processes in the caller either,\nMy understanding is to have one high level function which outlines the algorithm\nlike:\n\n loop:\n spawn_children_as_necessary\n make_sure_pipes_don't_overflow\n offer_early_output\n take_care_of_finished_children\n\nsuch that the main function reads more like a bullet point list explaining\nhow it roughly works. Each helper function should come up with its own strategy,\nso spawn_children_as_necessary could be\n\n spawn_children_as_necessary:\n while less than n children and there are more tasks:\n spawn_one_child\n\nfor now. Later we can add more logic if necessary there. But I'd prefer to have\nthese decisions put into the helpers.\n\nHaving written this, I think I'll separate the function to drain the pipes and\nthe early output and separate the helper to start children into two:\none to make the decision and one to actually start just one child.\n\n\n\n>\n>\n"}]}