[PATCH v3 0/6] submodule: parallelize diff

[PATCH v3 0/6] submodule: parallelize diff

[Calvin Wan]

Original cover letter for context:
https://lore.kernel.org/git/20221011232604.839941-1-calvinwan@google.com/

Changes since v2

Rebased on top of Avar's v3 run-command refactor[1]

pipe_output_fn has been decoupled from 'err' and now uses a new
variable in run_processes_parallel_opts, 'out'. This simplifies much
of the logic and testing difficulty of the previous patch, since 'out'
only holds output from child processes and nothing else.

Added a new patch to add hide_output to run_processes_parallel_opts. I
previously was reseting 'err' as to not print output from child
processes, but that was an unnecessary complication. A simple option to
not print output is much cleaner.

Added a setup patch before "submodule: move status parsing into function"

Fixed many stylistic changes and rewrote some documentation recommended
by Avar -- thank you for your reviews!

[1] https://lore.kernel.org/git/cover-v3-00.15-00000000000-20221012T205712Z-avarab@gmail.com/

Calvin Wan (6):
  run-command: add pipe_output_fn to run_processes_parallel_opts
  run-command: add hide_output to run_processes_parallel_opts
  submodule: strbuf variable rename
  submodule: move status parsing into function
  diff-lib: refactor match_stat_with_submodule
  diff-lib: parallelize run_diff_files for submodules

 Documentation/config/submodule.txt |  12 ++
 diff-lib.c                         | 102 +++++++++++--
 run-command.c                      |  29 +++-
 run-command.h                      |  30 ++++
 submodule.c                        | 232 +++++++++++++++++++++++++----
 submodule.h                        |   9 ++
 t/helper/test-run-command.c        |  19 +++
 t/t0061-run-command.sh             |  36 +++++
 t/t4027-diff-submodule.sh          |  19 +++
 t/t7506-status-submodule.sh        |  19 +++
 10 files changed, 457 insertions(+), 50 deletions(-)

-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Calvin Wan]

Add pipe_output_fn as an optionally set function in
run_process_parallel_opts. If set, output from each child process is
first separately stored in 'out' and then piped to the callback
function when the child process finishes to allow for separate parsing.

Two of the tests check for line count rather than an exact match
since the interleaved output order is not guaranteed to be exactly
the same every run through.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 run-command.c               | 21 +++++++++++++++++++--
 run-command.h               | 21 +++++++++++++++++++++
 t/helper/test-run-command.c | 13 +++++++++++++
 t/t0061-run-command.sh      | 30 ++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index c772acd743..03787bc7f5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1503,6 +1503,7 @@ struct parallel_processes {
 		enum child_state state;
 		struct child_process process;
 		struct strbuf err;
+		struct strbuf out;
 		void *data;
 	} *children;
 	/*
@@ -1560,6 +1561,9 @@ static void pp_init(struct parallel_processes *pp,
 
 	if (!opts->get_next_task)
 		BUG("you need to specify a get_next_task function");
+	
+	if (opts->pipe_output && opts->ungroup)
+		BUG("pipe_output and ungroup are incompatible with each other");
 
 	CALLOC_ARRAY(pp->children, n);
 	if (!opts->ungroup)
@@ -1567,6 +1571,8 @@ static void pp_init(struct parallel_processes *pp,
 
 	for (size_t i = 0; i < n; i++) {
 		strbuf_init(&pp->children[i].err, 0);
+		if (opts->pipe_output)
+			strbuf_init(&pp->children[i].out, 0);
 		child_process_init(&pp->children[i].process);
 		if (pp->pfd) {
 			pp->pfd[i].events = POLLIN | POLLHUP;
@@ -1586,6 +1592,7 @@ static void pp_cleanup(struct parallel_processes *pp,
 	trace_printf("run_processes_parallel: done");
 	for (size_t i = 0; i < opts->processes; i++) {
 		strbuf_release(&pp->children[i].err);
+		strbuf_release(&pp->children[i].out);
 		child_process_clear(&pp->children[i].process);
 	}
 
@@ -1680,8 +1687,12 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
 	for (size_t i = 0; i < opts->processes; i++) {
 		if (pp->children[i].state == GIT_CP_WORKING &&
 		    pp->pfd[i].revents & (POLLIN | POLLHUP)) {
-			int n = strbuf_read_once(&pp->children[i].err,
-						 pp->children[i].process.err, 0);
+			struct strbuf buf = STRBUF_INIT;
+			int n = strbuf_read_once(&buf, pp->children[i].process.err, 0);
+			strbuf_addbuf(&pp->children[i].err, &buf);
+			if (opts->pipe_output)
+				strbuf_addbuf(&pp->children[i].out, &buf);
+			strbuf_release(&buf);
 			if (n == 0) {
 				close(pp->children[i].process.err);
 				pp->children[i].state = GIT_CP_WAIT_CLEANUP;
@@ -1717,6 +1728,12 @@ static int pp_collect_finished(struct parallel_processes *pp,
 		if (i == opts->processes)
 			break;
 
+		if (opts->pipe_output) {
+			opts->pipe_output(&pp->children[i].out, opts->data,
+					  pp->children[i].data);
+			strbuf_reset(&pp->children[i].out);
+		}
+
 		code = finish_command(&pp->children[i].process);
 
 		if (opts->task_finished)
diff --git a/run-command.h b/run-command.h
index e3e1ea01ad..b4584c3698 100644
--- a/run-command.h
+++ b/run-command.h
@@ -440,6 +440,21 @@ typedef int (*start_failure_fn)(struct strbuf *out,
 				void *pp_cb,
 				void *pp_task_cb);
 
+/**
+ * This callback is called on every child process that finished processing.
+ * 
+ * "struct strbuf *process_out" contains the output from the finished child
+ * process.
+ *
+ * pp_cb is the callback cookie as passed into run_processes_parallel,
+ * pp_task_cb is the callback cookie as passed into get_next_task_fn.
+ *
+ * This function is incompatible with "ungroup"
+ */
+typedef void (*pipe_output_fn)(struct strbuf *process_out,
+			       void *pp_cb,
+			       void *pp_task_cb);
+
 /**
  * This callback is called on every child process that finished processing.
  *
@@ -493,6 +508,12 @@ struct run_process_parallel_opts
 	 */
 	start_failure_fn start_failure;
 
+	/**
+	 * pipe_output: See pipe_output_fn() above. This should be
+	 * NULL unless process specific output is needed
+	 */
+	pipe_output_fn pipe_output;
+
 	/**
 	 * task_finished: See task_finished_fn() above. This can be
 	 * NULL to omit any special handling.
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 3ecb830f4a..e9b41419a0 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -52,6 +52,13 @@ static int no_job(struct child_process *cp,
 	return 0;
 }
 
+static void pipe_output(struct strbuf *process_out,
+			void *pp_cb,
+			void *pp_task_cb)
+{
+	fprintf(stderr, "%s", process_out->buf);
+}
+
 static int task_finished(int result,
 			 struct strbuf *err,
 			 void *pp_cb,
@@ -439,6 +446,12 @@ int cmd__run_command(int argc, const char **argv)
 		opts.ungroup = 1;
 	}
 
+	if (!strcmp(argv[1], "--pipe-output")) {
+		argv += 1;
+		argc -= 1;
+		opts.pipe_output = pipe_output;
+	}
+
 	jobs = atoi(argv[2]);
 	strvec_clear(&proc.args);
 	strvec_pushv(&proc.args, (const char **)argv + 3);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 7b5423eebd..e50e57db89 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -134,6 +134,12 @@ test_expect_success 'run_command runs in parallel with more jobs available than
 	test_cmp expect actual
 '
 
+test_expect_success 'run_command runs in parallel with more jobs available than tasks --pipe-output' '
+	test-tool run-command --pipe-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+	test_must_be_empty out &&
+	test_line_count = 20 err
+'
+
 test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
 	test-tool run-command --ungroup run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
 	test_line_count = 8 out &&
@@ -145,6 +151,12 @@ test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'run_command runs in parallel with as many jobs as tasks --pipe-output' '
+	test-tool run-command --pipe-output run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+	test_must_be_empty out &&
+	test_line_count = 20 err
+'
+
 test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
 	test-tool run-command --ungroup run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
 	test_line_count = 8 out &&
@@ -156,6 +168,12 @@ test_expect_success 'run_command runs in parallel with more tasks than jobs avai
 	test_cmp expect actual
 '
 
+test_expect_success 'run_command runs in parallel with more tasks than jobs available --pipe-output' '
+	test-tool run-command --pipe-output run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+	test_must_be_empty out &&
+	test_line_count = 20 err
+'
+
 test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
 	test-tool run-command --ungroup run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
 	test_line_count = 8 out &&
@@ -176,6 +194,12 @@ test_expect_success 'run_command is asked to abort gracefully' '
 	test_cmp expect actual
 '
 
+test_expect_success 'run_command is asked to abort gracefully --pipe-output' '
+	test-tool run-command --pipe-output run-command-abort 3 false >out 2>err &&
+	test_must_be_empty out &&
+	test_cmp expect err
+'
+
 test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
 	test-tool run-command --ungroup run-command-abort 3 false >out 2>err &&
 	test_must_be_empty out &&
@@ -191,6 +215,12 @@ test_expect_success 'run_command outputs ' '
 	test_cmp expect actual
 '
 
+test_expect_success 'run_command outputs --pipe-output' '
+	test-tool run-command --pipe-output run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+	test_must_be_empty out &&
+	test_cmp expect err
+'
+
 test_expect_success 'run_command outputs (ungroup) ' '
 	test-tool run-command --ungroup run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
 	test_must_be_empty out &&
-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 2/6] run-command: add hide_output to run_processes_parallel_opts

[Calvin Wan]

Output from child processes and callbacks may not be necessary to
print out for every invoker of run_processes_parallel. Add
hide_output as an option to not print said output.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 run-command.c               | 8 +++++---
 run-command.h               | 9 +++++++++
 t/helper/test-run-command.c | 6 ++++++
 t/t0061-run-command.sh      | 6 ++++++
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 03787bc7f5..3aa28ad6dc 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1603,7 +1603,8 @@ static void pp_cleanup(struct parallel_processes *pp,
 	 * When get_next_task added messages to the buffer in its last
 	 * iteration, the buffered output is non empty.
 	 */
-	strbuf_write(&pp->buffered_output, stderr);
+	if (!opts->hide_output)
+		strbuf_write(&pp->buffered_output, stderr);
 	strbuf_release(&pp->buffered_output);
 
 	sigchain_pop_common();
@@ -1754,7 +1755,7 @@ static int pp_collect_finished(struct parallel_processes *pp,
 			pp->pfd[i].fd = -1;
 		child_process_init(&pp->children[i].process);
 
-		if (opts->ungroup) {
+		if (opts->ungroup || opts->hide_output) {
 			; /* no strbuf_*() work to do here */
 		} else if (i != pp->output_owner) {
 			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
@@ -1826,7 +1827,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 				pp.children[i].state = GIT_CP_WAIT_CLEANUP;
 		} else {
 			pp_buffer_stderr(&pp, opts, output_timeout);
-			pp_output(&pp);
+			if (!opts->hide_output)
+				pp_output(&pp);
 		}
 		code = pp_collect_finished(&pp, opts);
 		if (code) {
diff --git a/run-command.h b/run-command.h
index b4584c3698..4570812c08 100644
--- a/run-command.h
+++ b/run-command.h
@@ -496,6 +496,11 @@ struct run_process_parallel_opts
 	 */
 	unsigned int ungroup:1;
 
+	/**
+	 * hide_output: see 'hide_output' in run_processes_parallel() below.
+	 */
+	unsigned int hide_output:1;
+
 	/**
 	 * get_next_task: See get_next_task_fn() above. This must be
 	 * specified.
@@ -547,6 +552,10 @@ struct run_process_parallel_opts
  * NULL "struct strbuf *out" parameter, and are responsible for
  * emitting their own output, including dealing with any race
  * conditions due to writing in parallel to stdout and stderr.
+ * 
+ * If the "hide_output" option is set, any output in the "struct strbuf
+ * *out" parameter will not be printed by this function. This includes
+ * output from child processes as well as callbacks.
  */
 void run_processes_parallel(const struct run_process_parallel_opts *opts);
 
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index e9b41419a0..1af443db4d 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -446,6 +446,12 @@ int cmd__run_command(int argc, const char **argv)
 		opts.ungroup = 1;
 	}
 
+	if (!strcmp(argv[1], "--hide-output")) {
+		argv += 1;
+		argc -= 1;
+		opts.hide_output = 1;
+	}
+
 	if (!strcmp(argv[1], "--pipe-output")) {
 		argv += 1;
 		argc -= 1;
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index e50e57db89..a0219f6093 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -180,6 +180,12 @@ test_expect_success 'run_command runs ungrouped in parallel with more tasks than
 	test_line_count = 4 err
 '
 
+test_expect_success 'run_command hides output when run in parallel' '
+	test-tool run-command --hide-output run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err
+'
+
 cat >expect <<-EOF
 preloaded output of a child
 asking for a quick stop
-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 3/6] submodule: strbuf variable rename

[Calvin Wan]

A prepatory change for a future patch that moves the status parsing
logic to a separate function.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 submodule.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/submodule.c b/submodule.c
index f7c71f1f4b..ac214f250d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1900,25 +1900,28 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 
 	fp = xfdopen(cp.out, "r");
 	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
+		char *str = buf.buf;
+		const size_t len = buf.len;
+
 		/* regular untracked files */
-		if (buf.buf[0] == '?')
+		if (str[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 
-		if (buf.buf[0] == 'u' ||
-		    buf.buf[0] == '1' ||
-		    buf.buf[0] == '2') {
+		if (str[0] == 'u' ||
+		    str[0] == '1' ||
+		    str[0] == '2') {
 			/* T = line type, XY = status, SSSS = submodule state */
-			if (buf.len < strlen("T XY SSSS"))
+			if (len < strlen("T XY SSSS"))
 				BUG("invalid status --porcelain=2 line %s",
-				    buf.buf);
+				    str);
 
-			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+			if (str[5] == 'S' && str[8] == 'U')
 				/* nested untracked file */
 				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 
-			if (buf.buf[0] == 'u' ||
-			    buf.buf[0] == '2' ||
-			    memcmp(buf.buf + 5, "S..U", 4))
+			if (str[0] == 'u' ||
+			    str[0] == '2' ||
+			    memcmp(str + 5, "S..U", 4))
 				/* other change */
 				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}
-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 4/6] submodule: move status parsing into function

[Calvin Wan]

A future patch requires the ability to parse the output of git
status --porcelain=2. Move parsing code from is_submodule_modified to
parse_status_porcelain.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 submodule.c | 74 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/submodule.c b/submodule.c
index ac214f250d..289be0fb93 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1864,6 +1864,45 @@ int fetch_submodules(struct repository *r,
 	return spf.result;
 }
 
+static int parse_status_porcelain(char *str, size_t len,
+				  unsigned *dirty_submodule,
+				  int ignore_untracked)
+{
+	/* regular untracked files */
+	if (str[0] == '?')
+		*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+	if (str[0] == 'u' ||
+	    str[0] == '1' ||
+	    str[0] == '2') {
+		/* T = line type, XY = status, SSSS = submodule state */
+		if (len < strlen("T XY SSSS"))
+			BUG("invalid status --porcelain=2 line %s",
+			    str);
+
+		if (str[5] == 'S' && str[8] == 'U')
+			/* nested untracked file */
+			*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+		if (str[0] == 'u' ||
+		    str[0] == '2' ||
+		    memcmp(str + 5, "S..U", 4))
+			/* other change */
+			*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+	}
+
+	if ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+	    ((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+	     ignore_untracked)) {
+		/*
+		* We're not interested in any further information from
+		* the child any more, neither output nor its exit code.
+		*/
+		return 1;
+	}
+	return 0;
+}
+
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -1903,39 +1942,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		char *str = buf.buf;
 		const size_t len = buf.len;
 
-		/* regular untracked files */
-		if (str[0] == '?')
-			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-
-		if (str[0] == 'u' ||
-		    str[0] == '1' ||
-		    str[0] == '2') {
-			/* T = line type, XY = status, SSSS = submodule state */
-			if (len < strlen("T XY SSSS"))
-				BUG("invalid status --porcelain=2 line %s",
-				    str);
-
-			if (str[5] == 'S' && str[8] == 'U')
-				/* nested untracked file */
-				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-
-			if (str[0] == 'u' ||
-			    str[0] == '2' ||
-			    memcmp(str + 5, "S..U", 4))
-				/* other change */
-				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-		}
-
-		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
-		     ignore_untracked)) {
-			/*
-			 * We're not interested in any further information from
-			 * the child any more, neither output nor its exit code.
-			 */
-			ignore_cp_exit_code = 1;
+		ignore_cp_exit_code = parse_status_porcelain(str, len, &dirty_submodule,
+							     ignore_untracked);
+		if (ignore_cp_exit_code)
 			break;
-		}
 	}
 	fclose(fp);
 
-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 5/6] diff-lib: refactor match_stat_with_submodule

[Calvin Wan]

Flatten out the if statements in match_stat_with_submodule so the
logic is more readable and easier for future patches to add to.
orig_flags didn't need to be set if the cache entry wasn't a
GITLINK so defer setting it.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 diff-lib.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 2edea41a23..f5257c0c71 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -73,18 +73,25 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 				     unsigned *dirty_submodule)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
-	if (S_ISGITLINK(ce->ce_mode)) {
-		struct diff_flags orig_flags = diffopt->flags;
-		if (!diffopt->flags.override_submodule_config)
-			set_diffopt_flags_from_submodule_config(diffopt, ce->name);
-		if (diffopt->flags.ignore_submodules)
-			changed = 0;
-		else if (!diffopt->flags.ignore_dirty_submodules &&
-			 (!changed || diffopt->flags.dirty_submodules))
-			*dirty_submodule = is_submodule_modified(ce->name,
-								 diffopt->flags.ignore_untracked_in_submodules);
-		diffopt->flags = orig_flags;
+	struct diff_flags orig_flags;
+
+	if (!S_ISGITLINK(ce->ce_mode))
+		goto ret;
+
+	orig_flags = diffopt->flags;
+	if (!diffopt->flags.override_submodule_config)
+		set_diffopt_flags_from_submodule_config(diffopt, ce->name);
+	if (diffopt->flags.ignore_submodules) {
+		changed = 0;
+		goto cleanup;
 	}
+	if (!diffopt->flags.ignore_dirty_submodules &&
+	    (!changed || diffopt->flags.dirty_submodules))
+		*dirty_submodule = is_submodule_modified(ce->name,
+					 diffopt->flags.ignore_untracked_in_submodules);
+cleanup:
+	diffopt->flags = orig_flags;
+ret:
 	return changed;
 }
 
-- 
2.38.0.135.g90850a2211-goog

[PATCH v3 6/6] diff-lib: parallelize run_diff_files for submodules

[Calvin Wan]

During the iteration of the index entries in run_diff_files, whenever
a submodule is found and needs its status checked, a subprocess is
spawned for it. Instead of spawning the subprocess immediately and
waiting for its completion to continue, hold onto all submodules and
relevant information in a list. Then use that list to create tasks for
run_processes_parallel. Subprocess output is piped to
status_pipe_output which parses it. 

Add config option submodule.diffJobs to set the maximum number
of parallel jobs. The option defaults to 1 if unset. If set to 0, the
number of jobs is set to online_cpus().

Since run_diff_files is called from many different commands, I chose
to grab the config option in the function rather than adding variables
to every git command and then figuring out how to pass them all in.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 Documentation/config/submodule.txt |  12 +++
 diff-lib.c                         |  79 +++++++++++++--
 submodule.c                        | 157 +++++++++++++++++++++++++++++
 submodule.h                        |   9 ++
 t/t4027-diff-submodule.sh          |  19 ++++
 t/t7506-status-submodule.sh        |  19 ++++
 6 files changed, 289 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index 6490527b45..1144a5ad74 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -93,6 +93,18 @@ submodule.fetchJobs::
 	in parallel. A value of 0 will give some reasonable default.
 	If unset, it defaults to 1.
 
+submodule.diffJobs::
+	Specifies how many submodules are diffed at the same time. A
+	positive integer allows up to that number of submodules diffed
+	in parallel. A value of 0 will give the number of logical cores.
+	If unset, it defaults to 1. The diff operation is used by many
+	other git commands such as add, merge, diff, status, stash and
+	more. Note that the expensive part of the diff operation is
+	reading the index from cache or memory. Therefore multiple jobs
+	may be detrimental to performance if your hardware does not
+	support parallel reads or if the number of jobs greatly exceeds
+	the amount of supported reads.
+
 submodule.alternateLocation::
 	Specifies how the submodules obtain alternates when submodules are
 	cloned. Possible values are `no`, `superproject`.
diff --git a/diff-lib.c b/diff-lib.c
index f5257c0c71..0f30b4e478 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -14,6 +14,7 @@
 #include "dir.h"
 #include "fsmonitor.h"
 #include "commit-reach.h"
+#include "config.h"
 
 /*
  * diff-files
@@ -65,15 +66,20 @@ static int check_removed(const struct index_state *istate, const struct cache_en
  * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES
  * option is set, the caller does not only want to know if a submodule is
  * modified at all but wants to know all the conditions that are met (new
- * commits, untracked content and/or modified content).
+ * commits, untracked content and/or modified content). If
+ * defer_submodule_status bit is set, dirty_submodule will be left to the
+ * caller to set. defer_submodule_status can also be set to 0 in this
+ * function if there is no need to check if the submodule is modified.
  */
 static int match_stat_with_submodule(struct diff_options *diffopt,
 				     const struct cache_entry *ce,
 				     struct stat *st, unsigned ce_option,
-				     unsigned *dirty_submodule)
+				     unsigned *dirty_submodule, int *defer_submodule_status,
+				     unsigned *ignore_untracked)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
 	struct diff_flags orig_flags;
+	int defer = 0;
 
 	if (!S_ISGITLINK(ce->ce_mode))
 		goto ret;
@@ -86,12 +92,20 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 		goto cleanup;
 	}
 	if (!diffopt->flags.ignore_dirty_submodules &&
-	    (!changed || diffopt->flags.dirty_submodules))
-		*dirty_submodule = is_submodule_modified(ce->name,
+	    (!changed || diffopt->flags.dirty_submodules)) {
+		if (defer_submodule_status && *defer_submodule_status) {
+			defer = 1;
+			*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
+		} else {
+			*dirty_submodule = is_submodule_modified(ce->name,
 					 diffopt->flags.ignore_untracked_in_submodules);
+		}
+	}
 cleanup:
 	diffopt->flags = orig_flags;
 ret:
+	if (defer_submodule_status)
+		*defer_submodule_status = defer;
 	return changed;
 }
 
@@ -103,6 +117,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
 	uint64_t start = getnanotime();
 	struct index_state *istate = revs->diffopt.repo->index;
+	struct string_list submodules = STRING_LIST_INIT_NODUP;
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
@@ -227,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			newmode = ce->ce_mode;
 		} else {
 			struct stat st;
+			unsigned ignore_untracked = 0;
+			int defer_submodule_status = !!revs->repo;
 
 			changed = check_removed(istate, ce, &st);
 			if (changed) {
@@ -248,8 +265,24 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-							    ce_option, &dirty_submodule);
+							    ce_option, &dirty_submodule,
+							    &defer_submodule_status,
+							    &ignore_untracked);
 			newmode = ce_mode_from_stat(ce, st.st_mode);
+			if (defer_submodule_status) {
+				struct submodule_status_util tmp = {
+					.changed = changed,
+					.dirty_submodule = 0,
+					.ignore_untracked = ignore_untracked,
+					.newmode = newmode,
+					.ce = ce
+				};
+				struct string_list_item *item;
+				item = string_list_append(&submodules, ce->name);
+				item->util = xmalloc(sizeof(tmp));
+				memcpy(item->util, &tmp, sizeof(tmp));
+				continue;
+			}
 		}
 
 		if (!changed && !dirty_submodule) {
@@ -268,6 +301,40 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			    ce->name, 0, dirty_submodule);
 
 	}
+	if (submodules.nr > 0) {
+		int parallel_jobs;
+		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
+			parallel_jobs = 1;
+		else if (!parallel_jobs)
+			parallel_jobs = online_cpus();
+		else if (parallel_jobs < 0)
+			die(_("submodule.diffjobs cannot be negative"));
+
+		if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
+			BUG("submodule status failed");
+		for (size_t i = 0; i < submodules.nr; i++) {
+			struct submodule_status_util *util = submodules.items[i].util;
+			struct cache_entry *ce = util->ce;
+			unsigned int oldmode;
+			const struct object_id *old_oid, *new_oid;
+
+			if (!util->changed && !util->dirty_submodule) {
+				ce_mark_uptodate(ce);
+				mark_fsmonitor_valid(istate, ce);
+				if (!revs->diffopt.flags.find_copies_harder)
+					continue;
+			}
+			oldmode = ce->ce_mode;
+			old_oid = &ce->oid;
+			new_oid = util->changed ? null_oid() : &ce->oid;
+			diff_change(&revs->diffopt, oldmode, util->newmode,
+				    old_oid, new_oid,
+				    !is_null_oid(old_oid),
+				    !is_null_oid(new_oid),
+				    ce->name, 0, util->dirty_submodule);
+		}
+	}
+	string_list_clear(&submodules, 1);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	trace_performance_since(start, "diff-files");
@@ -315,7 +382,7 @@ static int get_stat_data(const struct index_state *istate,
 			return -1;
 		}
 		changed = match_stat_with_submodule(diffopt, ce, &st,
-						    0, dirty_submodule);
+						    0, dirty_submodule, NULL, NULL);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			oid = null_oid();
diff --git a/submodule.c b/submodule.c
index 289be0fb93..9aa1723a3b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1363,6 +1363,20 @@ int submodule_touches_in_range(struct repository *r,
 	return ret;
 }
 
+struct submodule_parallel_status {
+	size_t index_count;
+	int result;
+
+	struct string_list *submodule_names;
+	struct repository *r;
+
+	/* Pending statuses by OIDs */
+	struct status_task **oid_status_tasks;
+	int oid_status_tasks_nr, oid_status_tasks_alloc;
+};
+
+#define SPS_INIT { 0 }
+
 struct submodule_parallel_fetch {
 	/*
 	 * The index of the last index entry processed by
@@ -1445,6 +1459,13 @@ struct fetch_task {
 	struct oid_array *commits; /* Ensure these commits are fetched */
 };
 
+struct status_task {
+	struct repository *repo;
+	const char *path;
+	unsigned dirty_submodule;
+	int ignore_untracked;
+};
+
 /**
  * When a submodule is not defined in .gitmodules, we cannot access it
  * via the regular submodule-config. Create a fake submodule, which we can
@@ -1956,6 +1977,142 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	return dirty_submodule;
 }
 
+static struct status_task *
+get_status_task_from_index(struct submodule_parallel_status *sps,
+			   struct strbuf *err)
+{
+	for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
+		struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
+		const struct cache_entry *ce = util->ce;
+		struct status_task *task;
+		struct strbuf buf = STRBUF_INIT;
+		const char *git_dir;
+
+		strbuf_addf(&buf, "%s/.git", ce->name);
+		git_dir = read_gitfile(buf.buf);
+		if (!git_dir)
+			git_dir = buf.buf;
+		if (!is_git_directory(git_dir)) {
+			if (is_directory(git_dir))
+				die(_("'%s' not recognized as a git repository"), git_dir);
+			strbuf_release(&buf);
+			/* The submodule is not checked out, so it is not modified */
+			util->dirty_submodule = 0;
+			continue;
+		}
+		strbuf_release(&buf);
+
+		task = xmalloc(sizeof(*task));
+		memset(task, 0, sizeof(*task));
+		task->path = ce->name;
+		task->dirty_submodule = util->dirty_submodule;
+		task->ignore_untracked = util->ignore_untracked;
+		sps->index_count++;
+		return task;
+	}
+	return NULL;
+}
+
+
+static int get_next_submodule_status(struct child_process *cp,
+				     struct strbuf *err,
+				     void *data, void **task_cb)
+{
+	struct submodule_parallel_status *sps = data;
+	struct status_task *task = get_status_task_from_index(sps, err);
+
+	if (!task) {
+		return 0;
+	}
+
+	child_process_init(cp);
+	prepare_submodule_repo_env_in_gitdir(&cp->env);
+
+	strvec_init(&cp->args);
+	strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
+	if (task->ignore_untracked)
+		strvec_push(&cp->args, "-uno");
+
+	prepare_submodule_repo_env(&cp->env);
+	cp->git_cmd = 1;
+	cp->no_stdin = 1;
+	cp->dir = task->path;
+	*task_cb = task;
+	return 1;
+}
+
+static int status_start_failure(struct strbuf *err,
+			        void *cb, void *task_cb)
+{
+	struct submodule_parallel_status *sps = cb;
+
+	sps->result = 1;
+	return 0;
+}
+
+static void status_pipe_output(struct strbuf *process_out,
+			       void *cb, void *task_cb)
+{
+	struct status_task *task = task_cb;
+	struct string_list list = STRING_LIST_INIT_DUP;
+
+	string_list_split(&list, process_out->buf, '\n', -1);
+
+	for (size_t i = 0; i < list.nr; i++) {
+		if (parse_status_porcelain(list.items[i].string,
+				   strlen(list.items[i].string),
+				   &task->dirty_submodule, task->ignore_untracked))
+			break;
+	}
+	string_list_clear(&list, 0);
+}
+
+static int status_finish(int retvalue, struct strbuf *err,
+			 void *cb, void *task_cb)
+{
+	struct submodule_parallel_status *sps = cb;
+	struct status_task *task = task_cb;
+	struct string_list_item *it =
+		string_list_lookup(sps->submodule_names, task->path);
+	struct submodule_status_util *util = it->util;
+
+	util->dirty_submodule = task->dirty_submodule;
+	free(task);
+
+	return 0;
+}
+
+int get_submodules_status(struct repository *r,
+			  struct string_list *submodules,
+			  int max_parallel_jobs)
+{
+	struct submodule_parallel_status sps = SPS_INIT;
+	const struct run_process_parallel_opts opts = {
+		.tr2_category = "submodule",
+		.tr2_label = "parallel/status",
+
+		.processes = max_parallel_jobs,
+		.hide_output = 1,
+
+		.get_next_task = get_next_submodule_status,
+		.start_failure = status_start_failure,
+		.pipe_output = status_pipe_output,
+		.task_finished = status_finish,
+		.data = &sps,
+	};
+
+	sps.r = r;
+
+	if (repo_read_index(r) < 0)
+		die(_("index file corrupt"));
+
+	sps.submodule_names = submodules;
+	string_list_sort(sps.submodule_names);
+	run_processes_parallel(&opts);
+
+	return sps.result;
+}
+
 int submodule_uses_gitfile(const char *path)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
diff --git a/submodule.h b/submodule.h
index 6a9fec6de1..cbb7231a5d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,6 +41,12 @@ struct submodule_update_strategy {
 	.type = SM_UPDATE_UNSPECIFIED, \
 }
 
+struct submodule_status_util {
+	int changed, ignore_untracked;
+	unsigned dirty_submodule, newmode;
+	struct cache_entry *ce;
+};
+
 int is_gitmodules_unmerged(struct index_state *istate);
 int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
@@ -94,6 +100,9 @@ int fetch_submodules(struct repository *r,
 		     int command_line_option,
 		     int default_option,
 		     int quiet, int max_parallel_jobs);
+int get_submodules_status(struct repository *r,
+			  struct string_list *submodules,
+			  int max_parallel_jobs);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int submodule_uses_gitfile(const char *path);
 
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 40164ae07d..e08ee315a7 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -34,6 +34,25 @@ test_expect_success setup '
 	subtip=$3 subprev=$2
 '
 
+test_expect_success 'diff in superproject with submodules respects parallel settings' '
+	test_when_finished "rm -f trace.out" &&
+	(
+		GIT_TRACE=$(pwd)/trace.out git diff &&
+		grep "1 tasks" trace.out &&
+		>trace.out &&
+
+		git config submodule.diffJobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git diff &&
+		grep "8 tasks" trace.out &&
+		>trace.out &&
+
+		GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&
+		grep "preparing to run up to [0-9]* tasks" trace.out &&
+		! grep "up to 0 tasks" trace.out &&
+		>trace.out
+	)
+'
+
 test_expect_success 'git diff --raw HEAD' '
 	hexsz=$(test_oid hexsz) &&
 	git diff --raw --abbrev=$hexsz HEAD >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index f5426a8e58..dfdfb58bfc 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -410,4 +410,23 @@ test_expect_success 'status with added file in nested submodule (short)' '
 	EOF
 '
 
+test_expect_success 'status in superproject with submodules respects parallel settings' '
+	test_when_finished "rm -f trace.out" &&
+	(
+		GIT_TRACE=$(pwd)/trace.out git status &&
+		grep "1 tasks" trace.out &&
+		>trace.out &&
+
+		git config submodule.diffJobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git status &&
+		grep "8 tasks" trace.out &&
+		>trace.out &&
+
+		GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&
+		grep "preparing to run up to [0-9]* tasks" trace.out &&
+		! grep "up to 0 tasks" trace.out &&
+		>trace.out
+	)
+'
+
 test_done
-- 
2.38.0.135.g90850a2211-goog

Re: [PATCH v3 6/6] diff-lib: parallelize run_diff_files for submodules

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 20 2022, Calvin Wan wrote:

> @@ -103,6 +117,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			      ? CE_MATCH_RACY_IS_DIRTY : 0);
>  	uint64_t start = getnanotime();
>  	struct index_state *istate = revs->diffopt.repo->index;
> +	struct string_list submodules = STRING_LIST_INIT_NODUP;
>  
>  	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
>  
> @@ -227,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			newmode = ce->ce_mode;
>  		} else {
>  			struct stat st;
> +			unsigned ignore_untracked = 0;
> +			int defer_submodule_status = !!revs->repo;
>  
>  			changed = check_removed(istate, ce, &st);
>  			if (changed) {
> @@ -248,8 +265,24 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			}
>  
>  			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> -							    ce_option, &dirty_submodule);
> +							    ce_option, &dirty_submodule,
> +							    &defer_submodule_status,
> +							    &ignore_untracked);
>  			newmode = ce_mode_from_stat(ce, st.st_mode);
> +			if (defer_submodule_status) {
> +				struct submodule_status_util tmp = {
> +					.changed = changed,
> +					.dirty_submodule = 0,
> +					.ignore_untracked = ignore_untracked,
> +					.newmode = newmode,
> +					.ce = ce

Nit: We usually add the trailing "," to lists, except as a marker of
"don't add anything after this", i.e. only omit it when we have NULL as
the last element, but it's good to have it here.

> +				};
> +				struct string_list_item *item;

Nit: Another \n here to seperate the variables from the code, maybe...

> +				item = string_list_append(&submodules, ce->name);
> +				item->util = xmalloc(sizeof(tmp));
> +				memcpy(item->util, &tmp, sizeof(tmp));

This whole thing is much more readable, thanks.

> +				continue;
> +			}
>  		}
>  
>  		if (!changed && !dirty_submodule) {
> @@ -268,6 +301,40 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			    ce->name, 0, dirty_submodule);
>  
>  	}
> +	if (submodules.nr > 0) {
> +		int parallel_jobs;
> +		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
> +			parallel_jobs = 1;
> +		else if (!parallel_jobs)
> +			parallel_jobs = online_cpus();
> +		else if (parallel_jobs < 0)
> +			die(_("submodule.diffjobs cannot be negative"));
> +
> +		if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
> +			BUG("submodule status failed");

A BUG() is probably too strong if we're invoking a subprocess,
i.e. maybe the OS got tired of us and killed it, which is just an
exception, not a bug in git.

And, I may be missing something, but later in that function we do:

	if (repo_read_index(r) < 0)
        	die(_("index file corrupt"));

Do we need to read the index there if we're just invoking a "status"
command, isn't it reading it for us & reporting back?

> +struct submodule_parallel_status {
> +	size_t index_count;
> +	int result;
> +
> +	struct string_list *submodule_names;
> +	struct repository *r;
> +
> +	/* Pending statuses by OIDs */
> +	struct status_task **oid_status_tasks;
> +	int oid_status_tasks_nr, oid_status_tasks_alloc;
> +};
> +
> +#define SPS_INIT { 0 }

Nit: It's good to add *_INIT macros sometimes, but here we just have a
private API that's only used once, which is...

> [...]
> +int get_submodules_status(struct repository *r,
> +			  struct string_list *submodules,
> +			  int max_parallel_jobs)
> +{
> +	struct submodule_parallel_status sps = SPS_INIT;


...here, to copy some context around, and...

> +	const struct run_process_parallel_opts opts = {
> +		.tr2_category = "submodule",
> +		.tr2_label = "parallel/status",
> +
> +		.processes = max_parallel_jobs,
> +		.hide_output = 1,
> +
> +		.get_next_task = get_next_submodule_status,
> +		.start_failure = status_start_failure,
> +		.pipe_output = status_pipe_output,
> +		.task_finished = status_finish,
> +		.data = &sps,
> +	};
> +
> +	sps.r = r;
> +
> +	if (repo_read_index(r) < 0)
> +		die(_("index file corrupt"));
> +
> +	sps.submodule_names = submodules;

...the net result is needing to first assign SPS_INIT, then add the "r"
member, then "submodule_names". In this case this looks easier:

	struct submodule_parallel_status sps = {
		.r = r,
		.submodule_names = submodules,
	};
	const struct run_process_parallel_opts opts = {
        [...]

> +	string_list_sort(sps.submodule_names);
> +	run_processes_parallel(&opts);

> +
>  struct submodule_parallel_fetch {
>  	/*
>  	 * The index of the last index entry processed by
> @@ -1445,6 +1459,13 @@ struct fetch_task {
>  	struct oid_array *commits; /* Ensure these commits are fetched */
>  };
>  
> +struct status_task {
> +	struct repository *repo;
> +	const char *path;
> +	unsigned dirty_submodule;
> +	int ignore_untracked;
> +};
> +
>  /**
>   * When a submodule is not defined in .gitmodules, we cannot access it
>   * via the regular submodule-config. Create a fake submodule, which we can
> @@ -1956,6 +1977,142 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  	return dirty_submodule;
>  }
>  
> +static struct status_task *
> +get_status_task_from_index(struct submodule_parallel_status *sps,
> +			   struct strbuf *err)
> +{
> +	for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
> +		struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
> +		const struct cache_entry *ce = util->ce;
> +		struct status_task *task;
> +		struct strbuf buf = STRBUF_INIT;
> +		const char *git_dir;
> +
> +		strbuf_addf(&buf, "%s/.git", ce->name);
> +		git_dir = read_gitfile(buf.buf);

Okey, so the "NULL" variant of read_gitfile_gently(), as we don't care
about the sort of error we got, but...

> +		if (!git_dir)
> +			git_dir = buf.buf;
> +		if (!is_git_directory(git_dir)) {

Isn't this something we could have asked read_gitfile_gently() about
instead, i.e. the READ_GITFILE_ERR_NOT_A_REPO condition?

> +			if (is_directory(git_dir))
> +				die(_("'%s' not recognized as a git repository"), git_dir);

It would be a detour from this, but perhaps setup.c can be tasked with
knowing about this edge case and returning an error code, but even if we
punt on that we can just do the is_directory() here, but get the
!is_git_directory() for free it seems.

> +			strbuf_release(&buf);
> +			/* The submodule is not checked out, so it is not modified */
> +			util->dirty_submodule = 0;
> +			continue;
> +		}
> +		strbuf_release(&buf);
> +
> +		task = xmalloc(sizeof(*task));
> +		memset(task, 0, sizeof(*task));

s/xmalloc/xcalloc/?

> +		task->path = ce->name;
> +		task->dirty_submodule = util->dirty_submodule;
> +		task->ignore_untracked = util->ignore_untracked;

Cn do we the same readability trick here that we did with "struct
submodule_status_util tmp = {" earlier & the memcpy()? Looks like it...

> +static int get_next_submodule_status(struct child_process *cp,
> +				     struct strbuf *err,
> +				     void *data, void **task_cb)

Nit: Too early wrapping, "void *data" should be at the previous line
(which won't exceed 79 chars)?


> +{
> +	struct submodule_parallel_status *sps = data;
> +	struct status_task *task = get_status_task_from_index(sps, err);
> +
> +	if (!task) {
> +		return 0;
> +	}

Nit: Don't need {}-braces here.

> +
> +	child_process_init(cp);
> +	prepare_submodule_repo_env_in_gitdir(&cp->env);
> +
> +	strvec_init(&cp->args);
> +	strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
> +	if (task->ignore_untracked)
> +		strvec_push(&cp->args, "-uno");

Nit: Maybe worth spelling out --untracked-files=no (or maybe "-uno" is
more idiomatic at this point...)

> +
> +	prepare_submodule_repo_env(&cp->env);
> +	cp->git_cmd = 1;
> +	cp->no_stdin = 1;
> +	cp->dir = task->path;
> +	*task_cb = task;
> +	return 1;
> +}
> +
> +static int status_start_failure(struct strbuf *err,
> +			        void *cb, void *task_cb)
> +{
> +	struct submodule_parallel_status *sps = cb;
> +
> +	sps->result = 1;
> +	return 0;
> +}
> +
> +static void status_pipe_output(struct strbuf *process_out,
> +			       void *cb, void *task_cb)
> +{
> +	struct status_task *task = task_cb;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	string_list_split(&list, process_out->buf, '\n', -1);
> +
> +	for (size_t i = 0; i < list.nr; i++) {
> +		if (parse_status_porcelain(list.items[i].string,
> +				   strlen(list.items[i].string),

Nit: I think you can use for_each_string_list_item() here, saving some
verbosity..

> +int get_submodules_status(struct repository *r,
> +			  struct string_list *submodules,
> +			  int max_parallel_jobs);

s/int/size_t/ because at this point you've already die()'d in the one
caller if it's <0 from the config parser, so we know it's unsigned
(actually >1, but unsigned will have to do :).

Re: [PATCH v3 2/6] run-command: add hide_output to run_processes_parallel_opts

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 20 2022, Calvin Wan wrote:

> Output from child processes and callbacks may not be necessary to
> print out for every invoker of run_processes_parallel. Add
> hide_output as an option to not print said output.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  run-command.c               | 8 +++++---
>  run-command.h               | 9 +++++++++
>  t/helper/test-run-command.c | 6 ++++++
>  t/t0061-run-command.sh      | 6 ++++++
>  4 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 03787bc7f5..3aa28ad6dc 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1603,7 +1603,8 @@ static void pp_cleanup(struct parallel_processes *pp,
>  	 * When get_next_task added messages to the buffer in its last
>  	 * iteration, the buffered output is non empty.
>  	 */
> -	strbuf_write(&pp->buffered_output, stderr);
> +	if (!opts->hide_output)
> +		strbuf_write(&pp->buffered_output, stderr);
>  	strbuf_release(&pp->buffered_output);
>  
>  	sigchain_pop_common();
> @@ -1754,7 +1755,7 @@ static int pp_collect_finished(struct parallel_processes *pp,
>  			pp->pfd[i].fd = -1;
>  		child_process_init(&pp->children[i].process);
>  
> -		if (opts->ungroup) {
> +		if (opts->ungroup || opts->hide_output) {
>  			; /* no strbuf_*() work to do here */
>  		} else if (i != pp->output_owner) {
>  			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
> @@ -1826,7 +1827,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  				pp.children[i].state = GIT_CP_WAIT_CLEANUP;
>  		} else {
>  			pp_buffer_stderr(&pp, opts, output_timeout);
> -			pp_output(&pp);
> +			if (!opts->hide_output)
> +				pp_output(&pp);
>  		}
>  		code = pp_collect_finished(&pp, opts);
>  		if (code) {
> diff --git a/run-command.h b/run-command.h
> index b4584c3698..4570812c08 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -496,6 +496,11 @@ struct run_process_parallel_opts
>  	 */
>  	unsigned int ungroup:1;
>  
> +	/**
> +	 * hide_output: see 'hide_output' in run_processes_parallel() below.
> +	 */
> +	unsigned int hide_output:1;
> +
>  	/**
>  	 * get_next_task: See get_next_task_fn() above. This must be
>  	 * specified.
> @@ -547,6 +552,10 @@ struct run_process_parallel_opts
>   * NULL "struct strbuf *out" parameter, and are responsible for
>   * emitting their own output, including dealing with any race
>   * conditions due to writing in parallel to stdout and stderr.
> + * 
> + * If the "hide_output" option is set, any output in the "struct strbuf
> + * *out" parameter will not be printed by this function. This includes
> + * output from child processes as well as callbacks.
>   */
>  void run_processes_parallel(const struct run_process_parallel_opts *opts);
>  
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index e9b41419a0..1af443db4d 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -446,6 +446,12 @@ int cmd__run_command(int argc, const char **argv)
>  		opts.ungroup = 1;
>  	}
>  
> +	if (!strcmp(argv[1], "--hide-output")) {
> +		argv += 1;
> +		argc -= 1;
> +		opts.hide_output = 1;
> +	}
> +
>  	if (!strcmp(argv[1], "--pipe-output")) {
>  		argv += 1;
>  		argc -= 1;
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index e50e57db89..a0219f6093 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -180,6 +180,12 @@ test_expect_success 'run_command runs ungrouped in parallel with more tasks than
>  	test_line_count = 4 err
>  '
>  
> +test_expect_success 'run_command hides output when run in parallel' '
> +	test-tool run-command --hide-output run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err
> +'
> +
>  cat >expect <<-EOF
>  preloaded output of a child
>  asking for a quick stop

I may just be missing something, but doesn't "struct child_process"
already have e.g. "no_stderr", "no_stdout" etc. that we can use?
I.e. isn't this thing equivalent to running:

	your-command >/dev/null 2>/dev/null

Which is what the non-parallel API already supports.

Now, IIRC if you just set that in the "get_next_task" callback it won't
work in the parallel API, or you'll block waiting for I/O that'll never
come or whatever.

But that'll be because the parallel interface currently only suppors a
subset of the full "child_process" combination of options, and maybe it
doesn't grok this.

But if that's the case we should just extend the API to support
"no_stdout", "no_stderr" etc., no?

I.e. hypothetically the parallel one could support 100% of the "struct
child_process" combination of options, we just haven't bothered yet.

But I don't see why the parallel API should grow options that we already
have in "struct child_process", instead we should set them there, and it
should gradually learn to deal with them.

I think it's also fine to have some basic sanity checks there, e.g. I
could see how for something like this we don't want to support piping
only some children to /dev/null but not others, and that it should be
all or nothing (maybe it makes state management when we loop over them
easier).

Or again, maybe I'm missing something...

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 20 2022, Calvin Wan wrote:

> Add pipe_output_fn as an optionally set function in
> run_process_parallel_opts. If set, output from each child process is
> first separately stored in 'out' and then piped to the callback
> function when the child process finishes to allow for separate parsing.

The "when[...]finish[ed]" here seems a bit odd to me. Why isn't the API
to just stream this to callbacks as it comes in.

Then if a caller only cares about the output at the very end they can
manage that state between their streaming callbacks and "finish"
callback, i.e. buffer it & flush it themselves.

> diff --git a/run-command.c b/run-command.c
> index c772acd743..03787bc7f5 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1503,6 +1503,7 @@ struct parallel_processes {
>  		enum child_state state;
>  		struct child_process process;
>  		struct strbuf err;
> +		struct strbuf out;
>  		void *data;
>  	} *children;
>  	/*
> @@ -1560,6 +1561,9 @@ static void pp_init(struct parallel_processes *pp,
>  
>  	if (!opts->get_next_task)
>  		BUG("you need to specify a get_next_task function");
> +	
> +	if (opts->pipe_output && opts->ungroup)
> +		BUG("pipe_output and ungroup are incompatible with each other");
>  
>  	CALLOC_ARRAY(pp->children, n);
>  	if (!opts->ungroup)
> @@ -1567,6 +1571,8 @@ static void pp_init(struct parallel_processes *pp,
>  
>  	for (size_t i = 0; i < n; i++) {
>  		strbuf_init(&pp->children[i].err, 0);
> +		if (opts->pipe_output)
> +			strbuf_init(&pp->children[i].out, 0);

Even if we're not using this, let's init it for simplicity. We don't use
the "err" with ungroup and we're init-ing that, and...

>  		child_process_init(&pp->children[i].process);
>  		if (pp->pfd) {
>  			pp->pfd[i].events = POLLIN | POLLHUP;
> @@ -1586,6 +1592,7 @@ static void pp_cleanup(struct parallel_processes *pp,
>  	trace_printf("run_processes_parallel: done");
>  	for (size_t i = 0; i < opts->processes; i++) {
>  		strbuf_release(&pp->children[i].err);
> +		strbuf_release(&pp->children[i].out);

...here you're strbuf_relese()-ing a string that was never init'd, it's
not segfaulting because we check sb->alloc, and since we calloc'd this
whole thing it'll be 0, but let's just init it so it's a proper strbuf
(with slopbuf). It's cheap.

> +/**
> + * This callback is called on every child process that finished processing.
> + * 
> + * "struct strbuf *process_out" contains the output from the finished child
> + * process.
> + *
> + * pp_cb is the callback cookie as passed into run_processes_parallel,
> + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> + *
> + * This function is incompatible with "ungroup"
> + */
> +typedef void (*pipe_output_fn)(struct strbuf *process_out,
> +			       void *pp_cb,
> +			       void *pp_task_cb);
> +
>  /**
>   * This callback is called on every child process that finished processing.
>   *
> @@ -493,6 +508,12 @@ struct run_process_parallel_opts
>  	 */
>  	start_failure_fn start_failure;
>  
> +	/**
> +	 * pipe_output: See pipe_output_fn() above. This should be
> +	 * NULL unless process specific output is needed
> +	 */
> +	pipe_output_fn pipe_output;
> +
>  	/**
>  	 * task_finished: See task_finished_fn() above. This can be
>  	 * NULL to omit any special handling.
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 3ecb830f4a..e9b41419a0 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -52,6 +52,13 @@ static int no_job(struct child_process *cp,
>  	return 0;
>  }
>  
> +static void pipe_output(struct strbuf *process_out,
> +			void *pp_cb,
> +			void *pp_task_cb)
> +{
> +	fprintf(stderr, "%s", process_out->buf);

maybe print this with split lines prefixed with something so wour tests
can see that something actually happened here, & test-cmp it so we can
see what went where, as opposed to...

> +test_expect_success 'run_command runs in parallel with more jobs available than tasks --pipe-output' '
> +	test-tool run-command --pipe-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> +	test_must_be_empty out &&
> +	test_line_count = 20 err
> +'

Just checking the number of lines, which seems to leave a lot of leeway
for the output being mixed up in all sorts of ways & the test to still
pass..

(ditto below)

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Junio C Hamano]

Calvin Wan <calvinwan@google.com> writes:

> Add pipe_output_fn as an optionally set function in
> run_process_parallel_opts. If set, output from each child process is
> first separately stored in 'out' and then piped to the callback
> function when the child process finishes to allow for separate parsing.

In my review of one of the previous rounds, I asked which part of
this functionality fits the name "pipe", and I do not think I got a
satisfactory answer.  And after re-reading the patch in this round,
with the in-header comments, it still is not clear to me.

It looks more like sending the duplicate of the normal output to a
side channel, somewhat like the "tee" utility, but I am not sure if
that is the intended way to be used.

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Calvin Wan]

> In my review of one of the previous rounds, I asked which part of
> this functionality fits the name "pipe", and I do not think I got a
> satisfactory answer.  And after re-reading the patch in this round,
> with the in-header comments, it still is not clear to me.
>
> It looks more like sending the duplicate of the normal output to a
> side channel, somewhat like the "tee" utility, but I am not sure if
> that is the intended way to be used.
>

In this case, I was hoping "pipe" would refer to the redirection of
output from the child processes to a separate custom function, but
I can see that duplication != redirection. Maybe something like
"parse_child_output" or "parse_output" would make sense, however,
I didn't want to imply with that name that the only functionality is to
parse output. Besides that, I don't really have any other ideas of
what I can name it...

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Calvin Wan]

On Thu, Oct 20, 2022 at 8:31 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Thu, Oct 20 2022, Calvin Wan wrote:
>
> > Add pipe_output_fn as an optionally set function in
> > run_process_parallel_opts. If set, output from each child process is
> > first separately stored in 'out' and then piped to the callback
> > function when the child process finishes to allow for separate parsing.
>
> The "when[...]finish[ed]" here seems a bit odd to me. Why isn't the API
> to just stream this to callbacks as it comes in.
>
> Then if a caller only cares about the output at the very end they can
> manage that state between their streaming callbacks and "finish"
> callback, i.e. buffer it & flush it themselves.

That's a good idea. This also lets me remove the 'out' variable from
parallel_process.children.

>
> > diff --git a/run-command.c b/run-command.c
> > index c772acd743..03787bc7f5 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1503,6 +1503,7 @@ struct parallel_processes {
> >               enum child_state state;
> >               struct child_process process;
> >               struct strbuf err;
> > +             struct strbuf out;
> >               void *data;
> >       } *children;
> >       /*
> > @@ -1560,6 +1561,9 @@ static void pp_init(struct parallel_processes *pp,
> >
> >       if (!opts->get_next_task)
> >               BUG("you need to specify a get_next_task function");
> > +
> > +     if (opts->pipe_output && opts->ungroup)
> > +             BUG("pipe_output and ungroup are incompatible with each other");
> >
> >       CALLOC_ARRAY(pp->children, n);
> >       if (!opts->ungroup)
> > @@ -1567,6 +1571,8 @@ static void pp_init(struct parallel_processes *pp,
> >
> >       for (size_t i = 0; i < n; i++) {
> >               strbuf_init(&pp->children[i].err, 0);
> > +             if (opts->pipe_output)
> > +                     strbuf_init(&pp->children[i].out, 0);
>
> Even if we're not using this, let's init it for simplicity. We don't use
> the "err" with ungroup and we're init-ing that, and...

ack.

>
> >               child_process_init(&pp->children[i].process);
> >               if (pp->pfd) {
> >                       pp->pfd[i].events = POLLIN | POLLHUP;
> > @@ -1586,6 +1592,7 @@ static void pp_cleanup(struct parallel_processes *pp,
> >       trace_printf("run_processes_parallel: done");
> >       for (size_t i = 0; i < opts->processes; i++) {
> >               strbuf_release(&pp->children[i].err);
> > +             strbuf_release(&pp->children[i].out);
>
> ...here you're strbuf_relese()-ing a string that was never init'd, it's
> not segfaulting because we check sb->alloc, and since we calloc'd this
> whole thing it'll be 0, but let's just init it so it's a proper strbuf
> (with slopbuf). It's cheap.

ack.

> > +/**
> > + * This callback is called on every child process that finished processing.
> > + *
> > + * "struct strbuf *process_out" contains the output from the finished child
> > + * process.
> > + *
> > + * pp_cb is the callback cookie as passed into run_processes_parallel,
> > + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> > + *
> > + * This function is incompatible with "ungroup"
> > + */
> > +typedef void (*pipe_output_fn)(struct strbuf *process_out,
> > +                            void *pp_cb,
> > +                            void *pp_task_cb);
> > +
> >  /**
> >   * This callback is called on every child process that finished processing.
> >   *
> > @@ -493,6 +508,12 @@ struct run_process_parallel_opts
> >        */
> >       start_failure_fn start_failure;
> >
> > +     /**
> > +      * pipe_output: See pipe_output_fn() above. This should be
> > +      * NULL unless process specific output is needed
> > +      */
> > +     pipe_output_fn pipe_output;
> > +
> >       /**
> >        * task_finished: See task_finished_fn() above. This can be
> >        * NULL to omit any special handling.
> > diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> > index 3ecb830f4a..e9b41419a0 100644
> > --- a/t/helper/test-run-command.c
> > +++ b/t/helper/test-run-command.c
> > @@ -52,6 +52,13 @@ static int no_job(struct child_process *cp,
> >       return 0;
> >  }
> >
> > +static void pipe_output(struct strbuf *process_out,
> > +                     void *pp_cb,
> > +                     void *pp_task_cb)
> > +{
> > +     fprintf(stderr, "%s", process_out->buf);
>
> maybe print this with split lines prefixed with something so wour tests
> can see that something actually happened here, & test-cmp it so we can
> see what went where, as opposed to...
>
> > +test_expect_success 'run_command runs in parallel with more jobs available than tasks --pipe-output' '
> > +     test-tool run-command --pipe-output run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
> > +     test_must_be_empty out &&
> > +     test_line_count = 20 err
> > +'
>
> Just checking the number of lines, which seems to leave a lot of leeway
> for the output being mixed up in all sorts of ways & the test to still
> pass..
>
> (ditto below)

ack.

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Junio C Hamano]

Calvin Wan <calvinwan@google.com> writes:

>> In my review of one of the previous rounds, I asked which part of
>> this functionality fits the name "pipe", and I do not think I got a
>> satisfactory answer.  And after re-reading the patch in this round,
>> with the in-header comments, it still is not clear to me.
>>
>> It looks more like sending the duplicate of the normal output to a
>> side channel, somewhat like the "tee" utility, but I am not sure if
>> that is the intended way to be used.
>>
>
> In this case, I was hoping "pipe" would refer to the redirection of
> output from the child processes to a separate custom function, but
> I can see that duplication != redirection. Maybe something like
> "parse_child_output" or "parse_output" would make sense, however,
> I didn't want to imply with that name that the only functionality is to
> parse output. Besides that, I don't really have any other ideas of
> what I can name it...

Yeah, parsing is not to the point.  Sending a copy of output to
elsewhere is, so redirect is a better word than parse.  And piping
is not the only form of redirection, either.  If duplication is
really the point, then either giving it a name with a word that
signals "duplication" would make more sense.  "send_copy_fn"?  I am
not good at naming.

As a name that is not end-user facing, it is tempting to assume that
the readers have basic knowledge of Unix concepts and call it
"tee_fn", but it would be way too cryptic to uninitiated, so I would
not recommend it.

Hmm...

Re: [PATCH v3 2/6] run-command: add hide_output to run_processes_parallel_opts

[Calvin Wan]

> I may just be missing something, but doesn't "struct child_process"
> already have e.g. "no_stderr", "no_stdout" etc. that we can use?
> I.e. isn't this thing equivalent to running:
>
>         your-command >/dev/null 2>/dev/null
>
> Which is what the non-parallel API already supports.
>
> Now, IIRC if you just set that in the "get_next_task" callback it won't
> work in the parallel API, or you'll block waiting for I/O that'll never
> come or whatever.
>
> But that'll be because the parallel interface currently only suppors a
> subset of the full "child_process" combination of options, and maybe it
> doesn't grok this.
>
> But if that's the case we should just extend the API to support
> "no_stdout", "no_stderr" etc., no?
>
> I.e. hypothetically the parallel one could support 100% of the "struct
> child_process" combination of options, we just haven't bothered yet.
>
> But I don't see why the parallel API should grow options that we already
> have in "struct child_process", instead we should set them there, and it
> should gradually learn to deal with them.
>
> I think it's also fine to have some basic sanity checks there, e.g. I
> could see how for something like this we don't want to support piping
> only some children to /dev/null but not others, and that it should be
> all or nothing (maybe it makes state management when we loop over them
> easier).
>
> Or again, maybe I'm missing something...

Shouldn't the options that are set in "child_process" be abstracted away
from "parallel_processes"? Setting "no_stdout", "no_stderr", etc. in a
"child_process" shouldn't imply that we still pass the stdout and stderr to
 "parallel_processes" and then we send the output to "/dev/null".

That being said, I can understand the aversion to adding an option like
this that doesn't also add support for stdout and stderr. I can remove this
patch and instead reset the buffer inside of pipe_output and task_finished
in a later patch

Re: [PATCH v3 1/6] run-command: add pipe_output_fn to run_processes_parallel_opts

[Calvin Wan]

On Mon, Oct 24, 2022 at 12:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Calvin Wan <calvinwan@google.com> writes:
>
> >> In my review of one of the previous rounds, I asked which part of
> >> this functionality fits the name "pipe", and I do not think I got a
> >> satisfactory answer.  And after re-reading the patch in this round,
> >> with the in-header comments, it still is not clear to me.
> >>
> >> It looks more like sending the duplicate of the normal output to a
> >> side channel, somewhat like the "tee" utility, but I am not sure if
> >> that is the intended way to be used.
> >>
> >
> > In this case, I was hoping "pipe" would refer to the redirection of
> > output from the child processes to a separate custom function, but
> > I can see that duplication != redirection. Maybe something like
> > "parse_child_output" or "parse_output" would make sense, however,
> > I didn't want to imply with that name that the only functionality is to
> > parse output. Besides that, I don't really have any other ideas of
> > what I can name it...
>
> Yeah, parsing is not to the point.  Sending a copy of output to
> elsewhere is, so redirect is a better word than parse.  And piping
> is not the only form of redirection, either.  If duplication is
> really the point, then either giving it a name with a word that
> signals "duplication" would make more sense.  "send_copy_fn"?  I am
> not good at naming.
>
> As a name that is not end-user facing, it is tempting to assume that
> the readers have basic knowledge of Unix concepts and call it
> "tee_fn", but it would be way too cryptic to uninitiated, so I would
> not recommend it.
>
> Hmm...

Throwing some more ideas out there:
split_duplicate_fn
duplicate_output_fn
dup_output_fn

As you mention, it's not end-user facing so we should pick a name
that's close enough (and any confusion can always be resolved by
comments)

Re: [PATCH v3 2/6] run-command: add hide_output to run_processes_parallel_opts

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 24 2022, Calvin Wan wrote:

>> I may just be missing something, but doesn't "struct child_process"
>> already have e.g. "no_stderr", "no_stdout" etc. that we can use?
>> I.e. isn't this thing equivalent to running:
>>
>>         your-command >/dev/null 2>/dev/null
>>
>> Which is what the non-parallel API already supports.
>>
>> Now, IIRC if you just set that in the "get_next_task" callback it won't
>> work in the parallel API, or you'll block waiting for I/O that'll never
>> come or whatever.
>>
>> But that'll be because the parallel interface currently only suppors a
>> subset of the full "child_process" combination of options, and maybe it
>> doesn't grok this.
>>
>> But if that's the case we should just extend the API to support
>> "no_stdout", "no_stderr" etc., no?
>>
>> I.e. hypothetically the parallel one could support 100% of the "struct
>> child_process" combination of options, we just haven't bothered yet.
>>
>> But I don't see why the parallel API should grow options that we already
>> have in "struct child_process", instead we should set them there, and it
>> should gradually learn to deal with them.
>>
>> I think it's also fine to have some basic sanity checks there, e.g. I
>> could see how for something like this we don't want to support piping
>> only some children to /dev/null but not others, and that it should be
>> all or nothing (maybe it makes state management when we loop over them
>> easier).
>>
>> Or again, maybe I'm missing something...
>
> Shouldn't the options that are set in "child_process" be abstracted away
> from "parallel_processes"?

In general yes, and no :)

Our main interafce should probably be "just set
these in the 'struct child_process' we hand you", but the parallel API
might want to assert certain things about those settings, as some of
them may conflict with its assumptions.

> Setting "no_stdout", "no_stderr", etc. in a
> "child_process" shouldn't imply that we still pass the stdout and stderr to
>  "parallel_processes" and then we send the output to "/dev/null".

Sure, but if they're not producing any output because it's being piped
to /dev/null how worthwhile is it to optimize that?

We still can optimize it, but I still think the interface should just be
the equivalent of:

	parallel -k -j100% 'sleep 0.0$RANDOM && echo {} >/dev/null' ::: {1..100}

Whereas what you seem to be trying to implement is the equivalent of a:

	parallel -u -j100% 'sleep 0.0$RANDOM && echo {} ::: {1..100} >/dev/null

Except as an option to the parallel API, but the end result seems to be
equivalent.

> That being said, I can understand the aversion to adding an option like
> this that doesn't also add support for stdout and stderr. I can remove this
> patch and instead reset the buffer inside of pipe_output and task_finished
> in a later patch

I'm not necessarily opposed to it, just puzzled about it, maybe I don't
have the full picture.

In general I highly recomend looking at whatever GNU parallel is doing,
and seeing if new features in run-command.[ch] can map to that mental
model.

Our API is basically a small subset of its featureset, and I've found it
useful both to steal ideas from there, and to test
assumptions. E.g. "ungroup" is just a straight rip-off of the
"--ungroup" option, it's also had to think about combining various
options we don't have yet (but might want).

In that case the supervisor API/parallel(1) needs to do something
special, but for "I don't want output" it seems best to just do that at
the worker level, i.e. equivalent to piping to /dev/null.

Having a bias towards that approach also makes it easier to convert
things to running in parallel, i.e. you just (mostly) keep your current
"struct child_process", and don't need to find the equivalents in the
parallel API.

Re: [PATCH v3 2/6] run-command: add hide_output to run_processes_parallel_opts

[Calvin Wan]

> > Setting "no_stdout", "no_stderr", etc. in a
> > "child_process" shouldn't imply that we still pass the stdout and stderr to
> >  "parallel_processes" and then we send the output to "/dev/null".
>
> Sure, but if they're not producing any output because it's being piped
> to /dev/null how worthwhile is it to optimize that?
>
> We still can optimize it, but I still think the interface should just be
> the equivalent of:
>
>         parallel -k -j100% 'sleep 0.0$RANDOM && echo {} >/dev/null' ::: {1..100}
>
> Whereas what you seem to be trying to implement is the equivalent of a:
>
>         parallel -u -j100% 'sleep 0.0$RANDOM && echo {} ::: {1..100} >/dev/null
>
> Except as an option to the parallel API, but the end result seems to be
> equivalent.
>
> > That being said, I can understand the aversion to adding an option like
> > this that doesn't also add support for stdout and stderr. I can remove this
> > patch and instead reset the buffer inside of pipe_output and task_finished
> > in a later patch
>
> I'm not necessarily opposed to it, just puzzled about it, maybe I don't
> have the full picture.
>
> In general I highly recomend looking at whatever GNU parallel is doing,
> and seeing if new features in run-command.[ch] can map to that mental
> model.
>
> Our API is basically a small subset of its featureset, and I've found it
> useful both to steal ideas from there, and to test
> assumptions. E.g. "ungroup" is just a straight rip-off of the
> "--ungroup" option, it's also had to think about combining various
> options we don't have yet (but might want).
>
> In that case the supervisor API/parallel(1) needs to do something
> special, but for "I don't want output" it seems best to just do that at
> the worker level, i.e. equivalent to piping to /dev/null.

Well I want the output to be able to parse it, but not to print it. Piping
to /dev/null at the worker level denies me the ability to parse it in the
parent process.

Am I understanding correctly that what you're suggesting is if a child
process has "no_stderr" and "no_stdout" set to true, then
parallel_processes would temporarily set them to false before
start_command, and then honor it later after the output is read?
This would allow me to call pipe_output and parse it before sending
the output to /dev/null without the need for "hide_output"

Re: [PATCH v3 6/6] diff-lib: parallelize run_diff_files for submodules

[Calvin Wan]

> And, I may be missing something, but later in that function we do:
>
>         if (repo_read_index(r) < 0)
>                 die(_("index file corrupt"));
>
> Do we need to read the index there if we're just invoking a "status"
> command, isn't it reading it for us & reporting back?

You're correct I don't need to read the index there

> > +static struct status_task *
> > +get_status_task_from_index(struct submodule_parallel_status *sps,
> > +                        struct strbuf *err)
> > +{
> > +     for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
> > +             struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
> > +             const struct cache_entry *ce = util->ce;
> > +             struct status_task *task;
> > +             struct strbuf buf = STRBUF_INIT;
> > +             const char *git_dir;
> > +
> > +             strbuf_addf(&buf, "%s/.git", ce->name);
> > +             git_dir = read_gitfile(buf.buf);
>
> Okey, so the "NULL" variant of read_gitfile_gently(), as we don't care
> about the sort of error we got, but...
>
> > +             if (!git_dir)
> > +                     git_dir = buf.buf;
> > +             if (!is_git_directory(git_dir)) {
>
> Isn't this something we could have asked read_gitfile_gently() about
> instead, i.e. the READ_GITFILE_ERR_NOT_A_REPO condition?
> > +                     if (is_directory(git_dir))
> > +                             die(_("'%s' not recognized as a git repository"), git_dir);
>
> It would be a detour from this, but perhaps setup.c can be tasked with
> knowing about this edge case and returning an error code, but even if we
> punt on that we can just do the is_directory() here, but get the
> !is_git_directory() for free it seems.

So there are two non-fatal errors in read_gitfile_gently() that return
NULL rather than dieing:
READ_GITFILE_ERR_STAT_FAILED
READ_GITFILE_ERR_NOT_A_FILE

Then the edge case becomes:
Not a git file (or stat failed)
Not a git directory
Is a directory

I'm not seeing how we would get !is_git_directory() for free.

> > +             task->path = ce->name;
> > +             task->dirty_submodule = util->dirty_submodule;
> > +             task->ignore_untracked = util->ignore_untracked;
>
> Cn do we the same readability trick here that we did with "struct
> submodule_status_util tmp = {" earlier & the memcpy()? Looks like it...

Yes, if I make the readability change, then I should still be able to
use xmalloc

> > +
> > +     child_process_init(cp);
> > +     prepare_submodule_repo_env_in_gitdir(&cp->env);
> > +
> > +     strvec_init(&cp->args);
> > +     strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
> > +     if (task->ignore_untracked)
> > +             strvec_push(&cp->args, "-uno");
>
> Nit: Maybe worth spelling out --untracked-files=no (or maybe "-uno" is
> more idiomatic at this point...)

Same spelling as in is_submodule_modified(). Probably not worth
changing /shrug.

> > +int get_submodules_status(struct repository *r,
> > +                       struct string_list *submodules,
> > +                       int max_parallel_jobs);
>
> s/int/size_t/ because at this point you've already die()'d in the one
> caller if it's <0 from the config parser, so we know it's unsigned
> (actually >1, but unsigned will have to do :).

The caller is passing in an int anyways so I'm going to keep it as is
for consistency.

Took the rest of your suggestions. Thanks!