Merge branch 'sp/run-command'

* sp/run-command:
  Use run_command within send-pack
  Use run_command within receive-pack to invoke index-pack
  Use run_command within merge-index
  Use run_command for proxy connections
  Use RUN_GIT_CMD to run push backends
  Correct new compiler warnings in builtin-revert
  Replace fork_with_pipe in bundle with run_command
  Teach run-command to redirect stdout to /dev/null
  Teach run-command about stdout redirection

11 files changed, 146 insertions, 234 deletions

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 786808081..0a9b73867 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -4,7 +4,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "list-objects.h"
-#include "exec_cmd.h"
+#include "run-command.h"
 
 /*
  * Basic handler for bundle files to connect repositories via sneakernet.
@@ -99,67 +99,6 @@ static int read_header(const char *path, struct bundle_header *header) {
 	return fd;
 }
 
-/* if in && *in >= 0, take that as input file descriptor instead */
-static int fork_with_pipe(const char **argv, int *in, int *out)
-{
-	int needs_in, needs_out;
-	int fdin[2], fdout[2], pid;
-
-	needs_in = in && *in < 0;
-	if (needs_in) {
-		if (pipe(fdin) < 0)
-			return error("could not setup pipe");
-		*in = fdin[1];
-	}
-
-	needs_out = out && *out < 0;
-	if (needs_out) {
-		if (pipe(fdout) < 0)
-			return error("could not setup pipe");
-		*out = fdout[0];
-	}
-
-	if ((pid = fork()) < 0) {
-		if (needs_in) {
-			close(fdin[0]);
-			close(fdin[1]);
-		}
-		if (needs_out) {
-			close(fdout[0]);
-			close(fdout[1]);
-		}
-		return error("could not fork");
-	}
-	if (!pid) {
-		if (needs_in) {
-			dup2(fdin[0], 0);
-			close(fdin[0]);
-			close(fdin[1]);
-		} else if (in) {
-			dup2(*in, 0);
-			close(*in);
-		}
-		if (needs_out) {
-			dup2(fdout[1], 1);
-			close(fdout[0]);
-			close(fdout[1]);
-		} else if (out) {
-			dup2(*out, 1);
-			close(*out);
-		}
-		exit(execv_git_cmd(argv));
-	}
-	if (needs_in)
-		close(fdin[0]);
-	else if (in)
-		close(*in);
-	if (needs_out)
-		close(fdout[1]);
-	else if (out)
-		close(*out);
-	return pid;
-}
-
 static int list_refs(struct ref_list *r, int argc, const char **argv)
 {
 	int i;
@@ -263,9 +202,10 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	int bundle_fd = -1;
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(5 * sizeof(const char *));
-	int pid, in, out, i, status, ref_count = 0;
+	int i, ref_count = 0;
 	char buffer[1024];
 	struct rev_info revs;
+	struct child_process rls;
 
 	bundle_fd = (!strcmp(path, "-") ? 1 :
 			open(path, O_CREAT | O_EXCL | O_WRONLY, 0666));
@@ -285,11 +225,13 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	argv_boundary[1] = "--boundary";
 	argv_boundary[2] = "--pretty=oneline";
 	argv_boundary[argc + 2] = NULL;
-	out = -1;
-	pid = fork_with_pipe(argv_boundary, NULL, &out);
-	if (pid < 0)
+	memset(&rls, 0, sizeof(rls));
+	rls.argv = argv_boundary;
+	rls.out = -1;
+	rls.git_cmd = 1;
+	if (start_command(&rls))
 		return -1;
-	while ((i = read_string(out, buffer, sizeof(buffer))) > 0) {
+	while ((i = read_string(rls.out, buffer, sizeof(buffer))) > 0) {
 		unsigned char sha1[20];
 		if (buffer[0] == '-') {
 			write_or_die(bundle_fd, buffer, i);
@@ -303,11 +245,8 @@ static int create_bundle(struct bundle_header *header, const char *path,
 			object->flags |= SHOWN;
 		}
 	}
-	while ((i = waitpid(pid, &status, 0)) < 0)
-		if (errno != EINTR)
-			return error("rev-list died");
-	if (!WIFEXITED(status) || WEXITSTATUS(status))
-		return error("rev-list died %d", WEXITSTATUS(status));
+	if (finish_command(&rls))
+		return error("rev-list died");
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
@@ -352,26 +291,23 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	argv_pack[2] = "--stdout";
 	argv_pack[3] = "--thin";
 	argv_pack[4] = NULL;
-	in = -1;
-	out = bundle_fd;
-	pid = fork_with_pipe(argv_pack, &in, &out);
-	if (pid < 0)
+	memset(&rls, 0, sizeof(rls));
+	rls.argv = argv_pack;
+	rls.in = -1;
+	rls.out = bundle_fd;
+	rls.git_cmd = 1;
+	if (start_command(&rls))
 		return error("Could not spawn pack-objects");
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object *object = revs.pending.objects[i].item;
 		if (object->flags & UNINTERESTING)
-			write(in, "^", 1);
-		write(in, sha1_to_hex(object->sha1), 40);
-		write(in, "\n", 1);
+			write(rls.in, "^", 1);
+		write(rls.in, sha1_to_hex(object->sha1), 40);
+		write(rls.in, "\n", 1);
 	}
-	close(in);
-	while (waitpid(pid, &status, 0) < 0)
-		if (errno != EINTR)
-			return -1;
-	if (!WIFEXITED(status) || WEXITSTATUS(status))
+	if (finish_command(&rls))
 		return error ("pack-objects died");
-
-	return status;
+	return 0;
 }
 
 static int unbundle(struct bundle_header *header, int bundle_fd,
@@ -379,22 +315,17 @@ static int unbundle(struct bundle_header *header, int bundle_fd,
 {
 	const char *argv_index_pack[] = {"index-pack",
 		"--fix-thin", "--stdin", NULL};
-	int pid, status, dev_null;
+	struct child_process ip;
 
 	if (verify_bundle(header, 0))
 		return -1;
-	dev_null = open("/dev/null", O_WRONLY);
-	if (dev_null < 0)
-		return error("Could not open /dev/null");
-	pid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);
-	if (pid < 0)
-		return error("Could not spawn index-pack");
-	while (waitpid(pid, &status, 0) < 0)
-		if (errno != EINTR)
-			return error("index-pack died");
-	if (!WIFEXITED(status) || WEXITSTATUS(status))
-		return error("index-pack exited with status %d",
-				WEXITSTATUS(status));
+	memset(&ip, 0, sizeof(ip));
+	ip.argv = argv_index_pack;
+	ip.in = bundle_fd;
+	ip.no_stdout = 1;
+	ip.git_cmd = 1;
+	if (run_command(&ip))
+		return error("index-pack died");
 	return list_heads(header, argc, argv);
 }
 
diff --git a/builtin-push.c b/builtin-push.c
index 6ab9a28e8..70b1168fa 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -323,10 +323,10 @@ static int do_push(const char *repo)
 		int dest_refspec_nr = refspec_nr;
 		const char **dest_refspec = refspec;
 		const char *dest = uri[i];
-		const char *sender = "git-send-pack";
+		const char *sender = "send-pack";
 		if (!prefixcmp(dest, "http://") ||
 		    !prefixcmp(dest, "https://"))
-			sender = "git-http-push";
+			sender = "http-push";
 		else if (thin)
 			argv[dest_argc++] = "--thin";
 		argv[0] = sender;
@@ -336,7 +336,7 @@ static int do_push(const char *repo)
 		argv[dest_argc] = NULL;
 		if (verbose)
 			fprintf(stderr, "Pushing to %s\n", dest);
-		err = run_command_v_opt(argv, 0);
+		err = run_command_v_opt(argv, RUN_GIT_CMD);
 		if (!err)
 			continue;
 		switch (err) {
diff --git a/builtin-revert.c b/builtin-revert.c
index 652eece5a..f3f3f5c6e 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -235,8 +235,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	unsigned char head[20];
 	struct commit *base, *next;
 	int i;
-	char *oneline, *encoding, *reencoded_message = NULL;
-	const char *message;
+	char *oneline, *reencoded_message = NULL;
+	const char *message, *encoding;
 
 	git_config(git_default_config);
 	me = action == REVERT ? "revert" : "cherry-pick";
diff --git a/cache.h b/cache.h
index 3818e10f8..5396d3366 100644
--- a/cache.h
+++ b/cache.h
@@ -451,7 +451,7 @@ extern int check_repository_format_version(const char *var, const char *value);
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
 
-extern char *git_commit_encoding;
+extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
 
 extern int copy_fd(int ifd, int ofd);
diff --git a/connect.c b/connect.c
index 8a8a13bb7..504865363 100644
--- a/connect.c
+++ b/connect.c
@@ -3,6 +3,7 @@
 #include "pkt-line.h"
 #include "quote.h"
 #include "refs.h"
+#include "run-command.h"
 
 static char *server_capabilities;
 
@@ -598,8 +599,8 @@ static void git_proxy_connect(int fd[2], char *host)
 {
 	const char *port = STR(DEFAULT_GIT_PORT);
 	char *colon, *end;
-	int pipefd[2][2];
-	pid_t pid;
+	const char *argv[4];
+	struct child_process proxy;
 
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
@@ -618,25 +619,18 @@ static void git_proxy_connect(int fd[2], char *host)
 		port = colon + 1;
 	}
 
-	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
-		die("unable to create pipe pair for communication");
-	pid = fork();
-	if (!pid) {
-		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]);
-		execlp(git_proxy_command, git_proxy_command, host, port, NULL);
-		die("exec failed");
-	}
-	if (pid < 0)
-		die("fork failed");
-	fd[0] = pipefd[0][0];
-	fd[1] = pipefd[1][1];
-	close(pipefd[0][1]);
-	close(pipefd[1][0]);
+	argv[0] = git_proxy_command;
+	argv[1] = host;
+	argv[2] = port;
+	argv[3] = NULL;
+	memset(&proxy, 0, sizeof(proxy));
+	proxy.argv = argv;
+	proxy.in = -1;
+	proxy.out = -1;
+	if (start_command(&proxy))
+		die("cannot start proxy %s", argv[0]);
+	fd[0] = proxy.out; /* read from proxy stdout */
+	fd[1] = proxy.in;  /* write to proxy stdin */
 }
 
 #define MAX_CMD_LEN 1024
diff --git a/environment.c b/environment.c
index 0151ad072..fff4a4da5 100644
--- a/environment.c
+++ b/environment.c
@@ -20,7 +20,7 @@ int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int repository_format_version;
-char *git_commit_encoding;
+const char *git_commit_encoding;
 const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace;
diff --git a/merge-index.c b/merge-index.c
index 7027d7865..6df43944b 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 
 static const char *pgm;
 static const char *arguments[8];
@@ -7,24 +8,10 @@ static int err;
 
 static void run_program(void)
 {
-	pid_t pid = fork();
-	int status;
-
-	if (pid < 0)
-		die("unable to fork");
-	if (!pid) {
-		execlp(pgm, arguments[0],
-			    arguments[1],
-			    arguments[2],
-			    arguments[3],
-			    arguments[4],
-			    arguments[5],
-			    arguments[6],
-			    arguments[7],
-			    NULL);
-		die("unable to execute '%s'", pgm);
-	}
-	if (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status) || WEXITSTATUS(status)) {
+	struct child_process child;
+	memset(&child, 0, sizeof(child));
+	child.argv = arguments;
+	if (run_command(&child)) {
 		if (one_shot) {
 			err++;
 		} else {
diff --git a/receive-pack.c b/receive-pack.c
index 7cf58782e..26aa26bcb 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -382,10 +382,10 @@ static const char *unpack(void)
 		}
 	} else {
 		const char *keeper[6];
-		int fd[2], s, len, status;
-		pid_t pid;
+		int s, len, status;
 		char keep_arg[256];
 		char packname[46];
+		struct child_process ip;
 
 		s = sprintf(keep_arg, "--keep=receive-pack %i on ", getpid());
 		if (gethostname(keep_arg + s, sizeof(keep_arg) - s))
@@ -397,20 +397,12 @@ static const char *unpack(void)
 		keeper[3] = hdr_arg;
 		keeper[4] = keep_arg;
 		keeper[5] = NULL;
-
-		if (pipe(fd) < 0)
-			return "index-pack pipe failed";
-		pid = fork();
-		if (pid < 0)
+		memset(&ip, 0, sizeof(ip));
+		ip.argv = keeper;
+		ip.out = -1;
+		ip.git_cmd = 1;
+		if (start_command(&ip))
 			return "index-pack fork failed";
-		if (!pid) {
-			dup2(fd[1], 1);
-			close(fd[1]);
-			close(fd[0]);
-			execv_git_cmd(keeper);
-			die("execv of index-pack failed");
-		}
-		close(fd[1]);
 
 		/*
 		 * The first thing we expects from index-pack's output
@@ -420,9 +412,8 @@ static const char *unpack(void)
 		 * later on.  If we don't get that then tough luck with it.
 		 */
 		for (len = 0;
-		     len < 46 && (s = xread(fd[0], packname+len, 46-len)) > 0;
+		     len < 46 && (s = xread(ip.out, packname+len, 46-len)) > 0;
 		     len += s);
-		close(fd[0]);
 		if (len == 46 && packname[45] == '\n' &&
 		    memcmp(packname, "keep\t", 5) == 0) {
 			char path[PATH_MAX];
@@ -432,14 +423,8 @@ static const char *unpack(void)
 			pack_lockfile = xstrdup(path);
 		}
 
-		/* Then wrap our index-pack process. */
-		while (waitpid(pid, &status, 0) < 0)
-			if (errno != EINTR)
-				return "waitpid failed";
-		if (WIFEXITED(status)) {
-			int code = WEXITSTATUS(status);
-			if (code)
-				return "index-pack exited with error code";
+		status = finish_command(&ip);
+		if (!status) {
 			reprepare_packed_git();
 			return NULL;
 		}
diff --git a/run-command.c b/run-command.c
index bef9775e0..eff523e19 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,11 +8,19 @@ static inline void close_pair(int fd[2])
 	close(fd[1]);
 }
 
+static inline void dup_devnull(int to)
+{
+	int fd = open("/dev/null", O_RDWR);
+	dup2(fd, to);
+	close(fd);
+}
+
 int start_command(struct child_process *cmd)
 {
-	int need_in = !cmd->no_stdin && cmd->in < 0;
-	int fdin[2];
+	int need_in, need_out;
+	int fdin[2], fdout[2];
 
+	need_in = !cmd->no_stdin && cmd->in < 0;
 	if (need_in) {
 		if (pipe(fdin) < 0)
 			return -ERR_RUN_COMMAND_PIPE;
@@ -20,19 +28,32 @@ int start_command(struct child_process *cmd)
 		cmd->close_in = 1;
 	}
 
+	need_out = !cmd->no_stdout
+		&& !cmd->stdout_to_stderr
+		&& cmd->out < 0;
+	if (need_out) {
+		if (pipe(fdout) < 0) {
+			if (need_in)
+				close_pair(fdin);
+			return -ERR_RUN_COMMAND_PIPE;
+		}
+		cmd->out = fdout[0];
+		cmd->close_out = 1;
+	}
+
 	cmd->pid = fork();
 	if (cmd->pid < 0) {
 		if (need_in)
 			close_pair(fdin);
+		if (need_out)
+			close_pair(fdout);
 		return -ERR_RUN_COMMAND_FORK;
 	}
 
 	if (!cmd->pid) {
-		if (cmd->no_stdin) {
-			int fd = open("/dev/null", O_RDWR);
-			dup2(fd, 0);
-			close(fd);
-		} else if (need_in) {
+		if (cmd->no_stdin)
+			dup_devnull(0);
+		else if (need_in) {
 			dup2(fdin[0], 0);
 			close_pair(fdin);
 		} else if (cmd->in) {
@@ -40,8 +61,18 @@ int start_command(struct child_process *cmd)
 			close(cmd->in);
 		}
 
-		if (cmd->stdout_to_stderr)
+		if (cmd->no_stdout)
+			dup_devnull(1);
+		else if (cmd->stdout_to_stderr)
 			dup2(2, 1);
+		else if (need_out) {
+			dup2(fdout[1], 1);
+			close_pair(fdout);
+		} else if (cmd->out > 1) {
+			dup2(cmd->out, 1);
+			close(cmd->out);
+		}
+
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -55,6 +86,11 @@ int start_command(struct child_process *cmd)
 	else if (cmd->in)
 		close(cmd->in);
 
+	if (need_out)
+		close(fdout[1]);
+	else if (cmd->out > 1)
+		close(cmd->out);
+
 	return 0;
 }
 
@@ -62,6 +98,8 @@ int finish_command(struct child_process *cmd)
 {
 	if (cmd->close_in)
 		close(cmd->in);
+	if (cmd->close_out)
+		close(cmd->out);
 
 	for (;;) {
 		int status, code;
diff --git a/run-command.h b/run-command.h
index ff090679a..3680ef9d4 100644
--- a/run-command.h
+++ b/run-command.h
@@ -15,8 +15,11 @@ struct child_process {
 	const char **argv;
 	pid_t pid;
 	int in;
+	int out;
 	unsigned close_in:1;
+	unsigned close_out:1;
 	unsigned no_stdin:1;
+	unsigned no_stdout:1;
 	unsigned git_cmd:1; /* if this is to be git sub-command */
 	unsigned stdout_to_stderr:1;
 };
diff --git a/send-pack.c b/send-pack.c
index 512b660e9..d5b51628d 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -3,7 +3,7 @@
 #include "tag.h"
 #include "refs.h"
 #include "pkt-line.h"
-#include "exec_cmd.h"
+#include "run-command.h"
 
 static const char send_pack_usage[] =
 "git-send-pack [--all] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -19,46 +19,35 @@ static int use_thin_pack;
  */
 static int pack_objects(int fd, struct ref *refs)
 {
-	int pipe_fd[2];
-	pid_t pid;
-
-	if (pipe(pipe_fd) < 0)
-		return error("send-pack: pipe failed");
-	pid = fork();
-	if (pid < 0)
-		return error("send-pack: unable to fork git-pack-objects");
-	if (!pid) {
-		/*
-		 * The child becomes pack-objects --revs; we feed
-		 * the revision parameters to it via its stdin and
-		 * let its stdout go back to the other end.
-		 */
-		static const char *args[] = {
-			"pack-objects",
-			"--all-progress",
-			"--revs",
-			"--stdout",
-			NULL,
-			NULL,
-		};
-		if (use_thin_pack)
-			args[4] = "--thin";
-		dup2(pipe_fd[0], 0);
-		dup2(fd, 1);
-		close(pipe_fd[0]);
-		close(pipe_fd[1]);
-		close(fd);
-		execv_git_cmd(args);
-		die("git-pack-objects exec failed (%s)", strerror(errno));
-	}
+	/*
+	 * The child becomes pack-objects --revs; we feed
+	 * the revision parameters to it via its stdin and
+	 * let its stdout go back to the other end.
+	 */
+	const char *args[] = {
+		"pack-objects",
+		"--all-progress",
+		"--revs",
+		"--stdout",
+		NULL,
+		NULL,
+	};
+	struct child_process po;
+
+	if (use_thin_pack)
+		args[4] = "--thin";
+	memset(&po, 0, sizeof(po));
+	po.argv = args;
+	po.in = -1;
+	po.out = fd;
+	po.git_cmd = 1;
+	if (start_command(&po))
+		die("git-pack-objects failed (%s)", strerror(errno));
 
 	/*
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	close(pipe_fd[0]);
-	close(fd);
-
 	while (refs) {
 		char buf[42];
 
@@ -67,38 +56,23 @@ static int pack_objects(int fd, struct ref *refs)
 			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
 			buf[0] = '^';
 			buf[41] = '\n';
-			if (!write_or_whine(pipe_fd[1], buf, 42,
+			if (!write_or_whine(po.in, buf, 42,
 						"send-pack: send refs"))
 				break;
 		}
 		if (!is_null_sha1(refs->new_sha1)) {
 			memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
 			buf[40] = '\n';
-			if (!write_or_whine(pipe_fd[1], buf, 41,
+			if (!write_or_whine(po.in, buf, 41,
 						"send-pack: send refs"))
 				break;
 		}
 		refs = refs->next;
 	}
-	close(pipe_fd[1]);
-
-	for (;;) {
-		int status, code;
-		pid_t waiting = waitpid(pid, &status, 0);
 
-		if (waiting < 0) {
-			if (errno == EINTR)
-				continue;
-			return error("waitpid failed (%s)", strerror(errno));
-		}
-		if ((waiting != pid) || WIFSIGNALED(status) ||
-		    !WIFEXITED(status))
-			return error("pack-objects died with strange error");
-		code = WEXITSTATUS(status);
-		if (code)
-			return -code;
-		return 0;
-	}
+	if (finish_command(&po))
+		return error("pack-objects died with strange error");
+	return 0;
 }
 
 static void unmark_and_free(struct commit_list *list, unsigned int mark)