{"thread":{"id":"58768","subject":"[PATCH v4 0/5] submodule: parallelize diff","startedAt":"2022-11-08T18:42:21Z","lastAt":"2023-01-17T21:07:21Z","messageCount":26,"participants":["Calvin Wan","Glen Choo","Jonathan Tan","Elijah Newren","Ævar Arnfjörð Bjarmason","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"466873","messageId":"20221108184200.2813458-1-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 0/5] submodule: parallelize diff","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:41:55Z","receivedAt":"2022-11-08T18:42:21Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Original cover letter for context:\nhttps://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/\n\nChanges since v3\n\nRenamed pipe_output_fn to duplicate_output_fn and now passes an\nadditional \"strbuf* out\" parameter. Output is directly duplicated\nto that function rather than held in a separate variable.\nSlightly rewrote the tests to more accurately capture the expected\noutput of duplicate_output_fn.\n\nRemoved a patch that added an option to hide child process output.\nChild process output is now reset in status_duplicate_output.\n\nMore style changes as suggested by Avar\n\nCalvin Wan (5):\n run-command: add duplicate_output_fn to run_processes_parallel_opts\n submodule: strbuf variable rename\n submodule: move status parsing into function\n diff-lib: refactor match_stat_with_submodule\n diff-lib: parallelize run_diff_files for submodules\n\n Documentation/config/submodule.txt | 12 ++\n diff-lib.c | 103 +++++++++++--\n run-command.c | 13 +-\n run-command.h | 24 +++\n submodule.c | 229 +++++++++++++++++++++++++----\n submodule.h | 9 ++\n t/helper/test-run-command.c | 21 +++\n t/t0061-run-command.sh | 39 +++++\n t/t4027-diff-submodule.sh | 19 +++\n t/t7506-status-submodule.sh | 19 +++\n 10 files changed, 441 insertions(+), 47 deletions(-)\n\n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466874","messageId":"20221108184200.2813458-2-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:41:56Z","receivedAt":"2022-11-08T18:42:25Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Add duplicate_output_fn as an optionally set function in\nrun_process_parallel_opts. If set, output from each child process is\ncopied and passed to the callback function whenever output from the\nchild process is buffered to allow for separate parsing.\n\nSigned-off-by: Calvin Wan \n---\n run-command.c | 13 +++++++++++--\n run-command.h | 24 +++++++++++++++++++++++\n t/helper/test-run-command.c | 21 ++++++++++++++++++++\n t/t0061-run-command.sh | 39 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 95 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex c772acd743..b8f430eb03 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1560,6 +1560,9 @@ static void pp_init(struct parallel_processes *pp,\n \n \tif (!opts->get_next_task)\n \t\tBUG(\"you need to specify a get_next_task function\");\n+\t\n+\tif (opts->duplicate_output && opts->ungroup)\n+\t\tBUG(\"duplicate_output and ungroup are incompatible with each other\");\n \n \tCALLOC_ARRAY(pp->children, n);\n \tif (!opts->ungroup)\n@@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n \tfor (size_t i = 0; i < opts->processes; i++) {\n \t\tif (pp->children[i].state == GIT_CP_WORKING &&\n \t\t pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n-\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n-\t\t\t\t\t\t pp->children[i].process.err, 0);\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tint n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n+\t\t\tstrbuf_addbuf(&pp->children[i].err, &buf);\n+\t\t\tif (opts->duplicate_output)\n+\t\t\t\topts->duplicate_output(&buf, &pp->children[i].err,\n+\t\t\t\t\t opts->data,\n+\t\t\t\t\t pp->children[i].data);\n+\t\t\tstrbuf_release(&buf);\n \t\t\tif (n == 0) {\n \t\t\t\tclose(pp->children[i].process.err);\n \t\t\t\tpp->children[i].state = GIT_CP_WAIT_CLEANUP;\ndiff --git a/run-command.h b/run-command.h\nindex e3e1ea01ad..dd6d6a86c2 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -440,6 +440,24 @@ typedef int (*start_failure_fn)(struct strbuf *out,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\n \n+/**\n+ * This callback is called whenever output from a child process is buffered\n+ * \n+ * \"struct strbuf *process_out\" contains the output from the child process\n+ * \n+ * See run_processes_parallel() below for a discussion of the \"struct\n+ * strbuf *out\" parameter.\n+ *\n+ * pp_cb is the callback cookie as passed into run_processes_parallel,\n+ * pp_task_cb is the callback cookie as passed into get_next_task_fn.\n+ *\n+ * This function is incompatible with \"ungroup\"\n+ */\n+typedef void (*duplicate_output_fn)(struct strbuf *process_out,\n+\t\t\t\t struct strbuf *out,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb);\n+\n /**\n * This callback is called on every child process that finished processing.\n *\n@@ -493,6 +511,12 @@ struct run_process_parallel_opts\n \t */\n \tstart_failure_fn start_failure;\n \n+\t/**\n+\t * duplicate_output: See duplicate_output_fn() above. This should be\n+\t * NULL unless process specific output is needed\n+\t */\n+\tduplicate_output_fn duplicate_output;\n+\n \t/**\n \t * task_finished: See task_finished_fn() above. This can be\n \t * NULL to omit any special handling.\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 3ecb830f4a..40dd329e02 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,\n \treturn 0;\n }\n \n+static void duplicate_output(struct strbuf *process_out,\n+\t\t\tstruct strbuf *out,\n+\t\t\tvoid *pp_cb,\n+\t\t\tvoid *pp_task_cb)\n+{\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tstring_list_split(&list, process_out->buf, '\\n', -1);\n+\tfor (size_t i = 0; i < list.nr; i++) {\n+\t\tif (strlen(list.items[i].string) > 0)\n+\t\t\tfprintf(stderr, \"duplicate_output: %s\\n\", list.items[i].string);\n+\t}\n+\tstring_list_clear(&list, 0);\n+}\n+\n static int task_finished(int result,\n \t\t\t struct strbuf *err,\n \t\t\t void *pp_cb,\n@@ -439,6 +454,12 @@ int cmd__run_command(int argc, const char **argv)\n \t\topts.ungroup = 1;\n \t}\n \n+\tif (!strcmp(argv[1], \"--duplicate-output\")) {\n+\t\targv += 1;\n+\t\targc -= 1;\n+\t\topts.duplicate_output = duplicate_output;\n+\t}\n+\n \tjobs = atoi(argv[2]);\n \tstrvec_clear(&proc.args);\n \tstrvec_pushv(&proc.args, (const char **)argv + 3);\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 7b5423eebd..130aec7c68 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -134,6 +134,15 @@ test_expect_success 'run_command runs in parallel with more jobs available than\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'run_command runs in parallel with more jobs available than tasks --duplicate-output' '\n+\ttest-tool run-command --duplicate-output run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest 4 = $(grep -c \"duplicate_output: Hello\" err) &&\n+\ttest 4 = $(grep -c \"duplicate_output: World\" err) &&\n+\tsed \"/duplicate_output/d\" err > err1 &&\n+\ttest_cmp expect err1\n+'\n+\n test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '\n \ttest-tool run-command --ungroup run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n \ttest_line_count = 8 out &&\n@@ -145,6 +154,15 @@ test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'run_command runs in parallel with as many jobs as tasks --duplicate-output' '\n+\ttest-tool run-command --duplicate-output run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest 4 = $(grep -c \"duplicate_output: Hello\" err) &&\n+\ttest 4 = $(grep -c \"duplicate_output: World\" err) &&\n+\tsed \"/duplicate_output/d\" err > err1 &&\n+\ttest_cmp expect err1\n+'\n+\n test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '\n \ttest-tool run-command --ungroup run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n \ttest_line_count = 8 out &&\n@@ -156,6 +174,15 @@ 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 runs in parallel with more tasks than jobs available --duplicate-output' '\n+\ttest-tool run-command --duplicate-output run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest 4 = $(grep -c \"duplicate_output: Hello\" err) &&\n+\ttest 4 = $(grep -c \"duplicate_output: World\" err) &&\n+\tsed \"/duplicate_output/d\" err > err1 &&\n+\ttest_cmp expect err1\n+'\n+\n test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '\n \ttest-tool run-command --ungroup run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n \ttest_line_count = 8 out &&\n@@ -176,6 +203,12 @@ test_expect_success 'run_command is asked to abort gracefully' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'run_command is asked to abort gracefully --duplicate-output' '\n+\ttest-tool run-command --duplicate-output run-command-abort 3 false >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'run_command is asked to abort gracefully (ungroup)' '\n \ttest-tool run-command --ungroup run-command-abort 3 false >out 2>err &&\n \ttest_must_be_empty out &&\n@@ -191,6 +224,12 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'run_command outputs --duplicate-output' '\n+\ttest-tool run-command --duplicate-output run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'run_command outputs (ungroup) ' '\n \ttest-tool run-command --ungroup run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" >out 2>err &&\n \ttest_must_be_empty out &&\n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466875","messageId":"20221108184200.2813458-3-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 2/5] submodule: strbuf variable rename","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:41:57Z","receivedAt":"2022-11-08T18:42:32Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"A prepatory change for a future patch that moves the status parsing\nlogic to a separate function.\n\nSigned-off-by: Calvin Wan \n---\n submodule.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b958162d28..31ee53bd57 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1900,25 +1900,28 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \n \tfp = xfdopen(cp.out, \"r\");\n \twhile (strbuf_getwholeline(&buf, fp, '\\n') != EOF) {\n+\t\tchar *str = buf.buf;\n+\t\tconst size_t len = buf.len;\n+\n \t\t/* regular untracked files */\n-\t\tif (buf.buf[0] == '?')\n+\t\tif (str[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \n-\t\tif (buf.buf[0] == 'u' ||\n-\t\t buf.buf[0] == '1' ||\n-\t\t buf.buf[0] == '2') {\n+\t\tif (str[0] == 'u' ||\n+\t\t str[0] == '1' ||\n+\t\t str[0] == '2') {\n \t\t\t/* T = line type, XY = status, SSSS = submodule state */\n-\t\t\tif (buf.len < strlen(\"T XY SSSS\"))\n+\t\t\tif (len < strlen(\"T XY SSSS\"))\n \t\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n-\t\t\t\t buf.buf);\n+\t\t\t\t str);\n \n-\t\t\tif (buf.buf[5] == 'S' && buf.buf[8] == 'U')\n+\t\t\tif (str[5] == 'S' && str[8] == 'U')\n \t\t\t\t/* nested untracked file */\n \t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n \n-\t\t\tif (buf.buf[0] == 'u' ||\n-\t\t\t buf.buf[0] == '2' ||\n-\t\t\t memcmp(buf.buf + 5, \"S..U\", 4))\n+\t\t\tif (str[0] == 'u' ||\n+\t\t\t str[0] == '2' ||\n+\t\t\t memcmp(str + 5, \"S..U\", 4))\n \t\t\t\t/* other change */\n \t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466876","messageId":"20221108184200.2813458-4-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 3/5] submodule: move status parsing into function","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:41:58Z","receivedAt":"2022-11-08T18:42:35Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"A future patch requires the ability to parse the output of git\nstatus --porcelain=2. Move parsing code from is_submodule_modified to\nparse_status_porcelain.\n\nSigned-off-by: Calvin Wan \n---\n submodule.c | 74 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 42 insertions(+), 32 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 31ee53bd57..fd3385620c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1864,6 +1864,45 @@ int fetch_submodules(struct repository *r,\n \treturn spf.result;\n }\n \n+static int parse_status_porcelain(char *str, size_t len,\n+\t\t\t\t unsigned *dirty_submodule,\n+\t\t\t\t int ignore_untracked)\n+{\n+\t/* regular untracked files */\n+\tif (str[0] == '?')\n+\t\t*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\tif (str[0] == 'u' ||\n+\t str[0] == '1' ||\n+\t str[0] == '2') {\n+\t\t/* T = line type, XY = status, SSSS = submodule state */\n+\t\tif (len < strlen(\"T XY SSSS\"))\n+\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n+\t\t\t str);\n+\n+\t\tif (str[5] == 'S' && str[8] == 'U')\n+\t\t\t/* nested untracked file */\n+\t\t\t*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\n+\t\tif (str[0] == 'u' ||\n+\t\t str[0] == '2' ||\n+\t\t memcmp(str + 5, \"S..U\", 4))\n+\t\t\t/* other change */\n+\t\t\t*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t}\n+\n+\tif ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n+\t ((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n+\t ignore_untracked)) {\n+\t\t/*\n+\t\t* We're not interested in any further information from\n+\t\t* the child any more, neither output nor its exit code.\n+\t\t*/\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1903,39 +1942,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\tchar *str = buf.buf;\n \t\tconst size_t len = buf.len;\n \n-\t\t/* regular untracked files */\n-\t\tif (str[0] == '?')\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\n-\t\tif (str[0] == 'u' ||\n-\t\t str[0] == '1' ||\n-\t\t str[0] == '2') {\n-\t\t\t/* T = line type, XY = status, SSSS = submodule state */\n-\t\t\tif (len < strlen(\"T XY SSSS\"))\n-\t\t\t\tBUG(\"invalid status --porcelain=2 line %s\",\n-\t\t\t\t str);\n-\n-\t\t\tif (str[5] == 'S' && str[8] == 'U')\n-\t\t\t\t/* nested untracked file */\n-\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\n-\t\t\tif (str[0] == 'u' ||\n-\t\t\t str[0] == '2' ||\n-\t\t\t memcmp(str + 5, \"S..U\", 4))\n-\t\t\t\t/* other change */\n-\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t}\n-\n-\t\tif ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&\n-\t\t ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||\n-\t\t ignore_untracked)) {\n-\t\t\t/*\n-\t\t\t * We're not interested in any further information from\n-\t\t\t * the child any more, neither output nor its exit code.\n-\t\t\t */\n-\t\t\tignore_cp_exit_code = 1;\n+\t\tignore_cp_exit_code = parse_status_porcelain(str, len, &dirty_submodule,\n+\t\t\t\t\t\t\t ignore_untracked);\n+\t\tif (ignore_cp_exit_code)\n \t\t\tbreak;\n-\t\t}\n \t}\n \tfclose(fp);\n \n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466877","messageId":"20221108184200.2813458-5-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 4/5] diff-lib: refactor match_stat_with_submodule","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:41:59Z","receivedAt":"2022-11-08T18:42:37Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Flatten out the if statements in match_stat_with_submodule so the\nlogic is more readable and easier for future patches to add to.\norig_flags didn't need to be set if the cache entry wasn't a\nGITLINK so defer setting it.\n\nSigned-off-by: Calvin Wan \n---\n diff-lib.c | 29 ++++++++++++++++++-----------\n 1 file changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2edea41a23..f5257c0c71 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -73,18 +73,25 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t unsigned *dirty_submodule)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n-\tif (S_ISGITLINK(ce->ce_mode)) {\n-\t\tstruct diff_flags orig_flags = diffopt->flags;\n-\t\tif (!diffopt->flags.override_submodule_config)\n-\t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n-\t\tif (diffopt->flags.ignore_submodules)\n-\t\t\tchanged = 0;\n-\t\telse if (!diffopt->flags.ignore_dirty_submodules &&\n-\t\t\t (!changed || diffopt->flags.dirty_submodules))\n-\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n-\t\t\t\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n-\t\tdiffopt->flags = orig_flags;\n+\tstruct diff_flags orig_flags;\n+\n+\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\tgoto ret;\n+\n+\torig_flags = diffopt->flags;\n+\tif (!diffopt->flags.override_submodule_config)\n+\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n+\tif (diffopt->flags.ignore_submodules) {\n+\t\tchanged = 0;\n+\t\tgoto cleanup;\n \t}\n+\tif (!diffopt->flags.ignore_dirty_submodules &&\n+\t (!changed || diffopt->flags.dirty_submodules))\n+\t\t*dirty_submodule = is_submodule_modified(ce->name,\n+\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n+cleanup:\n+\tdiffopt->flags = orig_flags;\n+ret:\n \treturn changed;\n }\n \n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"466878","messageId":"20221108184200.2813458-6-calvinwan@google.com","threadId":"58768","inReplyTo":"https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/","subject":"[PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-08T18:42:00Z","receivedAt":"2022-11-08T18:42:45Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"During the iteration of the index entries in run_diff_files, whenever\na submodule is found and needs its status checked, a subprocess is\nspawned for it. Instead of spawning the subprocess immediately and\nwaiting for its completion to continue, hold onto all submodules and\nrelevant information in a list. Then use that list to create tasks for\nrun_processes_parallel. Subprocess output is duplicated and passed to\nstatus_pipe_output which parses it. \n\nAdd config option submodule.diffJobs to set the maximum number\nof parallel jobs. The option defaults to 1 if unset. If set to 0, the\nnumber of jobs is set to online_cpus().\n\nSince run_diff_files is called from many different commands, I chose\nto grab the config option in the function rather than adding variables\nto every git command and then figuring out how to pass them all in.\n\nSigned-off-by: Calvin Wan \n---\n Documentation/config/submodule.txt | 12 +++\n diff-lib.c | 80 +++++++++++++--\n submodule.c | 154 +++++++++++++++++++++++++++++\n submodule.h | 9 ++\n t/t4027-diff-submodule.sh | 19 ++++\n t/t7506-status-submodule.sh | 19 ++++\n 6 files changed, 287 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\nindex 6490527b45..1144a5ad74 100644\n--- a/Documentation/config/submodule.txt\n+++ b/Documentation/config/submodule.txt\n@@ -93,6 +93,18 @@ submodule.fetchJobs::\n \tin parallel. A value of 0 will give some reasonable default.\n \tIf unset, it defaults to 1.\n \n+submodule.diffJobs::\n+\tSpecifies how many submodules are diffed at the same time. A\n+\tpositive integer allows up to that number of submodules diffed\n+\tin parallel. A value of 0 will give the number of logical cores.\n+\tIf unset, it defaults to 1. The diff operation is used by many\n+\tother git commands such as add, merge, diff, status, stash and\n+\tmore. Note that the expensive part of the diff operation is\n+\treading the index from cache or memory. Therefore multiple jobs\n+\tmay be detrimental to performance if your hardware does not\n+\tsupport parallel reads or if the number of jobs greatly exceeds\n+\tthe amount of supported reads.\n+\n submodule.alternateLocation::\n \tSpecifies how the submodules obtain alternates when submodules are\n \tcloned. Possible values are `no`, `superproject`.\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f5257c0c71..30a3d9a2b5 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -14,6 +14,7 @@\n #include \"dir.h\"\n #include \"fsmonitor.h\"\n #include \"commit-reach.h\"\n+#include \"config.h\"\n \n /*\n * diff-files\n@@ -65,15 +66,20 @@ static int check_removed(const struct index_state *istate, const struct cache_en\n * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES\n * option is set, the caller does not only want to know if a submodule is\n * modified at all but wants to know all the conditions that are met (new\n- * commits, untracked content and/or modified content).\n+ * commits, untracked content and/or modified content). If\n+ * defer_submodule_status bit is set, dirty_submodule will be left to the\n+ * caller to set. defer_submodule_status can also be set to 0 in this\n+ * function if there is no need to check if the submodule is modified.\n */\n static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\t\t\t const struct cache_entry *ce,\n \t\t\t\t struct stat *st, unsigned ce_option,\n-\t\t\t\t unsigned *dirty_submodule)\n+\t\t\t\t unsigned *dirty_submodule, int *defer_submodule_status,\n+\t\t\t\t unsigned *ignore_untracked)\n {\n \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n \tstruct diff_flags orig_flags;\n+\tint defer = 0;\n \n \tif (!S_ISGITLINK(ce->ce_mode))\n \t\tgoto ret;\n@@ -86,12 +92,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\tgoto cleanup;\n \t}\n \tif (!diffopt->flags.ignore_dirty_submodules &&\n-\t (!changed || diffopt->flags.dirty_submodules))\n-\t\t*dirty_submodule = is_submodule_modified(ce->name,\n+\t (!changed || diffopt->flags.dirty_submodules)) {\n+\t\tif (defer_submodule_status && *defer_submodule_status) {\n+\t\t\tdefer = 1;\n+\t\t\t*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;\n+\t\t} else {\n+\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n \t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n+\t\t}\n+\t}\n cleanup:\n \tdiffopt->flags = orig_flags;\n ret:\n+\tif (defer_submodule_status)\n+\t\t*defer_submodule_status = defer;\n \treturn changed;\n }\n \n@@ -103,6 +117,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n \tuint64_t start = getnanotime();\n \tstruct index_state *istate = revs->diffopt.repo->index;\n+\tstruct string_list submodules = STRING_LIST_INIT_NODUP;\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n@@ -227,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tnewmode = ce->ce_mode;\n \t\t} else {\n \t\t\tstruct stat st;\n+\t\t\tunsigned ignore_untracked = 0;\n+\t\t\tint defer_submodule_status = !!revs->repo;\n \n \t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (changed) {\n@@ -248,8 +265,25 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t}\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n-\t\t\t\t\t\t\t ce_option, &dirty_submodule);\n+\t\t\t\t\t\t\t ce_option, &dirty_submodule,\n+\t\t\t\t\t\t\t &defer_submodule_status,\n+\t\t\t\t\t\t\t &ignore_untracked);\n \t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n+\t\t\tif (defer_submodule_status) {\n+\t\t\t\tstruct submodule_status_util tmp = {\n+\t\t\t\t\t.changed = changed,\n+\t\t\t\t\t.dirty_submodule = 0,\n+\t\t\t\t\t.ignore_untracked = ignore_untracked,\n+\t\t\t\t\t.newmode = newmode,\n+\t\t\t\t\t.ce = ce,\n+\t\t\t\t};\n+\t\t\t\tstruct string_list_item *item;\n+\n+\t\t\t\titem = string_list_append(&submodules, ce->name);\n+\t\t\t\titem->util = xmalloc(sizeof(tmp));\n+\t\t\t\tmemcpy(item->util, &tmp, sizeof(tmp));\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tif (!changed && !dirty_submodule) {\n@@ -268,6 +302,40 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t ce->name, 0, dirty_submodule);\n \n \t}\n+\tif (submodules.nr > 0) {\n+\t\tint parallel_jobs;\n+\t\tif (git_config_get_int(\"submodule.diffjobs\", ¶llel_jobs))\n+\t\t\tparallel_jobs = 1;\n+\t\telse if (!parallel_jobs)\n+\t\t\tparallel_jobs = online_cpus();\n+\t\telse if (parallel_jobs < 0)\n+\t\t\tdie(_(\"submodule.diffjobs cannot be negative\"));\n+\n+\t\tif (get_submodules_status(revs->repo, &submodules, parallel_jobs))\n+\t\t\tdie(_(\"submodule status failed\"));\n+\t\tfor (size_t i = 0; i < submodules.nr; i++) {\n+\t\t\tstruct submodule_status_util *util = submodules.items[i].util;\n+\t\t\tstruct cache_entry *ce = util->ce;\n+\t\t\tunsigned int oldmode;\n+\t\t\tconst struct object_id *old_oid, *new_oid;\n+\n+\t\t\tif (!util->changed && !util->dirty_submodule) {\n+\t\t\t\tce_mark_uptodate(ce);\n+\t\t\t\tmark_fsmonitor_valid(istate, ce);\n+\t\t\t\tif (!revs->diffopt.flags.find_copies_harder)\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\toldmode = ce->ce_mode;\n+\t\t\told_oid = &ce->oid;\n+\t\t\tnew_oid = util->changed ? null_oid() : &ce->oid;\n+\t\t\tdiff_change(&revs->diffopt, oldmode, util->newmode,\n+\t\t\t\t old_oid, new_oid,\n+\t\t\t\t !is_null_oid(old_oid),\n+\t\t\t\t !is_null_oid(new_oid),\n+\t\t\t\t ce->name, 0, util->dirty_submodule);\n+\t\t}\n+\t}\n+\tstring_list_clear(&submodules, 1);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \ttrace_performance_since(start, \"diff-files\");\n@@ -315,7 +383,7 @@ static int get_stat_data(const struct index_state *istate,\n \t\t\treturn -1;\n \t\t}\n \t\tchanged = match_stat_with_submodule(diffopt, ce, &st,\n-\t\t\t\t\t\t 0, dirty_submodule);\n+\t\t\t\t\t\t 0, dirty_submodule, NULL, NULL);\n \t\tif (changed) {\n \t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\toid = null_oid();\ndiff --git a/submodule.c b/submodule.c\nindex fd3385620c..763a05d523 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1363,6 +1363,18 @@ int submodule_touches_in_range(struct repository *r,\n \treturn ret;\n }\n \n+struct submodule_parallel_status {\n+\tsize_t index_count;\n+\tint result;\n+\n+\tstruct string_list *submodule_names;\n+\tstruct repository *r;\n+\n+\t/* Pending statuses by OIDs */\n+\tstruct status_task **oid_status_tasks;\n+\tint oid_status_tasks_nr, oid_status_tasks_alloc;\n+};\n+\n struct submodule_parallel_fetch {\n \t/*\n \t * The index of the last index entry processed by\n@@ -1445,6 +1457,12 @@ struct fetch_task {\n \tstruct oid_array *commits; /* Ensure these commits are fetched */\n };\n \n+struct status_task {\n+\tconst char *path;\n+\tunsigned dirty_submodule;\n+\tint ignore_untracked;\n+};\n+\n /**\n * When a submodule is not defined in .gitmodules, we cannot access it\n * via the regular submodule-config. Create a fake submodule, which we can\n@@ -1956,6 +1974,142 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \treturn dirty_submodule;\n }\n \n+static struct status_task *\n+get_status_task_from_index(struct submodule_parallel_status *sps,\n+\t\t\t struct strbuf *err)\n+{\n+\tfor (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {\n+\t\tstruct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;\n+\t\tconst struct cache_entry *ce = util->ce;\n+\t\tstruct status_task *task;\n+\t\tstruct status_task tmp = {\n+\t\t\t.path = ce->name,\n+\t\t\t.dirty_submodule = util->dirty_submodule,\n+\t\t\t.ignore_untracked = util->ignore_untracked,\n+\t\t};\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tconst char *git_dir;\n+\n+\t\tstrbuf_addf(&buf, \"%s/.git\", ce->name);\n+\t\tgit_dir = read_gitfile(buf.buf);\n+\t\tif (!git_dir)\n+\t\t\tgit_dir = buf.buf;\n+\t\tif (!is_git_directory(git_dir)) {\n+\t\t\tif (is_directory(git_dir))\n+\t\t\t\tdie(_(\"'%s' not recognized as a git repository\"), git_dir);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\t/* The submodule is not checked out, so it is not modified */\n+\t\t\tutil->dirty_submodule = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_release(&buf);\n+\n+\t\ttask = xmalloc(sizeof(*task));\n+\t\tmemcpy(task, &tmp, sizeof(*task));\n+\t\tsps->index_count++;\n+\t\treturn task;\n+\t}\n+\treturn NULL;\n+}\n+\n+\n+static int get_next_submodule_status(struct child_process *cp,\n+\t\t\t\t struct strbuf *err, void *data,\n+\t\t\t\t void **task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = data;\n+\tstruct status_task *task = get_status_task_from_index(sps, err);\n+\n+\tif (!task)\n+\t\treturn 0;\n+\n+\tchild_process_init(cp);\n+\tprepare_submodule_repo_env_in_gitdir(&cp->env);\n+\n+\tstrvec_init(&cp->args);\n+\tstrvec_pushl(&cp->args, \"status\", \"--porcelain=2\", NULL);\n+\tif (task->ignore_untracked)\n+\t\tstrvec_push(&cp->args, \"-uno\");\n+\n+\tprepare_submodule_repo_env(&cp->env);\n+\tcp->git_cmd = 1;\n+\tcp->dir = task->path;\n+\t*task_cb = task;\n+\treturn 1;\n+}\n+\n+static int status_start_failure(struct strbuf *err,\n+\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = cb;\n+\n+\tsps->result = 1;\n+\treturn 0;\n+}\n+\n+static void status_duplicate_output(struct strbuf *process_out,\n+\t\t\t\t struct strbuf *out,\n+\t\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct status_task *task = task_cb;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n+\n+\tstring_list_split(&list, process_out->buf, '\\n', -1);\n+\n+\tfor_each_string_list_item(item, &list) {\n+\t\tif (parse_status_porcelain(item->string,\n+\t\t\t\t strlen(item->string),\n+\t\t\t\t &task->dirty_submodule,\n+\t\t\t\t task->ignore_untracked))\n+\t\t\tbreak;\n+\t}\n+\tstring_list_clear(&list, 0);\n+\tstrbuf_reset(out);\n+}\n+\n+static int status_finish(int retvalue, struct strbuf *err,\n+\t\t\t void *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_status *sps = cb;\n+\tstruct status_task *task = task_cb;\n+\tstruct string_list_item *it =\n+\t\tstring_list_lookup(sps->submodule_names, task->path);\n+\tstruct submodule_status_util *util = it->util;\n+\n+\tutil->dirty_submodule = task->dirty_submodule;\n+\tfree(task);\n+\n+\treturn 0;\n+}\n+\n+int get_submodules_status(struct repository *r,\n+\t\t\t struct string_list *submodules,\n+\t\t\t int max_parallel_jobs)\n+{\n+\tstruct submodule_parallel_status sps = {\n+ .r = r,\n+ .submodule_names = submodules,\n+ };\n+\tconst struct run_process_parallel_opts opts = {\n+\t\t.tr2_category = \"submodule\",\n+\t\t.tr2_label = \"parallel/status\",\n+\n+\t\t.processes = max_parallel_jobs,\n+\n+\t\t.get_next_task = get_next_submodule_status,\n+\t\t.start_failure = status_start_failure,\n+\t\t.duplicate_output = status_duplicate_output,\n+\t\t.task_finished = status_finish,\n+\t\t.data = &sps,\n+\t};\n+\n+\tstring_list_sort(sps.submodule_names);\n+\trun_processes_parallel(&opts);\n+\n+\treturn sps.result;\n+}\n+\n int submodule_uses_gitfile(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\ndiff --git a/submodule.h b/submodule.h\nindex 6a9fec6de1..cbb7231a5d 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,6 +41,12 @@ struct submodule_update_strategy {\n \t.type = SM_UPDATE_UNSPECIFIED, \\\n }\n \n+struct submodule_status_util {\n+\tint changed, ignore_untracked;\n+\tunsigned dirty_submodule, newmode;\n+\tstruct cache_entry *ce;\n+};\n+\n int is_gitmodules_unmerged(struct index_state *istate);\n int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n@@ -94,6 +100,9 @@ int fetch_submodules(struct repository *r,\n \t\t int command_line_option,\n \t\t int default_option,\n \t\t int quiet, int max_parallel_jobs);\n+int get_submodules_status(struct repository *r,\n+\t\t\t struct string_list *submodules,\n+\t\t\t int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n \ndiff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh\nindex 40164ae07d..e08ee315a7 100755\n--- a/t/t4027-diff-submodule.sh\n+++ b/t/t4027-diff-submodule.sh\n@@ -34,6 +34,25 @@ test_expect_success setup '\n \tsubtip=$3 subprev=$2\n '\n \n+test_expect_success 'diff in superproject with submodules respects parallel settings' '\n+\ttest_when_finished \"rm -f trace.out\" &&\n+\t(\n+\t\tGIT_TRACE=$(pwd)/trace.out git diff &&\n+\t\tgrep \"1 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tgit config submodule.diffJobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git diff &&\n+\t\tgrep \"8 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tGIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&\n+\t\tgrep \"preparing to run up to [0-9]* tasks\" trace.out &&\n+\t\t! grep \"up to 0 tasks\" trace.out &&\n+\t\t>trace.out\n+\t)\n+'\n+\n test_expect_success 'git diff --raw HEAD' '\n \thexsz=$(test_oid hexsz) &&\n \tgit diff --raw --abbrev=$hexsz HEAD >actual &&\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex d050091345..52a82b703f 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -412,4 +412,23 @@ test_expect_success 'status with added file in nested submodule (short)' '\n \tEOF\n '\n \n+test_expect_success 'status in superproject with submodules respects parallel settings' '\n+\ttest_when_finished \"rm -f trace.out\" &&\n+\t(\n+\t\tGIT_TRACE=$(pwd)/trace.out git status &&\n+\t\tgrep \"1 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tgit config submodule.diffJobs 8 &&\n+\t\tGIT_TRACE=$(pwd)/trace.out git status &&\n+\t\tgrep \"8 tasks\" trace.out &&\n+\t\t>trace.out &&\n+\n+\t\tGIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&\n+\t\tgrep \"preparing to run up to [0-9]* tasks\" trace.out &&\n+\t\t! grep \"up to 0 tasks\" trace.out &&\n+\t\t>trace.out\n+\t)\n+'\n+\n test_done\n-- \n2.38.1.431.g37b22c650d-goog\n\n"},{"id":"467874","messageId":"kl6l35a9ef3c.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58768","inReplyTo":"20221108184200.2813458-1-calvinwan@google.com","subject":"Re: [PATCH v4 0/5] submodule: parallelize diff","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-23T17:49:11Z","receivedAt":"2022-11-23T17:49:18Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Calvin Wan writes:\n\n> Original cover letter for context:\n> https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/\n>\n> Changes since v3\n>\n> Renamed pipe_output_fn to duplicate_output_fn and now passes an\n> additional \"strbuf* out\" parameter. Output is directly duplicated\n> to that function rather than held in a separate variable.\n> Slightly rewrote the tests to more accurately capture the expected\n> output of duplicate_output_fn.\n>\n> Removed a patch that added an option to hide child process output.\n> Child process output is now reset in status_duplicate_output.\n>\n> More style changes as suggested by Avar\n\nFor ease of navigation, here are the previous versions:\n\nv3: https://lore.kernel.org/git/20221020232532.1128326-1-calvinwan@google.com/\nv2: https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/\nv1: https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/\n"},{"id":"468125","messageId":"20221128204538.2748977-1-jonathantanmy@google.com","threadId":"58768","inReplyTo":"20221108184200.2813458-2-calvinwan@google.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-28T20:45:38Z","receivedAt":"2022-11-28T20:45:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Calvin Wan writes:\n\n> +\tif (opts->duplicate_output && opts->ungroup)\n> +\t\tBUG(\"duplicate_output and ungroup are incompatible with each other\");\n\nThanks for spotting \"ungroup\" - that helps me to focus my review.\n\n> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n> \tfor (size_t i = 0; i < opts->processes; i++) {\n> \t\tif (pp->children[i].state == GIT_CP_WORKING &&\n> \t\t pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n> -\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n> -\t\t\t\t\t\t pp->children[i].process.err, 0);\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\t\tint n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n> +\t\t\tstrbuf_addbuf(&pp->children[i].err, &buf);\n> +\t\t\tif (opts->duplicate_output)\n> +\t\t\t\topts->duplicate_output(&buf, &pp->children[i].err,\n> +\t\t\t\t\t opts->data,\n> +\t\t\t\t\t pp->children[i].data);\n> +\t\t\tstrbuf_release(&buf);\n\n[snip]\n\n> Add duplicate_output_fn as an optionally set function in\n> run_process_parallel_opts. If set, output from each child process is\n> copied and passed to the callback function whenever output from the\n> child process is buffered to allow for separate parsing.\n\nLooking at this patch, since this new option is incompatible with \"ungroup\",\nI would have expected that the new functionality be in a place that already\ncontains an \"if (ungroup)\", and thus would go into the \"else\" block. Looking at\nthe code, it seems like a reasonable place would be in pp_collect_finished().\n\nIs the reason this is not there because we only want the output of the child\nprocess, not anything that the callback functions might write to the out\nstrbuf? If yes, is there a reason for that? If not, I think the code would\nbe simpler if we did what I suggested. (Maybe this has already been discussed\npreviously - if that is the case, the reason for doing it this way should be in\nthe commit message.)\n\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index 3ecb830f4a..40dd329e02 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,\n> \treturn 0;\n> }\n> \n> +static void duplicate_output(struct strbuf *process_out,\n> +\t\t\tstruct strbuf *out,\n> +\t\t\tvoid *pp_cb,\n> +\t\t\tvoid *pp_task_cb)\n> +{\n> +\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\n> +\tstring_list_split(&list, process_out->buf, '\\n', -1);\n> +\tfor (size_t i = 0; i < list.nr; i++) {\n> +\t\tif (strlen(list.items[i].string) > 0)\n> +\t\t\tfprintf(stderr, \"duplicate_output: %s\\n\", list.items[i].string);\n> +\t}\n> +\tstring_list_clear(&list, 0);\n> +}\n> +\n> static int task_finished(int result,\n> \t\t\t struct strbuf *err,\n> \t\t\t void *pp_cb,\n> @@ -439,6 +454,12 @@ int cmd__run_command(int argc, const char **argv)\n> \t\topts.ungroup = 1;\n> \t}\n> \n> +\tif (!strcmp(argv[1], \"--duplicate-output\")) {\n> +\t\targv += 1;\n> +\t\targc -= 1;\n> +\t\topts.duplicate_output = duplicate_output;\n> +\t}\n\nIn the tests, can we also write things from the callback functions? Whether we think that callback output should be\nduplicated or not, we should test what happens to them.\n"},{"id":"468126","messageId":"20221128210125.2751300-1-jonathantanmy@google.com","threadId":"58768","inReplyTo":"20221108184200.2813458-6-calvinwan@google.com","subject":"Re: [PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-28T21:01:25Z","receivedAt":"2022-11-28T21:01:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Calvin Wan writes:\n> submodule.c | 154 +++++++++++++++++++++++++++++\n\nI think the way to implement this is to have a parallel implementation,\nand then have the serial implementation call the parallel implementation's\nfunctions, or have a common set of functions that both the parallel\nimplementation and the serial implementation call. Here, it seems that\nthe parallel implementation exists completely separate from the serial\nimplementation, with no code shared. That makes it both more difficult to\nreview, and also makes it difficult to make changes to how we diff submodules\nin the future (since we would have to make changes in two parts of the code).\n\nI think that the layout of the code will be substantially different if we do\nthat, so I'll hold off on a more thorough review for now.\n"},{"id":"468157","messageId":"CABPp-BE7jNcNvF50rqJdSBr2ptwsicUwqtcjZcAV=YDKjNB-vA@mail.gmail.com","threadId":"58768","inReplyTo":"20221108184200.2813458-2-calvinwan@google.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-29T05:11:07Z","receivedAt":"2022-11-29T05:11:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 8, 2022 at 11:11 AM Calvin Wan wrote:\n>\n> Add duplicate_output_fn as an optionally set function in\n> run_process_parallel_opts. If set, output from each child process is\n> copied and passed to the callback function whenever output from the\n> child process is buffered to allow for separate parsing.\n>\n> Signed-off-by: Calvin Wan \n[...]\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index 3ecb830f4a..40dd329e02 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,\n> return 0;\n> }\n>\n> +static void duplicate_output(struct strbuf *process_out,\n> + struct strbuf *out,\n> + void *pp_cb,\n> + void *pp_task_cb)\n\nShould the last two parameters have an \"UNUSED\" annotation?\n"},{"id":"468158","messageId":"CABPp-BH3hxXhAGeHJ56m=S+GknsVqYExdJ7eKs3bJYkiMaWukQ@mail.gmail.com","threadId":"58768","inReplyTo":"20221108184200.2813458-6-calvinwan@google.com","subject":"Re: [PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-29T05:13:52Z","receivedAt":"2022-11-29T05:14:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 8, 2022 at 11:32 AM Calvin Wan wrote:\n>\n> During the iteration of the index entries in run_diff_files, whenever\n> a submodule is found and needs its status checked, a subprocess is\n> spawned for it. Instead of spawning the subprocess immediately and\n> waiting for its completion to continue, hold onto all submodules and\n> relevant information in a list. Then use that list to create tasks for\n> run_processes_parallel. Subprocess output is duplicated and passed to\n> status_pipe_output which parses it.\n>\n> Add config option submodule.diffJobs to set the maximum number\n> of parallel jobs. The option defaults to 1 if unset. If set to 0, the\n> number of jobs is set to online_cpus().\n>\n> Since run_diff_files is called from many different commands, I chose\n> to grab the config option in the function rather than adding variables\n> to every git command and then figuring out how to pass them all in.\n>\n> Signed-off-by: Calvin Wan \n> ---\n> Documentation/config/submodule.txt | 12 +++\n> diff-lib.c | 80 +++++++++++++--\n> submodule.c | 154 +++++++++++++++++++++++++++++\n> submodule.h | 9 ++\n> t/t4027-diff-submodule.sh | 19 ++++\n> t/t7506-status-submodule.sh | 19 ++++\n> 6 files changed, 287 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> index 6490527b45..1144a5ad74 100644\n> --- a/Documentation/config/submodule.txt\n> +++ b/Documentation/config/submodule.txt\n> @@ -93,6 +93,18 @@ submodule.fetchJobs::\n> in parallel. A value of 0 will give some reasonable default.\n> If unset, it defaults to 1.\n>\n> +submodule.diffJobs::\n> + Specifies how many submodules are diffed at the same time. A\n> + positive integer allows up to that number of submodules diffed\n> + in parallel. A value of 0 will give the number of logical cores.\n\nWhy hardcode that 0 gives the number of logical cores? Why not just\nstate that a value of 0 \"gives a guess at optimal parallelism\",\nallowing us to adjust it in the future if we can do some smart\nheuristics? It'd be nice to not have us tied down and prevented from\ntaking a smarter approach.\n\n> + If unset, it defaults to 1. The diff operation is used by many\n> + other git commands such as add, merge, diff, status, stash and\n> + more. Note that the expensive part of the diff operation is\n> + reading the index from cache or memory. Therefore multiple jobs\n> + may be detrimental to performance if your hardware does not\n> + support parallel reads or if the number of jobs greatly exceeds\n> + the amount of supported reads.\n\nSo, in the future, someone who wants to speed things up is going to\nneed to configure submodule.diffJobs, submodule.fetchJobs,\nsubmodule.checkoutJobs, submodule.grepJobs, submodule.mergeJobs, etc.?\n I worry that we're headed towards a bit of a suboptimal user\nexperience here. It'd be nice to have a more central configuration of\n\"yes, I want parallelism; please don't make me benchmark things in\norder to take advantage of it\", if that's possible. It may just be\nthat the \"optimal\" parallelism varies significantly between commands,\nand also varies a lot based on hardware, repository sizes, background\nload on the system, etc. such that we can't provide a reasonable\nsuggestion for those that want a value greater than 1. Or maybe in\nthe future we allow folks somehow to request our best guess at a good\nparallelization level and then let users override with these\nindividual flags. I'm just a little worried we might be making users\ndo work that we should somehow figure out.\n"},{"id":"468211","messageId":"kl6lr0xlqtrc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58768","inReplyTo":"20221128210125.2751300-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-29T22:29:43Z","receivedAt":"2022-11-29T22:30:17Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Jonathan Tan writes:\n\n> Calvin Wan writes:\n>> submodule.c | 154 +++++++++++++++++++++++++++++\n>\n> I think the way to implement this is to have a parallel implementation,\n> and then have the serial implementation call the parallel implementation's\n> functions, or have a common set of functions that both the parallel\n> implementation and the serial implementation call. Here, it seems that\n> the parallel implementation exists completely separate from the serial\n> implementation, with no code shared. That makes it both more difficult to\n> review, and also makes it difficult to make changes to how we diff submodules\n> in the future (since we would have to make changes in two parts of the code).\n\nIt seems that most of the code is copied from is_submodule_modified(),\nso a possible way to do this would be:\n\n- Split is_submodule_modified() into 2 functions, one that sets up the\n \"git status --porcelain=2\" process (named something like\n setup_status_porcelain()) and one that parses its output (this is\n parse_status_porcelain() in Patch 2). The serial implementation\n (is_submodule_modified()) uses both of these and has some extra logic\n to run the child process.\n\n- Refactor get_next_submodule_status() (from this patch) to use\n setup_status_porcelain().\n"},{"id":"468214","messageId":"kl6lo7spqqzg.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58768","inReplyTo":"20221108184200.2813458-2-calvinwan@google.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-29T23:29:39Z","receivedAt":"2022-11-29T23:30:13Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Calvin Wan writes:\n\n> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n> \tfor (size_t i = 0; i < opts->processes; i++) {\n> \t\tif (pp->children[i].state == GIT_CP_WORKING &&\n> \t\t pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n> -\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n> -\t\t\t\t\t\t pp->children[i].process.err, 0);\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\t\tint n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n> +\t\t\tstrbuf_addbuf(&pp->children[i].err, &buf);\n> +\t\t\tif (opts->duplicate_output)\n> +\t\t\t\topts->duplicate_output(&buf, &pp->children[i].err,\n> +\t\t\t\t\t opts->data,\n> +\t\t\t\t\t pp->children[i].data);\n> +\t\t\tstrbuf_release(&buf);\n> \t\t\tif (n == 0) {\n> \t\t\t\tclose(pp->children[i].process.err);\n> \t\t\t\tpp->children[i].state = GIT_CP_WAIT_CLEANUP;\n\nA common pattern is that optional behavior does not impose additional\ncosts on the non-optional part. Here, we unconditionally do a\nstrbuf_addbuf() even though we don't use the result in the \"else\" case.\n\nSo this might be more idiomatically written as:\n\n int n = strbuf_read_once(&pp->children[i].err,\n \t\t\t pp->children[i].process.err, 0);\n + if (opts->duplicate_output) {\n + struct strbuf buf = STRBUF_INIT;\n + strbuf_addbuf(&buf, &pp->children[i].err);\n + \topts->duplicate_output(&buf, &pp->children[i].err,\n + \t\t opts->data,\n + \t\t pp->children[i].data);\n + strbuf_release(&buf);\n + }\n\nwhich also has the nice benefit of not touching the strbuf_read_once()\nline.\n"},{"id":"468237","messageId":"221130.86lensiwy0.gmgdl@evledraar.gmail.com","threadId":"58768","inReplyTo":"kl6lo7spqqzg.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-30T09:53:10Z","receivedAt":"2022-11-30T10:00:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 29 2022, Glen Choo wrote:\n\n> Calvin Wan writes:\n>\n>> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n>> \tfor (size_t i = 0; i < opts->processes; i++) {\n>> \t\tif (pp->children[i].state == GIT_CP_WORKING &&\n>> \t\t pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n>> -\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n>> -\t\t\t\t\t\t pp->children[i].process.err, 0);\n>> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n>> +\t\t\tint n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n>> +\t\t\tstrbuf_addbuf(&pp->children[i].err, &buf);\n>> +\t\t\tif (opts->duplicate_output)\n>> +\t\t\t\topts->duplicate_output(&buf, &pp->children[i].err,\n>> +\t\t\t\t\t opts->data,\n>> +\t\t\t\t\t pp->children[i].data);\n>> +\t\t\tstrbuf_release(&buf);\n>> \t\t\tif (n == 0) {\n>> \t\t\t\tclose(pp->children[i].process.err);\n>> \t\t\t\tpp->children[i].state = GIT_CP_WAIT_CLEANUP;\n>\n> A common pattern is that optional behavior does not impose additional\n> costs on the non-optional part. Here, we unconditionally do a\n> strbuf_addbuf() even though we don't use the result in the \"else\" case.\n>\n> So this might be more idiomatically written as:\n>\n> int n = strbuf_read_once(&pp->children[i].err,\n> \t\t\t pp->children[i].process.err, 0);\n> + if (opts->duplicate_output) {\n> + struct strbuf buf = STRBUF_INIT;\n> + strbuf_addbuf(&buf, &pp->children[i].err);\n> + \topts->duplicate_output(&buf, &pp->children[i].err,\n> + \t\t opts->data,\n> + \t\t pp->children[i].data);\n> + strbuf_release(&buf);\n> + }\n>\n> which also has the nice benefit of not touching the strbuf_read_once()\n> line.\n\nWe should also use \"size_t n\" there, not \"int n\", which is what it\nreturns. It won't matter for overflow in this case, but let's not\ntruncate for no good reason, it makes spotting actual overflows in\ncompiler output harder.\n\nAnd why does \"&buf\" exist at all? Why can't we just pass\n&pp->children[i].err, and if this callback cares about the last thing we\nread let's pass it an offset, so it can know what came from the\nstrbuf_read_once() (I don't know if it actually cares about that\neither...).\n\nThat would avoid the copy entirely.\n"},{"id":"468239","messageId":"778b25b4-8eb2-6d02-3ba5-a21724ff2f2f@dunelm.org.uk","threadId":"58768","inReplyTo":"221130.86lensiwy0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-30T10:26:10Z","receivedAt":"2022-11-30T10:26:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Nov 29 2022, Glen Choo wrote:\n> \n>> Calvin Wan writes:\n>>\n>>> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n>>> \tfor (size_t i = 0; i < opts->processes; i++) {\n>>> \t\tif (pp->children[i].state == GIT_CP_WORKING &&\n>>> \t\t pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n>>> -\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n>>> -\t\t\t\t\t\t pp->children[i].process.err, 0);\n>>> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n>>> +\t\t\tint n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n>>> +\t\t\tstrbuf_addbuf(&pp->children[i].err, &buf);\n>>> +\t\t\tif (opts->duplicate_output)\n\nShouldn't we be checking if n < 0 before we do this?\n\n>>> +\t\t\t\topts->duplicate_output(&buf, &pp->children[i].err,\n>>> +\t\t\t\t\t opts->data,\n>>> +\t\t\t\t\t pp->children[i].data);\n>>> +\t\t\tstrbuf_release(&buf);\n>>> \t\t\tif (n == 0) {\n>>> \t\t\t\tclose(pp->children[i].process.err);\n>>> \t\t\t\tpp->children[i].state = GIT_CP_WAIT_CLEANUP;\n>>\n>> A common pattern is that optional behavior does not impose additional\n>> costs on the non-optional part. Here, we unconditionally do a\n>> strbuf_addbuf() even though we don't use the result in the \"else\" case.\n>>\n>> So this might be more idiomatically written as:\n>>\n>> int n = strbuf_read_once(&pp->children[i].err,\n>> \t\t\t pp->children[i].process.err, 0);\n>> + if (opts->duplicate_output) {\n>> + struct strbuf buf = STRBUF_INIT;\n>> + strbuf_addbuf(&buf, &pp->children[i].err);\n>> + \topts->duplicate_output(&buf, &pp->children[i].err,\n>> + \t\t opts->data,\n>> + \t\t pp->children[i].data);\n>> + strbuf_release(&buf);\n>> + }\n>>\n> [...]\n> And why does \"&buf\" exist at all? \n\nI was wondering that as too\n\n>Why can't we just pass\n> &pp->children[i].err, and if this callback cares about the last thing we\n> read let's pass it an offset, so it can know what came from the\n> strbuf_read_once() (I don't know if it actually cares about that\n> either...).\n\nOr we could just pass a `const char*`, `size_t` pair.\n\n> That would avoid the copy entirely.\n\nIs the copying quadratic at the moment? - it looks like each call to \nstrbuf_read_once() appends to the buffer and we copy the whole buffer \neach time.\n\nBest Wishes\n\nPhillip\n"},{"id":"468240","messageId":"5f6f0181-c251-0a98-a39f-a910b93f8c1a@dunelm.org.uk","threadId":"58768","inReplyTo":"221130.86lensiwy0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-30T10:28:17Z","receivedAt":"2022-11-30T10:28:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Nov 29 2022, Glen Choo wrote:\n> \n>> Calvin Wan writes:\n>> So this might be more idiomatically written as:\n>>\n>> int n = strbuf_read_once(&pp->children[i].err,\n>> \t\t\t pp->children[i].process.err, 0);\n>> + if (opts->duplicate_output) {\n>> + struct strbuf buf = STRBUF_INIT;\n>> + strbuf_addbuf(&buf, &pp->children[i].err);\n>> + \topts->duplicate_output(&buf, &pp->children[i].err,\n>> + \t\t opts->data,\n>> + \t\t pp->children[i].data);\n>> + strbuf_release(&buf);\n>> + }\n>>\n>> which also has the nice benefit of not touching the strbuf_read_once()\n>> line.\n> \n> We should also use \"size_t n\" there, not \"int n\", which is what it\n> returns.\n\nIt returns an ssize_t not size_t, lower down we test `n < 0` so we \ncertainly should not be using an unsigned type.\n\nBest Wishes\n\nPhillip\n"},{"id":"468242","messageId":"221130.86cz94iu9a.gmgdl@evledraar.gmail.com","threadId":"58768","inReplyTo":"5f6f0181-c251-0a98-a39f-a910b93f8c1a@dunelm.org.uk","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-30T10:57:38Z","receivedAt":"2022-11-30T10:58:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 30 2022, Phillip Wood wrote:\n\n> On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:\n>> On Tue, Nov 29 2022, Glen Choo wrote:\n>> \n>>> Calvin Wan writes:\n>>> So this might be more idiomatically written as:\n>>>\n>>> int n = strbuf_read_once(&pp->children[i].err,\n>>> \t\t\t pp->children[i].process.err, 0);\n>>> + if (opts->duplicate_output) {\n>>> + struct strbuf buf = STRBUF_INIT;\n>>> + strbuf_addbuf(&buf, &pp->children[i].err);\n>>> + \topts->duplicate_output(&buf, &pp->children[i].err,\n>>> + \t\t opts->data,\n>>> + \t\t pp->children[i].data);\n>>> + strbuf_release(&buf);\n>>> + }\n>>>\n>>> which also has the nice benefit of not touching the strbuf_read_once()\n>>> line.\n>> We should also use \"size_t n\" there, not \"int n\", which is what it\n>> returns.\n>\n> It returns an ssize_t not size_t, lower down we test `n < 0` so we\n> certainly should not be using an unsigned type.\n\nGood catch, I just skimmed it and missed the extra \"s\". In any case:\nlet's use the type it's returning, so ssize_t, not int.\n\n(DEVOPTS=extra-all etc. will also catch this, depending on your compiler\netc.)\n"},{"id":"468251","messageId":"19f91fea-a2a9-7dc6-d940-cc10f384fe76@dunelm.org.uk","threadId":"58768","inReplyTo":"20221108184200.2813458-5-calvinwan@google.com","subject":"Re: [PATCH v4 4/5] diff-lib: refactor match_stat_with_submodule","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-30T14:36:20Z","receivedAt":"2022-11-30T14:36:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Calven\n\nOn 08/11/2022 18:41, Calvin Wan wrote:\n> Flatten out the if statements in match_stat_with_submodule so the\n> logic is more readable and easier for future patches to add to.\n> orig_flags didn't need to be set if the cache entry wasn't a\n> GITLINK so defer setting it.\n\nThanks for splitting this change out. I have to say I find the original \nquite a bit easier to read. If you're worried about the code added in \nthe next commit being too indented perhaps you could move the body of \nthe if statement into a separate function.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Calvin Wan \n> ---\n> diff-lib.c | 29 ++++++++++++++++++-----------\n> 1 file changed, 18 insertions(+), 11 deletions(-)\n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2edea41a23..f5257c0c71 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -73,18 +73,25 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> \t\t\t\t unsigned *dirty_submodule)\n> {\n> \tint changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);\n> -\tif (S_ISGITLINK(ce->ce_mode)) {\n> -\t\tstruct diff_flags orig_flags = diffopt->flags;\n> -\t\tif (!diffopt->flags.override_submodule_config)\n> -\t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n> -\t\tif (diffopt->flags.ignore_submodules)\n> -\t\t\tchanged = 0;\n> -\t\telse if (!diffopt->flags.ignore_dirty_submodules &&\n> -\t\t\t (!changed || diffopt->flags.dirty_submodules))\n> -\t\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> -\t\t\t\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n> -\t\tdiffopt->flags = orig_flags;\n> +\tstruct diff_flags orig_flags;\n> +\n> +\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\tgoto ret;\n> +\n> +\torig_flags = diffopt->flags;\n> +\tif (!diffopt->flags.override_submodule_config)\n> +\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n> +\tif (diffopt->flags.ignore_submodules) {\n> +\t\tchanged = 0;\n> +\t\tgoto cleanup;\n> \t}\n> +\tif (!diffopt->flags.ignore_dirty_submodules &&\n> +\t (!changed || diffopt->flags.dirty_submodules))\n> +\t\t*dirty_submodule = is_submodule_modified(ce->name,\n> +\t\t\t\t\t diffopt->flags.ignore_untracked_in_submodules);\n> +cleanup:\n> +\tdiffopt->flags = orig_flags;\n> +ret:\n> \treturn changed;\n> }\n> \n"},{"id":"468262","messageId":"CAFySSZBJ1tQk0n+GEW-6MBDafGmTy75QS41oriwGFKiq8McR=Q@mail.gmail.com","threadId":"58768","inReplyTo":"CABPp-BH3hxXhAGeHJ56m=S+GknsVqYExdJ7eKs3bJYkiMaWukQ@mail.gmail.com","subject":"Re: [PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T18:04:00Z","receivedAt":"2022-11-30T18:04:16Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> > index 6490527b45..1144a5ad74 100644\n> > --- a/Documentation/config/submodule.txt\n> > +++ b/Documentation/config/submodule.txt\n> > @@ -93,6 +93,18 @@ submodule.fetchJobs::\n> > in parallel. A value of 0 will give some reasonable default.\n> > If unset, it defaults to 1.\n> >\n> > +submodule.diffJobs::\n> > + Specifies how many submodules are diffed at the same time. A\n> > + positive integer allows up to that number of submodules diffed\n> > + in parallel. A value of 0 will give the number of logical cores.\n>\n> Why hardcode that 0 gives the number of logical cores? Why not just\n> state that a value of 0 \"gives a guess at optimal parallelism\",\n> allowing us to adjust it in the future if we can do some smart\n> heuristics? It'd be nice to not have us tied down and prevented from\n> taking a smarter approach.\n\nI was unaware that the original intention of \"reasonable default\" was for\nflexibility (I have a WIP series standardizing these parallelism config\noptions that also used \"number of logical cores\" but I think that should\nprobably change now). There are other parallel config options that\nhardcode 0 as well, so my initial thought was that we should be using\nthe more precise wording -- the argument for flexibility now seems\nmore preferable, however.\n\n>\n> > + If unset, it defaults to 1. The diff operation is used by many\n> > + other git commands such as add, merge, diff, status, stash and\n> > + more. Note that the expensive part of the diff operation is\n> > + reading the index from cache or memory. Therefore multiple jobs\n> > + may be detrimental to performance if your hardware does not\n> > + support parallel reads or if the number of jobs greatly exceeds\n> > + the amount of supported reads.\n>\n> So, in the future, someone who wants to speed things up is going to\n> need to configure submodule.diffJobs, submodule.fetchJobs,\n> submodule.checkoutJobs, submodule.grepJobs, submodule.mergeJobs, etc.?\n> I worry that we're headed towards a bit of a suboptimal user\n> experience here. It'd be nice to have a more central configuration of\n> \"yes, I want parallelism; please don't make me benchmark things in\n> order to take advantage of it\", if that's possible. It may just be\n> that the \"optimal\" parallelism varies significantly between commands,\n> and also varies a lot based on hardware, repository sizes, background\n> load on the system, etc. such that we can't provide a reasonable\n> suggestion for those that want a value greater than 1. Or maybe in\n> the future we allow folks somehow to request our best guess at a good\n> parallelization level and then let users override with these\n> individual flags. I'm just a little worried we might be making users\n> do work that we should somehow figure out.\n\nI had the same worry as well -- see the discussion I had here:\nhttps://lore.kernel.org/git/CAFySSZAbsPuyPVX0+DQzArny2CEWs+GpQqJ3AOxUB_ffo8B3SQ@mail.gmail.com/\nI would like to also eventually solve this problem, but this patch\nwon't be the one to do so.\n"},{"id":"468266","messageId":"CAFySSZD8NjOHpM6PBCVxBzB-m2WuBCTppZWB0waP9svo=O1C0A@mail.gmail.com","threadId":"58768","inReplyTo":"kl6lr0xlqtrc.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 5/5] diff-lib: parallelize run_diff_files for submodules","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T18:11:30Z","receivedAt":"2022-11-30T18:11:45Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Tue, Nov 29, 2022 at 2:29 PM Glen Choo wrote:\n>\n> Jonathan Tan writes:\n>\n> > Calvin Wan writes:\n> >> submodule.c | 154 +++++++++++++++++++++++++++++\n> >\n> > I think the way to implement this is to have a parallel implementation,\n> > and then have the serial implementation call the parallel implementation's\n> > functions, or have a common set of functions that both the parallel\n> > implementation and the serial implementation call. Here, it seems that\n> > the parallel implementation exists completely separate from the serial\n> > implementation, with no code shared. That makes it both more difficult to\n> > review, and also makes it difficult to make changes to how we diff submodules\n> > in the future (since we would have to make changes in two parts of the code).\n>\n> It seems that most of the code is copied from is_submodule_modified(),\n> so a possible way to do this would be:\n>\n> - Split is_submodule_modified() into 2 functions, one that sets up the\n> \"git status --porcelain=2\" process (named something like\n> setup_status_porcelain()) and one that parses its output (this is\n> parse_status_porcelain() in Patch 2). The serial implementation\n> (is_submodule_modified()) uses both of these and has some extra logic\n> to run the child process.\n>\n> - Refactor get_next_submodule_status() (from this patch) to use\n> setup_status_porcelain().\n\nThat sounds like a reasonable plan to me. Thanks!\n"},{"id":"468272","messageId":"CAFySSZB6XBzRedrp5+9r+xB9XmXR7Q09j+kkD-8Ym6B7bu69sQ@mail.gmail.com","threadId":"58768","inReplyTo":"20221128204538.2748977-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T18:46:04Z","receivedAt":"2022-11-30T18:46:41Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Mon, Nov 28, 2022 at 12:45 PM Jonathan Tan wrote:\n\n> Looking at this patch, since this new option is incompatible with \"ungroup\",\n> I would have expected that the new functionality be in a place that already\n> contains an \"if (ungroup)\", and thus would go into the \"else\" block. Looking at\n> the code, it seems like a reasonable place would be in pp_collect_finished().\n\nThe code lives in pp_buffer_stderr(), which if you go one level higher, you'll\nnotice that the call to pp_buffer_stderr() is in the \"else\" block of an\n\"if (ungroup)\".\n\n> Is the reason this is not there because we only want the output of the child\n> process, not anything that the callback functions might write to the out\n> strbuf? If yes, is there a reason for that? If not, I think the code would\n> be simpler if we did what I suggested. (Maybe this has already been discussed\n> previously - if that is the case, the reason for doing it this way should be in\n> the commit message.)\n\nYes, inside of pp_output(), you'll see that if the process is the output_owner,\nthen \"pp->children[i].err\" is printed and reset, which is why the code\nlives before\npp_output(). The caller already has access to the callback functions and knows\nwhat will be written to the out strbuf -- the goal of this patch is to\nprovide access\nto all of the child output.\n\n>\n> > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> > index 3ecb830f4a..40dd329e02 100644\n> > --- a/t/helper/test-run-command.c\n> > +++ b/t/helper/test-run-command.c\n> > @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,\n> > return 0;\n> > }\n> >\n> > +static void duplicate_output(struct strbuf *process_out,\n> > + struct strbuf *out,\n> > + void *pp_cb,\n> > + void *pp_task_cb)\n> > +{\n> > + struct string_list list = STRING_LIST_INIT_DUP;\n> > +\n> > + string_list_split(&list, process_out->buf, '\\n', -1);\n> > + for (size_t i = 0; i < list.nr; i++) {\n> > + if (strlen(list.items[i].string) > 0)\n> > + fprintf(stderr, \"duplicate_output: %s\\n\", list.items[i].string);\n> > + }\n> > + string_list_clear(&list, 0);\n> > +}\n> > +\n> > static int task_finished(int result,\n> > struct strbuf *err,\n> > void *pp_cb,\n> > @@ -439,6 +454,12 @@ int cmd__run_command(int argc, const char **argv)\n> > opts.ungroup = 1;\n> > }\n> >\n> > + if (!strcmp(argv[1], \"--duplicate-output\")) {\n> > + argv += 1;\n> > + argc -= 1;\n> > + opts.duplicate_output = duplicate_output;\n> > + }\n>\n> In the tests, can we also write things from the callback functions? Whether we think that callback output should be\n> duplicated or not, we should test what happens to them.\n\nack.\n"},{"id":"468273","messageId":"CAFySSZDhA0kfVACcni7Gb60wNeNXm8Eg0VpC+_14Qt_HX=KFzA@mail.gmail.com","threadId":"58768","inReplyTo":"CABPp-BE7jNcNvF50rqJdSBr2ptwsicUwqtcjZcAV=YDKjNB-vA@mail.gmail.com","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T18:47:48Z","receivedAt":"2022-11-30T18:48:10Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Mon, Nov 28, 2022 at 9:11 PM Elijah Newren wrote:\n>\n> On Tue, Nov 8, 2022 at 11:11 AM Calvin Wan wrote:\n> >\n> > Add duplicate_output_fn as an optionally set function in\n> > run_process_parallel_opts. If set, output from each child process is\n> > copied and passed to the callback function whenever output from the\n> > child process is buffered to allow for separate parsing.\n> >\n> > Signed-off-by: Calvin Wan \n> [...]\n> > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> > index 3ecb830f4a..40dd329e02 100644\n> > --- a/t/helper/test-run-command.c\n> > +++ b/t/helper/test-run-command.c\n> > @@ -52,6 +52,21 @@ static int no_job(struct child_process *cp,\n> > return 0;\n> > }\n> >\n> > +static void duplicate_output(struct strbuf *process_out,\n> > + struct strbuf *out,\n> > + void *pp_cb,\n> > + void *pp_task_cb)\n>\n> Should the last two parameters have an \"UNUSED\" annotation?\n\nYes, good catch!\n"},{"id":"468274","messageId":"CAFySSZDfueOverb0TrYd_BZAgjVpS2pzqaN+=jHXUOrH6-eU+g@mail.gmail.com","threadId":"58768","inReplyTo":"778b25b4-8eb2-6d02-3ba5-a21724ff2f2f@dunelm.org.uk","subject":"Re: [PATCH v4 1/5] run-command: add duplicate_output_fn to run_processes_parallel_opts","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T19:02:06Z","receivedAt":"2022-11-30T19:05:09Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Wed, Nov 30, 2022 at 2:26 AM Phillip Wood wrote:\n>\n> On 30/11/2022 09:53, Ævar Arnfjörð Bjarmason wrote:\n> >\n> > On Tue, Nov 29 2022, Glen Choo wrote:\n> >\n> >> Calvin Wan writes:\n> >>\n> >>> @@ -1680,8 +1683,14 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n> >>> for (size_t i = 0; i < opts->processes; i++) {\n> >>> if (pp->children[i].state == GIT_CP_WORKING &&\n> >>> pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n> >>> - int n = strbuf_read_once(&pp->children[i].err,\n> >>> - pp->children[i].process.err, 0);\n> >>> + struct strbuf buf = STRBUF_INIT;\n> >>> + int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);\n> >>> + strbuf_addbuf(&pp->children[i].err, &buf);\n> >>> + if (opts->duplicate_output)\n>\n> Shouldn't we be checking if n < 0 before we do this?\n\nYes we should.\n>\n> >>> + opts->duplicate_output(&buf, &pp->children[i].err,\n> >>> + opts->data,\n> >>> + pp->children[i].data);\n> >>> + strbuf_release(&buf);\n> >>> if (n == 0) {\n> >>> close(pp->children[i].process.err);\n> >>> pp->children[i].state = GIT_CP_WAIT_CLEANUP;\n> >>\n> >> A common pattern is that optional behavior does not impose additional\n> >> costs on the non-optional part. Here, we unconditionally do a\n> >> strbuf_addbuf() even though we don't use the result in the \"else\" case.\n> >>\n> >> So this might be more idiomatically written as:\n> >>\n> >> int n = strbuf_read_once(&pp->children[i].err,\n> >> pp->children[i].process.err, 0);\n> >> + if (opts->duplicate_output) {\n> >> + struct strbuf buf = STRBUF_INIT;\n> >> + strbuf_addbuf(&buf, &pp->children[i].err);\n> >> + opts->duplicate_output(&buf, &pp->children[i].err,\n> >> + opts->data,\n> >> + pp->children[i].data);\n> >> + strbuf_release(&buf);\n> >> + }\n> >>\n> > [...]\n> > And why does \"&buf\" exist at all?\n>\n> I was wondering that as too\n>\n> >Why can't we just pass\n> > &pp->children[i].err, and if this callback cares about the last thing we\n> > read let's pass it an offset, so it can know what came from the\n> > strbuf_read_once() (I don't know if it actually cares about that\n> > either...).\n>\n> Or we could just pass a `const char*`, `size_t` pair.\n\nBoth you and Avar are correct that &buf isn't necessary. I think the\noffset idea works better so the function calls stay similar.\n\n>\n> > That would avoid the copy entirely.\n>\n> Is the copying quadratic at the moment? - it looks like each call to\n> strbuf_read_once() appends to the buffer and we copy the whole buffer\n> each time.\n>\n\nDepends on how you're defining it, but doesn't really matter since it's\ngoing away anyways ;)\n"},{"id":"468275","messageId":"CAFySSZBC5HbGQxtpsEL8_kxbogx0JX10DvMXJb9uotyQpg46Cw@mail.gmail.com","threadId":"58768","inReplyTo":"19f91fea-a2a9-7dc6-d940-cc10f384fe76@dunelm.org.uk","subject":"Re: [PATCH v4 4/5] diff-lib: refactor match_stat_with_submodule","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-11-30T19:08:09Z","receivedAt":"2022-11-30T19:08:23Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"> Thanks for splitting this change out. I have to say I find the original\n> quite a bit easier to read. If you're worried about the code added in\n> the next commit being too indented perhaps you could move the body of\n> the if statement into a separate function.\n\nThen we're just swapping if statement depth for function call depth, which\nseems even worse. I think the confusion comes from adding the \"ret:\" part\nwhich is currently unnecessary so I can get rid of that this patch.\n\nThanks\n"},{"id":"470382","messageId":"xmqqv8l8f8j8.fsf@gitster.g","threadId":"58768","inReplyTo":"20221108184200.2813458-1-calvinwan@google.com","subject":"Re: [PATCH v4 0/5] submodule: parallelize diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T09:31:55Z","receivedAt":"2023-01-15T09:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> Original cover letter for context:\n> https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/\n> ...\n> Calvin Wan (5):\n> run-command: add duplicate_output_fn to run_processes_parallel_opts\n> submodule: strbuf variable rename\n> submodule: move status parsing into function\n> diff-lib: refactor match_stat_with_submodule\n> diff-lib: parallelize run_diff_files for submodules\n>\n> Documentation/config/submodule.txt | 12 ++\n> diff-lib.c | 103 +++++++++++--\n> run-command.c | 13 +-\n> run-command.h | 24 +++\n> submodule.c | 229 +++++++++++++++++++++++++----\n> submodule.h | 9 ++\n> t/helper/test-run-command.c | 21 +++\n> t/t0061-run-command.sh | 39 +++++\n> t/t4027-diff-submodule.sh | 19 +++\n> t/t7506-status-submodule.sh | 19 +++\n> 10 files changed, 441 insertions(+), 47 deletions(-)\n\nWhile the topic is marked as \"Needs review\" in the recent \"What's\ncooking\" reports, merging this topic also breaks the \"linux-leaks\"\njob by causing many tests fail:\n\n t3040-subprojects-basic.sh\n t4010-diff-pathspec.sh\n t4015-diff-whitespace.sh\n t4027-diff-submodule.sh\n t7403-submodule-sync.sh\n t7409-submodule-detached-work-tree.sh\n t7416-submodule-dash-url.sh\n t7450-bad-git-dotfiles.sh\n t7506-status-submodule.sh\n\nTwo of the test scripts are touched by this topic, and their\nbreakage could be caused by newly using other git subcommands that\nwere known to be leaking (iow, not because this series introduced\nnew leaks). It also is possible that they fail because this series\nadded new leaks to the commands these two test scripts use. In\neither case, other tests that haven't been touched by this topic\nwere definitely broken by new leaks introduced by the changes made\nby this series.\n\nAnybody interested should be able to see the breakage themselves by\nchecking out 'seen' and running\n\n SANTIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true \\\n make test\n\nto see the tree with all in-flight topics are clean, and then by\nrunning the same test after merging this topic to 'seen'.\n\nThanks.\n"},{"id":"470537","messageId":"CAFySSZC9XpCLjh7jCSNvixXhfrfGHpVr_coqHrKAXZbYzoT+5w@mail.gmail.com","threadId":"58768","inReplyTo":"xmqqv8l8f8j8.fsf@gitster.g","subject":"Re: [PATCH v4 0/5] submodule: parallelize diff","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2023-01-17T19:31:37Z","receivedAt":"2023-01-17T21:07:21Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Hi Junio\n\nI've sent out a reroll to fix this. Thanks!\n\nPassing leaks CI at:\nhttps://github.com/calvin-wan-google/git/actions/runs/3942292098\n(linux-musl technically failed, but it looks like for other reasons)\n\n\nOn Sun, Jan 15, 2023 at 1:31 AM Junio C Hamano wrote:\n>\n> Calvin Wan writes:\n>\n> > Original cover letter for context:\n> > https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/\n> > ...\n> > Calvin Wan (5):\n> > run-command: add duplicate_output_fn to run_processes_parallel_opts\n> > submodule: strbuf variable rename\n> > submodule: move status parsing into function\n> > diff-lib: refactor match_stat_with_submodule\n> > diff-lib: parallelize run_diff_files for submodules\n> >\n> > Documentation/config/submodule.txt | 12 ++\n> > diff-lib.c | 103 +++++++++++--\n> > run-command.c | 13 +-\n> > run-command.h | 24 +++\n> > submodule.c | 229 +++++++++++++++++++++++++----\n> > submodule.h | 9 ++\n> > t/helper/test-run-command.c | 21 +++\n> > t/t0061-run-command.sh | 39 +++++\n> > t/t4027-diff-submodule.sh | 19 +++\n> > t/t7506-status-submodule.sh | 19 +++\n> > 10 files changed, 441 insertions(+), 47 deletions(-)\n>\n> While the topic is marked as \"Needs review\" in the recent \"What's\n> cooking\" reports, merging this topic also breaks the \"linux-leaks\"\n> job by causing many tests fail:\n>\n> t3040-subprojects-basic.sh\n> t4010-diff-pathspec.sh\n> t4015-diff-whitespace.sh\n> t4027-diff-submodule.sh\n> t7403-submodule-sync.sh\n> t7409-submodule-detached-work-tree.sh\n> t7416-submodule-dash-url.sh\n> t7450-bad-git-dotfiles.sh\n> t7506-status-submodule.sh\n>\n> Two of the test scripts are touched by this topic, and their\n> breakage could be caused by newly using other git subcommands that\n> were known to be leaking (iow, not because this series introduced\n> new leaks). It also is possible that they fail because this series\n> added new leaks to the commands these two test scripts use. In\n> either case, other tests that haven't been touched by this topic\n> were definitely broken by new leaks introduced by the changes made\n> by this series.\n>\n> Anybody interested should be able to see the breakage themselves by\n> checking out 'seen' and running\n>\n> SANTIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true \\\n> make test\n>\n> to see the tree with all in-flight topics are clean, and then by\n> running the same test after merging this topic to 'seen'.\n>\n> Thanks.\n"}]}