{"thread":{"id":"41494","subject":"[PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","startedAt":"2016-02-24T03:20:12Z","lastAt":"2016-02-25T02:56:33Z","messageCount":13,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"279114","messageId":"1456284017-26141-1-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":null,"subject":"[PATCHv15 0/5] Expose submodule parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:12Z","receivedAt":"2016-02-24T03:20:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This build on top of 163b9b1f919c762a4bfb693b3aa05ef1aa627fee\n(origin/sb/submodule-parallel-update~3) and replaces the commits \norigin/sb/submodule-parallel-update~2.. \n\n* Renamed inspect_clone_next_submodule to prepare_to_clone_next_submodule\n  and reordered the arguments thereof\n* Comments for the struct submodule_update_clone which is passed around\n* Better handling around LFs.\n* Renamed struct child_process *cp to *child\n* Print #unmatched in git-submodule.sh instead of the helper\n\n> \n> \tif (pp->update.type == SM_UPDATE_NONE\n> \t    || (pp->update.type == SM_UPDATE_UNSPECIFIED\n> \t        && sub->update_strategy.type == SM_UPDATE_NONE)) {\n> \n> What does pp stand for?\n\nI think I took it as parallel_process when starting off from the parallel\nprocessing machinery. I'll rename it (probably to suc as short for struct\nsubmodule_update_clone).\n\n> > +\tif (pp->recursive_prefix)\n> > +\t\tdisplaypath = relative_path(pp->recursive_prefix,\n> > +\t\t\t\t\t    ce->name, &displaypath_sb);\n> \n> Nit: could use braces.\n\nWhy? I would understand a few lines above where we have an if nested in an\nif with braces. But here we have a pretty straighforward one statement per case\ncondition.\n\n\n> > +\tsub = submodule_from_path(null_sha1, ce->name);\n> \n> It's common to call submodule_from_path with null_sha1 as a parameter\n> but I have trouble continuing to remember what that means.  Maybe\n> there should be a separate function that handles that?  As a\n> side-effect, the name and docstring of that function could explain\n> what it means, which I still am not sure about. :)\n\nI'll do that as a followup cleanup patch as it affects more than just the\nnew code.\n\n>> +\t\tOPT_STRING(0, \"reference\", &pp.reference, \"<repository>\",\n>> +\t\t\t   N_(\"Use the local reference repository \"\n>> +\t\t\t      \"instead of a full clone\")),\n\n> Is this allowed to be relative?  If so, what is it relative to?\n\nIt is passing on the argument to clone, so I assume the same rules apply as for\ngit-clone.\n\nThanks,\nStefan\n\nStefan Beller (5):\n  run-command: expose default_{start_failure, task_finished}\n  run_processes_parallel: add LF when caller is sloppy\n  git submodule update: have a dedicated helper for cloning\n  submodule update: expose parallelism to the user\n  clone: allow an explicit argument for parallel submodule clones\n\n Documentation/git-clone.txt     |   6 +-\n Documentation/git-submodule.txt |   7 +-\n builtin/clone.c                 |  19 ++-\n builtin/submodule--helper.c     | 255 +++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh                |  56 ++++-----\n run-command.c                   |  35 ++++--\n run-command.h                   |  19 +++\n t/t0061-run-command.sh          |  26 ++++\n t/t7406-submodule-update.sh     |  27 +++++\n test-run-command.c              |  18 +++\n 10 files changed, 412 insertions(+), 56 deletions(-)\n\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279115","messageId":"1456284017-26141-2-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":"1456284017-26141-1-git-send-email-sbeller@google.com","subject":"[PATCHv15 1/5] run-command: expose default_{start_failure, task_finished}","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:13Z","receivedAt":"2016-02-24T03:20:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We want to reuse the error reporting facilities in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c | 18 +++++++++---------\n run-command.h | 19 +++++++++++++++++++\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 51fd72c..d03ecaa 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -902,10 +902,10 @@ struct parallel_processes {\n \tstruct strbuf buffered_output; /* of finished children */\n };\n \n-static int default_start_failure(struct child_process *cp,\n-\t\t\t\t struct strbuf *err,\n-\t\t\t\t void *pp_cb,\n-\t\t\t\t void *pp_task_cb)\n+int default_start_failure(struct child_process *cp,\n+\t\t\t  struct strbuf *err,\n+\t\t\t  void *pp_cb,\n+\t\t\t  void *pp_task_cb)\n {\n \tint i;\n \n@@ -916,11 +916,11 @@ static int default_start_failure(struct child_process *cp,\n \treturn 0;\n }\n \n-static int default_task_finished(int result,\n-\t\t\t\t struct child_process *cp,\n-\t\t\t\t struct strbuf *err,\n-\t\t\t\t void *pp_cb,\n-\t\t\t\t void *pp_task_cb)\n+int default_task_finished(int result,\n+\t\t\t  struct child_process *cp,\n+\t\t\t  struct strbuf *err,\n+\t\t\t  void *pp_cb,\n+\t\t\t  void *pp_task_cb)\n {\n \tint i;\n \ndiff --git a/run-command.h b/run-command.h\nindex d5a57f9..a054fa6 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -164,6 +164,15 @@ typedef int (*start_failure_fn)(struct child_process *cp,\n \t\t\t\tvoid *pp_task_cb);\n \n /**\n+ * If a command fails to start, then print an error message stating the\n+ * exact command which failed.\n+ */\n+int default_start_failure(struct child_process *cp,\n+\t\t\t  struct strbuf *err,\n+\t\t\t  void *pp_cb,\n+\t\t\t  void *pp_task_cb);\n+\n+/**\n  * This callback is called on every child process that finished processing.\n  *\n  * You must not write to stdout or stderr in this function. Add your\n@@ -184,6 +193,16 @@ typedef int (*task_finished_fn)(int result,\n \t\t\t\tvoid *pp_task_cb);\n \n /**\n+ * If the child process returns with a non zero error code, print\n+ * an error message of the exact command which failed.\n+ */\n+int default_task_finished(int result,\n+\t\t\t  struct child_process *cp,\n+\t\t\t  struct strbuf *err,\n+\t\t\t  void *pp_cb,\n+\t\t\t  void *pp_task_cb);\n+\n+/**\n  * Runs up to n processes at the same time. Whenever a process can be\n  * started, the callback get_next_task_fn is called to obtain the data\n  * required to start another child process.\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279113","messageId":"1456284017-26141-3-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":"1456284017-26141-1-git-send-email-sbeller@google.com","subject":"[PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:14Z","receivedAt":"2016-02-24T03:20:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When the callers of parallel processing machine are sloppy with their\nmessages, make sure the output is terminated with LF after one child\nprocess is handled. This will not mess with the internal state of the\noutput, i.e. after all messages for one child process are process\nincluding the callbacks as well as the actual output of the child\nwe may add an LF if the output is not ended with an LF.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n>From a discussion on a later patch, Jonathan said:\n> It looks like pp_start_one takes the content of err without adding\n> a \\n.  That's a bug in pp_start_one and pp_collect_finished IMHO.\n\n run-command.c          | 17 +++++++++++++++--\n t/t0061-run-command.sh | 26 ++++++++++++++++++++++++++\n test-run-command.c     | 18 ++++++++++++++++++\n 3 files changed, 59 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d03ecaa..8abaae4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -897,6 +897,7 @@ struct parallel_processes {\n \tstruct pollfd *pfd;\n \n \tunsigned shutdown : 1;\n+\tunsigned ended_with_newline: 1;\n \n \tint output_owner;\n \tstruct strbuf buffered_output; /* of finished children */\n@@ -979,6 +980,7 @@ static void pp_init(struct parallel_processes *pp,\n \tpp->nr_processes = 0;\n \tpp->output_owner = 0;\n \tpp->shutdown = 0;\n+\tpp->ended_with_newline = 1;\n \tpp->children = xcalloc(n, sizeof(*pp->children));\n \tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n \tstrbuf_init(&pp->buffered_output, 0);\n@@ -1053,6 +1055,7 @@ static int pp_start_one(struct parallel_processes *pp)\n \t\t\t\t\t pp->data,\n \t\t\t\t\t &pp->children[i].data);\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\tstrbuf_addch(&pp->buffered_output, '\\n');\n \t\tstrbuf_reset(&pp->children[i].err);\n \t\tif (code)\n \t\t\tpp->shutdown = 1;\n@@ -1095,9 +1098,11 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n static void pp_output(struct parallel_processes *pp)\n {\n \tint i = pp->output_owner;\n-\tif (pp->children[i].state == GIT_CP_WORKING &&\n-\t    pp->children[i].err.len) {\n+\tsize_t len = pp->children[i].err.len;\n+\tif (pp->children[i].state == GIT_CP_WORKING && len) {\n \t\tfputs(pp->children[i].err.buf, stderr);\n+\t\tpp->ended_with_newline =\n+\t\t\t(pp->children[i].err.buf[len - 1] == '\\n');\n \t\tstrbuf_reset(&pp->children[i].err);\n \t}\n }\n@@ -1107,6 +1112,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \tint i, code;\n \tint n = pp->max_processes;\n \tint result = 0;\n+\tssize_t len;\n \n \twhile (pp->nr_processes > 0) {\n \t\tfor (i = 0; i < pp->max_processes; i++)\n@@ -1131,12 +1137,19 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \t\tpp->pfd[i].fd = -1;\n \t\tchild_process_init(&pp->children[i].process);\n \n+\t\tlen = pp->children[i].err.len - 1;\n+\t\tif (len >= 0 && pp->children[i].err.buf[len] != '\\n')\n+\t\t\tstrbuf_addch(&pp->children[i].err, '\\n');\n+\n \t\tif (i != pp->output_owner) {\n \t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n \t\t\tstrbuf_reset(&pp->children[i].err);\n \t\t} else {\n+\t\t\tif (len == -1 && !pp->ended_with_newline)\n+\t\t\t\tstrbuf_addch(&pp->children[i].err, '\\n');\n \t\t\tfputs(pp->children[i].err.buf, stderr);\n \t\t\tstrbuf_reset(&pp->children[i].err);\n+\t\t\tpp->ended_with_newline = 1;\n \n \t\t\t/* Output all other finished child processes */\n \t\t\tfputs(pp->buffered_output.buf, stderr);\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 12228b4..5c6822c 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -77,6 +77,32 @@ test_expect_success 'run_command runs in parallel with more tasks than jobs avai\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'run_command ensures each command ends in LF' '\n+\ttest-run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<-EOF\n+preloaded output of a child\n+preloaded output of a child\n+preloaded output of a child\n+preloaded output of a child\n+EOF\n+\n+test_expect_success 'run_command ensures each command ends in LF when output is only in starting cb' '\n+\ttest-run-command run-command-parallel 3 sh -c true  2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<-EOF\n+EOF\n+\n+test_expect_success 'run_command ensures each command ends in LF except when there is no output' '\n+\ttest-run-command run-command-parallel-silent 3 sh -c true  2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n cat >expect <<-EOF\n preloaded output of a child\n asking for a quick stop\ndiff --git a/test-run-command.c b/test-run-command.c\nindex fbe0a27..ca1ee97 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -31,6 +31,20 @@ static int parallel_next(struct child_process *cp,\n \treturn 1;\n }\n \n+static int parallel_next_silent(struct child_process *cp,\n+\t\t\t struct strbuf *err,\n+\t\t\t void *cb,\n+\t\t\t void **task_cb)\n+{\n+\tstruct child_process *d = cb;\n+\tif (number_callbacks >= 4)\n+\t\treturn 0;\n+\n+\targv_array_pushv(&cp->args, d->argv);\n+\tnumber_callbacks++;\n+\treturn 1;\n+}\n+\n static int no_job(struct child_process *cp,\n \t\t  struct strbuf *err,\n \t\t  void *cb,\n@@ -71,6 +85,10 @@ int main(int argc, char **argv)\n \tjobs = atoi(argv[2]);\n \tproc.argv = (const char **)argv + 3;\n \n+\tif (!strcmp(argv[1], \"run-command-parallel-silent\"))\n+\t\texit(run_processes_parallel(jobs, parallel_next_silent,\n+\t\t\t\t\t    NULL, NULL, &proc));\n+\n \tif (!strcmp(argv[1], \"run-command-parallel\"))\n \t\texit(run_processes_parallel(jobs, parallel_next,\n \t\t\t\t\t    NULL, NULL, &proc));\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279118","messageId":"1456284017-26141-4-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":"1456284017-26141-1-git-send-email-sbeller@google.com","subject":"[PATCHv15 3/5] git submodule update: have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:15Z","receivedAt":"2016-02-24T03:20:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This introduces a new helper function in git submodule--helper\nwhich takes care of cloning all submodules, which we want to\nparallelize eventually.\n\nSome tests (such as empty URL, update_mode=none) are required in the\nhelper to make the decision for cloning. These checks have been\nmoved into the C function as well (no need to repeat them in the\nshell script).\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/submodule--helper.c | 243 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  47 +++------\n 2 files changed, 256 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..85fb702 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,248 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct submodule_update_clone {\n+\t/* index into 'list', the list of submodules to look into for cloning */\n+\tint current;\n+\tstruct module_list list;\n+\tint warn_if_uninitialized : 1;\n+\t/* update parameter passed via commandline*/\n+\tstruct submodule_update_strategy update;\n+\t/* configuration parameters which are passed on to the children */\n+\tint quiet;\n+\tconst char *reference;\n+\tconst char *depth;\n+\tconst char *recursive_prefix;\n+\tconst char *prefix;\n+\t/* lines to be output */\n+\tstruct string_list projectlines;\n+\t/* If we want to stop as fast as possible and return an error */\n+\tint quickstop : 1;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n+\tSUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \\\n+\tSTRING_LIST_INIT_DUP, 0}\n+\n+/**\n+ * Inspect if 'ce' needs to be cloned. If so, prepare the 'child' to be running\n+ * the clone and return non zero.\n+ */\n+static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n+\t\t\t\t\t   struct child_process *child,\n+\t\t\t\t\t   struct submodule_update_clone *suc,\n+\t\t\t\t\t   struct strbuf *err)\n+{\n+\tconst struct submodule *sub = NULL;\n+\tstruct strbuf displaypath_sb = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *displaypath = NULL;\n+\tchar *url = NULL;\n+\tint needs_cloning = 0;\n+\n+\tif (ce_stage(ce)) {\n+\t\tif (suc->recursive_prefix) {\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t    suc->recursive_prefix, ce->name);\n+\t\t} else {\n+\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t    ce->name);\n+\t\t}\n+\t\tgoto cleanup;\n+\t}\n+\n+\tsub = submodule_from_path(null_sha1, ce->name);\n+\n+\tif (suc->recursive_prefix)\n+\t\tdisplaypath = relative_path(suc->recursive_prefix,\n+\t\t\t\t\t    ce->name, &displaypath_sb);\n+\telse\n+\t\tdisplaypath = ce->name;\n+\n+\tif (suc->update.type == SM_UPDATE_NONE\n+\t    || (suc->update.type == SM_UPDATE_UNSPECIFIED\n+\t\t&& sub->update_strategy.type == SM_UPDATE_NONE)) {\n+\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\",\n+\t\t\t    displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * Looking up the url in .git/config.\n+\t * We must not fall back to .gitmodules as we only want\n+\t * to process configured submodules.\n+\t */\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\tgit_config_get_string(sb.buf, &url);\n+\tif (!url) {\n+\t\t/*\n+\t\t * Only mention uninitialized submodules when its\n+\t\t * path have been specified\n+\t\t */\n+\t\tif (suc->warn_if_uninitialized)\n+\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t    \"Maybe you want to use 'update --init'?\\n\"),\n+\t\t\t\t    displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\tneeds_cloning = !file_exists(sb.buf);\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\tneeds_cloning, ce->name);\n+\tstring_list_append(&suc->projectlines, sb.buf);\n+\n+\tif (!needs_cloning)\n+\t\tgoto cleanup;\n+\n+\tchild->git_cmd = 1;\n+\tchild->no_stdin = 1;\n+\tchild->stdout_to_stderr = 1;\n+\tchild->err = -1;\n+\targv_array_push(&child->args, \"submodule--helper\");\n+\targv_array_push(&child->args, \"clone\");\n+\tif (suc->quiet)\n+\t\targv_array_push(&child->args, \"--quiet\");\n+\tif (suc->prefix)\n+\t\targv_array_pushl(&child->args, \"--prefix\", suc->prefix, NULL);\n+\targv_array_pushl(&child->args, \"--path\", sub->path, NULL);\n+\targv_array_pushl(&child->args, \"--name\", sub->name, NULL);\n+\targv_array_pushl(&child->args, \"--url\", url, NULL);\n+\tif (suc->reference)\n+\t\targv_array_push(&child->args, suc->reference);\n+\tif (suc->depth)\n+\t\targv_array_push(&child->args, suc->depth);\n+\n+cleanup:\n+\tfree(url);\n+\tstrbuf_reset(&displaypath_sb);\n+\tstrbuf_reset(&sb);\n+\n+\treturn needs_cloning;\n+}\n+\n+static int update_clone_get_next_task(struct child_process *child,\n+\t\t\t\t      struct strbuf *err,\n+\t\t\t\t      void *suc_cb,\n+\t\t\t\t      void **void_task_cb)\n+{\n+\tstruct submodule_update_clone *suc = suc_cb;\n+\n+\tfor (; suc->current < suc->list.nr; suc->current++) {\n+\t\tconst struct cache_entry *ce = suc->list.entries[suc->current];\n+\t\tif (prepare_to_clone_next_submodule(ce, child, suc, err)) {\n+\t\t\tsuc->current++;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static int update_clone_start_failure(struct child_process *child,\n+\t\t\t\t      struct strbuf *err,\n+\t\t\t\t      void *suc_cb,\n+\t\t\t\t      void *void_task_cb)\n+{\n+\tstruct submodule_update_clone *suc = suc_cb;\n+\n+\tdefault_start_failure(child, err, suc_cb, void_task_cb);\n+\tsuc->quickstop = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone_task_finished(int result,\n+\t\t\t\t      struct child_process *child,\n+\t\t\t\t      struct strbuf *err,\n+\t\t\t\t      void *suc_cb,\n+\t\t\t\t      void *void_task_cb)\n+{\n+\tstruct submodule_update_clone *suc = suc_cb;\n+\n+\tif (!result)\n+\t\treturn 0;\n+\n+\tdefault_task_finished(result, child, err, suc_cb, void_task_cb);\n+\tsuc->quickstop = 1;\n+\n+\treturn 1;\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *update = NULL;\n+\tstruct string_list_item *item;\n+\tstruct pathspec pathspec;\n+\tstruct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;\n+\n+\tstruct option module_update_clone_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive-prefix\", &suc.recursive_prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree, across nested \"\n+\t\t\t      \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &update,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_STRING(0, \"reference\", &suc.reference, N_(\"repo\"),\n+\t\t\t   N_(\"reference repository\")),\n+\t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n+\t\t\t   N_(\"Create a shallow clone truncated to the \"\n+\t\t\t      \"specified number of revisions\")),\n+\t\tOPT__QUIET(&suc.quiet, N_(\"don't print cloning progress\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper update_clone [--prefix=<path>] [<path>...]\"),\n+\t\tNULL\n+\t};\n+\tsuc.prefix = prefix;\n+\n+\targc = parse_options(argc, argv, prefix, module_update_clone_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tif (update)\n+\t\tif (parse_submodule_update_strategy(update, &suc.update) < 0)\n+\t\t\tdie(_(\"bad value for update parameter\"));\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)\n+\t\treturn 1;\n+\n+\tif (pathspec.nr)\n+\t\tsuc.warn_if_uninitialized = 1;\n+\n+\tgitmodules_config();\n+\t/* Overlay the parsed .gitmodules file with .git/config */\n+\tgit_config(submodule_config, NULL);\n+\trun_processes_parallel(1, update_clone_get_next_task,\n+\t\t\t\t  update_clone_start_failure,\n+\t\t\t\t  update_clone_task_finished,\n+\t\t\t\t  &suc);\n+\n+\t/*\n+\t * We saved the output and put it out all at once now.\n+\t * That means:\n+\t * - the listener does not have to interleave their (checkout)\n+\t *   work with our fetching.  The writes involved in a\n+\t *   checkout involve more straightforward sequential I/O.\n+\t * - the listener can avoid doing any work if fetching failed.\n+\t */\n+\tif (suc.quickstop)\n+\t\treturn 1;\n+\n+\tfor_each_string_list_item(item, &suc.projectlines)\n+\t\tutf8_fprintf(stdout, \"%s\", item->string);\n+\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +506,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n \t{\"clone\", module_clone},\n+\t{\"update-clone\", update_clone}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9ee86d4..a6a82d2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -664,17 +664,20 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\t{\n+\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n+\t\t${update:+--update \"$update\"} \\\n+\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${depth:+--depth \"$depth\"} \\\n+\t\t\"$@\" || echo \"#unmatched\"\n+\t} | {\n \terr=\n-\twhile read mode sha1 stage sm_path\n+\twhile read mode sha1 stage just_cloned sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n+\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n@@ -691,27 +694,10 @@ cmd_update()\n \n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo >&2 \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay >&2 \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n+\t\tif test $just_cloned -eq 1\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n+\t\t\tupdate_module=checkout\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -751,13 +737,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.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279116","messageId":"1456284017-26141-5-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":"1456284017-26141-1-git-send-email-sbeller@google.com","subject":"[PATCHv15 4/5] submodule update: expose parallelism to the user","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:16Z","receivedAt":"2016-02-24T03:20:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Expose possible parallelism either via the \"--jobs\" CLI parameter or\nthe \"submodule.fetchJobs\" setting.\n\nBy having the variable initialized to -1, we make sure 0 can be passed\ninto the parallel processing machine, which will then pick as many parallel\nworkers as there are CPUs.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-submodule.txt |  7 ++++++-\n builtin/submodule--helper.c     | 16 ++++++++++++----\n git-submodule.sh                |  9 +++++++++\n t/t7406-submodule-update.sh     | 12 ++++++++++++\n 4 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 1572f05..13adebf 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n 'git submodule' [--quiet] deinit [-f|--force] [--] <path>...\n 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n-\t      [--depth <depth>] [--recursive] [--] [<path>...]\n+\t      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]\n \t      [commit] [--] [<path>...]\n 'git submodule' [--quiet] foreach [--recursive] <command>\n@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.\n \tclone with a history truncated to the specified number of revisions.\n \tSee linkgit:git-clone[1]\n \n+-j <n>::\n+--jobs <n>::\n+\tThis option is only valid for the update command.\n+\tClone new submodules in parallel with as many jobs.\n+\tDefaults to the `submodule.fetchJobs` option.\n \n <path>...::\n \tPaths to submodule(s). When specified this will restrict the command\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85fb702..32254cd 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -429,6 +429,7 @@ static int update_clone_task_finished(int result,\n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *update = NULL;\n+\tint max_jobs = -1;\n \tstruct string_list_item *item;\n \tstruct pathspec pathspec;\n \tstruct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;\n@@ -449,6 +450,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n \t\t\t   N_(\"Create a shallow clone truncated to the \"\n \t\t\t      \"specified number of revisions\")),\n+\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t\t    N_(\"parallel jobs\")),\n \t\tOPT__QUIET(&suc.quiet, N_(\"don't print cloning progress\")),\n \t\tOPT_END()\n \t};\n@@ -475,10 +478,15 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tgitmodules_config();\n \t/* Overlay the parsed .gitmodules file with .git/config */\n \tgit_config(submodule_config, NULL);\n-\trun_processes_parallel(1, update_clone_get_next_task,\n-\t\t\t\t  update_clone_start_failure,\n-\t\t\t\t  update_clone_task_finished,\n-\t\t\t\t  &suc);\n+\n+\tif (max_jobs < 0)\n+\t\tmax_jobs = parallel_submodules();\n+\n+\trun_processes_parallel(max_jobs,\n+\t\t\t       update_clone_get_next_task,\n+\t\t\t       update_clone_start_failure,\n+\t\t\t       update_clone_task_finished,\n+\t\t\t       &suc);\n \n \t/*\n \t * We saved the output and put it out all at once now.\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a6a82d2..86018ee 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -645,6 +645,14 @@ cmd_update()\n \t\t--depth=*)\n \t\t\tdepth=$1\n \t\t\t;;\n+\t\t-j|--jobs)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tjobs=\"--jobs=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--jobs=*)\n+\t\t\tjobs=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -671,6 +679,7 @@ cmd_update()\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+--reference \"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n+\t\t${jobs:+$jobs} \\\n \t\t\"$@\" || echo \"#unmatched\"\n \t} | {\n \terr=\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex dda3929..7fd5142 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur\n \t test_i18ngrep \"Submodule path .deeper/submodule/subsubmodule.: checked out\" actual\n \t)\n '\n+\n+test_expect_success 'submodule update can be run in parallel' '\n+\t(cd super2 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 7 &&\n+\t grep \"7 tasks\" trace.out &&\n+\t git config submodule.fetchJobs 8 &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update &&\n+\t grep \"8 tasks\" trace.out &&\n+\t GIT_TRACE=$(pwd)/trace.out git submodule update --jobs 9 &&\n+\t grep \"9 tasks\" trace.out\n+\t)\n+'\n test_done\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279117","messageId":"1456284017-26141-6-git-send-email-sbeller@google.com","threadId":"41494","inReplyTo":"1456284017-26141-1-git-send-email-sbeller@google.com","subject":"[PATCHv15 5/5] clone: allow an explicit argument for parallel submodule clones","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:20:17Z","receivedAt":"2016-02-24T03:20:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just pass it along to \"git submodule update\", which may pick reasonable\ndefaults if you don't specify an explicit number.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-clone.txt |  6 +++++-\n builtin/clone.c             | 19 +++++++++++++------\n t/t7406-submodule-update.sh | 15 +++++++++++++++\n 3 files changed, 33 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 6bf000d..6db7b6d 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]\n \t  [--dissociate] [--separate-git-dir <git dir>]\n \t  [--depth <depth>] [--[no-]single-branch]\n-\t  [--recursive | --recurse-submodules] [--] <repository>\n+\t  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>\n \t  [<directory>]\n \n DESCRIPTION\n@@ -221,6 +221,10 @@ objects from the source repository into a pack in the cloned repository.\n \tThe result is Git repository can be separated from working\n \ttree.\n \n+-j <n>::\n+--jobs <n>::\n+\tThe number of submodules fetched at the same time.\n+\tDefaults to the `submodule.fetchJobs` option.\n \n <repository>::\n \tThe (possibly remote) repository to clone from.  See the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0b3cd9..b004fb4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -50,6 +50,7 @@ static int option_progress = -1;\n static struct string_list option_config;\n static struct string_list option_reference;\n static int option_dissociate;\n+static int max_jobs = -1;\n \n static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {\n \t\t    N_(\"initialize submodules in the clone\")),\n \tOPT_BOOL(0, \"recurse-submodules\", &option_recursive,\n \t\t    N_(\"initialize submodules in the clone\")),\n+\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\t    N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n \t\t   N_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {\n \tOPT_END()\n };\n \n-static const char *argv_submodule[] = {\n-\t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n-};\n-\n static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n@@ -724,8 +723,16 @@ static int checkout(void)\n \terr |= run_hook_le(NULL, \"post-checkout\", sha1_to_hex(null_sha1),\n \t\t\t   sha1_to_hex(sha1), \"1\", NULL);\n \n-\tif (!err && option_recursive)\n-\t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n+\tif (!err && option_recursive) {\n+\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n+\n+\t\tif (max_jobs != -1)\n+\t\t\targv_array_pushf(&args, \"--jobs=%d\", max_jobs);\n+\n+\t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n+\t\targv_array_clear(&args);\n+\t}\n \n \treturn err;\n }\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 7fd5142..090891e 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in parallel' '\n \t grep \"9 tasks\" trace.out\n \t)\n '\n+\n+test_expect_success 'git clone passes the parallel jobs config on to submodules' '\n+\ttest_when_finished \"rm -rf super4\" &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . super4 &&\n+\tgrep \"7 tasks\" trace.out &&\n+\trm -rf super4 &&\n+\tgit config --global submodule.fetchJobs 8 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&\n+\tgrep \"8 tasks\" trace.out &&\n+\trm -rf super4 &&\n+\tGIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . super4 &&\n+\tgrep \"9 tasks\" trace.out &&\n+\trm -rf super4\n+'\n+\n test_done\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"279168","messageId":"xmqqa8mpub1y.fsf@gitster.mtv.corp.google.com","threadId":"41494","inReplyTo":"1456284017-26141-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T20:07:37Z","receivedAt":"2016-02-24T20:07:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> @@ -1095,9 +1098,11 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n>  static void pp_output(struct parallel_processes *pp)\n>  {\n>  \tint i = pp->output_owner;\n> -\tif (pp->children[i].state == GIT_CP_WORKING &&\n> -\t    pp->children[i].err.len) {\n> +\tsize_t len = pp->children[i].err.len;\n> +\tif (pp->children[i].state == GIT_CP_WORKING && len) {\n>  \t\tfputs(pp->children[i].err.buf, stderr);\n> +\t\tpp->ended_with_newline =\n> +\t\t\t(pp->children[i].err.buf[len - 1] == '\\n');\n>  \t\tstrbuf_reset(&pp->children[i].err);\n>  \t}\n>  }\n\nThe child->err thing is treated as a counted byte array when the\ncode determines where the end of the buffer is, but is treated as a\nnul-terminated string when it is output with fputs().\n\nThe inconsistency may not hurt as long as (1) the producers of the\nmessage will never stuff a NUL in the middle, and (2) strbuf always\nhas the guard NUL after its contents.  Even though we know that the\nlatter will hold true for the foreseeable future, it also is easy to\ndo the right thing here, too, so why not?\n"},{"id":"279177","messageId":"20160224211947.GO28749@google.com","threadId":"41494","inReplyTo":"1456284017-26141-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-24T21:19:47Z","receivedAt":"2016-02-24T21:19:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> When the callers of parallel processing machine are sloppy with their\n> messages, make sure the output is terminated with LF after one child\n> process is handled.\n\nWhy not always add \\n here?\n\nThat would make callers simpler and would make it easier for callers to\nknow what to do.  It also makes it possible to end with \\n\\n if the\ncaller wants.\n\nThanks,\nJonathan\n"},{"id":"279178","messageId":"CAGZ79kYv-dKMnON=Hj2fUNEbUXz=8ti3x3tNG2WxWmdr5CA2tA@mail.gmail.com","threadId":"41494","inReplyTo":"xmqqa8mpub1y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T21:19:53Z","receivedAt":"2016-02-24T21:19:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 24, 2016 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> @@ -1095,9 +1098,11 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n>>  static void pp_output(struct parallel_processes *pp)\n>>  {\n>>       int i = pp->output_owner;\n>> -     if (pp->children[i].state == GIT_CP_WORKING &&\n>> -         pp->children[i].err.len) {\n>> +     size_t len = pp->children[i].err.len;\n>> +     if (pp->children[i].state == GIT_CP_WORKING && len) {\n>>               fputs(pp->children[i].err.buf, stderr);\n>> +             pp->ended_with_newline =\n>> +                     (pp->children[i].err.buf[len - 1] == '\\n');\n>>               strbuf_reset(&pp->children[i].err);\n>>       }\n>>  }\n>\n> The child->err thing is treated as a counted byte array when the\n> code determines where the end of the buffer is, but is treated as a\n> nul-terminated string when it is output with fputs().\n>\n> The inconsistency may not hurt as long as (1) the producers of the\n> message will never stuff a NUL in the middle, and (2) strbuf always\n> has the guard NUL after its contents.  Even though we know that the\n> latter will hold true for the foreseeable future, it also is easy to\n> do the right thing here, too, so why not?\n\nWhat is the right thing? I asked myself and obviously it is treating the\nchild->err the same in both cases of checking for a trailing LF and\nwhen outputting.\n\nBut what is the right way to look at child->err? I would argue that\nwe should allow for children to have a NUL in its output stream and\nreplay their output as literal as possible.\n\ni.e. my conclusion is to replace the fputs by fwrite as opposed to\nusing strlen to determine the length of string output.\n\nThanks,\nStefan\n"},{"id":"279180","messageId":"xmqqtwkxsszl.fsf@gitster.mtv.corp.google.com","threadId":"41494","inReplyTo":"CAGZ79kYv-dKMnON=Hj2fUNEbUXz=8ti3x3tNG2WxWmdr5CA2tA@mail.gmail.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T21:23:10Z","receivedAt":"2016-02-24T21:23:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Feb 24, 2016 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> The inconsistency may not hurt as long as (1) the producers of the\n>> message will never stuff a NUL in the middle, and (2) strbuf always\n>> has the guard NUL after its contents.  Even though we know that the\n>> latter will hold true for the foreseeable future, it also is easy to\n>> do the right thing here, too, so why not?\n>\n> What is the right thing? I asked myself and obviously it is treating the\n> child->err the same in both cases of checking for a trailing LF and\n> when outputting.\n>\n> But what is the right way to look at child->err? I would argue that\n> we should allow for children to have a NUL in its output stream and\n> replay their output as literal as possible.\n>\n> i.e. my conclusion is to replace the fputs by fwrite as opposed to\n> using strlen to determine the length of string output.\n\nYup, that is what I meant; sorry if I were too oblique.\n\nThere are two fputs() that assumes there is no embedded NUL around\nthere, by the way.\n\nThanks.\n"},{"id":"279183","messageId":"CAGZ79kZvD4zFA61XvSLa8fe1PzH91+4ii5vSeH-P+ER2wbQy2g@mail.gmail.com","threadId":"41494","inReplyTo":"20160224211947.GO28749@google.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T21:59:25Z","receivedAt":"2016-02-24T21:59:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 24, 2016 at 1:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Stefan Beller wrote:\n>\n>> When the callers of parallel processing machine are sloppy with their\n>> messages, make sure the output is terminated with LF after one child\n>> process is handled.\n>\n> Why not always add \\n here?\n\nSo you propose to always add a \\n if the output length was > 0 ?\n\n>\n> That would make callers simpler and would make it easier for callers to\n> know what to do.  It also makes it possible to end with \\n\\n if the\n> caller wants.\n\nIf the caller wants a \\n\\n it can do so as well using this patch?\n\nIf we add an \\n unconditionally we run the risk of having lots of empty lines\nin there. Consider the tests which are already there:\n\n    test-run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\"\nHello World\" >actual\n\nwould produce\n\ncat >expect <<-EOF\npreloaded output of a child\nHello\nWorld\n\npreloaded output of a child\nHello\nWorld\n\npreloaded output of a child\nHello\nWorld\n\npreloaded output of a child\nHello\nWorld\nEOF\n\nas both the child as well as we added a \\n, so one empty line was born.\nAnd as most child processes actually terminate with a reasonable \\n\nafter their output,\nI would not want to add another \\n because it is simpler or easier to predict.\n\nThanks,\nStefan\n\n\n>\n> Thanks,\n> Jonathan\n"},{"id":"279247","messageId":"20160225005513.GP28749@google.com","threadId":"41494","inReplyTo":"CAGZ79kZvD4zFA61XvSLa8fe1PzH91+4ii5vSeH-P+ER2wbQy2g@mail.gmail.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-02-25T00:55:13Z","receivedAt":"2016-02-25T00:55:13Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Wed, Feb 24, 2016 at 1:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> > Stefan Beller wrote:\n\n>>> When the callers of parallel processing machine are sloppy with their\n>>> messages, make sure the output is terminated with LF after one child\n>>> process is handled.\n>>\n>> Why not always add \\n here?\n>\n> So you propose to always add a \\n if the output length was > 0 ?\n\nAh, now I see where I was confused.\n\nI was seeing an analogy to functions like ref_transaction_begin(),\nwhich use a 'struct strbuf *err' argument to store the argument to\ndie() that describes why they failed.  They get used like this:\n\n\tstruct strbuf err = STRBUF_INIT;\n\tif (ref_transaction_begin(..., &err))\n\t\tdie(err.buf);\n\nand die() appends a \\n at the end.  They typically are implemented\nlike this:\n\n\tif (open(...)) {\n\t\tstrbuf_addf(&err,\n\t\t\t    \"cannot open '%s': %s\", ..., strerror(errno));\n\t\treturn -1;\n\t}\n\nWhen the function doesn't fail, err doesn't need to be inspected at all.\n\nget_next_task_fn et al looked similar to that pattern, but they are\ndoing something different.  The strbuf passed in is the same buffer\nthat is used to collect the child process's output.  Writing to that\nstrbuf is not a way to provide an error message for die() --- instead,\nit is a way to provide additional output that should be combined with\nthe child process's output.\n\nRenaming the parameter to something like 'struct strbuf *out' would\nhave avoided this clash of conventions and got me un-confused.  That\nwould make it clearer that the callback function should do\n\n\tstrbuf_addf(out, \"warning: foo\\n\");\n\nincluding both its own 'warning: ' prefix and its own newline.  It is\nproviding output meant to be passed as-is to the terminal.\n\nThat is a convenient API since if you want to write multiple messages,\nyou can do\n\n\tif (foo)\n\t\tstrbuf_addf(out, \"warning: foo\\n\");\n\n\t... do some other things ...\n\n\tif (bar)\n\t\tstrbuf_addf(out, \"warning: bar\\n\");\n\nThe newlines avoid the messages running together.\n\nThe functions default_start_failure and default_task_finished are\nbuggy under that API, since they do not include newlines in their\noutput.\n\nOnce they're fixed, there wouldn't be any need to add \\n, unless we\nare worried about a child process that writes output that doesn't end\nwith a newline.  It can be convenient for child processes to do things\nlike\n\n\tprintf '%s\\t' \"some information\"\n\nso I am not convinced this patch is helping.\n\nDoes that make sense?\n\nSorry for the confusion,\nJonathan\n"},{"id":"279263","messageId":"xmqqziuppkf2.fsf@gitster.mtv.corp.google.com","threadId":"41494","inReplyTo":"20160225005513.GP28749@google.com","subject":"Re: [PATCHv15 2/5] run_processes_parallel: add LF when caller is sloppy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-25T02:56:33Z","receivedAt":"2016-02-25T02:56:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Stefan Beller wrote:\n>> On Wed, Feb 24, 2016 at 1:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> > Stefan Beller wrote:\n>\n>>>> When the callers of parallel processing machine are sloppy with their\n>>>> messages, make sure the output is terminated with LF after one child\n>>>> process is handled.\n>>>\n>>> Why not always add \\n here?\n>>\n>> So you propose to always add a \\n if the output length was > 0 ?\n>\n> Ah, now I see where I was confused.\n> ...\n> get_next_task_fn et al looked similar to that pattern, but they are\n> doing something different.\n> ...\n> The functions default_start_failure and default_task_finished are\n> buggy under that API, since they do not include newlines in their\n> output.\n>\n> Once they're fixed, there wouldn't be any need to add \\n, unless we\n> are worried about a child process that writes output that doesn't end\n> with a newline.  It can be convenient for child processes to do things\n> like\n>\n> \tprintf '%s\\t' \"some information\"\n>\n> so I am not convinced this patch is helping.\n>\n> Does that make sense?\n\nSounds very sensible analysis.  Thanks.\n"}]}