{"thread":{"id":"40600","subject":"[PATCH 2/8] run-command: Call get_next_task with a clean child process.","startedAt":"2015-10-20T22:43:42Z","lastAt":"2015-10-21T22:14:55Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"272010","messageId":"1445381030-23912-1-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":null,"subject":"[PATCH 0/8] Fixes for the parallel processing engine and git submodule update","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:42Z","receivedAt":"2015-10-20T22:43:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Patches 1-6 replace the last 6 patches of sb/submodule-parallel-fetch\n(Patch 1,2 changed code, 3,4 stayed as is, 5 has more commit message, \nPatch 6 is the same again)\n\nPatches 7,8 are new in the series .\nPatch 7 keeps the update strategy in the cached submodue structs around,\nPatch 8 rewrites some small part of the git submodule update script in C\nby having another larger helper function in builtin/submodule--helper.c\nwhich takes care of the cloning new submodules without having all the\nintermediate steps as in previous versions of this series.\n\nThe patch 8 is just a rewrite/translation without enabling the parallel\nprocessing though. This will be done in a later patch once we have\nbike shedded enough how to name the user facing option for that.\n(I guess the CLI option would be --jobs again, but I'd rather hint at\nthe config option)\n\nThis supersedes \n[RFC PATCHv1 00/12] git submodule update in C with parallel cloning\n\nAny feedback welcome!\nThanks,\nStefan\n\nStefan Beller (8):\n  run-command: Fix early shutdown\n  run-command: Call get_next_task with a clean child process.\n  run-command: Initialize the shutdown flag\n  test-run-command: Test for gracefully aborting\n  test-run-command: Increase test coverage\n  run-command: Fix missing output from late callbacks\n  submodule config: Keep update strategy around\n  git submodule update: Have a dedicated helper for cloning\n\n builtin/submodule--helper.c | 222 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  45 +++------\n run-command.c               |  27 +++++-\n submodule-config.c          |  11 +++\n submodule-config.h          |   1 +\n t/t0061-run-command.sh      |  37 +++++++-\n t/t7400-submodule-basic.sh  |   4 +-\n test-run-command.c          |  37 +++++++-\n 8 files changed, 340 insertions(+), 44 deletions(-)\n\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272009","messageId":"1445381030-23912-2-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/8] run-command: Fix early shutdown","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:43Z","receivedAt":"2015-10-20T22:43:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The return value of `pp_collect_finished` indicates if we want to shutdown\nthe parallel processing early. Have just one return path to\nreturn any accumulated results.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex ef3da27..b9363da 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1077,7 +1077,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \twhile (pp->nr_processes > 0) {\n \t\tpid = waitpid(-1, &wait_status, WNOHANG);\n \t\tif (pid == 0)\n-\t\t\treturn 0;\n+\t\t\tbreak;\n \n \t\tif (pid < 0)\n \t\t\tdie_errno(\"wait\");\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272002","messageId":"1445381030-23912-3-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/8] run-command: Call get_next_task with a clean child process.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:44Z","receivedAt":"2015-10-20T22:43:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If the `get_next_task` did not explicitly called child_process_init\nand only filled in some fields, there may have been some stale data\nin the child process. This is hard to debug and also adds a review\nburden for each new user of that API. To improve the situation, we\npass only cleanly initialized child structs to the get_next_task.\n\nAs an invariant you can now assume any child not in use is\ncleaned up and ready for its next reuse.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b9363da..a5ef874 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)\n \targv_array_init(&child->env_array);\n }\n \n+void child_process_deinit(struct child_process *child)\n+{\n+\targv_array_clear(&child->args);\n+\targv_array_clear(&child->env_array);\n+}\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -962,6 +968,7 @@ static struct parallel_processes *pp_init(int n,\n \n \tfor (i = 0; i < n; i++) {\n \t\tstrbuf_init(&pp->children[i].err, 0);\n+\t\tchild_process_init(&pp->children[i].process);\n \t\tpp->pfd[i].events = POLLIN;\n \t\tpp->pfd[i].fd = -1;\n \t}\n@@ -973,8 +980,10 @@ static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n \n-\tfor (i = 0; i < pp->max_processes; i++)\n+\tfor (i = 0; i < pp->max_processes; i++) {\n \t\tstrbuf_release(&pp->children[i].err);\n+\t\tchild_process_deinit(&pp->children[i].process);\n+\t}\n \n \tfree(pp->children);\n \tfree(pp->pfd);\n@@ -1134,6 +1143,8 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \t\tpp->nr_processes--;\n \t\tpp->children[i].in_use = 0;\n \t\tpp->pfd[i].fd = -1;\n+\t\tchild_process_deinit(&pp->children[i].process);\n+\t\tchild_process_init(&pp->children[i].process);\n \n \t\tif (i != pp->output_owner) {\n \t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272004","messageId":"1445381030-23912-4-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/8] run-command: Initialize the shutdown flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:45Z","receivedAt":"2015-10-20T22:43:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It did work out without initializing the flag so far, but to make it\nfuture proof, we want to explicitly initialize the flag.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex a5ef874..d354c01 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -962,6 +962,7 @@ static struct parallel_processes *pp_init(int n,\n \n \tpp->nr_processes = 0;\n \tpp->output_owner = 0;\n+\tpp->shutdown = 0;\n \tpp->children = xcalloc(n, sizeof(*pp->children));\n \tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n \tstrbuf_init(&pp->buffered_output, 0);\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272003","messageId":"1445381030-23912-5-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/8] test-run-command: Test for gracefully aborting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:46Z","receivedAt":"2015-10-20T22:43:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We reuse the get_next_task callback which would stop after invoking the\ntest program 4 times. However as we have only 3 parallel processes started\n(We pass in 3 as max parallel processes and 3 is smaller than the spawn\ncap in run-command, so we will start the 3 processes in the first run\ndeterministically), then the abort logic from the new task_finished\ncallback kicks in and prevents the parallel processing engine to start\nany more processes.\n\nAs the task_finished is unconditional, we will see its output 3 times, too.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t0061-run-command.sh | 14 ++++++++++++++\n test-run-command.c     | 14 ++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 49aa3db..0af77cd 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -67,4 +67,18 @@ test_expect_success 'run_command runs in parallel' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<-EOF\n+preloaded output of a child\n+asking for a quick stop\n+preloaded output of a child\n+asking for a quick stop\n+preloaded output of a child\n+asking for a quick stop\n+EOF\n+\n+test_expect_success 'run_command is asked to abort gracefully' '\n+\ttest-run-command run-command-abort-3 false 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 699d9e9..4b59482 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -34,6 +34,16 @@ static int parallel_next(void** task_cb,\n \treturn 1;\n }\n \n+static int 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+\tstrbuf_addf(err, \"asking for a quick stop\\n\");\n+\treturn 1;\n+}\n+\n int main(int argc, char **argv)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n@@ -55,6 +65,10 @@ int main(int argc, char **argv)\n \t\texit(run_processes_parallel(4, parallel_next,\n \t\t\t\t\t    NULL, NULL, &proc));\n \n+\tif (!strcmp(argv[1], \"run-command-abort-3\"))\n+\t\texit(run_processes_parallel(3, parallel_next,\n+\t\t\t\t\t    NULL, task_finished, &proc));\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272007","messageId":"1445381030-23912-6-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/8] test-run-command: Increase test coverage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:47Z","receivedAt":"2015-10-20T22:43:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently we have exact 4 jobs to be run with at most 4 parallel\nprocesses. This sounds as if we're testing only one special case,\nbut in reality we want to have any number of tasks be processed\nsuccessfully, so test:\n* more tasks than max jobs (implying slots get reused)\n* equal number of jobs and max jobs\n* less tasks than max jobs (implying there are unused slots)\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t0061-run-command.sh | 16 +++++++++++++---\n test-run-command.c     | 14 +++++++++-----\n 2 files changed, 22 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 0af77cd..f27ada7 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -62,8 +62,18 @@ 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+test_expect_success 'run_command runs in parallel with more jobs available than tasks' '\n+\ttest-run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\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_expect_success 'run_command runs in parallel with more tasks than jobs available' '\n+\ttest-run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -77,7 +87,7 @@ asking for a quick stop\n EOF\n \n test_expect_success 'run_command is asked to abort gracefully' '\n-\ttest-run-command run-command-abort-3 false 2>actual &&\n+\ttest-run-command run-command-abort 3 false 2>actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/test-run-command.c b/test-run-command.c\nindex 4b59482..c8770c2 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -47,10 +47,11 @@ static int task_finished(int result,\n int main(int argc, char **argv)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tint jobs;\n \n \tif (argc < 3)\n \t\treturn 1;\n-\tproc.argv = (const char **)argv+2;\n+\tproc.argv = (const char **)argv + 2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\n \t\tif (start_command(&proc) < 0 && errno == ENOENT)\n@@ -61,12 +62,15 @@ 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, parallel_next,\n+\tjobs = atoi(argv[2]);\n+\tproc.argv = (const char **)argv + 3;\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 \n-\tif (!strcmp(argv[1], \"run-command-abort-3\"))\n-\t\texit(run_processes_parallel(3, parallel_next,\n+\tif (!strcmp(argv[1], \"run-command-abort\"))\n+\t\texit(run_processes_parallel(jobs, parallel_next,\n \t\t\t\t\t    NULL, task_finished, &proc));\n \n \tfprintf(stderr, \"check usage\\n\");\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272008","messageId":"1445381030-23912-7-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/8] run-command: Fix missing output from late callbacks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:48Z","receivedAt":"2015-10-20T22:43:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The callbacks in the parallel processing API were given the contract, that\nthey are not allowed to print anything to stdout/err, but the API will take\ncare of their output needs instead.\n\nIn case a child process is started, the callback can first add its messages\nto the buffer and then the child process output is buffered just in the\nsame buffer, and so the output will be taken care of eventually once the\nchild process is done.\n\nWhen no child process is run, we also need to fulfill our promise to\noutput the buffer eventually. So when no child process is started, we need\nto amend the contents of the buffer passed to the child to our buffer for\nfinished processes. We cannot output directly at that point in time as\nanother process may be in the middle of its output.\n\nThe buffer for finished child processes then also needs to be flushed in\nthe cleanup phase as it may contain data.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c          | 11 ++++++++++-\n t/t0061-run-command.sh |  9 +++++++++\n test-run-command.c     | 13 +++++++++++++\n 3 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d354c01..fa73dae 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -988,6 +988,12 @@ static void pp_cleanup(struct parallel_processes *pp)\n \n \tfree(pp->children);\n \tfree(pp->pfd);\n+\n+\t/*\n+\t * When get_next_task added messages to the buffer in its last\n+\t * iteration, the buffered output is non empty.\n+\t */\n+\tfputs(pp->buffered_output.buf, stderr);\n \tstrbuf_release(&pp->buffered_output);\n \n \tsigchain_pop_common();\n@@ -1023,8 +1029,11 @@ static int pp_start_one(struct parallel_processes *pp)\n \tif (!pp->get_next_task(&pp->children[i].data,\n \t\t\t       &pp->children[i].process,\n \t\t\t       &pp->children[i].err,\n-\t\t\t       pp->data))\n+\t\t\t       pp->data)) {\n+\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\tstrbuf_reset(&pp->children[i].err);\n \t\treturn 1;\n+\t}\n \n \tif (start_command(&pp->children[i].process)) {\n \t\tint code = pp->start_failure(&pp->children[i].process,\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex f27ada7..12228b4 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -91,4 +91,13 @@ test_expect_success 'run_command is asked to abort gracefully' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<-EOF\n+no further jobs available\n+EOF\n+\n+test_expect_success 'run_command outputs ' '\n+\ttest-run-command run-command-no-jobs 3 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 c8770c2..13e5d44 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -34,6 +34,15 @@ static int parallel_next(void** task_cb,\n \treturn 1;\n }\n \n+static int no_job(void** task_cb,\n+\t\t  struct child_process *cp,\n+\t\t  struct strbuf *err,\n+\t\t  void *cb)\n+{\n+\tstrbuf_addf(err, \"no further jobs available\\n\");\n+\treturn 0;\n+}\n+\n static int task_finished(int result,\n \t\t\t struct child_process *cp,\n \t\t\t struct strbuf *err,\n@@ -73,6 +82,10 @@ int main(int argc, char **argv)\n \t\texit(run_processes_parallel(jobs, parallel_next,\n \t\t\t\t\t    NULL, task_finished, &proc));\n \n+\tif (!strcmp(argv[1], \"run-command-no-jobs\"))\n+\t\texit(run_processes_parallel(jobs, no_job,\n+\t\t\t\t\t    NULL, task_finished, &proc));\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272005","messageId":"1445381030-23912-8-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 7/8] submodule config: Keep update strategy around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:49Z","receivedAt":"2015-10-20T22:43:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We need the submodule update strategies in a later patch.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis may conflict with origin/sb/submodule-config-parse, but only on a\nsyntactical level (this adds an else if {...} just after the refactoredd code).\nThere is no clash of functionality or semantics.\n\n submodule-config.c | 11 +++++++++++\n submodule-config.h |  1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..175bcbb 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n \n \tsubmodule->path = NULL;\n \tsubmodule->url = NULL;\n+\tsubmodule->update = NULL;\n \tsubmodule->fetch_recurse = RECURSE_SUBMODULES_NONE;\n \tsubmodule->ignore = NULL;\n \n@@ -326,6 +327,16 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\tfree((void *) submodule->url);\n \t\tstrbuf_addstr(&url, value);\n \t\tsubmodule->url = strbuf_detach(&url, NULL);\n+\t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->update != NULL)\n+\t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n+\t\t\t\t\t     \"update\");\n+\t\telse {\n+\t\t\tfree((void *)submodule->update);\n+\t\t\tsubmodule->update = xstrdup(value);\n+\t\t}\n \t}\n \n release_return:\ndiff --git a/submodule-config.h b/submodule-config.h\nindex 9061e4e..f9e2a29 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -14,6 +14,7 @@ struct submodule {\n \tconst char *url;\n \tint fetch_recurse;\n \tconst char *ignore;\n+\tconst char *update;\n \t/* the sha1 blob id of the responsible .gitmodules file */\n \tunsigned char gitmodules_sha1[20];\n };\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272006","messageId":"1445381030-23912-9-git-send-email-sbeller@google.com","threadId":"40600","inReplyTo":"1445381030-23912-1-git-send-email-sbeller@google.com","subject":"[PATCH 8/8] git submodule update: Have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T22:43:50Z","receivedAt":"2015-10-20T22:43:50Z","isPatch":true,"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 moved\ninto the C function as well. (No need to repeat them in the shell\nscript)\n\nAs we can only access the stderr channel from within the parallel\nprocessing engine, so we need to reroute the error message for\nspecified but initialized submodules to stderr. As it is an error\nmessage, this should have gone to stderr in the first place, so a\nbug fix along the way.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 222 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  45 +++------\n t/t7400-submodule-basic.sh  |   4 +-\n 3 files changed, 235 insertions(+), 36 deletions(-)\n \n Review is best done starting at the end and scrolling up, as that's how the\n code flows in submodule--helper.c.\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..6d4815a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -255,6 +255,227 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_submodule_config(const char *var, const char *value, void *cb)\n+{\n+\treturn parse_submodule_config_option(var, value);\n+}\n+\n+struct submodule_update_clone {\n+\tint count;\n+\tint quiet;\n+\tint print_unmatched;\n+\tchar *reference;\n+\tchar *depth;\n+\tchar *update;\n+\tconst char *recursive_prefix;\n+\tconst char *prefix;\n+\tstruct module_list list;\n+\tstruct string_list projectlines;\n+\tstruct pathspec pathspec;\n+};\n+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, MODULE_LIST_INIT, STRING_LIST_INIT_DUP}\n+\n+static void fill_clone_command(struct child_process *cp, int quiet,\n+\t\t\t       const char *prefix, const char *path,\n+\t\t\t       const char *name, const char *url,\n+\t\t\t       const char *reference, const char *depth)\n+{\n+\tcp->git_cmd = 1;\n+\tcp->no_stdin = 1;\n+\tcp->stdout_to_stderr = 1;\n+\tcp->err = -1;\n+\targv_array_push(&cp->args, \"submodule--helper\");\n+\targv_array_push(&cp->args, \"clone\");\n+\tif (quiet)\n+\t\targv_array_push(&cp->args, \"--quiet\");\n+\n+\tif (prefix) {\n+\t\targv_array_push(&cp->args, \"--prefix\");\n+\t\targv_array_push(&cp->args, prefix);\n+\t}\n+\targv_array_push(&cp->args, \"--path\");\n+\targv_array_push(&cp->args, path);\n+\n+\targv_array_push(&cp->args, \"--name\");\n+\targv_array_push(&cp->args, name);\n+\n+\targv_array_push(&cp->args, \"--url\");\n+\targv_array_push(&cp->args, url);\n+\tif (reference)\n+\t\targv_array_push(&cp->args, reference);\n+\tif (depth)\n+\t\targv_array_push(&cp->args, depth);\n+}\n+\n+static int get_next_task(void **pp_task_cb,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err,\n+\t\t\t void *pp_cb)\n+{\n+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tfor (; pp->count < pp->list.nr; pp->count++) {\n+\t\tconst struct submodule *sub = NULL;\n+\t\tconst char *displaypath = NULL;\n+\t\tconst struct cache_entry *ce = pp->list.entries[pp->count];\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tconst char *update_module = NULL;\n+\t\tconst char *url = NULL;\n+\t\tint just_cloned = 0;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tif (pp->recursive_prefix)\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s/%s\\n\",\n+\t\t\t\t\tpp->recursive_prefix, ce->name);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"Skipping unmerged submodule %s\\n\",\n+\t\t\t\t\tce->name);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tsub = submodule_from_path(null_sha1, ce->name);\n+\t\tif (pp->recursive_prefix)\n+\t\t\tdisplaypath = relative_path(pp->recursive_prefix, ce->name, &sb);\n+\t\telse\n+\t\t\tdisplaypath = ce->name;\n+\n+\t\tif (pp->update)\n+\t\t\tupdate_module = pp->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = sub->update;\n+\t\tif (!update_module)\n+\t\t\tupdate_module = \"checkout\";\n+\t\tif (!strcmp(update_module, \"none\")) {\n+\t\t\tstrbuf_addf(err, \"Skipping submodule '%s'\\n\", displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Looking up the url in .git/config.\n+\t\t * We cannot fall back to .gitmodules as we only want to process\n+\t\t * configured submodules. This renders the submodule lookup API\n+\t\t * useless, as it cannot lookup without fallback.\n+\t\t */\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n+\t\tgit_config_get_string_const(sb.buf, &url);\n+\t\tif (!url) {\n+\t\t\t/*\n+\t\t\t * Only mention uninitialized submodules when its\n+\t\t\t * path have been specified\n+\t\t\t */\n+\t\t\tif (pp->pathspec.nr)\n+\t\t\t\tstrbuf_addf(err, _(\"Submodule path '%s' not initialized\\n\"\n+\t\t\t\t\t\"Maybe you want to use 'update --init'?\"), displaypath);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n+\t\tjust_cloned = !file_exists(sb.buf);\n+\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n+\t\t\t\tsha1_to_hex(ce->sha1), ce_stage(ce),\n+\t\t\t\tjust_cloned, ce->name);\n+\t\tstring_list_append(&pp->projectlines, sb.buf);\n+\n+\t\tif (just_cloned) {\n+\t\t\tfill_clone_command(cp, pp->quiet, pp->prefix, ce->name,\n+\t\t\t\t\t   sub->name, url, pp->reference, pp->depth);\n+\t\t\tpp->count++;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static int 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+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tstrbuf_addf(err, \"error when starting a child process\");\n+\tpp->print_unmatched = 1;\n+\n+\treturn 1;\n+}\n+\n+static int 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+\tstruct submodule_update_clone *pp = pp_cb;\n+\n+\tif (!result) {\n+\t\treturn 0;\n+\t} else {\n+\t\tstrbuf_addf(err, \"error in one child process\");\n+\t\tpp->print_unmatched = 1;\n+\t\treturn 1;\n+\t}\n+}\n+\n+static int update_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"recursive_prefix\", &pp.recursive_prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree, across nested \"\n+\t\t\t      \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"update\", &pp.update,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"update command for submodules\")),\n+\t\tOPT_STRING(0, \"reference\", &pp.reference, \"<repository>\",\n+\t\t\tN_(\"Use the local reference repository \"\n+\t\t\t   \"instead of a full clone\")),\n+\t\tOPT_STRING(0, \"depth\", &pp.depth, \"<depth>\",\n+\t\t\tN_(\"Create a shallow clone truncated to the \"\n+\t\t\t   \"specified number of revisions\")),\n+\t\tOPT__QUIET(&pp.quiet, N_(\"do't print cloning progress\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper list [--prefix=<path>] [<path>...]\"),\n+\t\tNULL\n+\t};\n+\tpp.prefix = prefix;\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pp.pathspec, &pp.list) < 0) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tgitmodules_config();\n+\t/* Overlay the parsed .gitmodules file with .git/config */\n+\tgit_config(git_submodule_config, NULL);\n+\trun_processes_parallel(1, get_next_task, start_failure, task_finished, &pp);\n+\n+\tif (pp.print_unmatched) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor_each_string_list_item(item, &pp.projectlines) {\n+\t\tutf8_fprintf(stdout, \"%s\", item->string);\n+\t}\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -264,6 +485,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 8b0eb9a..ea883b9 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -655,17 +655,18 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n-\tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n+\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t${prefix:+--recursive_prefix \"$prefix\"} \\\n+\t\t${update:+--update \"$update\"} \\\n+\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${depth:+--depth \"$depth\"} \\\n+\t\t\"$@\" | {\n \terr=\n-\twhile read mode sha1 stage sm_path\n+\twhile read mode sha1 stage just_cloned sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n-\t\t\tcontinue\n-\t\tfi\n+\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n@@ -682,27 +683,10 @@ cmd_update()\n \n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \n-\t\tif test \"$update_module\" = \"none\"\n-\t\tthen\n-\t\t\techo \"Skipping submodule '$displaypath'\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif test -z \"$url\"\n-\t\tthen\n-\t\t\t# Only mention uninitialized submodules when its\n-\t\t\t# path have been specified\n-\t\t\ttest \"$#\" != \"0\" &&\n-\t\t\tsay \"$(eval_gettext \"Submodule path '\\$displaypath' not initialized\n-Maybe you want to use 'update --init'?\")\"\n-\t\t\tcontinue\n-\t\tfi\n-\n-\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n+\t\tif test $just_cloned -eq 1\n \t\tthen\n-\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n-\t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n+\t\t\tupdate_module=checkout\n \t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n@@ -742,13 +726,6 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n-\t\t\t# Is this something we just cloned?\n-\t\t\tcase \";$cloned_modules;\" in\n-\t\t\t*\";$name;\"*)\n-\t\t\t\t# then there is no local change to integrate\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n-\n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..5991e3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -462,7 +462,7 @@ test_expect_success 'update --init' '\n \tgit config --remove-section submodule.example &&\n \ttest_must_fail git config submodule.example.url &&\n \n-\tgit submodule update init > update.out &&\n+\tgit submodule update init 2> update.out &&\n \tcat update.out &&\n \ttest_i18ngrep \"not initialized\" update.out &&\n \ttest_must_fail git rev-parse --resolve-git-dir init/.git &&\n@@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' '\n \tmkdir -p sub &&\n \t(\n \t\tcd sub &&\n-\t\tgit submodule update ../init >update.out &&\n+\t\tgit submodule update ../init 2>update.out &&\n \t\tcat update.out &&\n \t\ttest_i18ngrep \"not initialized\" update.out &&\n \t\ttest_must_fail git rev-parse --resolve-git-dir ../init/.git &&\n-- \n2.5.0.275.gbfc1651.dirty\n"},{"id":"272012","messageId":"xmqq4mhl6sza.fsf@gitster.mtv.corp.google.com","threadId":"40600","inReplyTo":"1445381030-23912-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T23:05:45Z","receivedAt":"2015-10-20T23:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> If the `get_next_task` did not explicitly called child_process_init\n> and only filled in some fields, there may have been some stale data\n> in the child process. This is hard to debug and also adds a review\n> burden for each new user of that API. To improve the situation, we\n> pass only cleanly initialized child structs to the get_next_task.\n>\n> As an invariant you can now assume any child not in use is\n> cleaned up and ready for its next reuse.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  run-command.c | 13 ++++++++++++-\n>  1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index b9363da..a5ef874 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)\n>  \targv_array_init(&child->env_array);\n>  }\n>  \n> +void child_process_deinit(struct child_process *child)\n> +{\n> +\targv_array_clear(&child->args);\n> +\targv_array_clear(&child->env_array);\n> +}\n> +\n\nIs this necessary (and is it necessary to make it global)?\nI thought that finish_command() already clears them....\n\n    ... goes and looks ...\n\nAhh, of course, pp_*() functions do use start_command() but do not\nuse finish_command(), which sort of breaks symmetry, but that cannot\nbe helped.  Because we want to wait for any of the multiple tasks\nrunning, we cannot call finish_command() that explicitly says \"I\nwant to wait for this one to finish\".\n\nAnd that is why you already have two calls to array-clear inside\ncollect_finished(), just after calling task_finished().\n\nAnd of course we already have these array-clear calls in\nfinish_command().\n\nSo I agree that deinit helper should exist, but\n\n * it should be file-scope static;\n\n * it should be called by finish_command(); and\n\n * if you are calling it from collect_finished(), then existing\n   calls to array-clear should go.\n\nOther than that, this looks good.\n\nThansk.\n"},{"id":"272013","messageId":"xmqq37x56sz6.fsf@gitster.mtv.corp.google.com","threadId":"40600","inReplyTo":"1445381030-23912-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T23:05:49Z","receivedAt":"2015-10-20T23:05:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> If the `get_next_task` did not explicitly called child_process_init\n> and only filled in some fields, there may have been some stale data\n> in the child process. This is hard to debug and also adds a review\n> burden for each new user of that API. To improve the situation, we\n> pass only cleanly initialized child structs to the get_next_task.\n>\n> As an invariant you can now assume any child not in use is\n> cleaned up and ready for its next reuse.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  run-command.c | 13 ++++++++++++-\n>  1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index b9363da..a5ef874 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)\n>  \targv_array_init(&child->env_array);\n>  }\n>  \n> +void child_process_deinit(struct child_process *child)\n> +{\n> +\targv_array_clear(&child->args);\n> +\targv_array_clear(&child->env_array);\n> +}\n> +\n\nIs this necessary (and is it necessary to make it global)?\nI thought that finish_command() already clears them....\n\n    ... goes and looks ...\n\nAhh, of course, pp_*() functions do use start_command() but do not\nuse finish_command(), which sort of breaks symmetry, but that cannot\nbe helped.  Because we want to wait for any of the multiple tasks\nrunning, we cannot call finish_command() that explicitly says \"I\nwant to wait for this one to finish\".\n\nAnd that is why you already have two calls to array-clear inside\ncollect_finished(), just after calling task_finished().\n\nAnd of course we already have these array-clear calls in\nfinish_command().\n\nSo I agree that deinit helper should exist, but\n\n * it should be file-scope static;\n\n * it should be called by finish_command(); and\n\n * if you are calling it from collect_finished(), then existing\n   calls to array-clear should go.\n\nOther than that, this looks good.\n\nThanks.\n"},{"id":"272064","messageId":"xmqqh9lkufpt.fsf@gitster.mtv.corp.google.com","threadId":"40600","inReplyTo":"xmqq37x56sz6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-21T20:30:38Z","receivedAt":"2015-10-21T20:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> And of course we already have these array-clear calls in\n> finish_command().\n>\n> So I agree that deinit helper should exist, but\n>\n>  * it should be file-scope static;\n>\n>  * it should be called by finish_command(); and\n>\n>  * if you are calling it from collect_finished(), then existing\n>    calls to array-clear should go.\n>\n> Other than that, this looks good.\n\nI'll queue this instead (the above squashed in and description\ncorrected).\n\n-- >8 --\nFrom: Stefan Beller <sbeller@google.com>\nDate: Tue, 20 Oct 2015 15:43:44 -0700\nSubject: [PATCH] run-command: clear leftover state from child_process structure\n\npp_start_one() function finds an unused child_process structure\npasses it to start_command(), but the structure may have already\nbeen used earlier.  finish_command() has code to clear leftover\nstates in the structure so that it can be reused, but the parallel\nexecution machinery does not (and cannot) use it, and instead has\nits own pp_collect_finished().  This function, after culling a\nprocess that has just finished, forgot to clear the child_process\nstructure before marking it ready for reuse.\n\nIntroduce child_process_deinit() helper function that clears two\ninstances of argv_array (one for arg, the other for env) in the\nstructure, make the existing codepaths that clear them call the\nhelper instead (which in turn will make sure that we will not leak\nresources later even when we add new fields to the structure), and\nalso add a call to it in pp_collect_finished() before the function\nmarks the structure read for reuse.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n run-command.c | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b9363da..684ccee 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)\n \targv_array_init(&child->env_array);\n }\n \n+static void child_process_deinit(struct child_process *child)\n+{\n+\targv_array_clear(&child->args);\n+\targv_array_clear(&child->env_array);\n+}\n+\n struct child_to_clean {\n \tpid_t pid;\n \tstruct child_to_clean *next;\n@@ -338,8 +344,7 @@ int start_command(struct child_process *cmd)\n fail_pipe:\n \t\t\terror(\"cannot create %s pipe for %s: %s\",\n \t\t\t\tstr, cmd->argv[0], strerror(failed_errno));\n-\t\t\targv_array_clear(&cmd->args);\n-\t\t\targv_array_clear(&cmd->env_array);\n+\t\t\tchild_process_deinit(cmd);\n \t\t\terrno = failed_errno;\n \t\t\treturn -1;\n \t\t}\n@@ -525,8 +530,7 @@ fail_pipe:\n \t\t\tclose_pair(fderr);\n \t\telse if (cmd->err)\n \t\t\tclose(cmd->err);\n-\t\targv_array_clear(&cmd->args);\n-\t\targv_array_clear(&cmd->env_array);\n+\t\tchild_process_deinit(cmd);\n \t\terrno = failed_errno;\n \t\treturn -1;\n \t}\n@@ -552,8 +556,7 @@ fail_pipe:\n int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n-\targv_array_clear(&cmd->args);\n-\targv_array_clear(&cmd->env_array);\n+\tchild_process_deinit(cmd);\n \treturn ret;\n }\n \n@@ -962,6 +965,7 @@ static struct parallel_processes *pp_init(int n,\n \n \tfor (i = 0; i < n; i++) {\n \t\tstrbuf_init(&pp->children[i].err, 0);\n+\t\tchild_process_init(&pp->children[i].process);\n \t\tpp->pfd[i].events = POLLIN;\n \t\tpp->pfd[i].fd = -1;\n \t}\n@@ -973,8 +977,10 @@ static void pp_cleanup(struct parallel_processes *pp)\n {\n \tint i;\n \n-\tfor (i = 0; i < pp->max_processes; i++)\n+\tfor (i = 0; i < pp->max_processes; i++) {\n \t\tstrbuf_release(&pp->children[i].err);\n+\t\tchild_process_deinit(&pp->children[i].process);\n+\t}\n \n \tfree(pp->children);\n \tfree(pp->pfd);\n@@ -1128,12 +1134,11 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \t\t\t\t      &pp->children[i].data))\n \t\t\tresult = 1;\n \n-\t\targv_array_clear(&pp->children[i].process.args);\n-\t\targv_array_clear(&pp->children[i].process.env_array);\n-\n \t\tpp->nr_processes--;\n \t\tpp->children[i].in_use = 0;\n \t\tpp->pfd[i].fd = -1;\n+\t\tchild_process_deinit(&pp->children[i].process);\n+\t\tchild_process_init(&pp->children[i].process);\n \n \t\tif (i != pp->output_owner) {\n \t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n-- \n2.6.2-394-gc0a4d5b\n"},{"id":"272067","messageId":"xmqqd1w8uewx.fsf@gitster.mtv.corp.google.com","threadId":"40600","inReplyTo":"1445381030-23912-9-git-send-email-sbeller@google.com","subject":"Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-21T20:47:58Z","receivedAt":"2015-10-21T20:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This introduces a new helper function in git submodule--helper\n> which takes care of cloning all submodules, which we want to\n> parallelize eventually.\n>\n> Some tests (such as empty URL, update_mode==none) are required in the\n> helper to make the decision for cloning. These checks have been moved\n> into the C function as well. (No need to repeat them in the shell\n> script)\n>\n> As we can only access the stderr channel from within the parallel\n> processing engine, so we need to reroute the error message for\n> specified but initialized submodules to stderr. As it is an error\n> message, this should have gone to stderr in the first place, so a\n> bug fix along the way.\n\nThe last paragraph is hard to parse; perhaps it is slightly\nungrammatical.\n\nIt would be a really good idea to split the small bit to redirect\nthe output that should have gone to the standard error to where it\nshould as a preparatory step before showing this patch.\n\nI sense that this one is still a WIP/RFC, so I'll only skim it in\nthis round (but I may come back and read it again later with finer\ntoothed comb).\n\n> +static int get_next_task(void **pp_task_cb,\n> +\t\t\t struct child_process *cp,\n> +\t\t\t struct strbuf *err,\n> +\t\t\t void *pp_cb)\n\nWill you have only one caller of the parallel run-command API in\nthis file, or will you be adding more to allow various different\noperations run in parallel as more things are rewritten?  I am\nguessing that it would be the latter, but if that is the case,\nperhaps the function wants to be named a bit more specificly for\nthis first user, no?  Same for start_failure and task_finished.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 8b0eb9a..ea883b9 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -655,17 +655,18 @@ cmd_update()\n>  \t\tcmd_init \"--\" \"$@\" || return\n>  \tfi\n>  \n> -\tcloned_modules=\n> -\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n> +\tgit submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n> +\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n> +\t\t${prefix:+--recursive_prefix \"$prefix\"} \\\n> +\t\t${update:+--update \"$update\"} \\\n> +\t\t${reference:+--reference \"$reference\"} \\\n> +\t\t${depth:+--depth \"$depth\"} \\\n> +\t\t\"$@\" | {\n>  \terr=\n> -\twhile read mode sha1 stage sm_path\n> +\twhile read mode sha1 stage just_cloned sm_path\n>  \tdo\n\nI wonder if you really want this to be upstream of a pipe.  When the\ndownstream loop needs to abort, what happens to the remainder of the\n\"clone\" part of the processing that is still ongoing in the upstream\nof the pipe?  I would imagine that the \"update-clone\" network\naccessing phase is the more human-time consuming part, so I suspect\nthat it would be much better to let the cloning part go and finish\nfirst (during which time the human-user can spend time for other\nthings, like getting cup of coffee or filling expense reports) and\nbefore moving to the loop that can stop and ask the human-user for\nhelp.\n\nThe fix for the above could be trivial (do not pipe, just take the\noutput to a temporary file, and then feed the \"while read\" loop from\nthat temporary file), and I suspect it would make a big difference\nfor usability.\n\nThanks.\n"},{"id":"272070","messageId":"CAGZ79kaa6b49rDW204ydtY0cf4NugSKOm+sBHKBYYoEsVugVfw@mail.gmail.com","threadId":"40600","inReplyTo":"xmqqd1w8uewx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-21T21:06:37Z","receivedAt":"2015-10-21T21:06:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 21, 2015 at 1:47 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This introduces a new helper function in git submodule--helper\n>> which takes care of cloning all submodules, which we want to\n>> parallelize eventually.\n>>\n>> Some tests (such as empty URL, update_mode==none) are required in the\n>> helper to make the decision for cloning. These checks have been moved\n>> into the C function as well. (No need to repeat them in the shell\n>> script)\n>>\n>> As we can only access the stderr channel from within the parallel\n>> processing engine, so we need to reroute the error message for\n>> specified but initialized submodules to stderr. As it is an error\n>> message, this should have gone to stderr in the first place, so a\n>> bug fix along the way.\n>\n> The last paragraph is hard to parse; perhaps it is slightly\n> ungrammatical.\n\nI seem to have started a habit starting my sentences with \"so...\"\neven in spoken English. If left out, this may be easier to read:\n\n    As we can only access the stderr channel from within the parallel\n    processing engine, we need to reroute the error message for\n    \"specified but initialized submodules\" to stderr. As it is an error\n    message, this should have gone to stderr in the first place.\n    It's a bug fix along the way.\n\n>\n> It would be a really good idea to split the small bit to redirect\n> the output that should have gone to the standard error to where it\n> should as a preparatory step before showing this patch.\n\nok.\n\n>\n> I sense that this one is still a WIP/RFC, so I'll only skim it in\n> this round (but I may come back and read it again later with finer\n> toothed comb).\n>\n>> +static int get_next_task(void **pp_task_cb,\n>> +                      struct child_process *cp,\n>> +                      struct strbuf *err,\n>> +                      void *pp_cb)\n>\n> Will you have only one caller of the parallel run-command API in\n> this file, or will you be adding more to allow various different\n> operations run in parallel as more things are rewritten?  I am\n> guessing that it would be the latter, but if that is the case,\n> perhaps the function wants to be named a bit more specificly for\n> this first user, no?  Same for start_failure and task_finished.\n\nOk, will rename.\nAlthough I am not sure if I need to rewrite more in C for \"git submodule\".\n\nI only rewrite git submodule update because git clone --recurse is just\nblindly calling out to git submodule update.  So instead of parallelizing\n\"submodule update\" I could have put a parallel submodule clone into\nthe clone command. (That looks strangely appealing now, because it\nmay be even faster as there is no downstream pipe with sequential\ncheckouts, so you could have one parallel pool with chained clone\nand checkout commands).\n\n>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 8b0eb9a..ea883b9 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -655,17 +655,18 @@ cmd_update()\n>>               cmd_init \"--\" \"$@\" || return\n>>       fi\n>>\n>> -     cloned_modules=\n>> -     git submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n>> +     git submodule--helper update-clone ${GIT_QUIET:+--quiet} \\\n>> +             ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n>> +             ${prefix:+--recursive_prefix \"$prefix\"} \\\n>> +             ${update:+--update \"$update\"} \\\n>> +             ${reference:+--reference \"$reference\"} \\\n>> +             ${depth:+--depth \"$depth\"} \\\n>> +             \"$@\" | {\n>>       err=\n>> -     while read mode sha1 stage sm_path\n>> +     while read mode sha1 stage just_cloned sm_path\n>>       do\n>\n> I wonder if you really want this to be upstream of a pipe.  When the\n> downstream loop needs to abort, what happens to the remainder of the\n> \"clone\" part of the processing that is still ongoing in the upstream\n> of the pipe?  I would imagine that the \"update-clone\" network\n> accessing phase is the more human-time consuming part, so I suspect\n> that it would be much better to let the cloning part go and finish\n> first (during which time the human-user can spend time for other\n> things, like getting cup of coffee or filling expense reports) and\n> before moving to the loop that can stop and ask the human-user for\n> help.\n>\n> The fix for the above could be trivial (do not pipe, just take the\n> output to a temporary file, and then feed the \"while read\" loop from\n> that temporary file), and I suspect it would make a big difference\n> for usability.\n\nI'd like to counter your argument with quoting code from update_clone\nmethod:\n\n     run_processes_parallel(1, get_next_task, start_failure,\ntask_finished, &pp);\n\n     if (pp.print_unmatched) {\n         printf(\"#unmatched\\n\");\n         return 1;\n     }\n\n     for_each_string_list_item(item, &pp.projectlines) {\n         utf8_fprintf(stdout, \"%s\", item->string);\n     }\n\nSo we do already all the cloning first, and then once we did all of that\nwe just put out all accumulated lines of text. (It was harder to come up with\na sufficient file name than just storing it in memory. I don't think\nmemory is an\nissue here, only a few bytes per submodule. So even 1000 submodules would\nconsume maybe 100kB)\n\nHaving a file though would allow us to continue after human interaction fixed\none problem.\n\n\n\n\n>\n> Thanks.\n"},{"id":"272071","messageId":"CAGZ79kacfCG67yRLhM000wdxGxFCXXB840hiEd_vKRmOn7EzeQ@mail.gmail.com","threadId":"40600","inReplyTo":"xmqqh9lkufpt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-21T21:07:32Z","receivedAt":"2015-10-21T21:07:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 21, 2015 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> And of course we already have these array-clear calls in\n>> finish_command().\n>>\n>> So I agree that deinit helper should exist, but\n>>\n>>  * it should be file-scope static;\n>>\n>>  * it should be called by finish_command(); and\n>>\n>>  * if you are calling it from collect_finished(), then existing\n>>    calls to array-clear should go.\n>>\n>> Other than that, this looks good.\n>\n> I'll queue this instead (the above squashed in and description\n> corrected).\n>\n\nThanks,\nStefan\n"},{"id":"272073","messageId":"xmqqvb9zudai.fsf@gitster.mtv.corp.google.com","threadId":"40600","inReplyTo":"CAGZ79kaa6b49rDW204ydtY0cf4NugSKOm+sBHKBYYoEsVugVfw@mail.gmail.com","subject":"Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-21T21:23:01Z","receivedAt":"2015-10-21T21:23:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I'd like to counter your argument with quoting code from update_clone\n> method:\n> \n>      run_processes_parallel(1, get_next_task, start_failure,\n> task_finished, &pp);\n>\n>      if (pp.print_unmatched) {\n>          printf(\"#unmatched\\n\");\n>          return 1;\n>      }\n>\n>      for_each_string_list_item(item, &pp.projectlines) {\n>          utf8_fprintf(stdout, \"%s\", item->string);\n>      }\n>\n> So we do already all the cloning first, and then once we did all of that\n> we just put out all accumulated lines of text. (It was harder to come up with\n> a sufficient file name than just storing it in memory. I don't think\n> memory is an\n> issue here, only a few bytes per submodule. So even 1000 submodules would\n> consume maybe 100kB)\n\nThat does not sound like a counter-argument; two bad design choices\ncompensating each other's shortcomings, perhaps ;-)\n\n> Having a file though would allow us to continue after human\n> interaction fixed one problem.\n\nYes.  That does sound like a better design.\n\nThis obviously depends on the impact to the other part of what\ncmd_update() does, but your earlier idea to investigate the\nfeasibility and usefulness of updating \"clone --recurse-submodules\"\ndoes sound like a good thing to do, too.  That's an excellent point.\n"},{"id":"272075","messageId":"CAGZ79kZiwf7AeCFmeA+FyF8p92+K5ZCEDySx07g+VJ4Dg8MK1Q@mail.gmail.com","threadId":"40600","inReplyTo":"xmqqvb9zudai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-21T22:14:55Z","receivedAt":"2015-10-21T22:14:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 21, 2015 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I'd like to counter your argument with quoting code from update_clone\n>> method:\n>>\n>>      run_processes_parallel(1, get_next_task, start_failure,\n>> task_finished, &pp);\n>>\n>>      if (pp.print_unmatched) {\n>>          printf(\"#unmatched\\n\");\n>>          return 1;\n>>      }\n>>\n>>      for_each_string_list_item(item, &pp.projectlines) {\n>>          utf8_fprintf(stdout, \"%s\", item->string);\n>>      }\n>>\n>> So we do already all the cloning first, and then once we did all of that\n>> we just put out all accumulated lines of text. (It was harder to come up with\n>> a sufficient file name than just storing it in memory. I don't think\n>> memory is an\n>> issue here, only a few bytes per submodule. So even 1000 submodules would\n>> consume maybe 100kB)\n>\n> That does not sound like a counter-argument; two bad design choices\n> compensating each other's shortcomings, perhaps ;-)\n\nI was phrasing it worse than I meant to. I should have pointed out the\npositive aspect of having first all clones done and then the\nlocal post processing in the downstream pipe afterwards.\n\n>\n>> Having a file though would allow us to continue after human\n>> interaction fixed one problem.\n>\n> Yes.  That does sound like a better design.\n\nI don't think the proposed patches make it worse than it already is.\nBefore we have the \"submodule--helper list\" which tells downpipe to\ndo all the things. Now we just take out the cloning and make it an\nupfront action, eventually faster due to parallelism.\n\nAlso I think we should not promote \"git submodule\" and specially\nits update sub-command to be the best command available. Ideally\nwe want to rather implement implicit submodule handling in the\nother commands such as clone, pull, fetch, checkout, merge, rebase.\nand by that I mean better defaults than just \"don't touch the submodules,\nas that's the safest thing to do now\".\n\n>\n> This obviously depends on the impact to the other part of what\n> cmd_update() does, but your earlier idea to investigate the\n> feasibility and usefulness of updating \"clone --recurse-submodules\"\n> does sound like a good thing to do, too.  That's an excellent point.\n\nI investigated and I think it's a bad idea now :)\nBecause of the --recursive switch we would need to do more than just\n\n    submodules_init()\n    run_parallel(.. clone_and_checkout...);\n\nbut each cloned submodule would need to be inspected for recursive\nsubmodules again and then we would need to add that to the list of\nsubmodules to process.\n\nI estimate this is about as much work as improving \"git submodule update\"\nto do uncontroversial checkouts in the first parallel phase.\n"}]}