[PATCH 0/5] fork/exec removal series

[PATCH 0/5] fork/exec removal series

[Johannes Sixt]

Here is a series of patches that removes a number fork/exec pairs.
They are replaced by delegating to start_command/finish_command/run_command.
You can regard this as the beginning of the MinGW port integration.

There still remain a few forks, which fall into these categories:

- They are in tools or code that are not (yet) ported to MinGW.[*]

- The fork()s are not followed by exec(). These need a different
  implementation. I am thinking of a start_coroutine()/finish_coroutine()
  API that is implemented with threads in MinGW. (Suggestions of a better
  as well as implementations are welcome.)

- The upload-pack case calls for an entirely different solution.

[*] Just this weekend Mike Pape ported git-daemon, but I didn't find the
time to have it integrated in this series - if that were possible at all.

Patch 2 depends on Patch 1; otherwise, there are no dependencies.
The series goes on top of next (it touches str_buf stuff in connect.c).

 builtin-archive.c    |    8 +--
 builtin-fetch-pack.c |   63 ++++++++-----------------
 cache.h              |    4 +-
 connect.c            |  126 ++++++++++++++++++++++++--------------------------
 convert.c            |   36 ++++----------
 diff.c               |   38 +--------------
 peek-remote.c        |    8 +--
 send-pack.c          |    8 +--
 transport.c          |    9 +---
 9 files changed, 106 insertions(+), 194 deletions(-)

-- Hannes

[PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Sixt]

This prepares the API of git_connect() and finish_connect() to operate on
a struct child_process. Currently, we just use that object as a placeholder
for the pid that we used to return. A follow-up patch will change the
implementation of git_connect() and finish_connect() to make full use
of the object.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-archive.c    |    8 +++-----
 builtin-fetch-pack.c |    8 +++-----
 cache.h              |    4 ++--
 connect.c            |   28 +++++++++++++++-------------
 peek-remote.c        |    8 +++-----
 send-pack.c          |    8 +++-----
 transport.c          |    9 ++-------
 7 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 04385de..76db6cf 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
 {
 	char *url, buf[LARGE_PACKET_MAX];
 	int fd[2], i, len, rv;
-	pid_t pid;
+	struct child_process *chld;
 	const char *exec = "git-upload-archive";
 	int exec_at = 0;
 
@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	}
 
 	url = xstrdup(remote);
-	pid = git_connect(fd, url, exec, 0);
-	if (pid < 0)
-		return pid;
+	chld = git_connect(fd, url, exec, 0);
 
 	for (i = 1; i < argc; i++) {
 		if (i == exec_at)
@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	rv = recv_sideband("archive", fd[0], 1, 2);
 	close(fd[0]);
 	close(fd[1]);
-	rv |= finish_connect(pid);
+	rv |= finish_connect(chld);
 
 	return !!rv;
 }
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 8f25d50..19c144d 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -762,7 +762,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 {
 	int i, ret;
 	int fd[2];
-	pid_t pid;
+	struct child_process *chld;
 	struct ref *ref;
 	struct stat st;
 
@@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 			st.st_mtime = 0;
 	}
 
-	pid = git_connect(fd, (char *)dest, uploadpack,
+	chld = git_connect(fd, (char *)dest, uploadpack,
                           args.verbose ? CONNECT_VERBOSE : 0);
-	if (pid < 0)
-		return NULL;
 	if (heads && nr_heads)
 		nr_heads = remove_duplicates(nr_heads, heads);
 	ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);
 	close(fd[0]);
 	close(fd[1]);
-	ret = finish_connect(pid);
+	ret = finish_connect(chld);
 
 	if (!ret && nr_heads) {
 		/* If the heads to pull were given, we should have
diff --git a/cache.h b/cache.h
index 27485d3..32ce8a7 100644
--- a/cache.h
+++ b/cache.h
@@ -503,8 +503,8 @@ struct ref {
 #define REF_TAGS	(1u << 2)
 
 #define CONNECT_VERBOSE       (1u << 0)
-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
-extern int finish_connect(pid_t pid);
+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);
+extern int finish_connect(struct child_process *chld);
 extern int path_match(const char *path, int nr, char **match);
 extern int get_ack(int fd, unsigned char *result_sha1);
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);
diff --git a/connect.c b/connect.c
index 3d5c4ab..f6dcab9 100644
--- a/connect.c
+++ b/connect.c
@@ -468,21 +468,22 @@ char *get_port(char *host)
 }
 
 /*
- * This returns 0 if the transport protocol does not need fork(2),
+ * This returns NULL if the transport protocol does not need fork(2),
  * or a process id if it does.  Once done, finish the connection
  * with finish_connect() with the value returned from this function
- * (it is safe to call finish_connect() with 0 to support the former
+ * (it is safe to call finish_connect() with NULL to support the former
  * case).
  *
- * Does not return a negative value on error; it just dies.
+ * If it returns, the connect is successful; it just dies on errors.
  */
-pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
+struct child_process *git_connect(int fd[2], char *url,
+				  const char *prog, int flags)
 {
 	char *host, *path = url;
 	char *end;
 	int c;
 	int pipefd[2][2];
-	pid_t pid;
+	struct child_process *chld;
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;
 	char *port = NULL;
@@ -568,15 +569,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 		free(target_host);
 		if (free_path)
 			free(path);
-		return 0;
+		return NULL;
 	}
 
 	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
 		die("unable to create pipe pair for communication");
-	pid = fork();
-	if (pid < 0)
+	chld = xcalloc(1, sizeof(*chld));
+	chld->pid = fork();
+	if (chld->pid < 0)
 		die("unable to fork");
-	if (!pid) {
+	if (!chld->pid) {
 		struct strbuf cmd;
 
 		strbuf_init(&cmd, MAX_CMD_LEN);
@@ -625,15 +627,15 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 	close(pipefd[1][0]);
 	if (free_path)
 		free(path);
-	return pid;
+	return chld;
 }
 
-int finish_connect(pid_t pid)
+int finish_connect(struct child_process *chld)
 {
-	if (pid == 0)
+	if (chld == NULL)
 		return 0;
 
-	while (waitpid(pid, NULL, 0) < 0) {
+	while (waitpid(chld->pid, NULL, 0) < 0) {
 		if (errno != EINTR)
 			return -1;
 	}
diff --git a/peek-remote.c b/peek-remote.c
index ceb7871..59cc8eb 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -25,7 +25,7 @@ int main(int argc, char **argv)
 	int i, ret;
 	char *dest = NULL;
 	int fd[2];
-	pid_t pid;
+	struct child_process *chld;
 	int nongit = 0;
 	unsigned flags = 0;
 
@@ -64,12 +64,10 @@ int main(int argc, char **argv)
 	if (!dest || i != argc - 1)
 		usage(peek_remote_usage);
 
-	pid = git_connect(fd, dest, uploadpack, 0);
-	if (pid < 0)
-		return 1;
+	chld = git_connect(fd, dest, uploadpack, 0);
 	ret = peek_remote(fd, flags);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret |= finish_connect(chld);
 	return !!ret;
 }
diff --git a/send-pack.c b/send-pack.c
index 4533d1b..e9257fd 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -362,7 +362,7 @@ int main(int argc, char **argv)
 	char *dest = NULL;
 	char **heads = NULL;
 	int fd[2], ret;
-	pid_t pid;
+	struct child_process *chld;
 	char *remote_name = NULL;
 	struct remote *remote = NULL;
 
@@ -426,12 +426,10 @@ int main(int argc, char **argv)
 		}
 	}
 
-	pid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
-	if (pid < 0)
-		return 1;
+	chld = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
 	ret = send_pack(fd[0], fd[1], remote, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret |= finish_connect(chld);
 	return !!ret;
 }
diff --git a/transport.c b/transport.c
index 3475cca..54ed9bc 100644
--- a/transport.c
+++ b/transport.c
@@ -279,18 +279,13 @@ static struct ref *get_refs_via_connect(const struct transport *transport)
 	struct git_transport_data *data = transport->data;
 	struct ref *refs;
 	int fd[2];
-	pid_t pid;
 	char *dest = xstrdup(transport->url);
-
-	pid = git_connect(fd, dest, data->uploadpack, 0);
-
-	if (pid < 0)
-		die("Failed to connect to \"%s\"", transport->url);
+	struct child_process *chld = git_connect(fd, dest, data->uploadpack, 0);
 
 	get_remote_heads(fd[0], &refs, 0, NULL, 0);
 	packet_flush(fd[1]);
 
-	finish_connect(pid);
+	finish_connect(chld);
 
 	free(dest);
 
-- 
1.5.3.3.1134.gee562

[PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.

[Johannes Sixt]

The child process handling is delegated to start_command() and
finish_command().

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 connect.c |  104 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 49 insertions(+), 55 deletions(-)

diff --git a/connect.c b/connect.c
index f6dcab9..29fd431 100644
--- a/connect.c
+++ b/connect.c
@@ -482,11 +482,12 @@ struct child_process *git_connect(int fd[2], char *url,
 	char *host, *path = url;
 	char *end;
 	int c;
-	int pipefd[2][2];
 	struct child_process *chld;
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;
 	char *port = NULL;
+	const char **arg;
+	struct strbuf cmd;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -572,72 +573,65 @@ struct child_process *git_connect(int fd[2], char *url,
 		return NULL;
 	}
 
-	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
-		die("unable to create pipe pair for communication");
 	chld = xcalloc(1, sizeof(*chld));
-	chld->pid = fork();
-	if (chld->pid < 0)
-		die("unable to fork");
-	if (!chld->pid) {
-		struct strbuf cmd;
-
-		strbuf_init(&cmd, MAX_CMD_LEN);
-		strbuf_addstr(&cmd, prog);
-		strbuf_addch(&cmd, ' ');
-		sq_quote_buf(&cmd, path);
-		if (cmd.len >= MAX_CMD_LEN)
-			die("command line too long");
-
-		dup2(pipefd[1][0], 0);
-		dup2(pipefd[0][1], 1);
-		close(pipefd[0][0]);
-		close(pipefd[0][1]);
-		close(pipefd[1][0]);
-		close(pipefd[1][1]);
-		if (protocol == PROTO_SSH) {
-			const char *ssh, *ssh_basename;
-			ssh = getenv("GIT_SSH");
-			if (!ssh) ssh = "ssh";
-			ssh_basename = strrchr(ssh, '/');
-			if (!ssh_basename)
-				ssh_basename = ssh;
-			else
-				ssh_basename++;
 
-			if (!port)
-				execlp(ssh, ssh_basename, host, cmd.buf, NULL);
-			else
-				execlp(ssh, ssh_basename, "-p", port, host,
-				       cmd.buf, NULL);
+	strbuf_init(&cmd, MAX_CMD_LEN);
+	strbuf_addstr(&cmd, prog);
+	strbuf_addch(&cmd, ' ');
+	sq_quote_buf(&cmd, path);
+	if (cmd.len >= MAX_CMD_LEN)
+		die("command line too long");
+
+	chld->in = chld->out = -1;
+	chld->argv = arg = xcalloc(6, sizeof(*arg));
+	if (protocol == PROTO_SSH) {
+		const char *ssh = getenv("GIT_SSH");
+		if (!ssh) ssh = "ssh";
+
+		*arg++ = ssh;
+		if (port) {
+			*arg++ = "-p";
+			*arg++ = port;
 		}
-		else {
-			unsetenv(ALTERNATE_DB_ENVIRONMENT);
-			unsetenv(DB_ENVIRONMENT);
-			unsetenv(GIT_DIR_ENVIRONMENT);
-			unsetenv(GIT_WORK_TREE_ENVIRONMENT);
-			unsetenv(GRAFT_ENVIRONMENT);
-			unsetenv(INDEX_ENVIRONMENT);
-			execlp("sh", "sh", "-c", cmd.buf, NULL);
-		}
-		die("exec failed");
+		*arg++ = host;
+	}
+	else {
+		/* remove these from the environment */
+		const char *env[] = {
+			ALTERNATE_DB_ENVIRONMENT,
+			DB_ENVIRONMENT,
+			GIT_DIR_ENVIRONMENT,
+			GIT_WORK_TREE_ENVIRONMENT,
+			GRAFT_ENVIRONMENT,
+			INDEX_ENVIRONMENT,
+			NULL
+		};
+		chld->env = env;
+		*arg++ = "sh";
+		*arg++ = "-c";
 	}
-	fd[0] = pipefd[0][0];
-	fd[1] = pipefd[1][1];
-	close(pipefd[0][1]);
-	close(pipefd[1][0]);
+	*arg++ = cmd.buf;
+	*arg = NULL;
+
+	if (start_command(chld))
+		die("unable to fork");
+
+	fd[0] = chld->out; /* read from child's stdout */
+	fd[1] = chld->in;  /* write to child's stdin */
 	if (free_path)
 		free(path);
+	strbuf_release(&cmd);
 	return chld;
 }
 
 int finish_connect(struct child_process *chld)
 {
+	int code;
 	if (chld == NULL)
 		return 0;
 
-	while (waitpid(chld->pid, NULL, 0) < 0) {
-		if (errno != EINTR)
-			return -1;
-	}
-	return 0;
+	code = finish_command(chld);
+	free(chld->argv);
+	free(chld);
+	return code;
 }
-- 
1.5.3.3.1134.gee562

[PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 convert.c |   36 ++++++++++--------------------------
 1 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/convert.c b/convert.c
index 0d5e909..560679d 100644
--- a/convert.c
+++ b/convert.c
@@ -196,40 +196,24 @@ static int filter_buffer(const char *path, const char *src,
 	/*
 	 * Spawn cmd and feed the buffer contents through its stdin.
 	 */
-	struct child_process child_process;
-	int pipe_feed[2];
+	struct child_process chld;
 	int write_err, status;
+	const char *argv[] = { "sh", "-c", cmd, NULL };
 
-	memset(&child_process, 0, sizeof(child_process));
+	memset(&chld, 0, sizeof(chld));
+	chld.argv = argv;
+	chld.in = -1;
 
-	if (pipe(pipe_feed) < 0) {
-		error("cannot create pipe to run external filter %s", cmd);
-		return 1;
-	}
+	if (start_command(&chld))
+		return error("cannot fork to run external filter %s", cmd);
 
-	child_process.pid = fork();
-	if (child_process.pid < 0) {
-		error("cannot fork to run external filter %s", cmd);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		return 1;
-	}
-	if (!child_process.pid) {
-		dup2(pipe_feed[0], 0);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		execlp("sh", "sh", "-c", cmd, NULL);
-		return 1;
-	}
-	close(pipe_feed[0]);
-
-	write_err = (write_in_full(pipe_feed[1], src, size) < 0);
-	if (close(pipe_feed[1]))
+	write_err = (write_in_full(chld.in, src, size) < 0);
+	if (close(chld.in))
 		write_err = 1;
 	if (write_err)
 		error("cannot feed the input to external filter %s", cmd);
 
-	status = finish_command(&child_process);
+	status = finish_command(&chld);
 	if (status)
 		error("external filter %s failed %d", cmd, -status);
 	return (write_err || status);
-- 
1.5.3.3.1134.gee562

[PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 diff.c |   38 +++-----------------------------------
 1 files changed, 3 insertions(+), 35 deletions(-)

diff --git a/diff.c b/diff.c
index 0bd7e24..29dfa82 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,7 @@
 #include "xdiff-interface.h"
 #include "color.h"
 #include "attr.h"
+#include "run-command.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1748,40 +1749,6 @@ static void remove_tempfile_on_signal(int signo)
 	raise(signo);
 }
 
-static int spawn_prog(const char *pgm, const char **arg)
-{
-	pid_t pid;
-	int status;
-
-	fflush(NULL);
-	pid = fork();
-	if (pid < 0)
-		die("unable to fork");
-	if (!pid) {
-		execvp(pgm, (char *const*) arg);
-		exit(255);
-	}
-
-	while (waitpid(pid, &status, 0) < 0) {
-		if (errno == EINTR)
-			continue;
-		return -1;
-	}
-
-	/* Earlier we did not check the exit status because
-	 * diff exits non-zero if files are different, and
-	 * we are not interested in knowing that.  It was a
-	 * mistake which made it harder to quit a diff-*
-	 * session that uses the git-apply-patch-script as
-	 * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF
-	 * should also exit non-zero only when it wants to
-	 * abort the entire diff-* session.
-	 */
-	if (WIFEXITED(status) && !WEXITSTATUS(status))
-		return 0;
-	return -1;
-}
-
 /* An external diff command takes:
  *
  * diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -1834,7 +1801,8 @@ static void run_external_diff(const char *pgm,
 		*arg++ = name;
 	}
 	*arg = NULL;
-	retval = spawn_prog(pgm, spawn_arg);
+	fflush(NULL);
+	retval = run_command_v_opt(spawn_arg, 0);
 	remove_tempfile();
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
-- 
1.5.3.3.1134.gee562

[PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-fetch-pack.c |   55 ++++++++++++++-----------------------------------
 1 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 19c144d..d0eca2d 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -7,6 +7,7 @@
 #include "pack.h"
 #include "sideband.h"
 #include "fetch-pack.h"
+#include "run-command.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -499,11 +500,13 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	char hdr_arg[256];
 	const char **av;
 	int do_keep = args.keep_pack;
-	int keep_pipe[2];
+	struct child_process cmd;
 
 	side_pid = setup_sideband(fd, xd);
 
-	av = argv;
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.argv = av = argv;
+
 	*hdr_arg = 0;
 	if (!args.keep_pack && unpack_limit) {
 		struct pack_header header;
@@ -519,8 +522,8 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	}
 
 	if (do_keep) {
-		if (pack_lockfile && pipe(keep_pipe))
-			die("fetch-pack: pipe setup failure: %s", strerror(errno));
+		if (pack_lockfile)
+			cmd.out = -1;
 		*av++ = "index-pack";
 		*av++ = "--stdin";
 		if (!args.quiet && !args.no_progress)
@@ -544,43 +547,17 @@ static int get_pack(int xd[2], char **pack_lockfile)
 		*av++ = hdr_arg;
 	*av++ = NULL;
 
-	pid = fork();
-	if (pid < 0)
+	cmd.in = fd[0];
+	cmd.git_cmd = 1;
+	if (start_command(&cmd))
 		die("fetch-pack: unable to fork off %s", argv[0]);
-	if (!pid) {
-		dup2(fd[0], 0);
-		if (do_keep && pack_lockfile) {
-			dup2(keep_pipe[1], 1);
-			close(keep_pipe[0]);
-			close(keep_pipe[1]);
-		}
-		close(fd[0]);
-		close(fd[1]);
-		execv_git_cmd(argv);
-		die("%s exec failed", argv[0]);
-	}
-	close(fd[0]);
 	close(fd[1]);
-	if (do_keep && pack_lockfile) {
-		close(keep_pipe[1]);
-		*pack_lockfile = index_pack_lockfile(keep_pipe[0]);
-		close(keep_pipe[0]);
-	}
-	while (waitpid(pid, &status, 0) < 0) {
-		if (errno != EINTR)
-			die("waiting for %s: %s", argv[0], strerror(errno));
-	}
-	if (WIFEXITED(status)) {
-		int code = WEXITSTATUS(status);
-		if (code)
-			die("%s died with error code %d", argv[0], code);
-		return 0;
-	}
-	if (WIFSIGNALED(status)) {
-		int sig = WTERMSIG(status);
-		die("%s died of signal %d", argv[0], sig);
-	}
-	die("%s died of unnatural causes %d", argv[0], status);
+	if (do_keep && pack_lockfile)
+		*pack_lockfile = index_pack_lockfile(cmd.out);
+
+	if (finish_command(&cmd))
+		die("%s failed", argv[0]);
+	return 0;
 }
 
 static struct ref *do_fetch_pack(int fd[2],
-- 
1.5.3.3.1134.gee562

Re: [PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.

[Johannes Schindelin]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

>  diff.c |   38 +++-----------------------------------
>  1 files changed, 3 insertions(+), 35 deletions(-)

_Nice_!

Ciao,
Dscho

Re: [PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.

[Johannes Schindelin]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

> -	struct child_process child_process;
> +	struct child_process chld;

Why?  This patch is less readable because of that rename.

Ciao,
Dscho

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> This prepares the API of git_connect() and finish_connect() to operate on
> a struct child_process. Currently, we just use that object as a placeholder
> for the pid that we used to return. A follow-up patch will change the
> implementation of git_connect() and finish_connect() to make full use
> of the object.

Good description, except removal of checks for negative return
of the calling sites raised my eyebrow and had me spend a few
more minutes to review than necessary (see below).

> diff --git a/builtin-archive.c b/builtin-archive.c
> index 04385de..76db6cf 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
>  {
>  	char *url, buf[LARGE_PACKET_MAX];
>  	int fd[2], i, len, rv;
> -	pid_t pid;
> +	struct child_process *chld;

Is "child" a reserved keyword or something that we need to avoid
its use as an identifier?

>  	const char *exec = "git-upload-archive";
>  	int exec_at = 0;
>  
> @@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
>  	}
>  
>  	url = xstrdup(remote);
> -	pid = git_connect(fd, url, exec, 0);
> -	if (pid < 0)
> -		return pid;
> +	chld = git_connect(fd, url, exec, 0);
>  

Interesting.  This and other callers of git_connect() were
prepared to see git_connect() to report errors with negative PID
but the callee simply died --- so this change is not regressing
by removing an early error return.  It would be better to have
something like this in the commit log message:

	Old code had early-return-on-error checks at the calling
	sites of git_connect(), but the callee simply died on
	errors without returning negative values.  This patch
	removes such bogosity.

> diff --git a/connect.c b/connect.c
> index 3d5c4ab..f6dcab9 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -468,21 +468,22 @@ char *get_port(char *host)
>  }
>  
>  /*
> - * This returns 0 if the transport protocol does not need fork(2),
> + * This returns NULL if the transport protocol does not need fork(2),
>   * or a process id if it does.  Once done, finish the connection

It does not return a process ID anymore, so this comment needs
to be updated.  Instead, you now return a struct child_process
that is newly allocated, and needs to be deallocated somehow.
At the end of finish_connect() might be a good place to do so.

>   * with finish_connect() with the value returned from this function
> - * (it is safe to call finish_connect() with 0 to support the former
> + * (it is safe to call finish_connect() with NULL to support the former
>   * case).
>   *
> - * Does not return a negative value on error; it just dies.
> + * If it returns, the connect is successful; it just dies on errors.
>   */
> -pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
> +struct child_process *git_connect(int fd[2], char *url,
> +				  const char *prog, int flags)
>  {
> ...
> -int finish_connect(pid_t pid)
> +int finish_connect(struct child_process *chld)
>  {
> -	if (pid == 0)
> +	if (chld == NULL)
>  		return 0;
>  
> -	while (waitpid(pid, NULL, 0) < 0) {
> +	while (waitpid(chld->pid, NULL, 0) < 0) {
>  		if (errno != EINTR)
>  			return -1;
>  	}

But it seems you don't, leaking the struct.  I see this is fixed
in the next patch in the series, but it would be nicer to have
the free from the very beginning.

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Sixt]

On Sunday 30 September 2007 22:43, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > +	struct child_process *chld;
>
> Is "child" a reserved keyword or something that we need to avoid
> its use as an identifier?

Allow me to call it "conn": We are using this as a "connection object".

Will integrate your and Dscho's suggestions and resend when the strbuf series 
appears in master.

-- Hannes

[PATCH 0/5, resend] fork/exec removal series

[Johannes Sixt]

Here is a series of patches that removes a number fork/exec pairs.
They are replaced by delegating to start_command/finish_command/run_command.

I've addressed all issues that were raised. However, concerning whether
git_connect() shoud return NULL or not, I decided to stay with my earlier
approach to return NULL for non-forking connections. Consequently, I had to
remove all error checks because there is now no unique value that can
indicate failure. The motivation is that the alternate approach (to always
return non-NULL for any successful connection and NULL for failure) would
benefit *only* libification; but *if* that ever happens, a major audit
(and possibly) rewrite in the callers has to be done anyway because
currently we are juggling, leaking, and double-closing file descriptors
like mad - in error paths and normal paths.

Patch 2 depends on Patch 1; otherwise, there are no dependencies.
The series goes on top of 'master'. Note that 'next' has an additional
call of git_connect() in the new transport.c.

 builtin-archive.c |    8 +--
 cache.h           |    4 +-
 connect.c         |  128 +++++++++++++++++++++++++---------------------------
 convert.c         |   30 +++----------
 diff.c            |   38 +--------------
 fetch-pack.c      |   43 +++++------------
 peek-remote.c     |    8 +--
 send-pack.c       |    8 +--
 8 files changed, 96 insertions(+), 171 deletions(-)

-- Hannes

[PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Sixt]

This prepares the API of git_connect() and finish_connect() to operate on
a struct child_process. Currently, we just use that object as a placeholder
for the pid that we used to return. A follow-up patch will change the
implementation of git_connect() and finish_connect() to make full use
of the object.

Old code had early-return-on-error checks at the calling sites of
git_connect(), but since git_connect() dies on errors anyway, these checks
were removed.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
	I don't replace the waitpid() in finish_connect() by
	finish_command() because (1) that's not the purpose of this patch
	and (2) there is no corresponding start_command(), yet.

	-- Hannes

 builtin-archive.c |    8 +++-----
 cache.h           |    4 ++--
 connect.c         |   31 +++++++++++++++++--------------
 fetch-pack.c      |    8 +++-----
 peek-remote.c     |    8 +++-----
 send-pack.c       |    8 +++-----
 6 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/builtin-archive.c b/builtin-archive.c
index 04385de..76f8d3d 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
 {
 	char *url, buf[LARGE_PACKET_MAX];
 	int fd[2], i, len, rv;
-	pid_t pid;
+	struct child_process *conn;
 	const char *exec = "git-upload-archive";
 	int exec_at = 0;
 
@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	}
 
 	url = xstrdup(remote);
-	pid = git_connect(fd, url, exec, 0);
-	if (pid < 0)
-		return pid;
+	conn = git_connect(fd, url, exec, 0);
 
 	for (i = 1; i < argc; i++) {
 		if (i == exec_at)
@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	rv = recv_sideband("archive", fd[0], 1, 2);
 	close(fd[0]);
 	close(fd[1]);
-	rv |= finish_connect(pid);
+	rv |= finish_connect(conn);
 
 	return !!rv;
 }
diff --git a/cache.h b/cache.h
index e0abcd6..548fb52 100644
--- a/cache.h
+++ b/cache.h
@@ -502,8 +502,8 @@ struct ref {
 #define REF_TAGS	(1u << 2)
 
 #define CONNECT_VERBOSE       (1u << 0)
-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
-extern int finish_connect(pid_t pid);
+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);
+extern int finish_connect(struct child_process *conn);
 extern int path_match(const char *path, int nr, char **match);
 extern int get_ack(int fd, unsigned char *result_sha1);
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);
diff --git a/connect.c b/connect.c
index 06d279e..458a502 100644
--- a/connect.c
+++ b/connect.c
@@ -468,21 +468,22 @@ char *get_port(char *host)
 }
 
 /*
- * This returns 0 if the transport protocol does not need fork(2),
- * or a process id if it does.  Once done, finish the connection
+ * This returns NULL if the transport protocol does not need fork(2), or a
+ * struct child_process object if it does.  Once done, finish the connection
  * with finish_connect() with the value returned from this function
- * (it is safe to call finish_connect() with 0 to support the former
+ * (it is safe to call finish_connect() with NULL to support the former
  * case).
  *
- * Does not return a negative value on error; it just dies.
+ * If it returns, the connect is successful; it just dies on errors.
  */
-pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
+struct child_process *git_connect(int fd[2], char *url,
+				  const char *prog, int flags)
 {
 	char *host, *path = url;
 	char *end;
 	int c;
 	int pipefd[2][2];
-	pid_t pid;
+	struct child_process *conn;
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;
 	char *port = NULL;
@@ -568,15 +569,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 		free(target_host);
 		if (free_path)
 			free(path);
-		return 0;
+		return NULL;
 	}
 
 	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
 		die("unable to create pipe pair for communication");
-	pid = fork();
-	if (pid < 0)
+	conn = xcalloc(1, sizeof(*conn));
+	conn->pid = fork();
+	if (conn->pid < 0)
 		die("unable to fork");
-	if (!pid) {
+	if (!conn->pid) {
 		struct strbuf cmd;
 
 		strbuf_init(&cmd, MAX_CMD_LEN);
@@ -625,17 +627,18 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 	close(pipefd[1][0]);
 	if (free_path)
 		free(path);
-	return pid;
+	return conn;
 }
 
-int finish_connect(pid_t pid)
+int finish_connect(struct child_process *conn)
 {
-	if (pid == 0)
+	if (conn == NULL)
 		return 0;
 
-	while (waitpid(pid, NULL, 0) < 0) {
+	while (waitpid(conn->pid, NULL, 0) < 0) {
 		if (errno != EINTR)
 			return -1;
 	}
+	free(conn);
 	return 0;
 }
diff --git a/fetch-pack.c b/fetch-pack.c
index 9c81305..d06b5ec 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -668,7 +668,7 @@ int main(int argc, char **argv)
 	int i, ret, nr_heads;
 	char *dest = NULL, **heads;
 	int fd[2];
-	pid_t pid;
+	struct child_process *conn;
 	struct stat st;
 
 	setup_git_directory();
@@ -733,15 +733,13 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	pid = git_connect(fd, dest, uploadpack, verbose ? CONNECT_VERBOSE : 0);
-	if (pid < 0)
-		return 1;
+	conn = git_connect(fd, dest, uploadpack, verbose ? CONNECT_VERBOSE : 0);
 	if (heads && nr_heads)
 		nr_heads = remove_duplicates(nr_heads, heads);
 	ret = fetch_pack(fd, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret |= finish_connect(conn);
 
 	if (!ret && nr_heads) {
 		/* If the heads to pull were given, we should have
diff --git a/peek-remote.c b/peek-remote.c
index ceb7871..8d20f7c 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -25,7 +25,7 @@ int main(int argc, char **argv)
 	int i, ret;
 	char *dest = NULL;
 	int fd[2];
-	pid_t pid;
+	struct child_process *conn;
 	int nongit = 0;
 	unsigned flags = 0;
 
@@ -64,12 +64,10 @@ int main(int argc, char **argv)
 	if (!dest || i != argc - 1)
 		usage(peek_remote_usage);
 
-	pid = git_connect(fd, dest, uploadpack, 0);
-	if (pid < 0)
-		return 1;
+	conn = git_connect(fd, dest, uploadpack, 0);
 	ret = peek_remote(fd, flags);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret |= finish_connect(conn);
 	return !!ret;
 }
diff --git a/send-pack.c b/send-pack.c
index f74e66a..f4c7bbf 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -362,7 +362,7 @@ int main(int argc, char **argv)
 	char *dest = NULL;
 	char **heads = NULL;
 	int fd[2], ret;
-	pid_t pid;
+	struct child_process *conn;
 	char *remote_name = NULL;
 	struct remote *remote = NULL;
 
@@ -426,12 +426,10 @@ int main(int argc, char **argv)
 		}
 	}
 
-	pid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
-	if (pid < 0)
-		return 1;
+	conn = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
 	ret = send_pack(fd[0], fd[1], remote, nr_heads, heads);
 	close(fd[0]);
 	close(fd[1]);
-	ret |= finish_connect(pid);
+	ret |= finish_connect(conn);
 	return !!ret;
 }
-- 
1.5.3.3.1134.gee562

[PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.

[Johannes Sixt]

The child process handling is delegated to start_command() and
finish_command().

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 connect.c |  103 ++++++++++++++++++++++++++++--------------------------------
 1 files changed, 48 insertions(+), 55 deletions(-)

diff --git a/connect.c b/connect.c
index 458a502..bb55d19 100644
--- a/connect.c
+++ b/connect.c
@@ -482,11 +482,12 @@ struct child_process *git_connect(int fd[2], char *url,
 	char *host, *path = url;
 	char *end;
 	int c;
-	int pipefd[2][2];
 	struct child_process *conn;
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;
 	char *port = NULL;
+	const char **arg;
+	struct strbuf cmd;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -572,73 +573,65 @@ struct child_process *git_connect(int fd[2], char *url,
 		return NULL;
 	}
 
-	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
-		die("unable to create pipe pair for communication");
 	conn = xcalloc(1, sizeof(*conn));
-	conn->pid = fork();
-	if (conn->pid < 0)
-		die("unable to fork");
-	if (!conn->pid) {
-		struct strbuf cmd;
-
-		strbuf_init(&cmd, MAX_CMD_LEN);
-		strbuf_addstr(&cmd, prog);
-		strbuf_addch(&cmd, ' ');
-		sq_quote_buf(&cmd, path);
-		if (cmd.len >= MAX_CMD_LEN)
-			die("command line too long");
-
-		dup2(pipefd[1][0], 0);
-		dup2(pipefd[0][1], 1);
-		close(pipefd[0][0]);
-		close(pipefd[0][1]);
-		close(pipefd[1][0]);
-		close(pipefd[1][1]);
-		if (protocol == PROTO_SSH) {
-			const char *ssh, *ssh_basename;
-			ssh = getenv("GIT_SSH");
-			if (!ssh) ssh = "ssh";
-			ssh_basename = strrchr(ssh, '/');
-			if (!ssh_basename)
-				ssh_basename = ssh;
-			else
-				ssh_basename++;
 
-			if (!port)
-				execlp(ssh, ssh_basename, host, cmd.buf, NULL);
-			else
-				execlp(ssh, ssh_basename, "-p", port, host,
-				       cmd.buf, NULL);
+	strbuf_init(&cmd, MAX_CMD_LEN);
+	strbuf_addstr(&cmd, prog);
+	strbuf_addch(&cmd, ' ');
+	sq_quote_buf(&cmd, path);
+	if (cmd.len >= MAX_CMD_LEN)
+		die("command line too long");
+
+	conn->in = conn->out = -1;
+	conn->argv = arg = xcalloc(6, sizeof(*arg));
+	if (protocol == PROTO_SSH) {
+		const char *ssh = getenv("GIT_SSH");
+		if (!ssh) ssh = "ssh";
+
+		*arg++ = ssh;
+		if (port) {
+			*arg++ = "-p";
+			*arg++ = port;
 		}
-		else {
-			unsetenv(ALTERNATE_DB_ENVIRONMENT);
-			unsetenv(DB_ENVIRONMENT);
-			unsetenv(GIT_DIR_ENVIRONMENT);
-			unsetenv(GIT_WORK_TREE_ENVIRONMENT);
-			unsetenv(GRAFT_ENVIRONMENT);
-			unsetenv(INDEX_ENVIRONMENT);
-			execlp("sh", "sh", "-c", cmd.buf, NULL);
-		}
-		die("exec failed");
+		*arg++ = host;
+	}
+	else {
+		/* remove these from the environment */
+		const char *env[] = {
+			ALTERNATE_DB_ENVIRONMENT,
+			DB_ENVIRONMENT,
+			GIT_DIR_ENVIRONMENT,
+			GIT_WORK_TREE_ENVIRONMENT,
+			GRAFT_ENVIRONMENT,
+			INDEX_ENVIRONMENT,
+			NULL
+		};
+		conn->env = env;
+		*arg++ = "sh";
+		*arg++ = "-c";
 	}
-	fd[0] = pipefd[0][0];
-	fd[1] = pipefd[1][1];
-	close(pipefd[0][1]);
-	close(pipefd[1][0]);
+	*arg++ = cmd.buf;
+	*arg = NULL;
+
+	if (start_command(conn))
+		die("unable to fork");
+
+	fd[0] = conn->out; /* read from child's stdout */
+	fd[1] = conn->in;  /* write to child's stdin */
 	if (free_path)
 		free(path);
+	strbuf_release(&cmd);
 	return conn;
 }
 
 int finish_connect(struct child_process *conn)
 {
+	int code;
 	if (conn == NULL)
 		return 0;
 
-	while (waitpid(conn->pid, NULL, 0) < 0) {
-		if (errno != EINTR)
-			return -1;
-	}
+	code = finish_command(conn);
+	free(conn->argv);
 	free(conn);
-	return 0;
+	return code;
 }
-- 
1.5.3.3.1134.gee562

[PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 convert.c |   30 +++++++-----------------------
 1 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/convert.c b/convert.c
index 0d5e909..6a8c806 100644
--- a/convert.c
+++ b/convert.c
@@ -197,34 +197,18 @@ static int filter_buffer(const char *path, const char *src,
 	 * Spawn cmd and feed the buffer contents through its stdin.
 	 */
 	struct child_process child_process;
-	int pipe_feed[2];
 	int write_err, status;
+	const char *argv[] = { "sh", "-c", cmd, NULL };
 
 	memset(&child_process, 0, sizeof(child_process));
+	child_process.argv = argv;
+	child_process.in = -1;
 
-	if (pipe(pipe_feed) < 0) {
-		error("cannot create pipe to run external filter %s", cmd);
-		return 1;
-	}
-
-	child_process.pid = fork();
-	if (child_process.pid < 0) {
-		error("cannot fork to run external filter %s", cmd);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		return 1;
-	}
-	if (!child_process.pid) {
-		dup2(pipe_feed[0], 0);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		execlp("sh", "sh", "-c", cmd, NULL);
-		return 1;
-	}
-	close(pipe_feed[0]);
+	if (start_command(&child_process))
+		return error("cannot fork to run external filter %s", cmd);
 
-	write_err = (write_in_full(pipe_feed[1], src, size) < 0);
-	if (close(pipe_feed[1]))
+	write_err = (write_in_full(child_process.in, src, size) < 0);
+	if (close(child_process.in))
 		write_err = 1;
 	if (write_err)
 		error("cannot feed the input to external filter %s", cmd);
-- 
1.5.3.3.1134.gee562

[PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 diff.c |   38 +++-----------------------------------
 1 files changed, 3 insertions(+), 35 deletions(-)

diff --git a/diff.c b/diff.c
index 6648e01..30b7544 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,7 @@
 #include "xdiff-interface.h"
 #include "color.h"
 #include "attr.h"
+#include "run-command.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1748,40 +1749,6 @@ static void remove_tempfile_on_signal(int signo)
 	raise(signo);
 }
 
-static int spawn_prog(const char *pgm, const char **arg)
-{
-	pid_t pid;
-	int status;
-
-	fflush(NULL);
-	pid = fork();
-	if (pid < 0)
-		die("unable to fork");
-	if (!pid) {
-		execvp(pgm, (char *const*) arg);
-		exit(255);
-	}
-
-	while (waitpid(pid, &status, 0) < 0) {
-		if (errno == EINTR)
-			continue;
-		return -1;
-	}
-
-	/* Earlier we did not check the exit status because
-	 * diff exits non-zero if files are different, and
-	 * we are not interested in knowing that.  It was a
-	 * mistake which made it harder to quit a diff-*
-	 * session that uses the git-apply-patch-script as
-	 * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF
-	 * should also exit non-zero only when it wants to
-	 * abort the entire diff-* session.
-	 */
-	if (WIFEXITED(status) && !WEXITSTATUS(status))
-		return 0;
-	return -1;
-}
-
 /* An external diff command takes:
  *
  * diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -1834,7 +1801,8 @@ static void run_external_diff(const char *pgm,
 		*arg++ = name;
 	}
 	*arg = NULL;
-	retval = spawn_prog(pgm, spawn_arg);
+	fflush(NULL);
+	retval = run_command_v_opt(spawn_arg, 0);
 	remove_tempfile();
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
-- 
1.5.3.3.1134.gee562

[PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.

[Johannes Sixt]

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 fetch-pack.c |   35 ++++++++++-------------------------
 1 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index d06b5ec..80268e1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -6,6 +6,7 @@
 #include "exec_cmd.h"
 #include "pack.h"
 #include "sideband.h"
+#include "run-command.h"
 
 static int keep_pack;
 static int transfer_unpack_limit = -1;
@@ -502,9 +503,12 @@ static int get_pack(int xd[2])
 	char hdr_arg[256];
 	const char **av;
 	int do_keep = keep_pack;
+	struct child_process cmd;
 
 	side_pid = setup_sideband(fd, xd);
 
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.argv = argv;
 	av = argv;
 	*hdr_arg = 0;
 	if (unpack_limit) {
@@ -544,33 +548,14 @@ static int get_pack(int xd[2])
 		*av++ = hdr_arg;
 	*av++ = NULL;
 
-	pid = fork();
-	if (pid < 0)
+	cmd.in = fd[0];
+	cmd.git_cmd = 1;
+	if (start_command(&cmd))
 		die("fetch-pack: unable to fork off %s", argv[0]);
-	if (!pid) {
-		dup2(fd[0], 0);
-		close(fd[0]);
-		close(fd[1]);
-		execv_git_cmd(argv);
-		die("%s exec failed", argv[0]);
-	}
-	close(fd[0]);
 	close(fd[1]);
-	while (waitpid(pid, &status, 0) < 0) {
-		if (errno != EINTR)
-			die("waiting for %s: %s", argv[0], strerror(errno));
-	}
-	if (WIFEXITED(status)) {
-		int code = WEXITSTATUS(status);
-		if (code)
-			die("%s died with error code %d", argv[0], code);
-		return 0;
-	}
-	if (WIFSIGNALED(status)) {
-		int sig = WTERMSIG(status);
-		die("%s died of signal %d", argv[0], sig);
-	}
-	die("%s died of unnatural causes %d", argv[0], status);
+	if (finish_command(&cmd))
+		die("%s failed", argv[0]);
+	return 0;
 }
 
 static int fetch_pack(int fd[2], int nr_match, char **match)
-- 
1.5.3.3.1134.gee562

Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  fetch-pack.c |   35 ++++++++++-------------------------
>  1 files changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index d06b5ec..80268e1 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -502,9 +503,12 @@ static int get_pack(int xd[2])
>  	char hdr_arg[256];
>  	const char **av;
>  	int do_keep = keep_pack;
> +	struct child_process cmd;
>  
>  	side_pid = setup_sideband(fd, xd);
>  
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.argv = argv;
>  	av = argv;
>  	*hdr_arg = 0;
>  	if (unpack_limit) {

Your patch to this function makes status and pid unused
variables, which I've fixed up locally.

The tip of your topic is currently queued to the tip of 'pu';
there were quite severe merge conflicts (textual conflicts in
builtin-fetch-pack.c, and adjustment to transport.c for
semantics change was also needed), so I ended up doing an evil
merge there, which I am not very happy about.

I suspect the evil-merge's changes to builtin-fetch-pack to
handle the connection to the index-pack process may be quite
busted, but I ran out of time.  Please check if the result makes
sense, Ok?

I think Daniel and Shawn's git-fetch-in-C should graduate
'master' before this series.  If you can re-send the series
rebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore
default verbosity for http fetches.), it would be much
appreciated.

Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.

[Johannes Sixt]

Junio C Hamano schrieb:
> I think Daniel and Shawn's git-fetch-in-C should graduate
> 'master' before this series.  If you can re-send the series
> rebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore
> default verbosity for http fetches.), it would be much
> appreciated.

Sure, will do this evening.

-- Hannes

Re: [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.

[Johannes Sixt]

On Thursday 04 October 2007 11:22, Johannes Sixt wrote:
> Junio C Hamano schrieb:
> > I think Daniel and Shawn's git-fetch-in-C should graduate
> > 'master' before this series.  If you can re-send the series
> > rebased on 2b5a06edca8f7237aad6464b349b79772024d2a2 (Restore
> > default verbosity for http fetches.), it would be much
> > appreciated.
>
> Sure, will do this evening.

I've difficulties with this rebase: 2b5a06 does not compile for me due to 
7155b727c9 (Introduce remove_dir_recursively()), which needs strbuf, but that 
is not merged into this branch. I can rebase on its parent (bundle transport: 
fix an alloc_ref() call) or 17df81ff (Merge branch 'db/fetch-pack' into 
next). I'd prefer the latter since it also has strbuf.

-- Hannes

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
>> This prepares the API of git_connect() and finish_connect() to operate on
>> a struct child_process. Currently, we just use that object as a placeholder
>> for the pid that we used to return. A follow-up patch will change the
>> implementation of git_connect() and finish_connect() to make full use
>> of the object.
> 
> Good description, except removal of checks for negative return
> of the calling sites raised my eyebrow and had me spend a few
> more minutes to review than necessary (see below).

I've thought about this issue a bit more.

Letting git_connect() die on error unconditionally is poison for any 
libification efforts. So here's a plan:

1. Let git_connect() return a struct child_process even for the
    non-forking case. This way a return value of NULL can uniquely
    identify a failure.

2. Keep the error checks in the callers (adjust to test for NULL).

3. Change the die() calls to return failure.

4. Note that the int fd[2] parameter to git_connect() is really an
    output: Remove it and use .in and .out of the returned struct
    child_process instead.

And maybe:

5. Reuse somehow the struct child_process that git_proxy_connect()
    already fills in.

Since my patch doesn't do (1), it can't do (2), i.e. keep the error checks - 
they must be removed because no unique failure value exists. So I could 
complete (1) in a new version of this patch, in order to also do (2). What 
is your preference?

-- Hannes

PS: I've postponed the completion of this plan - in favor of the MinGW port 
integration - because it only helps libification.

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> Johannes Sixt <johannes.sixt@telecom.at> writes:
>>
>>> This prepares the API of git_connect() and finish_connect() to operate on
>>> a struct child_process. Currently, we just use that object as a placeholder
>>> for the pid that we used to return. A follow-up patch will change the
>>> implementation of git_connect() and finish_connect() to make full use
>>> of the object.
>>
>> Good description, except removal of checks for negative return
>> of the calling sites raised my eyebrow and had me spend a few
>> more minutes to review than necessary (see below).
>
> I've thought about this issue a bit more.
>
> Letting git_connect() die on error unconditionally is poison for any
> libification efforts. So here's a plan:
>
> 1. Let git_connect() return a struct child_process even for the
>    non-forking case. This way a return value of NULL can uniquely
>    identify a failure.
> ...
> Since my patch doesn't do (1), it can't do (2), i.e. keep the error
> checks -
> they must be removed because no unique failure value exists. So I
> could complete (1) in a new version of this patch, in order to also do
> (2). What is your preference?

In any case, I'd rather first have one that hides fork/exec
behind child_process first without changing the call to die() in
git_connect() in this round.  I am still in "post feature
release clean-up" mood ;-)

As to error indication, it somehow does not feel right to return
something called "child _process_" structure when we want to
tell the caller that there is no process to wait for in the
no-error case, although the fact that we can use .in/.out fd in
the structure when we _do_ have child process is attractive.

As an alternative, we could keep the "NULL return means there
was no need to fork" semantics of git_connect(), and instead add
"int *status_ret" parameter for the caller to check.

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> Letting git_connect() die on error unconditionally is poison for any
>> libification efforts. So here's a plan:
>>
>> 1. Let git_connect() return a struct child_process even for the
>>    non-forking case. This way a return value of NULL can uniquely
>>    identify a failure.
>> ...
>> Since my patch doesn't do (1), it can't do (2), i.e. keep the error
>> checks -
>> they must be removed because no unique failure value exists. So I
>> could complete (1) in a new version of this patch, in order to also do
>> (2). What is your preference?
> 
> In any case, I'd rather first have one that hides fork/exec
> behind child_process first without changing the call to die() in
> git_connect() in this round.  I am still in "post feature
> release clean-up" mood ;-)

Sure: The die()s are converted in a later step.

My problem is that if I don't wrap the non-fork connections somehow in this 
first round, I *must* remove the error checks because there is no unique 
failure return value anymore.

> As to error indication, it somehow does not feel right to return
> something called "child _process_" structure when we want to
> tell the caller that there is no process to wait for in the
> no-error case, although the fact that we can use .in/.out fd in
> the structure when we _do_ have child process is attractive.

Did you mean: "even if we don't have a child process"?

How about a typedef if you dislike the name?

> As an alternative, we could keep the "NULL return means there
> was no need to fork" semantics of git_connect(), and instead add
> "int *status_ret" parameter for the caller to check.

Seriously? Add an *out* parameter when we can get rid of one and have a 
return value, too?

-- Hannes

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
> ...
>> As to error indication, it somehow does not feel right to return
>> something called "child _process_" structure when we want to
>> tell the caller that there is no process to wait for in the
>> no-error case, although the fact that we can use .in/.out fd in
>> the structure when we _do_ have child process is attractive.
>
> Did you mean: "even if we don't have a child process"?
>
> How about a typedef if you dislike the name?
>
>> As an alternative, we could keep the "NULL return means there
>> was no need to fork" semantics of git_connect(), and instead add
>> "int *status_ret" parameter for the caller to check.
>
> Seriously? Add an *out* parameter when we can get rid of one and have
> a return value, too?

Ah, I somehow got confused and thought that the caller decides
not to do the waitpid business at the end of the connection, but
as everybody calls finish_connect() with what it got from
git_connect(), as long as the fake "child_process" structure
records something to let finish_connect() know that it should
not waitpid() on the process, all is well.

It might make sense to teach finish_command() that a magic value
of (cmd->pid == 0) means there is no process to wait for and
this "child_process" structure is only about the in/out stream
to the other side.

Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Johannes Schindelin]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

> -	pid_t pid;
> +	struct child_process *chld;

Why not call it "child"?  It's only one letter more, and much less 
confusing.

> -	pid = git_connect(fd, url, exec, 0);
> -	if (pid < 0)
> -		return pid;
> +	chld = git_connect(fd, url, exec, 0);

chld never gets free()d.  (I'm was about to suggest doing that in 
finish_command(), but I'm not quite sure what this would break.)

> @@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
>  			st.st_mtime = 0;
>  	}
>  
> -	pid = git_connect(fd, (char *)dest, uploadpack,
> +	chld = git_connect(fd, (char *)dest, uploadpack,
>                            args.verbose ? CONNECT_VERBOSE : 0);
> -	if (pid < 0)
> -		return NULL;
>  	if (heads && nr_heads)
>  		nr_heads = remove_duplicates(nr_heads, heads);
>  	ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);

In general, I would not lightly do away with the "return NULL" on error, 
since it is really hard to assess what do_fetch_pack() or packet_write() 
do (or don't) when git_connect() failed.

> -	while (waitpid(pid, NULL, 0) < 0) {
> +	while (waitpid(chld->pid, NULL, 0) < 0) {

Shouldn't this be converted to finish_command() already?

Ciao,
Dscho

Re: [PATCH 0/5] fork/exec removal series

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Here is a series of patches that removes a number fork/exec pairs.
> They are replaced by delegating to start_command/finish_command/run_command.
> You can regard this as the beginning of the MinGW port integration.

Yay!

> Patch 2 depends on Patch 1; otherwise, there are no dependencies.
> The series goes on top of next (it touches str_buf stuff in connect.c).

Thanks for an advance warning.

I'll keep the series in my mailbox (promise) and queue it for
'pu' for now (but only if I have time), as I intend to merge the
strbuf stuff to 'master' soon.

Re: [PATCH 0/5] fork/exec removal series

[Johannes Schindelin]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

> You can regard this as the beginning of the MinGW port integration.

Thank you very much!  This effort cannot be praised enough.

> There still remain a few forks, which fall into these categories:
> 
> - They are in tools or code that are not (yet) ported to MinGW.[*]

The nice thing about the integration effort: It does not need to be done 
in one go.

> - The fork()s are not followed by exec(). These need a different
>   implementation. I am thinking of a start_coroutine()/finish_coroutine()
>   API that is implemented with threads in MinGW. (Suggestions of a better
>   as well as implementations are welcome.)

Is there more than the case I introduced with shallow clones?

Thanks,
Dscho

Re: [PATCH 0/5] fork/exec removal series

[Johannes Sixt]

On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:
> On Sun, 30 Sep 2007, Johannes Sixt wrote:
> > - The fork()s are not followed by exec(). These need a different
> >   implementation. I am thinking of a start_coroutine()/finish_coroutine()
> >   API that is implemented with threads in MinGW. (Suggestions of a better
> >   as well as implementations are welcome.)
>
> Is there more than the case I introduced with shallow clones?

I don't know which one you are refering to.

These cases I hope to be able to treat as "coroutine":

- sideband demultiplexer in builtin-fetch-pack.c
- internal rev-list in upload-pack
- the two-way pipe handling in convert.c and builtin-upload-archive.c

There are probably more in daemon.c and imap-send.c.

BTW, the convert.c case (apply_filter) is most interesting for me, since I 
have a real-world use-case for a clean-filter.

-- Hannes

Re: [PATCH 0/5] fork/exec removal series

[Johannes Schindelin]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

> On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:
>
> > Is there more than the case I introduced with shallow clones?
> 
> I don't know which one you are refering to.

The rev-list one in upload-pack.

> These cases I hope to be able to treat as "coroutine":
> 
> - sideband demultiplexer in builtin-fetch-pack.c
> - internal rev-list in upload-pack
> - the two-way pipe handling in convert.c and builtin-upload-archive.c
> 
> There are probably more in daemon.c and imap-send.c.
> 
> BTW, the convert.c case (apply_filter) is most interesting for me, since I 
> have a real-world use-case for a clean-filter.

Calling it coroutine is interesting... But yes, I agree that these three 
cases cannot be handled otherwise.

Ciao,
Dscho

Re: [PATCH 0/5] fork/exec removal series

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Sun, 30 Sep 2007, Johannes Sixt wrote:
>> These cases I hope to be able to treat as "coroutine":
>>
>> - sideband demultiplexer in builtin-fetch-pack.c
>> - internal rev-list in upload-pack
>> - the two-way pipe handling in convert.c and builtin-upload-archive.c
>>
>> There are probably more in daemon.c and imap-send.c.
>>
>> BTW, the convert.c case (apply_filter) is most interesting for me, since I 
>> have a real-world use-case for a clean-filter.
> 
> Calling it coroutine is interesting... But yes, I agree that these three 
> cases cannot be handled otherwise.

Suggestions for a better name are appreciated!

-- Hannes

Re: [PATCH 0/5] fork/exec removal series

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Schindelin schrieb:
>> On Sun, 30 Sep 2007, Johannes Sixt wrote:
>>> These cases I hope to be able to treat as "coroutine":
>>>
>>> - sideband demultiplexer in builtin-fetch-pack.c
>>> - internal rev-list in upload-pack
>>> - the two-way pipe handling in convert.c and builtin-upload-archive.c
>>>
>>> There are probably more in daemon.c and imap-send.c.
>>>
>>> BTW, the convert.c case (apply_filter) is most interesting for me,
>>> since I have a real-world use-case for a clean-filter.
>>
>> Calling it coroutine is interesting... But yes, I agree that these
>> three cases cannot be handled otherwise.
>
> Suggestions for a better name are appreciated!

I think coroutine is commonly used as the name for _synchronous_
context switches aka message passing.  Basically the same as
subroutine calls, except that the called subroutine has its own
dynamic context (instruction pointer, call stack, control flow) that
gets activated and suspended.

If there is parallelism implied, "thread" is the more appropriate
name.

-- 
David Kastrup