[PATCHv3 00/13] fetch submodules in parallel and a preview on parallel "submodule update"

[PATCHv3 00/13] fetch submodules in parallel and a preview on parallel "submodule update"

[Stefan Beller]

This build on top of origin/sb/submodule-helper

Patches 1-8 are parallelising fetching submodules.
Thanks Eric, Jeff and Junio for the discussion and review!
Based on your feedback:

Patch 2 is optional and may be spun out as its own mini series.
Instead of spinning we can poll until data are ready on the fd.

Patch 3,4 have been revamped to include your suggestions, having 
xread_nonblock and strbuf_read_once as their own functions not
to be integrated into other existing functions.

Patch 5 was fixed to return 0 or -1 in the error case.
Patch 6 has major syntactically changes such as no slots any more,
but in_use flags. The return code was flipped (1 means there is a child
to be executed, while 0 means we're done).

Patch 7,8 convert git fetch --recuse-submodules to use the fruit of patch 6.

Patches 9 an onwards are RFC, so I could spin them out as their
own series as they convert parts of "submodule update" to C using
the new asynchronous run command code.
Patch 9 adds the update strategy to the submodule cache
Patches 10,11,12 are just moving code around to make cmd_update leaner.
Patch 13 shows how I plan to parallelize updating the submodules for now.

Any feedback welcome,
Thanks,
Stefan

Jonathan Nieder (1):
  Sending "Fetching submodule <foo>" output to stderr

Stefan Beller (13):
  xread: poll on non blocking fds
  xread_nonblock: add functionality to read from fds nonblockingly
  strbuf: add strbuf_read_once to read without blocking
  run-command: factor out return value computation
  run-command: add an asynchronous parallel child processor
  fetch_populated_submodules: use new parallel job processing
  submodules: allow parallel fetching, add tests and documentation
  submodule config: keep update strategy around
  git submodule update: pass --prefix only with a non empty prefix
  git submodule update: cmd_update_recursive
  git submodule update: cmd_update_clone
  git submodule update: cmd_update_fetch
  Rewrite submodule update in C

 Documentation/fetch-options.txt |   7 +
 builtin/fetch.c                 |   6 +-
 builtin/pull.c                  |   6 +
 builtin/submodule--helper.c     | 247 +++++++++++++++++++++++++++++
 git-compat-util.h               |   1 +
 git-submodule.sh                | 339 ++++++++++++++--------------------------
 run-command.c                   | 329 +++++++++++++++++++++++++++++++++++---
 run-command.h                   |  38 +++++
 strbuf.c                        |  11 ++
 strbuf.h                        |   6 +
 submodule-config.c              |  16 ++
 submodule-config.h              |   1 +
 submodule.c                     | 119 +++++++++-----
 submodule.h                     |   2 +-
 t/t0061-run-command.sh          |  20 +++
 t/t5526-fetch-submodules.sh     |  70 ++++++---
 test-run-command.c              |  24 +++
 wrapper.c                       |  41 ++++-
 18 files changed, 972 insertions(+), 311 deletions(-)

-- 
2.5.0.275.ge015d2a

[PATCHv3 01/13] Sending "Fetching submodule <foo>" output to stderr

[Stefan Beller]

From: Jonathan Nieder <jrnieder@gmail.com>

The "Pushing submodule <foo>" progress output correctly goes to
stderr, but "Fetching submodule <foo>" is going to stdout by mistake.
Fix it to write to stderr.

Noticed while trying to implement a parallel submodule fetch.  When
this particular output line went to a different file descriptor, it
was buffered separately, resulting in wrongly interleaved output if
we copied it to the terminal naively.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 |  2 +-
 t/t5526-fetch-submodules.sh | 51 +++++++++++++++++++++++----------------------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9fcc86f..1d64e57 100644
--- a/submodule.c
+++ b/submodule.c
@@ -694,7 +694,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 			git_dir = submodule_git_dir.buf;
 		if (is_directory(git_dir)) {
 			if (!quiet)
-				printf("Fetching submodule %s%s\n", prefix, ce->name);
+				fprintf(stderr, "Fetching submodule %s%s\n", prefix, ce->name);
 			cp.dir = submodule_path.buf;
 			argv_array_push(&argv, default_argv);
 			argv_array_push(&argv, "--submodule-prefix");
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index a4532b0..17759b1 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -16,7 +16,8 @@ add_upstream_commit() {
 		git add subfile &&
 		git commit -m new subfile &&
 		head2=$(git rev-parse --short HEAD) &&
-		echo "From $pwd/submodule" > ../expect.err &&
+		echo "Fetching submodule submodule" > ../expect.err &&
+		echo "From $pwd/submodule" >> ../expect.err &&
 		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err
 	) &&
 	(
@@ -27,6 +28,7 @@ add_upstream_commit() {
 		git add deepsubfile &&
 		git commit -m new deepsubfile &&
 		head2=$(git rev-parse --short HEAD) &&
+		echo "Fetching submodule submodule/subdir/deepsubmodule" >> ../expect.err
 		echo "From $pwd/deepsubmodule" >> ../expect.err &&
 		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err
 	)
@@ -56,9 +58,7 @@ test_expect_success setup '
 	(
 		cd downstream &&
 		git submodule update --init --recursive
-	) &&
-	echo "Fetching submodule submodule" > expect.out &&
-	echo "Fetching submodule submodule/subdir/deepsubmodule" >> expect.out
+	)
 '
 
 test_expect_success "fetch --recurse-submodules recurses into submodules" '
@@ -67,7 +67,7 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -96,7 +96,7 @@ test_expect_success "using fetchRecurseSubmodules=true in .gitmodules recurses i
 		git config -f .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
 		git fetch >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -127,7 +127,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 		git config --unset -f .gitmodules submodule.submodule.fetchRecurseSubmodules &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -146,7 +146,7 @@ test_expect_success "--dry-run propagates to submodules" '
 		cd downstream &&
 		git fetch --recurse-submodules --dry-run >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -155,7 +155,7 @@ test_expect_success "Without --dry-run propagates to submodules" '
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -166,7 +166,7 @@ test_expect_success "recurseSubmodules=true propagates into submodules" '
 		git config fetch.recurseSubmodules true
 		git fetch >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -180,7 +180,7 @@ test_expect_success "--recurse-submodules overrides config in submodule" '
 		) &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -214,16 +214,15 @@ test_expect_success "Recursion stops when no new submodule commits are fetched"
 	git add submodule &&
 	git commit -m "new submodule" &&
 	head2=$(git rev-parse --short HEAD) &&
-	echo "Fetching submodule submodule" > expect.out.sub &&
 	echo "From $pwd/." > expect.err.sub &&
 	echo "   $head1..$head2  master     -> origin/master" >>expect.err.sub &&
-	head -2 expect.err >> expect.err.sub &&
+	head -3 expect.err >> expect.err.sub &&
 	(
 		cd downstream &&
 		git fetch >../actual.out 2>../actual.err
 	) &&
 	test_i18ncmp expect.err.sub actual.err &&
-	test_i18ncmp expect.out.sub actual.out
+	test_must_be_empty actual.out
 '
 
 test_expect_success "Recursion doesn't happen when new superproject commits don't change any submodules" '
@@ -269,7 +268,7 @@ test_expect_success "Recursion picks up config in submodule" '
 		)
 	) &&
 	test_i18ncmp expect.err.sub actual.err &&
-	test_i18ncmp expect.out actual.out
+	test_must_be_empty actual.out
 '
 
 test_expect_success "Recursion picks up all submodules when necessary" '
@@ -285,7 +284,8 @@ test_expect_success "Recursion picks up all submodules when necessary" '
 		git add subdir/deepsubmodule &&
 		git commit -m "new deepsubmodule"
 		head2=$(git rev-parse --short HEAD) &&
-		echo "From $pwd/submodule" > ../expect.err.sub &&
+		echo "Fetching submodule submodule" > ../expect.err.sub &&
+		echo "From $pwd/submodule" >> ../expect.err.sub &&
 		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err.sub
 	) &&
 	head1=$(git rev-parse --short HEAD) &&
@@ -295,13 +295,13 @@ test_expect_success "Recursion picks up all submodules when necessary" '
 	echo "From $pwd/." > expect.err.2 &&
 	echo "   $head1..$head2  master     -> origin/master" >> expect.err.2 &&
 	cat expect.err.sub >> expect.err.2 &&
-	tail -2 expect.err >> expect.err.2 &&
+	tail -3 expect.err >> expect.err.2 &&
 	(
 		cd downstream &&
 		git fetch >../actual.out 2>../actual.err
 	) &&
 	test_i18ncmp expect.err.2 actual.err &&
-	test_i18ncmp expect.out actual.out
+	test_must_be_empty actual.out
 '
 
 test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no new commits are fetched in the superproject (and ignores config)" '
@@ -317,7 +317,8 @@ test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no ne
 		git add subdir/deepsubmodule &&
 		git commit -m "new deepsubmodule" &&
 		head2=$(git rev-parse --short HEAD) &&
-		echo "From $pwd/submodule" > ../expect.err.sub &&
+		echo Fetching submodule submodule > ../expect.err.sub &&
+		echo "From $pwd/submodule" >> ../expect.err.sub &&
 		echo "   $head1..$head2  master     -> origin/master" >> ../expect.err.sub
 	) &&
 	(
@@ -335,7 +336,7 @@ test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
 	git add submodule &&
 	git commit -m "new submodule" &&
 	head2=$(git rev-parse --short HEAD) &&
-	tail -2 expect.err > expect.err.deepsub &&
+	tail -3 expect.err > expect.err.deepsub &&
 	echo "From $pwd/." > expect.err &&
 	echo "   $head1..$head2  master     -> origin/master" >>expect.err &&
 	cat expect.err.sub >> expect.err &&
@@ -354,7 +355,7 @@ test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
 			git config --unset -f .gitmodules submodule.subdir/deepsubmodule.fetchRecursive
 		)
 	) &&
-	test_i18ncmp expect.out actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err
 '
 
@@ -388,7 +389,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 	head2=$(git rev-parse --short HEAD) &&
 	echo "From $pwd/." > expect.err.2 &&
 	echo "   $head1..$head2  master     -> origin/master" >>expect.err.2 &&
-	head -2 expect.err >> expect.err.2 &&
+	head -3 expect.err >> expect.err.2 &&
 	(
 		cd downstream &&
 		git config fetch.recurseSubmodules on-demand &&
@@ -399,7 +400,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 		cd downstream &&
 		git config --unset fetch.recurseSubmodules
 	) &&
-	test_i18ncmp expect.out.sub actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err.2 actual.err
 '
 
@@ -416,7 +417,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 	head2=$(git rev-parse --short HEAD) &&
 	echo "From $pwd/." > expect.err.2 &&
 	echo "   $head1..$head2  master     -> origin/master" >>expect.err.2 &&
-	head -2 expect.err >> expect.err.2 &&
+	head -3 expect.err >> expect.err.2 &&
 	(
 		cd downstream &&
 		git config submodule.submodule.fetchRecurseSubmodules on-demand &&
@@ -427,7 +428,7 @@ test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 		cd downstream &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
-	test_i18ncmp expect.out.sub actual.out &&
+	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err.2 actual.err
 '
 
-- 
2.5.0.275.ge015d2a

[PATCHv3 02/13] xread: poll on non blocking fds

[Stefan Beller]

>From the man page:
EAGAIN The file descriptor fd refers to a file other than a socket
       and has been marked nonblocking (O_NONBLOCK), and the read
       would block.

EAGAIN or EWOULDBLOCK
       The file descriptor fd refers to a socket and has been marked
       nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001
       allows either error to be returned for this case, and does not
       require these constants to have the same value, so a portable
       application should check for both possibilities.

So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.
As the intend of xread is to read as much as possible either until the
fd is EOF or an actual error occurs, we can ease the feeder of the fd
by not spinning the whole time, but rather wait for it politely by not
busy waiting.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 wrapper.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index ff49807..50267a4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -201,8 +201,23 @@ ssize_t xread(int fd, void *buf, size_t len)
 	    len = MAX_IO_SIZE;
 	while (1) {
 		nr = read(fd, buf, len);
-		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
-			continue;
+		if (nr < 0) {
+			if (errno == EINTR)
+				continue;
+			if (errno == EAGAIN || errno == EWOULDBLOCK) {
+				struct pollfd pfd;
+				int i;
+				pfd.events = POLLIN;
+				pfd.fd = fd;
+				i = poll(&pfd, 1, 100);
+				if (i < 0) {
+					if (errno == EINTR || errno == ENOMEM)
+						continue;
+					else
+						die_errno("poll");
+				}
+			}
+		}
 		return nr;
 	}
 }
-- 
2.5.0.275.ge015d2a

[PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Stefan Beller]

This wrapper just restarts on EINTR, but not on EAGAIN like xread
does. This gives less guarantees on the actual reading rather than
on returning fast.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-compat-util.h |  1 +
 wrapper.c         | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index c6d391f..9ccea85 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 extern int xdup(int fd);
diff --git a/wrapper.c b/wrapper.c
index 50267a4..54ce231 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -223,6 +223,28 @@ ssize_t xread(int fd, void *buf, size_t len)
 }
 
 /*
+ * xread_nonblock() is the same a read(), but it automatically restarts read()
+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
+ * "len" bytes is read even if the data is available.
+ */
+ssize_t xread_nonblock(int fd, void *buf, size_t len)
+{
+	ssize_t nr;
+	if (len > MAX_IO_SIZE)
+	    len = MAX_IO_SIZE;
+	while (1) {
+		nr = read(fd, buf, len);
+		if (nr < 0) {
+			if (errno == EINTR)
+				continue;
+			if (errno == EWOULDBLOCK)
+				errno = EAGAIN;
+		}
+		return nr;
+	}
+}
+
+/*
  * xwrite() is the same a write(), but it automatically restarts write()
  * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
  * GUARANTEE that "len" bytes is written even if the operation is successful.
-- 
2.5.0.275.ge015d2a

[PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking

[Stefan Beller]

The new call will read a fd into a strbuf once. The underlying call
xread_nonblock is meant to execute non blockingly if the fd is set to
O_NONBLOCK.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 strbuf.c | 11 +++++++++++
 strbuf.h |  6 ++++++
 2 files changed, 17 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index cce5eed..35e71b8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 	return sb->len - oldlen;
 }
 
+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
+{
+	ssize_t cnt;
+
+	strbuf_grow(sb, hint ? hint : 8192);
+	cnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
+	if (cnt > 0)
+		strbuf_setlen(sb, sb->len + cnt);
+	return cnt;
+}
+
 #define STRBUF_MAXLINK (2*PATH_MAX)
 
 int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
diff --git a/strbuf.h b/strbuf.h
index aef2794..4d4e5b1 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -367,6 +367,12 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 
 /**
+ * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.
+ * The fd must have set O_NONBLOCK.
+ */
+extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
+
+/**
  * Read the contents of a file, specified by its path. The third argument
  * can be used to give a hint about the file size, to avoid reallocs.
  */
-- 
2.5.0.275.ge015d2a

[PATCHv3 05/13] run-command: factor out return value computation

[Stefan Beller]

We will need computing the return value in a later patch without the
wait.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/run-command.c b/run-command.c
index 28e1d55..674e348 100644
--- a/run-command.c
+++ b/run-command.c
@@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
+static int determine_return_value(int wait_status,
+				  int *result,
+				  int *error_code,
+				  const char *argv0)
+{
+	if (WIFSIGNALED(wait_status)) {
+		*result = WTERMSIG(wait_status);
+		if (*result != SIGINT && *result != SIGQUIT)
+			error("%s died of signal %d", argv0, *result);
+		/*
+		 * This return value is chosen so that code & 0xff
+		 * mimics the exit code that a POSIX shell would report for
+		 * a program that died from this signal.
+		 */
+		*result += 128;
+	} else if (WIFEXITED(wait_status)) {
+		*result = WEXITSTATUS(wait_status);
+		/*
+		 * Convert special exit code when execvp failed.
+		 */
+		if (*result == 127) {
+			*result = -1;
+			*error_code = ENOENT;
+		}
+	} else
+		return -1;
+	return 0;
+}
+
 static int wait_or_whine(pid_t pid, const char *argv0)
 {
 	int status, code = -1;
@@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 	if (waiting < 0) {
 		failed_errno = errno;
 		error("waitpid for %s failed: %s", argv0, strerror(errno));
-	} else if (waiting != pid) {
-		error("waitpid is confused (%s)", argv0);
-	} else if (WIFSIGNALED(status)) {
-		code = WTERMSIG(status);
-		if (code != SIGINT && code != SIGQUIT)
-			error("%s died of signal %d", argv0, code);
-		/*
-		 * This return value is chosen so that code & 0xff
-		 * mimics the exit code that a POSIX shell would report for
-		 * a program that died from this signal.
-		 */
-		code += 128;
-	} else if (WIFEXITED(status)) {
-		code = WEXITSTATUS(status);
-		/*
-		 * Convert special exit code when execvp failed.
-		 */
-		if (code == 127) {
-			code = -1;
-			failed_errno = ENOENT;
-		}
 	} else {
-		error("waitpid is confused (%s)", argv0);
+		if (waiting != pid
+		   || (determine_return_value(status, &code, &failed_errno, argv0) < 0))
+			error("waitpid is confused (%s)", argv0);
 	}
 
 	clear_child_for_cleanup(pid);
-- 
2.5.0.275.ge015d2a

[PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Stefan Beller]

This allows to run external commands in parallel with ordered output
on stderr.

If we run external commands in parallel we cannot pipe the output directly
to the our stdout/err as it would mix up. So each process's output will
flow through a pipe, which we buffer. One subprocess can be directly
piped to out stdout/err for a low latency feedback to the user.

Example:
Let's assume we have 5 submodules A,B,C,D,E and each fetch takes a
different amount of time as the different submodules vary in size, then
the output of fetches in sequential order might look like this:

 time -->
 output: |---A---| |-B-| |-------C-------| |-D-| |-E-|

When we schedule these submodules into maximal two parallel processes,
a schedule and sample output over time may look like this:

process 1: |---A---| |-D-| |-E-|

process 2: |-B-| |-------C-------|

output:    |---A---|B|---C-------|DE

So A will be perceived as it would run normally in the single child
version. As B has finished by the time A is done, we can dump its whole
progress buffer on stderr, such that it looks like it finished in no
time. Once that is done, C is determined to be the visible child and
its progress will be reported in real time.

So this way of output is really good for human consumption, as it only
changes the timing, not the actual output.

For machine consumption the output needs to be prepared in the tasks,
by either having a prefix per line or per block to indicate whose tasks
output is displayed, because the output order may not follow the
original sequential ordering:

 |----A----| |--B--| |-C-|

will be scheduled to be all parallel:

process 1: |----A----|
process 2: |--B--|
process 3: |-C-|
output:    |----A----|CB

This happens because C finished before B did, so it will be queued for
output before B.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c          | 275 +++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h          |  36 +++++++
 t/t0061-run-command.sh |  20 ++++
 test-run-command.c     |  24 +++++
 4 files changed, 355 insertions(+)

diff --git a/run-command.c b/run-command.c
index 674e348..06d5a5d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -3,6 +3,8 @@
 #include "exec_cmd.h"
 #include "sigchain.h"
 #include "argv-array.h"
+#include "thread-utils.h"
+#include "strbuf.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -862,3 +864,276 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
 	close(cmd->out);
 	return finish_command(cmd);
 }
+
+struct parallel_processes {
+	void *data;
+
+	int max_processes;
+	int nr_processes;
+	unsigned all_tasks_started : 1;
+
+	get_next_task_fn get_next_task;
+	start_failure_fn start_failure;
+	return_value_fn return_value;
+
+	struct {
+		unsigned in_use : 1;
+		struct child_process process;
+		struct strbuf err;
+	} *children;
+	/*
+	 * The struct pollfd is logically part of *children,
+	 * but the system call expects it as its own array.
+	 */
+	struct pollfd *pfd;
+
+	int output_owner;
+	struct strbuf buffered_output; /* of finished children */
+};
+
+void default_start_failure(void *data,
+			   struct child_process *cp,
+			   struct strbuf *err)
+{
+	int i;
+	struct strbuf sb = STRBUF_INIT;
+
+	for (i = 0; cp->argv[i]; i++)
+		strbuf_addf(&sb, "%s ", cp->argv[i]);
+
+	die_errno("Starting a child failed:\n%s", sb.buf);
+}
+
+void default_return_value(void *data,
+			  struct child_process *cp,
+			  int result)
+{
+	int i;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!result)
+		return;
+
+	for (i = 0; cp->argv[i]; i++)
+		strbuf_addf(&sb, "%s ", cp->argv[i]);
+
+	die_errno("A child failed with return code:\n%s\n%d", sb.buf, result);
+}
+
+static void run_processes_parallel_init(struct parallel_processes *pp,
+					int n, void *data,
+					get_next_task_fn get_next_task,
+					start_failure_fn start_failure,
+					return_value_fn return_value)
+{
+	int i;
+
+	if (n < 1)
+		n = online_cpus();
+
+	pp->max_processes = n;
+	pp->data = data;
+	if (!get_next_task)
+		die("BUG: you need to specify a get_next_task function");
+	pp->get_next_task = get_next_task;
+
+	pp->start_failure = start_failure ? start_failure : default_start_failure;
+	pp->return_value = return_value ? return_value : default_return_value;
+
+	pp->nr_processes = 0;
+	pp->all_tasks_started = 0;
+	pp->output_owner = 0;
+	pp->children = xcalloc(n, sizeof(*pp->children));
+	pp->pfd = xcalloc(n, sizeof(*pp->pfd));
+	strbuf_init(&pp->buffered_output, 0);
+
+	for (i = 0; i < n; i++) {
+		strbuf_init(&pp->children[i].err, 0);
+		pp->pfd[i].events = POLLIN;
+		pp->pfd[i].fd = -1;
+	}
+}
+
+static void run_processes_parallel_cleanup(struct parallel_processes *pp)
+{
+	int i;
+	for (i = 0; i < pp->max_processes; i++)
+		strbuf_release(&pp->children[i].err);
+
+	free(pp->children);
+	free(pp->pfd);
+	strbuf_release(&pp->buffered_output);
+}
+
+static void set_nonblocking(int fd)
+{
+	int flags = fcntl(fd, F_GETFL);
+	if (flags < 0)
+		warning("Could not get file status flags, "
+			"output will be degraded");
+	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
+		warning("Could not set file status flags, "
+			"output will be degraded");
+}
+
+static void run_processes_parallel_start_one(struct parallel_processes *pp)
+{
+	int i;
+
+	for (i = 0; i < pp->max_processes; i++)
+		if (!pp->children[i].in_use)
+			break;
+	if (i == pp->max_processes)
+		die("BUG: bookkeeping is hard");
+
+	if (!pp->get_next_task(pp->data,
+			       &pp->children[i].process,
+			       &pp->children[i].err)) {
+		pp->all_tasks_started = 1;
+		return;
+	}
+	if (start_command(&pp->children[i].process))
+		pp->start_failure(pp->data,
+				  &pp->children[i].process,
+				  &pp->children[i].err);
+
+	set_nonblocking(pp->children[i].process.err);
+
+	pp->nr_processes++;
+	pp->children[i].in_use = 1;
+	pp->pfd[i].fd = pp->children[i].process.err;
+}
+
+static void run_processes_parallel_start_as_needed(struct parallel_processes *pp)
+{
+	while (pp->nr_processes < pp->max_processes &&
+	       !pp->all_tasks_started)
+		run_processes_parallel_start_one(pp);
+}
+
+static void run_processes_parallel_buffer_stderr(struct parallel_processes *pp)
+{
+	int i;
+
+	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
+		if (errno == EINTR)
+			continue;
+		run_processes_parallel_cleanup(pp);
+		die_errno("poll");
+	}
+
+	/* Buffer output from all pipes. */
+	for (i = 0; i < pp->max_processes; i++) {
+		if (pp->children[i].in_use &&
+		    pp->pfd[i].revents & POLLIN)
+			if (strbuf_read_once(&pp->children[i].err,
+					     pp->children[i].process.err, 0) < 0)
+				if (errno != EAGAIN)
+					die_errno("read");
+	}
+}
+
+static void run_processes_parallel_output(struct parallel_processes *pp)
+{
+	int i = pp->output_owner;
+	if (pp->children[i].in_use &&
+	    pp->children[i].err.len) {
+		fputs(pp->children[i].err.buf, stderr);
+		strbuf_reset(&pp->children[i].err);
+	}
+}
+
+static void run_processes_parallel_collect_finished(struct parallel_processes *pp)
+{
+	int i = 0;
+	pid_t pid;
+	int wait_status, code;
+	int n = pp->max_processes;
+
+	while (pp->nr_processes > 0) {
+		pid = waitpid(-1, &wait_status, WNOHANG);
+		if (pid == 0)
+			return;
+
+		if (pid < 0)
+			die_errno("wait");
+
+		for (i = 0; i < pp->max_processes; i++)
+			if (pp->children[i].in_use &&
+			    pid == pp->children[i].process.pid)
+				break;
+		if (i == pp->max_processes)
+			/*
+			 * waitpid returned another process id
+			 * which we are not waiting for.
+			 */
+			return;
+
+		if (strbuf_read_once(&pp->children[i].err,
+				     pp->children[i].process.err, 0) < 0 &&
+		    errno != EAGAIN)
+			die_errno("strbuf_read_once");
+
+
+		if (determine_return_value(wait_status, &code, &errno,
+					   pp->children[i].process.argv[0]) < 0)
+			error("waitpid is confused (%s)",
+			      pp->children[i].process.argv[0]);
+
+		pp->return_value(pp->data, &pp->children[i].process, code);
+
+		argv_array_clear(&pp->children[i].process.args);
+		argv_array_clear(&pp->children[i].process.env_array);
+
+		pp->nr_processes--;
+		pp->children[i].in_use = 0;
+		pp->pfd[i].fd = -1;
+
+		if (i != pp->output_owner) {
+			strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
+			strbuf_reset(&pp->children[i].err);
+		} else {
+			fputs(pp->children[i].err.buf, stderr);
+			strbuf_reset(&pp->children[i].err);
+
+			/* Output all other finished child processes */
+			fputs(pp->buffered_output.buf, stderr);
+			strbuf_reset(&pp->buffered_output);
+
+			/*
+			 * Pick next process to output live.
+			 * NEEDSWORK:
+			 * For now we pick it randomly by doing a round
+			 * robin. Later we may want to pick the one with
+			 * the most output or the longest or shortest
+			 * running process time.
+			 */
+			for (i = 0; i < n; i++)
+				if (pp->children[(pp->output_owner + i) % n].in_use)
+					break;
+			pp->output_owner = (pp->output_owner + i) % n;
+		}
+	}
+}
+
+int run_processes_parallel(int n, void *data,
+			   get_next_task_fn get_next_task,
+			   start_failure_fn start_failure,
+			   return_value_fn return_value)
+{
+	struct parallel_processes pp;
+	run_processes_parallel_init(&pp, n, data,
+				    get_next_task,
+				    start_failure,
+				    return_value);
+
+	while (!pp.all_tasks_started || pp.nr_processes > 0) {
+		run_processes_parallel_start_as_needed(&pp);
+		run_processes_parallel_buffer_stderr(&pp);
+		run_processes_parallel_output(&pp);
+		run_processes_parallel_collect_finished(&pp);
+	}
+	run_processes_parallel_cleanup(&pp);
+
+	return 0;
+}
diff --git a/run-command.h b/run-command.h
index 5b4425a..3807fd1 100644
--- a/run-command.h
+++ b/run-command.h
@@ -119,4 +119,40 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 
+/**
+ * This callback should initialize the child process and preload the
+ * error channel. The preloading of is useful if you want to have a message
+ * printed directly before the output of the child process.
+ * You MUST set stdout_to_stderr.
+ *
+ * Return 1 if the next child is ready to run.
+ * Return 0 if there are no more tasks to be processed.
+ */
+typedef int (*get_next_task_fn)(void *data,
+				struct child_process *cp,
+				struct strbuf *err);
+
+typedef void (*start_failure_fn)(void *data,
+				 struct child_process *cp,
+				 struct strbuf *err);
+
+typedef void (*return_value_fn)(void *data,
+				struct child_process *cp,
+				int result);
+
+/**
+ * Runs up to n processes at the same time. Whenever a process can be
+ * started, the callback `get_next_task` is called to obtain the data
+ * fed to the child process.
+ *
+ * The children started via this function run in parallel and their output
+ * to stderr is buffered, while one of the children will directly output
+ * to stderr.
+ */
+
+int run_processes_parallel(int n, void *data,
+			   get_next_task_fn,
+			   start_failure_fn,
+			   return_value_fn);
+
 #endif
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 9acf628..49aa3db 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -47,4 +47,24 @@ test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' '
 	test_cmp expect actual
 '
 
+cat >expect <<-EOF
+preloaded output of a child
+Hello
+World
+preloaded output of a child
+Hello
+World
+preloaded output of a child
+Hello
+World
+preloaded output of a child
+Hello
+World
+EOF
+
+test_expect_success 'run_command runs in parallel' '
+	test-run-command run-command-parallel-4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-run-command.c b/test-run-command.c
index 89c7de2..94c6eee 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -10,9 +10,29 @@
 
 #include "git-compat-util.h"
 #include "run-command.h"
+#include "argv-array.h"
+#include "strbuf.h"
 #include <string.h>
 #include <errno.h>
 
+static int number_callbacks;
+int parallel_next(void *data,
+		  struct child_process *cp,
+		  struct strbuf *err)
+{
+	struct child_process *d = data;
+	if (number_callbacks >= 4)
+		return 0;
+
+	argv_array_pushv(&cp->args, d->argv);
+	cp->stdout_to_stderr = 1;
+	cp->no_stdin = 1;
+	cp->err = -1;
+	strbuf_addf(err, "preloaded output of a child\n");
+	number_callbacks++;
+	return 1;
+}
+
 int main(int argc, char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
@@ -30,6 +50,10 @@ int main(int argc, char **argv)
 	if (!strcmp(argv[1], "run-command"))
 		exit(run_command(&proc));
 
+	if (!strcmp(argv[1], "run-command-parallel-4"))
+		exit(run_processes_parallel(4, &proc, parallel_next,
+					 NULL, NULL));
+
 	fprintf(stderr, "check usage\n");
 	return 1;
 }
-- 
2.5.0.275.ge015d2a

[PATCHv3 07/13] fetch_populated_submodules: use new parallel job processing

[Stefan Beller]

In a later patch we enable parallel processing of submodules, this
only adds the possibility for it. So this change should not change
any user facing behavior.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/fetch.c |   3 +-
 submodule.c     | 119 +++++++++++++++++++++++++++++++++++++++-----------------
 submodule.h     |   2 +-
 3 files changed, 87 insertions(+), 37 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee1f1a9..6620ed0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		result = fetch_populated_submodules(&options,
 						    submodule_prefix,
 						    recurse_submodules,
-						    verbosity < 0);
+						    verbosity < 0,
+						    0);
 		argv_array_clear(&options);
 	}
 
diff --git a/submodule.c b/submodule.c
index 1d64e57..a0e06e8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -12,6 +12,7 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 #include "blob.h"
+#include "thread-utils.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static struct string_list changed_submodule_paths;
@@ -615,37 +616,79 @@ static void calculate_changed_submodule_paths(void)
 	initialized_fetch_ref_tips = 0;
 }
 
+struct submodule_parallel_fetch {
+	int count;
+	struct argv_array args;
+	const char *work_tree;
+	const char *prefix;
+	int command_line_option;
+	int quiet;
+	int result;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
+
+int get_next_submodule(void *data, struct child_process *cp,
+		       struct strbuf *err);
+
+void handle_submodule_fetch_start_err(void *data, struct child_process *cp, struct strbuf *err)
+{
+	struct submodule_parallel_fetch *spf = data;
+	spf->result = 1;
+}
+
+void handle_submodule_fetch_finish( void *data, struct child_process *cp, int retvalue)
+{
+	struct submodule_parallel_fetch *spf = data;
+
+	if (retvalue)
+		spf->result = 1;
+}
+
 int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
-			       int quiet)
+			       int quiet, int max_parallel_jobs)
 {
-	int i, result = 0;
-	struct child_process cp = CHILD_PROCESS_INIT;
-	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *work_tree = get_git_work_tree();
-	if (!work_tree)
+	int i;
+	struct submodule_parallel_fetch spf = SPF_INIT;
+	spf.work_tree = get_git_work_tree();
+	spf.command_line_option = command_line_option;
+	spf.quiet = quiet;
+	spf.prefix = prefix;
+	if (!spf.work_tree)
 		goto out;
 
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	argv_array_push(&argv, "fetch");
+	argv_array_push(&spf.args, "fetch");
 	for (i = 0; i < options->argc; i++)
-		argv_array_push(&argv, options->argv[i]);
-	argv_array_push(&argv, "--recurse-submodules-default");
+		argv_array_push(&spf.args, options->argv[i]);
+	argv_array_push(&spf.args, "--recurse-submodules-default");
 	/* default value, "--submodule-prefix" and its value are added later */
 
-	cp.env = local_repo_env;
-	cp.git_cmd = 1;
-	cp.no_stdin = 1;
-
 	calculate_changed_submodule_paths();
+	run_processes_parallel(max_parallel_jobs, &spf,
+			       get_next_submodule,
+			       handle_submodule_fetch_start_err,
+			       handle_submodule_fetch_finish);
+
+	argv_array_clear(&spf.args);
+out:
+	string_list_clear(&changed_submodule_paths, 1);
+	return spf.result;
+}
+
+int get_next_submodule(void *data, struct child_process *cp,
+		       struct strbuf *err)
+{
+	int ret = 0;
+	struct submodule_parallel_fetch *spf = data;
 
-	for (i = 0; i < active_nr; i++) {
+	for ( ; spf->count < active_nr; spf->count++) {
 		struct strbuf submodule_path = STRBUF_INIT;
 		struct strbuf submodule_git_dir = STRBUF_INIT;
 		struct strbuf submodule_prefix = STRBUF_INIT;
-		const struct cache_entry *ce = active_cache[i];
+		const struct cache_entry *ce = active_cache[spf->count];
 		const char *git_dir, *default_argv;
 		const struct submodule *submodule;
 
@@ -657,7 +700,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 			submodule = submodule_from_name(null_sha1, ce->name);
 
 		default_argv = "yes";
-		if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
+		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
 			if (submodule &&
 			    submodule->fetch_recurse !=
 						RECURSE_SUBMODULES_NONE) {
@@ -680,40 +723,46 @@ int fetch_populated_submodules(const struct argv_array *options,
 					default_argv = "on-demand";
 				}
 			}
-		} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
+		} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
 			if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
 				continue;
 			default_argv = "on-demand";
 		}
 
-		strbuf_addf(&submodule_path, "%s/%s", work_tree, ce->name);
+		strbuf_addf(&submodule_path, "%s/%s", spf->work_tree, ce->name);
 		strbuf_addf(&submodule_git_dir, "%s/.git", submodule_path.buf);
-		strbuf_addf(&submodule_prefix, "%s%s/", prefix, ce->name);
+		strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name);
 		git_dir = read_gitfile(submodule_git_dir.buf);
 		if (!git_dir)
 			git_dir = submodule_git_dir.buf;
 		if (is_directory(git_dir)) {
-			if (!quiet)
-				fprintf(stderr, "Fetching submodule %s%s\n", prefix, ce->name);
-			cp.dir = submodule_path.buf;
-			argv_array_push(&argv, default_argv);
-			argv_array_push(&argv, "--submodule-prefix");
-			argv_array_push(&argv, submodule_prefix.buf);
-			cp.argv = argv.argv;
-			if (run_command(&cp))
-				result = 1;
-			argv_array_pop(&argv);
-			argv_array_pop(&argv);
-			argv_array_pop(&argv);
+			child_process_init(cp);
+			cp->dir = strbuf_detach(&submodule_path, NULL);
+			cp->git_cmd = 1;
+			cp->no_stdout = 1;
+			cp->no_stdin = 1;
+			cp->stdout_to_stderr = 1;
+			cp->err = -1;
+			cp->env = local_repo_env;
+			if (!spf->quiet)
+				strbuf_addf(err, "Fetching submodule %s%s\n",
+					    spf->prefix, ce->name);
+			argv_array_init(&cp->args);
+			argv_array_pushv(&cp->args, spf->args.argv);
+			argv_array_push(&cp->args, default_argv);
+			argv_array_push(&cp->args, "--submodule-prefix");
+			argv_array_push(&cp->args, submodule_prefix.buf);
+			ret = 1;
 		}
 		strbuf_release(&submodule_path);
 		strbuf_release(&submodule_git_dir);
 		strbuf_release(&submodule_prefix);
+		if (ret) {
+			spf->count++;
+			return 0;
+		}
 	}
-	argv_array_clear(&argv);
-out:
-	string_list_clear(&changed_submodule_paths, 1);
-	return result;
+	return 1;
 }
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
diff --git a/submodule.h b/submodule.h
index 5507c3d..cbc0003 100644
--- a/submodule.h
+++ b/submodule.h
@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
-			       int quiet);
+			       int quiet, int max_parallel_jobs);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int submodule_uses_gitfile(const char *path);
 int ok_to_remove_submodule(const char *path);
-- 
2.5.0.275.ge015d2a

[PATCHv3 08/13] submodules: allow parallel fetching, add tests and documentation

[Stefan Beller]

This enables the work of the previous patches.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/fetch-options.txt |  7 +++++++
 builtin/fetch.c                 |  5 ++++-
 builtin/pull.c                  |  6 ++++++
 submodule.c                     |  4 ++--
 t/t5526-fetch-submodules.sh     | 19 +++++++++++++++++++
 5 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 45583d8..6b109f6 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -100,6 +100,13 @@ ifndef::git-pull[]
 	reference to a commit that isn't already in the local submodule
 	clone.
 
+-j::
+--jobs=<n>::
+	Number of parallel children to be used for fetching submodules.
+	Each will fetch from different submodules, such that fetching many
+	submodules will be faster. By default submodules will be fetched
+	one at a time.
+
 --no-recurse-submodules::
 	Disable recursive fetching of submodules (this has the same effect as
 	using the '--recurse-submodules=no' option).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6620ed0..f28eac6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
+static int max_children = 1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
+	OPT_INTEGER('j', "jobs", &max_children,
+		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
@@ -1218,7 +1221,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 						    submodule_prefix,
 						    recurse_submodules,
 						    verbosity < 0,
-						    0);
+						    max_children);
 		argv_array_clear(&options);
 	}
 
diff --git a/builtin/pull.c b/builtin/pull.c
index 722a83c..f0af196 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -94,6 +94,7 @@ static int opt_force;
 static char *opt_tags;
 static char *opt_prune;
 static char *opt_recurse_submodules;
+static char *max_children;
 static int opt_dry_run;
 static char *opt_keep;
 static char *opt_depth;
@@ -177,6 +178,9 @@ static struct option pull_options[] = {
 		N_("on-demand"),
 		N_("control recursive fetching of submodules"),
 		PARSE_OPT_OPTARG),
+	OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
+		N_("number of submodules pulled in parallel"),
+		PARSE_OPT_OPTARG),
 	OPT_BOOL(0, "dry-run", &opt_dry_run,
 		N_("dry run")),
 	OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, opt_prune);
 	if (opt_recurse_submodules)
 		argv_array_push(&args, opt_recurse_submodules);
+	if (max_children)
+		argv_array_push(&args, max_children);
 	if (opt_dry_run)
 		argv_array_push(&args, "--dry-run");
 	if (opt_keep)
diff --git a/submodule.c b/submodule.c
index a0e06e8..d15364f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -759,10 +759,10 @@ int get_next_submodule(void *data, struct child_process *cp,
 		strbuf_release(&submodule_prefix);
 		if (ret) {
 			spf->count++;
-			return 0;
+			return 1;
 		}
 	}
-	return 1;
+	return 0;
 }
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 17759b1..1b4ce69 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -71,6 +71,16 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
+	add_upstream_commit &&
+	(
+		cd downstream &&
+		git fetch --recurse-submodules -j2 2>../actual.err
+	) &&
+	test_must_be_empty actual.out &&
+	test_i18ncmp expect.err actual.err
+'
+
 test_expect_success "fetch alone only fetches superproject" '
 	add_upstream_commit &&
 	(
@@ -140,6 +150,15 @@ test_expect_success "--quiet propagates to submodules" '
 	! test -s actual.err
 '
 
+test_expect_success "--quiet propagates to parallel submodules" '
+	(
+		cd downstream &&
+		git fetch --recurse-submodules -j 2 --quiet  >../actual.out 2>../actual.err
+	) &&
+	! test -s actual.out &&
+	! test -s actual.err
+'
+
 test_expect_success "--dry-run propagates to submodules" '
 	add_upstream_commit &&
 	(
-- 
2.5.0.275.ge015d2a

[PATCHv3 09/13] submodule config: keep update strategy around

[Stefan Beller]

We need the submodule update strategies in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 16 ++++++++++++++++
 submodule-config.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index 393de53..0298a60 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 
 	submodule->path = NULL;
 	submodule->url = NULL;
+	submodule->update = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
 
@@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)
 		free((void *) submodule->url);
 		strbuf_addstr(&url, value);
 		submodule->url = strbuf_detach(&url, NULL);
+	} else if (!strcmp(item.buf, "update")) {
+		struct strbuf update = STRBUF_INIT;
+		if (!value) {
+			ret = config_error_nonbool(var);
+			goto release_return;
+		}
+		if (!me->overwrite && submodule->update != NULL) {
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					"update");
+			goto release_return;
+		}
+
+		free((void *) submodule->update);
+		strbuf_addstr(&update, value);
+		submodule->update = strbuf_detach(&update, NULL);
 	}
 
 release_return:
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
 	const char *url;
 	int fetch_recurse;
 	const char *ignore;
+	const char *update;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
 };
-- 
2.5.0.275.ge015d2a

[PATCHv3 10/13] git submodule update: cmd_update_recursive

[Stefan Beller]

split the recursion part out to its own function

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 8964b1d..71385cb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -582,6 +582,31 @@ cmd_deinit()
 	done
 }
 
+
+cmd_update_recursive()
+{
+	if test -n "$recursive"
+	then
+		(
+			prefix="$prefix$sm_path/"
+			clear_local_git_env
+			cd "$sm_path" &&
+			eval cmd_update
+		)
+		res=$?
+		if test $res -gt 0
+		then
+			die_msg="$(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"
+			if test $res -eq 1
+			then
+				err="${err};$die_msg"
+			else
+				die_with_status $res "$die_msg"
+			fi
+		fi
+	fi
+}
+
 #
 # Update each submodule path to correct revision, using clone and checkout as needed
 #
@@ -790,27 +815,7 @@ Maybe you want to use 'update --init'?")"
 			fi
 		fi
 
-		if test -n "$recursive"
-		then
-			(
-				prefix="$prefix$sm_path/"
-				clear_local_git_env
-				cd "$sm_path" &&
-				eval cmd_update
-			)
-			res=$?
-			if test $res -gt 0
-			then
-				die_msg="$(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"
-				if test $res -eq 1
-				then
-					err="${err};$die_msg"
-					continue
-				else
-					die_with_status $res "$die_msg"
-				fi
-			fi
-		fi
+		cmd_update_recursive
 	done
 
 	if test -n "$err"
-- 
2.5.0.275.ge015d2a

[PATCHv3 11/13] git submodule update: cmd_update_clone

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 71385cb..7f11158 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -607,6 +607,24 @@ cmd_update_recursive()
 	fi
 }
 
+cmd_update_clone()
+{
+	command="git checkout $subforce -q"
+	die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
+	say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
+
+	git submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
+
+	if (clear_local_git_env; cd "$sm_path" && $command "$sha1")
+	then
+		say "$say_msg"
+	else
+		err="${err};$die_msg"
+		return
+	fi
+	cmd_update_recursive
+}
+
 #
 # Update each submodule path to correct revision, using clone and checkout as needed
 #
@@ -680,7 +698,6 @@ cmd_update()
 		cmd_init "--" "$@" || return
 	fi
 
-	cloned_modules=
 	git submodule--helper list --prefix "$wt_prefix" "$@" | {
 	err=
 	while read mode sha1 stage sm_path
@@ -725,9 +742,8 @@ Maybe you want to use 'update --init'?")"
 
 		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
 		then
-			git submodule--helper clone ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
-			cloned_modules="$cloned_modules;$name"
-			subsha1=
+			cmd_update_clone
+			continue
 		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
@@ -767,13 +783,6 @@ Maybe you want to use 'update --init'?")"
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 
-			# Is this something we just cloned?
-			case ";$cloned_modules;" in
-			*";$name;"*)
-				# then there is no local change to integrate
-				update_module=checkout ;;
-			esac
-
 			must_die_on_failure=
 			case "$update_module" in
 			checkout)
-- 
2.5.0.275.ge015d2a

[PATCHv3 12/13] git submodule update: cmd_update_fetch

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 164 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 84 insertions(+), 80 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 7f11158..a1bc8d5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -625,6 +625,89 @@ cmd_update_clone()
 	cmd_update_recursive
 }
 
+cmd_update_fetch()
+{
+	subsha1=$(clear_local_git_env; cd "$sm_path" &&
+		git rev-parse --verify HEAD) ||
+	die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
+
+	if test -n "$remote"
+	then
+		if test -z "$nofetch"
+		then
+			# Fetch remote before determining tracking $sha1
+			(clear_local_git_env; cd "$sm_path" && git-fetch) ||
+			die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+		fi
+		remote_name=$(clear_local_git_env; cd "$sm_path" && get_default_remote)
+		sha1=$(clear_local_git_env; cd "$sm_path" &&
+			git rev-parse --verify "${remote_name}/${branch}") ||
+		die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"
+	fi
+
+	if test "$subsha1" != "$sha1" || test -n "$force"
+	then
+		subforce=$force
+		# If we don't already have a -f flag and the submodule has never been checked out
+		if test -z "$subsha1" && test -z "$force"
+		then
+			subforce="-f"
+		fi
+
+		if test -z "$nofetch"
+		then
+			# Run fetch only if $sha1 isn't present or it
+			# is not reachable from a ref.
+			(clear_local_git_env; cd "$sm_path" &&
+				( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
+				 test -z "$rev") || git-fetch)) ||
+			die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
+		fi
+
+		must_die_on_failure=
+		case "$update_module" in
+		checkout)
+			command="git checkout $subforce -q"
+			die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
+			say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
+			;;
+		rebase)
+			command="git rebase"
+			die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")"
+			say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")"
+			must_die_on_failure=yes
+			;;
+		merge)
+			command="git merge"
+			die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")"
+			say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")"
+			must_die_on_failure=yes
+			;;
+		!*)
+			command="${update_module#!}"
+			die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule  path '\$prefix\$sm_path'")"
+			say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"
+			must_die_on_failure=yes
+			;;
+		*)
+			die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
+		esac
+
+		if (clear_local_git_env; cd "$sm_path" && $command "$sha1")
+		then
+			say "$say_msg"
+		elif test -n "$must_die_on_failure"
+		then
+			die_with_status 2 "$die_msg"
+		else
+			err="${err};$die_msg"
+			return
+		fi
+	fi
+
+	cmd_update_recursive
+}
+
 #
 # Update each submodule path to correct revision, using clone and checkout as needed
 #
@@ -743,88 +826,9 @@ Maybe you want to use 'update --init'?")"
 		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
 		then
 			cmd_update_clone
-			continue
 		else
-			subsha1=$(clear_local_git_env; cd "$sm_path" &&
-				git rev-parse --verify HEAD) ||
-			die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
+			cmd_update_fetch
 		fi
-
-		if test -n "$remote"
-		then
-			if test -z "$nofetch"
-			then
-				# Fetch remote before determining tracking $sha1
-				(clear_local_git_env; cd "$sm_path" && git-fetch) ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
-			fi
-			remote_name=$(clear_local_git_env; cd "$sm_path" && get_default_remote)
-			sha1=$(clear_local_git_env; cd "$sm_path" &&
-				git rev-parse --verify "${remote_name}/${branch}") ||
-			die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"
-		fi
-
-		if test "$subsha1" != "$sha1" || test -n "$force"
-		then
-			subforce=$force
-			# If we don't already have a -f flag and the submodule has never been checked out
-			if test -z "$subsha1" && test -z "$force"
-			then
-				subforce="-f"
-			fi
-
-			if test -z "$nofetch"
-			then
-				# Run fetch only if $sha1 isn't present or it
-				# is not reachable from a ref.
-				(clear_local_git_env; cd "$sm_path" &&
-					( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
-					 test -z "$rev") || git-fetch)) ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
-			fi
-
-			must_die_on_failure=
-			case "$update_module" in
-			checkout)
-				command="git checkout $subforce -q"
-				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
-				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
-				;;
-			rebase)
-				command="git rebase"
-				die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")"
-				say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")"
-				must_die_on_failure=yes
-				;;
-			merge)
-				command="git merge"
-				die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")"
-				say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")"
-				must_die_on_failure=yes
-				;;
-			!*)
-				command="${update_module#!}"
-				die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule  path '\$prefix\$sm_path'")"
-				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"
-				must_die_on_failure=yes
-				;;
-			*)
-				die "$(eval_gettext "Invalid update mode '$update_module' for submodule '$name'")"
-			esac
-
-			if (clear_local_git_env; cd "$sm_path" && $command "$sha1")
-			then
-				say "$say_msg"
-			elif test -n "$must_die_on_failure"
-			then
-				die_with_status 2 "$die_msg"
-			else
-				err="${err};$die_msg"
-				continue
-			fi
-		fi
-
-		cmd_update_recursive
 	done
 
 	if test -n "$err"
-- 
2.5.0.275.ge015d2a

[PATCHv3 13/13] Rewrite submodule update in C

[Stefan Beller]

This will make parallelisation easier in a followup patch. This is just
a translation from shell to C, hopefully not introducing any bugs.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 247 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 135 +-----------------------
 run-command.h               |   2 +
 3 files changed, 252 insertions(+), 132 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..baa7563 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,252 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct module_update_data {
+	struct module_list list;
+	int count;
+
+	struct pathspec pathspec;
+
+	int no_fetch;
+	int force;
+	int update;
+	int quiet;
+	int recursive;
+	int remote;
+	char *prefix;
+	char *reference;
+	char *depth;
+
+	struct argv_array args;
+
+	int result;
+};
+
+static void module_update_data_init(struct module_update_data *mud)
+{
+	mud->list.entries = NULL;
+	mud->list.alloc = 0;
+	mud->list.nr = 0;
+
+	memset(&mud->pathspec, 0, sizeof(mud->pathspec));
+
+	mud->count = 0;
+	mud->no_fetch = 0;
+	mud->force = 0;
+	mud->update = 0;
+	mud->quiet = 0;
+	mud->remote = 0;
+	mud->recursive = 0;
+	mud->result = 0;
+
+	mud->prefix = NULL;
+	mud->reference = NULL;
+	mud->depth = NULL;
+
+	argv_array_init(&mud->args);
+}
+
+static int update_next_task(void *data,
+		     struct child_process *cp,
+		     struct strbuf *err)
+{
+	int i;
+	struct module_update_data *mud = data;
+	struct strbuf sb = STRBUF_INIT;
+	const char *displaypath;
+
+	for (; mud->count < mud->list.nr; mud->count++) {
+		const char *update_module;
+		const char *sm_gitdir;
+		const struct submodule *sub;
+		const struct cache_entry *ce = mud->list.entries[mud->count];
+
+		displaypath = relative_path(ce->name, mud->prefix, &sb);
+		strbuf_reset(&sb);
+
+		if (ce_stage(ce)) {
+			strbuf_addf(err, "Skipping unmerged submodule %s",
+				    displaypath);
+			continue;
+		}
+
+		sub = submodule_from_path(null_sha1, ce->name);
+		if (!sub) {
+			mud->result = 1;
+			return 0;
+		}
+
+		switch (mud->update) {
+		case 'r':
+			update_module = "rebase";
+			break;
+		case 'c':
+			update_module = "checkout";
+			break;
+		case 'm':
+			update_module = "merge";
+			break;
+		case 0:
+			/* not specified by command line */
+			if (sub->update)
+				update_module = sub->update;
+			else
+				update_module = "checkout";
+			break;
+		default:
+			die("BUG: update mode not covered");
+		}
+
+		if (!strcmp(update_module, "none")) {
+			strbuf_addf(err, "Skipping submodule '%s'", displaypath);
+			continue;
+		}
+
+		if (!sub->url) {
+			/*
+			 * Only mention uninitialized submodules when its
+			 * path have been specified
+			 */
+			if (!mud->pathspec.nr)
+				continue;
+
+			strbuf_addf(err,
+				    _("Submodule path '%s' not initialized \n"
+				    "Maybe you want to use 'update --init'?"),
+				    displaypath);
+			continue;
+		}
+
+		strbuf_addf(&sb, "%s/.git", ce->name);
+		sm_gitdir = strbuf_detach(&sb, NULL);
+
+		child_process_init(cp);
+		for (i = 0; local_repo_env[i]; i++)
+			argv_array_pushf(&cp->env_array, "%s", local_repo_env[i]);
+
+		argv_array_pushf(&cp->env_array, "displaypath=%s", displaypath);
+		argv_array_pushf(&cp->env_array, "sm_path=%s", sub->path);
+		argv_array_pushf(&cp->env_array, "name=%s", sub->name);
+		argv_array_pushf(&cp->env_array, "url=%s", sub->url);
+		argv_array_pushf(&cp->env_array, "update_module=%s", update_module);
+
+		cp->git_cmd = 1;
+		cp->stdout_to_stderr = 1;
+		argv_array_push(&cp->args, "submodule");
+		if (!file_exists(sm_gitdir))
+			argv_array_push(&cp->args, "update_clone");
+		else
+			argv_array_push(&cp->args, "update_fetch");
+
+		argv_array_pushf(&cp->args, "%s", ce->name);
+		mud->count++;
+		return 1;
+	}
+	return 0;
+}
+
+void update_subcommand_failure(void *data,
+			       struct child_process *cp,
+			       struct strbuf *err)
+{
+	struct module_update_data *mud = data;
+	strbuf_addf(err, _("Could not start child process"));
+	mud->result = 1;
+}
+
+void update_child_return(void *data,
+			 struct child_process *cp,
+			 int result)
+{
+	struct module_update_data *mud = data;
+	mud->result = 1;
+}
+
+static int module_update(int argc, const char **argv, const char *prefix)
+{
+	int init;
+	struct module_update_data mud;
+
+	struct option module_list_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_BOOL('i', "init", &init,
+			N_("Initialize the submodule if not yet done")),
+		OPT_BOOL(0, "remote", &mud.remote,
+			N_("Update the submodule to the remote branch instead "
+			   "of the superprojects specification")),
+		OPT_BOOL('N', "no-fetch", &mud.no_fetch,
+			N_("Don’t fetch new objects from the remote site.")),
+		OPT_BOOL('f', "force", &mud.force,
+			N_("Ignore local changes in submodules")),
+		OPT_CMDMODE('r', "rebase", &mud.update,
+			N_("Rebase local changes in submodules"), 'r'),
+		OPT_CMDMODE('m', "merge", &mud.update,
+			N_("Merge local changes in submodules"), 'm'),
+		OPT_CMDMODE(0, "checkout", &mud.update,
+			N_("Checkout to a detached HEAD in submodules"), 'c'),
+		OPT_BOOL(0, "recursive", &mud.recursive,
+			N_("Update nested submodules")),
+		OPT_STRING(0, "reference", &mud.reference, "<repository>",
+			N_("Use the local reference repository "
+			   "instead of a full clone")),
+		OPT_STRING(0, "depth", &mud.depth, "<depth>",
+			N_("Create a shallow clone truncated to the "
+			   "specified number of revisions")),
+		OPT__QUIET(&mud.quiet, N_("be quiet")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		NULL
+	};
+
+	module_update_data_init(&mud);
+	gitmodules_config();
+
+	argc = parse_options(argc, argv, prefix, module_list_options,
+			     git_submodule_helper_usage, 0);
+
+	if (mud.force)
+		argv_array_push(&mud.args, "force=1");
+	if (mud.quiet)
+		argv_array_push(&mud.args, "GIT_QUIET=1");
+	if (mud.recursive)
+		argv_array_push(&mud.args, "recursive=1");
+	if (mud.prefix)
+		argv_array_pushf(&mud.args, "prefix=%s", mud.prefix);
+	if (mud.reference)
+		argv_array_pushf(&mud.args, "reference=%s", mud.reference);
+	if (mud.depth)
+		argv_array_pushf(&mud.args, "depth=%s", mud.depth);
+
+	if (module_list_compute(argc, argv, prefix, &mud.pathspec, &mud.list) < 0)
+		return 1;
+
+	if (init) {
+		const char **argv_init = xmalloc((2 + mud.list.nr) * sizeof(char*));
+		int argc = 0, i, code;
+		argv_init[argc++] = "submodule";
+		argv_init[argc++] = "init";
+
+		for (i = 0; i < mud.list.nr; i++) {
+			const struct cache_entry *ce = mud.list.entries[i];
+			argv_init[argc++] = ce->name;
+		}
+		code = run_command_v_opt(argv_init, RUN_GIT_CMD);
+			if (code)
+				return code;
+	}
+
+	run_processes_parallel(1, &mud,
+			       update_next_task,
+			       update_subcommand_failure,
+			       update_child_return);
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -264,6 +510,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
+	{"update", module_update}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a1bc8d5..63e9b3b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -640,6 +640,7 @@ cmd_update_fetch()
 			die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
 		fi
 		remote_name=$(clear_local_git_env; cd "$sm_path" && get_default_remote)
+		branch=$(get_submodule_config "$name" branch master)
 		sha1=$(clear_local_git_env; cd "$sm_path" &&
 			git rev-parse --verify "${remote_name}/${branch}") ||
 		die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"
@@ -715,137 +716,7 @@ cmd_update_fetch()
 #
 cmd_update()
 {
-	# parse $args after "submodule ... update".
-	while test $# -ne 0
-	do
-		case "$1" in
-		-q|--quiet)
-			GIT_QUIET=1
-			;;
-		-i|--init)
-			init=1
-			;;
-		--remote)
-			remote=1
-			;;
-		-N|--no-fetch)
-			nofetch=1
-			;;
-		-f|--force)
-			force=$1
-			;;
-		-r|--rebase)
-			update="rebase"
-			;;
-		--reference)
-			case "$2" in '') usage ;; esac
-			reference="--reference=$2"
-			shift
-			;;
-		--reference=*)
-			reference="$1"
-			;;
-		-m|--merge)
-			update="merge"
-			;;
-		--recursive)
-			recursive=1
-			;;
-		--checkout)
-			update="checkout"
-			;;
-		--depth)
-			case "$2" in '') usage ;; esac
-			depth="--depth=$2"
-			shift
-			;;
-		--depth=*)
-			depth=$1
-			;;
-		--)
-			shift
-			break
-			;;
-		-*)
-			usage
-			;;
-		*)
-			break
-			;;
-		esac
-		shift
-	done
-
-	if test -n "$init"
-	then
-		cmd_init "--" "$@" || return
-	fi
-
-	git submodule--helper list --prefix "$wt_prefix" "$@" | {
-	err=
-	while read mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode"
-		if test "$stage" = U
-		then
-			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
-			continue
-		fi
-		name=$(git submodule--helper name "$sm_path") || exit
-		url=$(git config submodule."$name".url)
-		branch=$(get_submodule_config "$name" branch master)
-		if ! test -z "$update"
-		then
-			update_module=$update
-		else
-			update_module=$(git config submodule."$name".update)
-			if test -z "$update_module"
-			then
-				update_module="checkout"
-			fi
-		fi
-
-		displaypath=$(relative_path "$prefix$sm_path")
-
-		if test "$update_module" = "none"
-		then
-			echo "Skipping submodule '$displaypath'"
-			continue
-		fi
-
-		if test -z "$url"
-		then
-			# Only mention uninitialized submodules when its
-			# path have been specified
-			test "$#" != "0" &&
-			say "$(eval_gettext "Submodule path '\$displaypath' not initialized
-Maybe you want to use 'update --init'?")"
-			continue
-		fi
-
-		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
-		then
-			cmd_update_clone
-		else
-			cmd_update_fetch
-		fi
-	done
-
-	if test -n "$err"
-	then
-		OIFS=$IFS
-		IFS=';'
-		for e in $err
-		do
-			if test -n "$e"
-			then
-				echo >&2 "$e"
-			fi
-		done
-		IFS=$OIFS
-		exit 1
-	fi
-	}
+	git submodule--helper update ${prefix:+--prefix "$prefix"} "$@"
 }
 
 set_name_rev () {
@@ -1243,7 +1114,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | init | deinit | update | update_fetch | update_clone | status | summary | sync)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/run-command.h b/run-command.h
index 3807fd1..0c1b363 100644
--- a/run-command.h
+++ b/run-command.h
@@ -155,4 +155,6 @@ int run_processes_parallel(int n, void *data,
 			   start_failure_fn,
 			   return_value_fn);
 
+void run_processes_parallel_schedule_error(struct strbuf *err);
+
 #endif
-- 
2.5.0.275.ge015d2a

Re: [PATCHv3 01/13] Sending "Fetching submodule <foo>" output to stderr

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Subject: Re: [PATCHv3 01/13] Sending "Fetching submodule <foo>" output to stderr

Subject: submodule: send "...." to standard error

or something?

>  		if (is_directory(git_dir)) {
>  			if (!quiet)
> -				printf("Fetching submodule %s%s\n", prefix, ce->name);
> +				fprintf(stderr, "Fetching submodule %s%s\n", prefix, ce->name);
>  			cp.dir = submodule_path.buf;
>  			argv_array_push(&argv, default_argv);
>  			argv_array_push(&argv, "--submodule-prefix");

The change itself (above) and updates to the tests (omitted) looked
sensible.

Thanks.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.
> As the intend of xread is to read as much as possible either until the
> fd is EOF or an actual error occurs, we can ease the feeder of the fd
> by not spinning the whole time, but rather wait for it politely by not
> busy waiting.

As you said in the cover letter, this does look questionable.  It is
sweeping the problem under the rug (the hard-coded 100ms is a good
clue to tell that).  If the caller does want us to read thru to the
end, then we would need to make it easier for such a caller to stop
marking the file descriptor to be non-blocking, but this does not do
anything to help that.  An alternative might be to automatically
turn nonblocking off temporarily once we get EAGAIN (and turn it on
again before leaving); that would be an approach to make it
unnecessary to fix the caller (which has its own set of problems,
though).

> +				if (i < 0) {
> +					if (errno == EINTR || errno == ENOMEM)
> +						continue;

I can sort of see EINTR but why is ENOMEM any special than other
errors?

> +					else
> +						die_errno("poll");
> +				}
> +			}
> +		}
>  		return nr;
>  	}
>  }

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Eric Sunshine]

On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller <sbeller@google.com> wrote:
> From the man page:
> [...]
> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.

s/So/&,/
s/error/&,/

> As the intend of xread is to read as much as possible either until the

s/intend/intent/

> fd is EOF or an actual error occurs, we can ease the feeder of the fd
> by not spinning the whole time, but rather wait for it politely by not
> busy waiting.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -201,8 +201,23 @@ ssize_t xread(int fd, void *buf, size_t len)
>             len = MAX_IO_SIZE;
>         while (1) {
>                 nr = read(fd, buf, len);
> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
> -                       continue;
> +               if (nr < 0) {
> +                       if (errno == EINTR)
> +                               continue;
> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
> +                               struct pollfd pfd;
> +                               int i;
> +                               pfd.events = POLLIN;
> +                               pfd.fd = fd;
> +                               i = poll(&pfd, 1, 100);

Why is this poll() using a timeout? Isn't that still a busy wait of
sorts (even if less aggressive)?

> +                               if (i < 0) {
> +                                       if (errno == EINTR || errno == ENOMEM)
> +                                               continue;
> +                                       else
> +                                               die_errno("poll");
> +                               }
> +                       }
> +               }
>                 return nr;
>         }
>  }
> --
> 2.5.0.275.ge015d2a

Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This wrapper just restarts on EINTR, but not on EAGAIN like xread
> does. This gives less guarantees on the actual reading rather than
> on returning fast.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-compat-util.h |  1 +
>  wrapper.c         | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index c6d391f..9ccea85 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);
>  extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
>  extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
>  extern ssize_t xread(int fd, void *buf, size_t len);
> +extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
>  extern ssize_t xwrite(int fd, const void *buf, size_t len);
>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
>  extern int xdup(int fd);
> diff --git a/wrapper.c b/wrapper.c
> index 50267a4..54ce231 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -223,6 +223,28 @@ ssize_t xread(int fd, void *buf, size_t len)
>  }
>  
>  /*
> + * xread_nonblock() is the same a read(), but it automatically restarts read()
> + * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
> + * "len" bytes is read even if the data is available.
> + */

This comment is somewhat misleading to readers who knew there was
xread() and this new one is a variant of it.  xread() does not say
anything about "if the data is available" (it just blocks and
insists reading that much if the data is not ready yet).  If we drop
the "even if ..." from the end, that confusion would be avoided.

Turning EWOULDBLOCK to EAGAIN is a very useful thing to help the
callers, and it deserves to be described in the above comment.

> +ssize_t xread_nonblock(int fd, void *buf, size_t len)
> +{
> +	ssize_t nr;
> +	if (len > MAX_IO_SIZE)
> +	    len = MAX_IO_SIZE;

This line is under-indented?

> +	while (1) {
> +		nr = read(fd, buf, len);
> +		if (nr < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			if (errno == EWOULDBLOCK)
> +				errno = EAGAIN;
> +		}
> +		return nr;
> +	}
> +}
> +
> +/*
>   * xwrite() is the same a write(), but it automatically restarts write()
>   * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
>   * GUARANTEE that "len" bytes is written even if the operation is successful.

Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This wrapper just restarts on EINTR, but not on EAGAIN like xread
> does. This gives less guarantees on the actual reading rather than
> on returning fast.

The last sentence is a bit hard to parse, by the way.

It lets caller make some progress without stalling by marking the
file descriptor as nonblocking and calling this function, compared
to calling xread() which will not give control back until reading
the asked size (or EOF).

It is not like "reading in full is always the desireable thing to
do, and returning early is something that needs compromise on that
desirable behaviour to achieve", which was the way I read your
version.  These two kinds of callers want two different things, but
the phrase "less guarantees" conveys a misguided value judgement (in
that "wanting to read in full always trumps other considerations" is
the stance the person who is making the judgement makes) more than
useful information (e.g. "the caller may want the control back
without stalling, in which case this is a useful thing to use").

The implementation itself is exactly like we discussed, and looks
good.

Thanks.

Re: [PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The new call will read a fd into a strbuf once. The underlying call

"read from a fd"

> xread_nonblock is meant to execute non blockingly if the fd is set to
> O_NONBLOCK.

The latter sentence adds more questions than it answers.  If the
file descriptor is not set to non-blocking, what happens?  Is it a
bug in the caller, and if so do we give help to diagnose such a bug?



> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  strbuf.c | 11 +++++++++++
>  strbuf.h |  6 ++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index cce5eed..35e71b8 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
>  	return sb->len - oldlen;
>  }
>  
> +ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
> +{
> +	ssize_t cnt;
> +
> +	strbuf_grow(sb, hint ? hint : 8192);
> +	cnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
> +	if (cnt > 0)
> +		strbuf_setlen(sb, sb->len + cnt);
> +	return cnt;

OK.  So the caller that receives a negative value can check errno to
see if we got EAGAIN.  How would the caller tell when it got an EOF?

>  /**
> + * Same as strbuf_read, just returns non-blockingly by ignoring EAGAIN.

I do not think you want to say "same as" for this one.
strbuf_read() is about reading thru to the end, but this is about
making some progress without blocking.

    Read from a file descriptor that is marked as O_NONBLOCK without
    blocking.  Returns the number of new bytes appended to the sb.
    Negative return value signals there was an error returned from
    underlying read(2), in which case the caller should check errno.
    e.g. errno == EAGAIN when the read may have blocked.

or something?  Again, how would a caller tell when it got an EOF?

> + * The fd must have set O_NONBLOCK.
> + */
> +extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
> +
> +/**
>   * Read the contents of a file, specified by its path. The third argument
>   * can be used to give a hint about the file size, to avoid reallocs.
>   */

Re: [PATCHv3 05/13] run-command: factor out return value computation

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We will need computing the return value in a later patch without the
> wait.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 54 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 28e1d55..674e348 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -232,6 +232,35 @@ static inline void set_cloexec(int fd)
>  		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>  }
>  
> +static int determine_return_value(int wait_status,
> +				  int *result,
> +				  int *error_code,
> +				  const char *argv0)
> +{
> +	if (WIFSIGNALED(wait_status)) {
> +		*result = WTERMSIG(wait_status);
> +		if (*result != SIGINT && *result != SIGQUIT)
> +			error("%s died of signal %d", argv0, *result);
> +		/*
> +		 * This return value is chosen so that code & 0xff
> +		 * mimics the exit code that a POSIX shell would report for
> +		 * a program that died from this signal.
> +		 */

Not a new problem but "from these signals", I think.

> @@ -244,29 +273,10 @@ static int wait_or_whine(pid_t pid, const char *argv0)
>  	if (waiting < 0) {
>  		failed_errno = errno;
>  		error("waitpid for %s failed: %s", argv0, strerror(errno));
> -...
>  	} else {
> -		error("waitpid is confused (%s)", argv0);
> +		if (waiting != pid
> +		   || (determine_return_value(status, &code, &failed_errno, argv0) < 0))

Move "||" to the end of the previous line?

> +			error("waitpid is confused (%s)", argv0);
>  	}
>  
>  	clear_child_for_cleanup(pid);

Re: [PATCHv3 09/13] submodule config: keep update strategy around

[Eric Sunshine]

On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller <sbeller@google.com> wrote:
> We need the submodule update strategies in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> @@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)
>                 free((void *) submodule->url);
>                 strbuf_addstr(&url, value);
>                 submodule->url = strbuf_detach(&url, NULL);
> +       } else if (!strcmp(item.buf, "update")) {
> +               struct strbuf update = STRBUF_INIT;
> +               if (!value) {
> +                       ret = config_error_nonbool(var);
> +                       goto release_return;
> +               }
> +               if (!me->overwrite && submodule->update != NULL) {
> +                       warn_multiple_config(me->commit_sha1, submodule->name,
> +                                       "update");
> +                       goto release_return;
> +               }
> +
> +               free((void *) submodule->update);
> +               strbuf_addstr(&update, value);
> +               submodule->update = strbuf_detach(&update, NULL);
>         }
>
>  release_return:

Why the complicated logic flow? Also, why is strbuf 'update' needed?

I'd have expected to see something simpler, such as:

    } else if (!strcmp(item.buf, "update")) {
        if (!value)
            ret = config_error_nonbool(var);
        else if (!me->overwrite && ...)
            warn_multiple_config(...);
        else {
            free((void *)submodule->update);
            submodule->update = xstrdup(value);
        }
    }

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +void default_start_failure(void *data,
> +			   struct child_process *cp,
> +			   struct strbuf *err)
> +{
> +	int i;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	for (i = 0; cp->argv[i]; i++)
> +		strbuf_addf(&sb, "%s ", cp->argv[i]);
> +	die_errno("Starting a child failed:\n%s", sb.buf);

Do we want that trailing SP after the last element of argv[]?
Same question applies to the one in "return-value".

> +static void run_processes_parallel_init(struct parallel_processes *pp,
> +					int n, void *data,
> +					get_next_task_fn get_next_task,
> +					start_failure_fn start_failure,
> +					return_value_fn return_value)
> +{
> +	int i;
> +
> +	if (n < 1)
> +		n = online_cpus();
> +
> +	pp->max_processes = n;
> +	pp->data = data;
> +	if (!get_next_task)
> +		die("BUG: you need to specify a get_next_task function");
> +	pp->get_next_task = get_next_task;
> +
> +	pp->start_failure = start_failure ? start_failure : default_start_failure;
> +	pp->return_value = return_value ? return_value : default_return_value;

I would actually have expected that leaving these to NULL will just
skip pp->fn calls, instead of a "default implementation", but a pair
of very simple default implementation would not hrtut.

> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)
> +{
> +	int i;

Have a blank between the decl block and the first stmt here (and
elsewhere, too---which you got correct in the function above)?

> +	for (i = 0; i < pp->max_processes; i++)
> +		strbuf_release(&pp->children[i].err);

> +static void run_processes_parallel_start_one(struct parallel_processes *pp)
> +{
> +	int i;
> +
> +	for (i = 0; i < pp->max_processes; i++)
> +		if (!pp->children[i].in_use)
> +			break;
> +	if (i == pp->max_processes)
> +		die("BUG: bookkeeping is hard");

Mental note: the caller is responsible for not calling this when all
slots are taken.

> +	if (!pp->get_next_task(pp->data,
> +			       &pp->children[i].process,
> +			       &pp->children[i].err)) {
> +		pp->all_tasks_started = 1;
> +		return;
> +	}

Mental note: but it is OK to call this if get_next_task() previously
said "no more task".

The above two shows a slight discrepancy (nothing earth-breaking).

I have this suspicion that the all-tasks-started bit may turn out to
be a big mistake that we may later regret.  Don't we want to allow
pp->more_task() to say "no more task to run at this moment" implying
"but please do ask me later, because I may have found more to do by
the time you ask me again"?

That is one of the reasons why I do not think the "very top level is
a bulleted list" organization is a good idea in general.  A good
scheduling decision can seldom be made in isolation without taking
global picture into account.

> +static void run_processes_parallel_collect_finished(struct parallel_processes *pp)
> +{
> +	int i = 0;
> +	pid_t pid;
> +	int wait_status, code;
> +	int n = pp->max_processes;
> +
> +	while (pp->nr_processes > 0) {
> +		pid = waitpid(-1, &wait_status, WNOHANG);
> +		if (pid == 0)
> +			return;
> +
> +		if (pid < 0)
> +			die_errno("wait");
> +
> +		for (i = 0; i < pp->max_processes; i++)
> +			if (pp->children[i].in_use &&
> +			    pid == pp->children[i].process.pid)
> +				break;
> +		if (i == pp->max_processes)
> +			/*
> +			 * waitpid returned another process id
> +			 * which we are not waiting for.
> +			 */
> +			return;

If we culled a child process that this machinery is not in charge
of, waitpid() in other places that wants to see that child will not
see it.  Perhaps such a situation might even warrant an error() or
BUG()?  Do we want a "NEEDSWORK: Is this a bug?" comment here at
least?

> +		if (strbuf_read_once(&pp->children[i].err,
> +				     pp->children[i].process.err, 0) < 0 &&
> +		    errno != EAGAIN)
> +			die_errno("strbuf_read_once");

Don't we want to read thru to the end here?  The reason read_once()
did not read thru to the end may not have anything to do with
NONBLOCK (e.g. xread_nonblock() caps len, and it does not loop).

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Torsten Bögershausen]

On 09/22/2015 01:55 AM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So if we get an EAGAIN or EWOULDBLOCK error the fd must be nonblocking.
>> As the intend of xread is to read as much as possible either until the
>> fd is EOF or an actual error occurs, we can ease the feeder of the fd
>> by not spinning the whole time, but rather wait for it politely by not
>> busy waiting.
> As you said in the cover letter, this does look questionable.  It is
> sweeping the problem under the rug (the hard-coded 100ms is a good
> clue to tell that).  If the caller does want us to read thru to the
> end, then we would need to make it easier for such a caller to stop
> marking the file descriptor to be non-blocking, but this does not do
> anything to help that.  An alternative might be to automatically
> turn nonblocking off temporarily once we get EAGAIN (and turn it on
> again before leaving); that would be an approach to make it
> unnecessary to fix the caller (which has its own set of problems,
> though).
Wouldn'  that function be somewhat mis-named and/or mis-behaved?
read_in_full_with_hard_coded_timeout_and_fix_O_NONBLOCK()
could make sure that the user of this function knows what's going on
under the hood.

More seriously, if someone calls xread() with a non-blocking socket,
the caller wants to return and does want to his own timeout handling.

If we want to have a timeouted read(), we can call it

xread_timout(int fd, voxread(int fd, void *buf, size_t len, int timeout)

(Or something similar) to make clear that there is an underlying
timeout handling.
Another option could be to name the function

read_in_full_timeout().

But in any case I suggest to  xread() as it is, and not to change the 
functionality
behind the back of the users.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Jacob Keller]

On Mon, Sep 21, 2015 at 9:55 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> But in any case I suggest to  xread() as it is, and not to change the
> functionality
> behind the back of the users.
>
>

I don't think this patch actually changes behavior as it stands now. I
think Junio's suggestion does. Personally, I'd prefer some sort of
warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather
see it somehow warn so that we can find the bug (since we really
really shouldn't be calling xread with a blocking socket, especially
if we have xread_noblock or similar as in this series.

Not sure if we really want to handle that, but I know we don't want to
change external behavior of xread... I think that polling is better
than the current "spinning" behavior.

Regards,
Jake

Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Jacob Keller]

On Mon, Sep 21, 2015 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This wrapper just restarts on EINTR, but not on EAGAIN like xread
>> does. This gives less guarantees on the actual reading rather than
>> on returning fast.
>
> The last sentence is a bit hard to parse, by the way.
>

I'd imagine something more like:

Provide a wrapper to read(), similar to xread(), that restarts on
EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
handle polling itself, possibly polling multiple sockets or performing
some other action.

Regards,
Jake

Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Jacob Keller]

On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller <sbeller@google.com> wrote:

Maybe change the title to "without blocking" instead of "nonblockingly"?

Regards,
Jake

Re: [PATCHv3 04/13] strbuf: add strbuf_read_once to read without blocking

[Jacob Keller]

On Mon, Sep 21, 2015 at 5:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The new call will read a fd into a strbuf once. The underlying call
>
> "read from a fd"
>
>> xread_nonblock is meant to execute non blockingly if the fd is set to
>> O_NONBLOCK.
>
> The latter sentence adds more questions than it answers.  If the
> file descriptor is not set to non-blocking, what happens?  Is it a
> bug in the caller, and if so do we give help to diagnose such a bug?
>

I would vote for adding debugging for calling xread with O_NONBLOCK
and for calling xread_nonblock without O_NONBLOCK.. I'm not sure the
best way to do this, but it certainly feels like a bug to call xread
with O_NONBLOCK set... And the same here...

Regards,
Jake

Re: [PATCHv3 09/13] submodule config: keep update strategy around

[Stefan Beller]

On Mon, Sep 21, 2015 at 5:56 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Sep 21, 2015 at 6:39 PM, Stefan Beller <sbeller@google.com> wrote:
>> We need the submodule update strategies in a later patch.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/submodule-config.c b/submodule-config.c
>> @@ -326,6 +327,21 @@ static int parse_config(const char *var, const char *value, void *data)
>>                 free((void *) submodule->url);
>>                 strbuf_addstr(&url, value);
>>                 submodule->url = strbuf_detach(&url, NULL);
>> +       } else if (!strcmp(item.buf, "update")) {
>> +               struct strbuf update = STRBUF_INIT;
>> +               if (!value) {
>> +                       ret = config_error_nonbool(var);
>> +                       goto release_return;
>> +               }
>> +               if (!me->overwrite && submodule->update != NULL) {
>> +                       warn_multiple_config(me->commit_sha1, submodule->name,
>> +                                       "update");
>> +                       goto release_return;
>> +               }
>> +
>> +               free((void *) submodule->update);
>> +               strbuf_addstr(&update, value);
>> +               submodule->update = strbuf_detach(&update, NULL);
>>         }
>>
>>  release_return:
>
> Why the complicated logic flow? Also, why is strbuf 'update' needed?

To be honest, I just copied it from above, where the url is read using
the exact same workflow. In the reroll I'll fix both.

>
> I'd have expected to see something simpler, such as:
>
>     } else if (!strcmp(item.buf, "update")) {
>         if (!value)
>             ret = config_error_nonbool(var);
>         else if (!me->overwrite && ...)
>             warn_multiple_config(...);
>         else {
>             free((void *)submodule->update);
>             submodule->update = xstrdup(value);
>         }
>     }

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>         while (1) {
>>                 nr = read(fd, buf, len);
>> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>> -                       continue;
>> +               if (nr < 0) {
>> +                       if (errno == EINTR)
>> +                               continue;
>> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
>> +                               struct pollfd pfd;
>> +                               int i;
>> +                               pfd.events = POLLIN;
>> +                               pfd.fd = fd;
>> +                               i = poll(&pfd, 1, 100);
>
> Why is this poll() using a timeout? Isn't that still a busy wait of
> sorts (even if less aggressive)?

Good point.  If we _were_ to have this kind of "hiding issues under
the rug and continuing without issues" approach, I do not think we
would need timeout for this poll(2).  The caller accepted that it is
willing to wait until we read up to len (which is capped, though) by
not calling the nonblocking variant.

Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller <sbeller@google.com> wrote:
>
> Maybe change the title to "without blocking" instead of "nonblockingly"?

Both suggestions make sense ;-)

Re: [PATCHv3 07/13] fetch_populated_submodules: use new parallel job processing

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In a later patch we enable parallel processing of submodules, this
> only adds the possibility for it. So this change should not change
> any user facing behavior.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/fetch.c |   3 +-
>  submodule.c     | 119 +++++++++++++++++++++++++++++++++++++++-----------------
>  submodule.h     |   2 +-
>  3 files changed, 87 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ee1f1a9..6620ed0 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		result = fetch_populated_submodules(&options,
>  						    submodule_prefix,
>  						    recurse_submodules,
> -						    verbosity < 0);
> +						    verbosity < 0,
> +						    0);

This one...

>  int fetch_populated_submodules(const struct argv_array *options,
>  			       const char *prefix, int command_line_option,
> -			       int quiet)
> +			       int quiet, int max_parallel_jobs)
>  {
> -	int i, result = 0;
> -	struct child_process cp = CHILD_PROCESS_INIT;

... together with this one, could have been made easier to follow if
you didn't add a new parameter to the function.  Instead, you could
define a local variable max_parallel_jobs initialized to 1 in this
function without changing the function signature (and the caller) in
this step, and then did the above two changes in the same commit as
the one that actually enables the feature.

That would be more in line with the stated "only adds the possiblity
for it" goal.

As posted, I suspect that by passing 0 to max_parallel_jobs, you are
telling run_processes_parallel_init() to check online_cpus() and run
that many processes in parallel, no?

> +int get_next_submodule(void *data, struct child_process *cp,
> +		       struct strbuf *err)
> +{
> +	int ret = 0;
> +	struct submodule_parallel_fetch *spf = data;
> ...
> +			child_process_init(cp);
> +			cp->dir = strbuf_detach(&submodule_path, NULL);
> +			cp->git_cmd = 1;
> +			cp->no_stdout = 1;
> +			cp->no_stdin = 1;

In run-commands.c::start_command(), no_stdout triggers
dup_devnull(1) and causes dup2(2, 1) that is triggered by
stdout_to_stderrd to be bypassed.  This looks wrong to me.

> +			cp->stdout_to_stderr = 1;
> +			cp->err = -1;

OK, the original left the standard error stream connected to the
invoker's standard error, but now we are capturing it via a pipe.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Stefan Beller]

On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>>         while (1) {
>>>                 nr = read(fd, buf, len);
>>> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>>> -                       continue;
>>> +               if (nr < 0) {
>>> +                       if (errno == EINTR)
>>> +                               continue;
>>> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
>>> +                               struct pollfd pfd;
>>> +                               int i;
>>> +                               pfd.events = POLLIN;
>>> +                               pfd.fd = fd;
>>> +                               i = poll(&pfd, 1, 100);
>>
>> Why is this poll() using a timeout? Isn't that still a busy wait of
>> sorts (even if less aggressive)?
>

True. Maybe we could have just a warning for now?

    if (errno == EAGAIN) {
        warning("Using xread with a non blocking fd");
        continue; /* preserve previous behavior */
    }

I think I am going to drop this patch off the main series and spin it out
as an extra patch as the discussion is a bit unclear to me at the moment
where we're heading.

> Good point.  If we _were_ to have this kind of "hiding issues under
> the rug and continuing without issues" approach, I do not think we
> would need timeout for this poll(2).  The caller accepted that it is
> willing to wait until we read up to len (which is capped, though) by
> not calling the nonblocking variant.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>>>         while (1) {
>>>>                 nr = read(fd, buf, len);
>>>> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>>>> -                       continue;
>>>> +               if (nr < 0) {
>>>> +                       if (errno == EINTR)
>>>> +                               continue;
>>>> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
>>>> +                               struct pollfd pfd;
>>>> +                               int i;
>>>> +                               pfd.events = POLLIN;
>>>> +                               pfd.fd = fd;
>>>> +                               i = poll(&pfd, 1, 100);
>>>
>>> Why is this poll() using a timeout? Isn't that still a busy wait of
>>> sorts (even if less aggressive)?
>
> True. Maybe we could have just a warning for now?
>
>     if (errno == EAGAIN) {
>         warning("Using xread with a non blocking fd");
>         continue; /* preserve previous behavior */
>     }

It is very likely that you will hit the same EAGAIN immediately by
continuing and end up spewing tons of the same warning, no?

We may want to slow down and think before sending a knee-jerk
response (this applies to both of us).

I actually think a blocking poll(2) here would not be such a bad
idea.  It would preserves the previous behaviour for callers who
unknowingly inherited a O_NONBLOCK file descriptor from its
environment without forcing them to waste CPU cycles.

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Stefan Beller]

On Mon, Sep 21, 2015 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +void default_start_failure(void *data,
>> +                        struct child_process *cp,
>> +                        struct strbuf *err)
>> +{
>> +     int i;
>> +     struct strbuf sb = STRBUF_INIT;
>> +
>> +     for (i = 0; cp->argv[i]; i++)
>> +             strbuf_addf(&sb, "%s ", cp->argv[i]);
>> +     die_errno("Starting a child failed:\n%s", sb.buf);
>
> Do we want that trailing SP after the last element of argv[]?
> Same question applies to the one in "return-value".

done

>
>> +static void run_processes_parallel_init(struct parallel_processes *pp,
>> +                                     int n, void *data,
>> +                                     get_next_task_fn get_next_task,
>> +                                     start_failure_fn start_failure,
>> +                                     return_value_fn return_value)
>> +{
>> +     int i;
>> +
>> +     if (n < 1)
>> +             n = online_cpus();
>> +
>> +     pp->max_processes = n;
>> +     pp->data = data;
>> +     if (!get_next_task)
>> +             die("BUG: you need to specify a get_next_task function");
>> +     pp->get_next_task = get_next_task;
>> +
>> +     pp->start_failure = start_failure ? start_failure : default_start_failure;
>> +     pp->return_value = return_value ? return_value : default_return_value;
>
> I would actually have expected that leaving these to NULL will just
> skip pp->fn calls, instead of a "default implementation", but a pair
> of very simple default implementation would not hrtut.

Ok, I think the default implementation provided is a reasonable default, as
it provides enough information in case of an error.

>
>> +static void run_processes_parallel_cleanup(struct parallel_processes *pp)
>> +{
>> +     int i;
>
> Have a blank between the decl block and the first stmt here (and
> elsewhere, too---which you got correct in the function above)?

done

>
>> +     for (i = 0; i < pp->max_processes; i++)
>> +             strbuf_release(&pp->children[i].err);
>
>> +static void run_processes_parallel_start_one(struct parallel_processes *pp)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < pp->max_processes; i++)
>> +             if (!pp->children[i].in_use)
>> +                     break;
>> +     if (i == pp->max_processes)
>> +             die("BUG: bookkeeping is hard");
>
> Mental note: the caller is responsible for not calling this when all
> slots are taken.
>
>> +     if (!pp->get_next_task(pp->data,
>> +                            &pp->children[i].process,
>> +                            &pp->children[i].err)) {
>> +             pp->all_tasks_started = 1;
>> +             return;
>> +     }
>
> Mental note: but it is OK to call this if get_next_task() previously
> said "no more task".
>
> The above two shows a slight discrepancy (nothing earth-breaking).

I see. Maybe this can be improved by having the
run_processes_parallel_start_as_needed call get_next_task
and pass the information into the run_processes_parallel_start_one
or as we had it before, combine these two functions again.

>
> I have this suspicion that the all-tasks-started bit may turn out to
> be a big mistake that we may later regret.  Don't we want to allow
> pp->more_task() to say "no more task to run at this moment" implying
> "but please do ask me later, because I may have found more to do by
> the time you ask me again"?

And this task would arise because the current running children produce
more work to be done?
So you would have a
    more_tasks() question. If that returns true
    get_next_task() must provide that next task?

In case we had more work to do, which is based on the outcome of the
children, we could just wait in get_next_task for a semaphore/condition
variable from the return_value. Though that would stop progress reporting
end maybe lock up the whole program due to pipe clogging.

It seems to be a better design as we come back to the main loop fast
which does the polling. Although I feel like it is over engineered for now.

So how would you find out when we are done?
* more_tasks() could have different return values in an enum
  (YES_THERE_ARE, NO_BUT_ASK_LATER, NO_NEVER_ASK_AGAIN)
* There could be yet another callback more_tasks_available() and
   parallel_processing_should_stop()
* Hand back a callback ourselfs [Call signal_parallel_processing_done(void*)
  when more_tasks will never return true again, with a void* we provide to
  more_tasks()]
* ...

>
> That is one of the reasons why I do not think the "very top level is
> a bulleted list" organization is a good idea in general.  A good
> scheduling decision can seldom be made in isolation without taking
> global picture into account.
>
>> +static void run_processes_parallel_collect_finished(struct parallel_processes *pp)
>> +{
>> +     int i = 0;
>> +     pid_t pid;
>> +     int wait_status, code;
>> +     int n = pp->max_processes;
>> +
>> +     while (pp->nr_processes > 0) {
>> +             pid = waitpid(-1, &wait_status, WNOHANG);
>> +             if (pid == 0)
>> +                     return;
>> +
>> +             if (pid < 0)
>> +                     die_errno("wait");
>> +
>> +             for (i = 0; i < pp->max_processes; i++)
>> +                     if (pp->children[i].in_use &&
>> +                         pid == pp->children[i].process.pid)
>> +                             break;
>> +             if (i == pp->max_processes)
>> +                     /*
>> +                      * waitpid returned another process id
>> +                      * which we are not waiting for.
>> +                      */
>> +                     return;
>
> If we culled a child process that this machinery is not in charge
> of, waitpid() in other places that wants to see that child will not
> see it.  Perhaps such a situation might even warrant an error() or
> BUG()?  Do we want a "NEEDSWORK: Is this a bug?" comment here at
> least?
>
>> +             if (strbuf_read_once(&pp->children[i].err,
>> +                                  pp->children[i].process.err, 0) < 0 &&
>> +                 errno != EAGAIN)
>> +                     die_errno("strbuf_read_once");
>
> Don't we want to read thru to the end here?  The reason read_once()
> did not read thru to the end may not have anything to do with
> NONBLOCK (e.g. xread_nonblock() caps len, and it does not loop).

right.

>

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Torsten Bögershausen]

On 22.09.15 08:23, Jacob Keller wrote:
> On Mon, Sep 21, 2015 at 9:55 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> But in any case I suggest to  xread() as it is, and not to change the
>> functionality
>> behind the back of the users.
>>
>>
> 
> I don't think this patch actually changes behavior as it stands now. I
> think Junio's suggestion does. Personally, I'd prefer some sort of
> warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather
> see it somehow warn so that we can find the bug (since we really
> really shouldn't be calling xread with a blocking socket, especially
> if we have xread_noblock or similar as in this series.
> 
> Not sure if we really want to handle that, but I know we don't want to
> change external behavior of xread... I think that polling is better
> than the current "spinning" behavior.
> 
> Regards,
> Jake
Oh sorry for my comment, I mis-read the whole thing completely.

And yes, a warning would be better than a poll()

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Stefan Beller]

On Tue, Sep 22, 2015 at 11:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>>>         while (1) {
>>>>>                 nr = read(fd, buf, len);
>>>>> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>>>>> -                       continue;
>>>>> +               if (nr < 0) {
>>>>> +                       if (errno == EINTR)
>>>>> +                               continue;
>>>>> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
>>>>> +                               struct pollfd pfd;
>>>>> +                               int i;
>>>>> +                               pfd.events = POLLIN;
>>>>> +                               pfd.fd = fd;
>>>>> +                               i = poll(&pfd, 1, 100);
>>>>
>>>> Why is this poll() using a timeout? Isn't that still a busy wait of
>>>> sorts (even if less aggressive)?
>>
>> True. Maybe we could have just a warning for now?
>>
>>     if (errno == EAGAIN) {
>>         warning("Using xread with a non blocking fd");
>>         continue; /* preserve previous behavior */
>>     }
>
> It is very likely that you will hit the same EAGAIN immediately by
> continuing and end up spewing tons of the same warning, no?

Right.

>
> We may want to slow down and think before sending a knee-jerk
> response (this applies to both of us).

Earlier this year I was doing lots of work in Gerrit, which somehow
prevented crossing emails even though I was knee-jerking all the time.
Maybe I picked up habits in Gerrit land which are only positive in Gerrit
land, while rather harming in a mailing list base workflow such as here
in Git land.

>
> I actually think a blocking poll(2) here would not be such a bad
> idea.  It would preserves the previous behaviour for callers who
> unknowingly inherited a O_NONBLOCK file descriptor from its
> environment without forcing them to waste CPU cycles.

So rather a combination of both, with the warning only spewing every
5 seconds or such?

I mean we identified this as a potential bug which we want to fix eventually
by having the caller make sure they only pass in blocking fds.

I feel like this is similar to the && call chain detection Jeff made a few
months back. At first there were bugreports coming in left and right about
broken test scripts, now it's all fixed hopefully. The difference is
this is not in
the test suite though, so maybe we can rollout this patch early next cycle
and get all the bugs fixed before we get serious with a release again?

It's risky though. Maybe there is a legit use case for "I have a non blocking fd
for $REASONS and I know it, but I still want to read it now until it's done"
Actually I thought about doing that exactly myself as part of the cleanup in the
asynchronous paralllel processing. We'd collect progress information using
this strbuf_read_once and polling. Once a process is done, we would need to get
all its output (i.e. up to EOF), so then we could just call
strbuf_read as we know the
child has already terminated, so all we want is getting the last bytes
out of the pipe.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> I don't think this patch actually changes behavior as it stands now. I
> think Junio's suggestion does. Personally, I'd prefer some sort of
> warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather
> see it somehow warn so that we can find the bug (since we really
> really shouldn't be calling xread with a blocking socket, especially
> if we have xread_noblock or similar as in this series.

One caveat is that the caller may not know in the first place.

The last time I checked the existing callers of xread(), there were
a few that read from a file descriptor they did not open themselves
(e.g. unpack-objects that read from standard input).  The invoker of
these processes is free to do O_NONBLOCK their input stream for
whatever reason.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Jeff King]

On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > I don't think this patch actually changes behavior as it stands now. I
> > think Junio's suggestion does. Personally, I'd prefer some sort of
> > warning when you use xread and get EAGAIN or EWOULDBLOCK. I'd rather
> > see it somehow warn so that we can find the bug (since we really
> > really shouldn't be calling xread with a blocking socket, especially
> > if we have xread_noblock or similar as in this series.
> 
> One caveat is that the caller may not know in the first place.
> 
> The last time I checked the existing callers of xread(), there were
> a few that read from a file descriptor they did not open themselves
> (e.g. unpack-objects that read from standard input).  The invoker of
> these processes is free to do O_NONBLOCK their input stream for
> whatever reason.

Yeah. I do not think this is a bug at all; the user might have their
reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean
"try to read from fd until we get a real error, some data, or an EOF",
then it is perfectly reasonable to replace spinning on read() (which we
do now) with a poll() for efficiency. The caller (and the user) does not
have to care, and should not notice; the outcome will be the same.

-Peff

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So how would you find out when we are done?

	while (1) {
		if (we have available slot)
			ask to start a new one;
		if (nobody is running anymore)
                	break;
		collect the output from running ones;
                drain the output from the foreground one;
		cull the finished process;
        }

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:
>
>> One caveat is that the caller may not know in the first place.
>> 
>> The last time I checked the existing callers of xread(), there were
>> a few that read from a file descriptor they did not open themselves
>> (e.g. unpack-objects that read from standard input).  The invoker of
>> these processes is free to do O_NONBLOCK their input stream for
>> whatever reason.
>
> Yeah. I do not think this is a bug at all; the user might have their
> reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean
> "try to read from fd until we get a real error, some data, or an EOF",
> then it is perfectly reasonable to replace spinning on read() (which we
> do now) with a poll() for efficiency. The caller (and the user) does not
> have to care, and should not notice; the outcome will be the same.

I think we are in agreement, and that answers the question/guidance
Stefan asked earlier in $gmane/278414, which was:

> So rather a combination of both, with the warning only spewing every
> 5 seconds or such?

and the answer obviously is "No warning, do a poll() without timeout
to block".

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Stefan Beller]

On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So how would you find out when we are done?
>
>         while (1) {
>                 if (we have available slot)
>                         ask to start a new one;
>                 if (nobody is running anymore)
>                         break;
>                 collect the output from running ones;
>                 drain the output from the foreground one;
>                 cull the finished process;
>         }
>

Personally I do not like the break; in the middle of
the loop, but that's personal preference, I'd guess.
I'll also move the condition for (we have available slot)
back inside the called function.

So I am thinking about using this in the reroll instead:

    run_processes_parallel_start_as_needed(&pp);
    while (pp.nr_processes > 0) {
        run_processes_parallel_buffer_stderr(&pp);
        run_processes_parallel_output(&pp);
        run_processes_parallel_collect_finished(&pp);
        run_processes_parallel_start_as_needed(&pp);
    }


This eliminates the need for the flag and also exits the loop just
after the possible startup of new processes.

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> So how would you find out when we are done?
>>
>>         while (1) {
>>                 if (we have available slot)
>>                         ask to start a new one;
>>                 if (nobody is running anymore)
>>                         break;
>>                 collect the output from running ones;
>>                 drain the output from the foreground one;
>>                 cull the finished process;
>>         }
>>
>
> Personally I do not like the break; in the middle of
> the loop, but that's personal preference, I'd guess.
> I'll also move the condition for (we have available slot)
> back inside the called function.
>
> So I am thinking about using this in the reroll instead:
>
>     run_processes_parallel_start_as_needed(&pp);
>     while (pp.nr_processes > 0) {
>         run_processes_parallel_buffer_stderr(&pp);
>         run_processes_parallel_output(&pp);
>         run_processes_parallel_collect_finished(&pp);
>         run_processes_parallel_start_as_needed(&pp);
>     }

If you truly think the latter is easier to follow its logic (with
the duplicated call to the same function only to avoid break that
makes it clear why we are quitting the loop, and without the
explicit "start only if we can afford to"), then I have to say that
our design tastes are fundamentally incompatible.  I have nothing
more to add.

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Stefan Beller]

On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> So how would you find out when we are done?
>>>
>>>         while (1) {
>>>                 if (we have available slot)
>>>                         ask to start a new one;
>>>                 if (nobody is running anymore)
>>>                         break;
>>>                 collect the output from running ones;
>>>                 drain the output from the foreground one;
>>>                 cull the finished process;
>>>         }
>>>
>>
>> Personally I do not like the break; in the middle of
>> the loop, but that's personal preference, I'd guess.
>> I'll also move the condition for (we have available slot)
>> back inside the called function.
>>
>> So I am thinking about using this in the reroll instead:
>>
>>     run_processes_parallel_start_as_needed(&pp);
>>     while (pp.nr_processes > 0) {
>>         run_processes_parallel_buffer_stderr(&pp);
>>         run_processes_parallel_output(&pp);
>>         run_processes_parallel_collect_finished(&pp);
>>         run_processes_parallel_start_as_needed(&pp);
>>     }
>
> If you truly think the latter is easier to follow its logic (with
> the duplicated call to the same function only to avoid break that
> makes it clear why we are quitting the loop,

I dislike having the call twice, too.

> and without the
> explicit "start only if we can afford to"), then I have to say that
> our design tastes are fundamentally incompatible.

Well the "start only if we can afford to" is not as easy as just

>                 if (we have available slot)
>                         ask to start a new one;

because that's only half the condition. If we don't start a new one
as the callback get_next_task returned without a new task.
So it becomes

    while (pp.nr_processes > 0) {
        while (pp->nr_processes < pp->max_processes &&
                start_one_process(&pp))
                        ; /* nothing */
        if (!pp.nr_processes)
            break;
        buffer(..);
        output_live_child(..);
        cull_finished(..);
    }

I'll think about that.

Re: [PATCHv3 06/13] run-command: add an asynchronous parallel child processor

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Sep 22, 2015 at 2:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Tue, Sep 22, 2015 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Stefan Beller <sbeller@google.com> writes:
>>>>
>>>>> So how would you find out when we are done?
>>>>
>>>>         while (1) {
>>>>                 if (we have available slot)
>>>>                         ask to start a new one;
>>>>                 if (nobody is running anymore)
>>>>                         break;
>>>>                 collect the output from running ones;
>>>>                 drain the output from the foreground one;
>>>>                 cull the finished process;
>>>>         }
>>>>
>>>
>>> Personally I do not like the break; in the middle of
>>> the loop, but that's personal preference, I'd guess.
>>> I'll also move the condition for (we have available slot)
>>> back inside the called function.
>>>
>>> So I am thinking about using this in the reroll instead:
>>>
>>>     run_processes_parallel_start_as_needed(&pp);
>>>     while (pp.nr_processes > 0) {
>>>         run_processes_parallel_buffer_stderr(&pp);
>>>         run_processes_parallel_output(&pp);
>>>         run_processes_parallel_collect_finished(&pp);
>>>         run_processes_parallel_start_as_needed(&pp);
>>>     }
>>
>> If you truly think the latter is easier to follow its logic (with
>> the duplicated call to the same function only to avoid break that
>> makes it clear why we are quitting the loop,
>
> I dislike having the call twice, too.
> ...
>> and without the
>> explicit "start only if we can afford to"), then I have to say that
>> our design tastes are fundamentally incompatible.
> ...
> I'll think about that.

Don't waste too much time on it ;-)  This is largely a taste thing,
and taste is very hard to reason about, understand, teach and learn.

Having said that, if I can pick one thing that I foresee to be
problematic the most (aside from these overlong names of the
functions that are private and do not need such long names), it is
that "start as many without giving any control to the caller"
interface.  I wrote "start *a* new one" in the outline above for a
reason.

Even if you want to utilize a moderate number of processes, say 16,
working at the steady state, I suspect that we would find it
suboptimal from perceived latency point of view, if we spin up 16
processes all at once at the very beginning before we start to
collect output from the designated foreground process and relay its
first line of output.  We may want to be able to tweak the caller to
spin up just a few first and let the loop ramp up to the full blast
gradually so that we can give an early feedback to the end user.
That is not something easy to arrange with "start as many without
giving control to the caller" interface.  We probably will discover
other kinds of scheduling issues once we get familiar with this
machinery and would find need for finer grained control.

And I consider such a ramp-up logic shouldn't be hidden inside the
"start_as_needed()" helper function---it is the way how the calling
loop wants its processes started, and I'd prefer to have the logic
visible in the caller's loop.

But that is also largely a "taste" thing.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Stefan Beller]

On Tue, Sep 22, 2015 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Tue, Sep 22, 2015 at 12:45:51PM -0700, Junio C Hamano wrote:
>>
>>> One caveat is that the caller may not know in the first place.
>>>
>>> The last time I checked the existing callers of xread(), there were
>>> a few that read from a file descriptor they did not open themselves
>>> (e.g. unpack-objects that read from standard input).  The invoker of
>>> these processes is free to do O_NONBLOCK their input stream for
>>> whatever reason.
>>
>> Yeah. I do not think this is a bug at all; the user might have their
>> reasons for handing off an O_NONBLOCK pipe. If we take xread() to mean
>> "try to read from fd until we get a real error, some data, or an EOF",
>> then it is perfectly reasonable to replace spinning on read() (which we
>> do now) with a poll() for efficiency. The caller (and the user) does not
>> have to care, and should not notice; the outcome will be the same.
>
> I think we are in agreement, and that answers the question/guidance
> Stefan asked earlier in $gmane/278414, which was:
>
>> So rather a combination of both, with the warning only spewing every
>> 5 seconds or such?
>
> and the answer obviously is "No warning, do a poll() without timeout
> to block".

Ok. Expect that in a reroll.

To answer the first question upthread:

> I can sort of see EINTR but why is ENOMEM any special than other errors?

So we have 4 kinds of additional errors, when adding the poll. When we want to
keep the behavior as close to the original as possible, we should not error out
for things we had under control previously.

       EFAULT The array given as argument was not contained in the
calling program's address space.

       EINTR  A signal occurred before any requested event; see signal(7).

       EINVAL The nfds value exceeds the RLIMIT_NOFILE value.

       ENOMEM There was no space to allocate file descriptor tables.

I don't see a way EFAULT can be returned as we have the array hard
coded on the stack.
(No tricks with the heap or anything, we'll have it on the stack)

EINTR is possible while waiting. Actually it doesn't matter if we
retry the poll or read first
and then poll again, so we can easily just continue and do the read.

EINVAL is highly unlikely (as nfds == 1 in the case here), but can happen.
ENOMEM is similar to EINVAL.

We should not care if the call to poll failed, as we're in an infinite loop and
can only get out with the correct read(..). So maybe an implementation like this
would already suffice:

ssize_t xread(int fd, void *buf, size_t len)
{
    ssize_t nr;
    if (len > MAX_IO_SIZE)
        len = MAX_IO_SIZE;
    while (1) {
        nr = read(fd, buf, len);
        if (nr < 0) {
            if (errno == EINTR)
                continue;
            if (errno == EAGAIN || errno == EWOULDBLOCK) {
                struct pollfd pfd;
                pfd.events = POLLIN;
                pfd.fd = fd;
                /* We deliberately ignore the return value of poll. */
                poll(&pfd, 1, -1);
                continue;
            }
        }
        return nr;
    }
}

In the resend I'll only check for EINTR and in other cases of early poll
failure, we just die(..).

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> We should not care if the call to poll failed, as we're in an infinite loop and
> can only get out with the correct read(..).

I think I agree with that reasoning.  The only thing we want out of
this call to poll(2) is to delay calling read(2) again when we know
we would get EAGAIN again; whether the reason why poll(2) returned
is because of some error or because some data has become ready on
the file descriptor, we no longer are in that "we know we would get
EAGAIN" situation and do want to call read(2), which would return
either the right error or perform a successful read to make some
progress.

Sounds sensible.

Re: [PATCHv3 02/13] xread: poll on non blocking fds

[Jeff King]

On Tue, Sep 22, 2015 at 05:14:42PM -0700, Stefan Beller wrote:

> We should not care if the call to poll failed, as we're in an infinite loop and
> can only get out with the correct read(..). So maybe an implementation like this
> would already suffice:
> 
> ssize_t xread(int fd, void *buf, size_t len)
> {
>     ssize_t nr;
>     if (len > MAX_IO_SIZE)
>         len = MAX_IO_SIZE;
>     while (1) {
>         nr = read(fd, buf, len);
>         if (nr < 0) {
>             if (errno == EINTR)
>                 continue;
>             if (errno == EAGAIN || errno == EWOULDBLOCK) {
>                 struct pollfd pfd;
>                 pfd.events = POLLIN;
>                 pfd.fd = fd;
>                 /* We deliberately ignore the return value of poll. */
>                 poll(&pfd, 1, -1);
>                 continue;
>             }
>         }
>         return nr;
>     }
> }

FWIW, that is what I had imagined.

-Peff