{"thread":{"id":"40589","subject":"[PATCH 0/5] Fixes for the parallel processing","startedAt":"2015-10-19T18:24:41Z","lastAt":"2015-10-20T19:07:13Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"271938","messageId":"1445279086-31066-1-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":null,"subject":"[PATCH 0/5] Fixes for the parallel processing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:41Z","receivedAt":"2015-10-19T18:24:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I noticed a problem with the gracefully abortion in the parallel processing,\nthat is fixed in patch 1.\n\nPatch 2 makes the API more maintainable/usable (the caller may forget to\ncall child_process_init and only fill in fields which the callback is\ninterested in)\n\nPatch 3 is another fixup. It actually initializes the shutdown flag properly.\n(Apparently it had the right value so far)\n\nPatches 4 and 5 add more tests both for the gracefully aborting case as well\nas the standard use case.\n\nThis applies on top of sb/submodule-parallel-fetch which is already in next.\nJunio, do you want me to reroll that series with these patches squashed in\nappropriately, or just put it on top of that series ?\n\nThanks,\nStefan\n\nStefan Beller (5):\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\n run-command.c | 5 ++++-\n t/t0061-run-command.sh | 28 ++++++++++++++++++++++++++--\n test-run-command.c | 22 ++++++++++++++++++++--\n 3 files changed, 50 insertions(+), 5 deletions(-)\n\n-- \n2.5.0.285.g8fe9b61.dirty\n"},{"id":"271939","messageId":"1445279086-31066-2-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/5] run-command: Fix early shutdown","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:42Z","receivedAt":"2015-10-19T18:24:42Z","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. Both returns from that function should\nreturn any accumulated results.\n\nSigned-off-by: Stefan Beller \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..8f47c6e 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\treturn result;\n \n \t\tif (pid < 0)\n \t\t\tdie_errno(\"wait\");\n-- \n2.5.0.285.g8fe9b61.dirty\n"},{"id":"271941","messageId":"1445279086-31066-3-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/5] run-command: Call get_next_task with a clean child process.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:43Z","receivedAt":"2015-10-19T18:24:43Z","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\nSigned-off-by: Stefan Beller \n---\n run-command.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8f47c6e..b8b5747 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)\n \tif (i == pp->max_processes)\n \t\tdie(\"BUG: bookkeeping is hard\");\n \n+\tchild_process_init(&pp->children[i].process);\n+\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-- \n2.5.0.285.g8fe9b61.dirty\n"},{"id":"271940","messageId":"1445279086-31066-4-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/5] run-command: Initialize the shutdown flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:44Z","receivedAt":"2015-10-19T18:24:44Z","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 \n---\n run-command.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex b8b5747..108b930 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -956,6 +956,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.285.g8fe9b61.dirty\n"},{"id":"271942","messageId":"1445279086-31066-5-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/5] test-run-command: test for gracefully aborting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:45Z","receivedAt":"2015-10-19T18:24:45Z","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 \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.285.g8fe9b61.dirty\n"},{"id":"271943","messageId":"1445279086-31066-6-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/5] test-run-command: Increase test coverage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-19T18:24:46Z","receivedAt":"2015-10-19T18:24:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n t/t0061-run-command.sh | 16 +++++++++++++---\n test-run-command.c | 12 ++++++++----\n 2 files changed, 21 insertions(+), 7 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..44710c3 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -47,6 +47,7 @@ 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@@ -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.285.g8fe9b61.dirty\n"},{"id":"271984","messageId":"1445363460-17616-1-git-send-email-sbeller@google.com","threadId":"40589","inReplyTo":"1445279086-31066-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/5] run-command: Fix missing output from late callbacks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T17:51:00Z","receivedAt":"2015-10-20T17:51:00Z","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 \n---\n\nAnother issue I just found. This goes on top of the series\n [PATCH 0/5] Fixes for the parallel processing\n \nThanks,\nStefan\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 108b930..707e0a6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -979,6 +979,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@@ -1016,8 +1022,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 44710c3..91e94e8 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.273.gfd9d19f.dirty\n"},{"id":"271986","messageId":"xmqqa8rd8jsg.fsf@gitster.mtv.corp.google.com","threadId":"40589","inReplyTo":"1445279086-31066-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/5] run-command: Fix early shutdown","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T18:41:19Z","receivedAt":"2015-10-20T18:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The return value of `pp_collect_finished` indicates if we want to shutdown\n> the parallel processing early. Both returns from that function should\n> return any accumulated results.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nMakes sense. The code could \"break\" out of the loop, leaving only\none return site in the function, which would be a way to ensure that\nwe'd consistently return the accumulated result, but this would also\ndo.\n\nThanks.\n\n> run-command.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index ef3da27..8f47c6e 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\treturn result;\n> \n> \t\tif (pid < 0)\n> \t\t\tdie_errno(\"wait\");\n"},{"id":"271987","messageId":"xmqq61218jfd.fsf@gitster.mtv.corp.google.com","threadId":"40589","inReplyTo":"1445279086-31066-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/5] run-command: Call get_next_task with a clean child process.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T18:49:10Z","receivedAt":"2015-10-20T18:49:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> If the `get_next_task` did not explicitly called child_process_init\n\nI locally did \"If get_next_task did not explicitly call child_process_init\"\n\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> Signed-off-by: Stefan Beller \n> ---\n\nAgain this makes sense.\n\nAnother way, which I suspect may be conceptually cleaner, would be\nto do this clean-up where pp->children[i].in_use is set to false, as\nthat is where the particular task is declared to be available for\nreuse. That location should be responsible to ensure that the task\nindeed is reusable by calling child_process_init().\n\nBy the way, does child_process_init() leak old argv/env arrays, or\nhave they already been cleared by calling finish_command() when we\ncome to this codepath?\n\n> run-command.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 8f47c6e..b8b5747 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)\n> \tif (i == pp->max_processes)\n> \t\tdie(\"BUG: bookkeeping is hard\");\n> \n> +\tchild_process_init(&pp->children[i].process);\n> +\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"},{"id":"271988","messageId":"xmqq1tcp8j82.fsf@gitster.mtv.corp.google.com","threadId":"40589","inReplyTo":"1445279086-31066-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/5] test-run-command: Increase test coverage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-20T18:53:33Z","receivedAt":"2015-10-20T18:53:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n> t/t0061-run-command.sh | 16 +++++++++++++---\n> test-run-command.c | 12 ++++++++----\n> 2 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\n> index 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> \n> diff --git a/test-run-command.c b/test-run-command.c\n> index 4b59482..44710c3 100644\n> --- a/test-run-command.c\n> +++ b/test-run-command.c\n> @@ -47,6 +47,7 @@ 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> @@ -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\tproc.argv = (const char **)argv + 3;\n\nor\n\n\tproc.argv = (const char **)&argv[3];\n\nGiven the line immediately before refers to argv[2], the latter\nmight be easier on the eyes to follow.\n\nIn what way does this \"Increase\" test coverage? By allowing the\ncaller to specify arbitrarily higher parallelism?\n\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"},{"id":"271989","messageId":"CAGZ79kYAv6uVmTZEcstu7a5LjOuUvWiOgJh-UfPZyLAi_v4ojA@mail.gmail.com","threadId":"40589","inReplyTo":"xmqq1tcp8j82.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] test-run-command: Increase test coverage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T19:05:01Z","receivedAt":"2015-10-20T19:05:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 20, 2015 at 11:53 AM, Junio C Hamano wrote:\n>\n> proc.argv = (const char **)argv + 3;\n>\n> or\n>\n> proc.argv = (const char **)&argv[3];\n\nok, will fix\n\n>\n> Given the line immediately before refers to argv[2], the latter\n> might be easier on the eyes to follow.\n>\n> In what way does this \"Increase\" test coverage? By allowing the\n> caller to specify arbitrarily higher parallelism?\n\nCurrently 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 the test I came up with is 3 cases\n* more tasks than max jobs (implying reused slots)\n* equal number of jobs and max jobs\n* less tasks than max jobs (implying unused slots)\n\n>\n>> +\n>> + if (!strcmp(argv[1], \"run-command-parallel\"))\n>> + exit(run_processes_parallel(jobs, parallel_next,\n>> NULL, NULL, &proc));\n>>\n>> - if (!strcmp(argv[1], \"run-command-abort-3\"))\n>> - exit(run_processes_parallel(3, parallel_next,\n>> + if (!strcmp(argv[1], \"run-command-abort\"))\n>> + exit(run_processes_parallel(jobs, parallel_next,\n>> NULL, task_finished, &proc));\n>>\n>> fprintf(stderr, \"check usage\\n\");\n"},{"id":"271990","messageId":"CAGZ79kbzVXaO+WHHfwQYJnEdQU+3kUrwcKUPKTjL1YxTev5g8Q@mail.gmail.com","threadId":"40589","inReplyTo":"xmqq61218jfd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] run-command: Call get_next_task with a clean child process.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-20T19:07:13Z","receivedAt":"2015-10-20T19:07:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 20, 2015 at 11:49 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> If the `get_next_task` did not explicitly called child_process_init\n>\n> I locally did \"If get_next_task did not explicitly call child_process_init\"\n>\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>> Signed-off-by: Stefan Beller \n>> ---\n>\n> Again this makes sense.\n>\n> Another way, which I suspect may be conceptually cleaner, would be\n> to do this clean-up where pp->children[i].in_use is set to false, as\n> that is where the particular task is declared to be available for\n> reuse. That location should be responsible to ensure that the task\n> indeed is reusable by calling child_process_init().\n>\n> By the way, does child_process_init() leak old argv/env arrays, or\n> have they already been cleared by calling finish_command() when we\n> come to this codepath?\n\nIt does, yes. In the reroll I move the initialization to both pp_init (to init\nfor the first use) and to the place where in_use is set to false.\n\n>\n>> run-command.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 8f47c6e..b8b5747 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)\n>> if (i == pp->max_processes)\n>> die(\"BUG: bookkeeping is hard\");\n>>\n>> + child_process_init(&pp->children[i].process);\n>> +\n>> if (!pp->get_next_task(&pp->children[i].data,\n>> &pp->children[i].process,\n>> &pp->children[i].err,\n"}]}