Merge branch 'jk/argv-array-for-child-process'

* jk/argv-array-for-child-process:
  argv-array: drop "detach" code
  get_importer: use run-command's internal argv_array
  get_exporter: use argv_array
  get_helper: use run-command's internal argv_array
  git_connect: use argv_array
  run_column_filter: use argv_array
  run-command: store an optional argv_array

Documentation/technical/api-argv-array.txt

@@ -53,11 +53,3 @@ Functions
 `argv_array_clear`::
 	Free all memory associated with the array and return it to the
 	initial, empty state.
-
-`argv_array_detach`::
-	Detach the argv array from the `struct argv_array`, transferring
-	ownership of the allocated array and strings.
-
-`argv_array_free_detached`::
-	Free the memory allocated by a `struct argv_array` that was later
-	detached and is now no longer needed.

Documentation/technical/api-run-command.txt

@@ -109,6 +109,13 @@ terminated), of which .argv[0] is the program name to run (usually
 without a path). If the command to run is a git command, set argv[0] to
 the command name without the 'git-' prefix and set .git_cmd = 1.
 
+Note that the ownership of the memory pointed to by .argv stays with the
+caller, but it should survive until `finish_command` completes. If the
+.argv member is NULL, `start_command` will point it at the .args
+`argv_array` (so you may use one or the other, but you must use exactly
+one). The memory in .args will be cleaned up automatically during
+`finish_command` (or during `start_command` when it is unsuccessful).
+
 The members .in, .out, .err are used to redirect stdin, stdout,
 stderr as follows:
 

argv-array.c

@@ -68,23 +68,3 @@ void argv_array_clear(struct argv_array *array)
 	}
 	argv_array_init(array);
 }
-
-const char **argv_array_detach(struct argv_array *array, int *argc)
-{
-	const char **argv =
-		array->argv == empty_argv || array->argc == 0 ? NULL : array->argv;
-	if (argc)
-		*argc = array->argc;
-	argv_array_init(array);
-	return argv;
-}
-
-void argv_array_free_detached(const char **argv)
-{
-	if (argv) {
-		int i;
-		for (i = 0; argv[i]; i++)
-			free((char **)argv[i]);
-		free(argv);
-	}
-}

argv-array.h

@@ -19,7 +19,5 @@ LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
-const char **argv_array_detach(struct argv_array *array, int *argc);
-void argv_array_free_detached(const char **argv);
 
 #endif /* ARGV_ARRAY_H */

column.c

@@ -370,46 +370,29 @@ static struct child_process column_process;
 
 int run_column_filter(int colopts, const struct column_options *opts)
 {
-	const char *av[10];
-	int ret, ac = 0;
-	struct strbuf sb_colopt  = STRBUF_INIT;
-	struct strbuf sb_width   = STRBUF_INIT;
-	struct strbuf sb_padding = STRBUF_INIT;
+	struct argv_array *argv;
 
 	if (fd_out != -1)
 		return -1;
 
-	av[ac++] = "column";
-	strbuf_addf(&sb_colopt, "--raw-mode=%d", colopts);
-	av[ac++] = sb_colopt.buf;
-	if (opts && opts->width) {
-		strbuf_addf(&sb_width, "--width=%d", opts->width);
-		av[ac++] = sb_width.buf;
-	}
-	if (opts && opts->indent) {
-		av[ac++] = "--indent";
-		av[ac++] = opts->indent;
-	}
-	if (opts && opts->padding) {
-		strbuf_addf(&sb_padding, "--padding=%d", opts->padding);
-		av[ac++] = sb_padding.buf;
-	}
-	av[ac] = NULL;
+	memset(&column_process, 0, sizeof(column_process));
+	argv = &column_process.args;
+
+	argv_array_push(argv, "column");
+	argv_array_pushf(argv, "--raw-mode=%d", colopts);
+	if (opts && opts->width)
+		argv_array_pushf(argv, "--width=%d", opts->width);
+	if (opts && opts->indent)
+		argv_array_pushf(argv, "--indent=%s", opts->indent);
+	if (opts && opts->padding)
+		argv_array_pushf(argv, "--padding=%d", opts->padding);
 
 	fflush(stdout);
-	memset(&column_process, 0, sizeof(column_process));
 	column_process.in = -1;
 	column_process.out = dup(1);
 	column_process.git_cmd = 1;
-	column_process.argv = av;
-
-	ret = start_command(&column_process);
-
-	strbuf_release(&sb_colopt);
-	strbuf_release(&sb_width);
-	strbuf_release(&sb_padding);
 
-	if (ret)
+	if (start_command(&column_process))
 		return -2;
 
 	fd_out = dup(1);

connect.c

@@ -534,22 +534,18 @@ static int git_use_proxy(const char *host)
 static struct child_process *git_proxy_connect(int fd[2], char *host)
 {
 	const char *port = STR(DEFAULT_GIT_PORT);
-	const char **argv;
 	struct child_process *proxy;
 
 	get_host_and_port(&host, &port);
 
-	argv = xmalloc(sizeof(*argv) * 4);
-	argv[0] = git_proxy_command;
-	argv[1] = host;
-	argv[2] = port;
-	argv[3] = NULL;
 	proxy = xcalloc(1, sizeof(*proxy));
-	proxy->argv = argv;
+	argv_array_push(&proxy->args, git_proxy_command);
+	argv_array_push(&proxy->args, host);
+	argv_array_push(&proxy->args, port);
 	proxy->in = -1;
 	proxy->out = -1;
 	if (start_command(proxy))
-		die("cannot start proxy %s", argv[0]);
+		die("cannot start proxy %s", git_proxy_command);
 	fd[0] = proxy->out; /* read from proxy stdout */
 	fd[1] = proxy->in;  /* write to proxy stdin */
 	return proxy;
@@ -663,7 +659,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
-	const char **arg;
 	struct strbuf cmd = STRBUF_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
@@ -707,7 +702,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 		sq_quote_buf(&cmd, path);
 
 		conn->in = conn->out = -1;
-		conn->argv = arg = xcalloc(7, sizeof(*arg));
 		if (protocol == PROTO_SSH) {
 			const char *ssh = getenv("GIT_SSH");
 			int putty = ssh && strcasestr(ssh, "plink");
@@ -718,22 +712,21 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 			if (!ssh) ssh = "ssh";
 
-			*arg++ = ssh;
+			argv_array_push(&conn->args, ssh);
 			if (putty && !strcasestr(ssh, "tortoiseplink"))
-				*arg++ = "-batch";
+				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				/* P is for PuTTY, p is for OpenSSH */
-				*arg++ = putty ? "-P" : "-p";
-				*arg++ = port;
+				argv_array_push(&conn->args, putty ? "-P" : "-p");
+				argv_array_push(&conn->args, port);
 			}
-			*arg++ = ssh_host;
+			argv_array_push(&conn->args, ssh_host);
 		} else {
 			/* remove repo-local variables from the environment */
 			conn->env = local_repo_env;
 			conn->use_shell = 1;
 		}
-		*arg++ = cmd.buf;
-		*arg = NULL;
+		argv_array_push(&conn->args, cmd.buf);
 
 		if (start_command(conn))
 			die("unable to fork");
@@ -759,7 +752,6 @@ int finish_connect(struct child_process *conn)
 		return 0;
 
 	code = finish_command(conn);
-	free(conn->argv);
 	free(conn);
 	return code;
 }

run-command.c

@@ -279,6 +279,9 @@ int start_command(struct child_process *cmd)
 	int failed_errno;
 	char *str;
 
+	if (!cmd->argv)
+		cmd->argv = cmd->args.argv;
+
 	/*
 	 * In case of errors we must keep the promise to close FDs
 	 * that have been passed in via ->in and ->out.
@@ -328,6 +331,7 @@ int start_command(struct child_process *cmd)
 fail_pipe:
 			error("cannot create %s pipe for %s: %s",
 				str, cmd->argv[0], strerror(failed_errno));
+			argv_array_clear(&cmd->args);
 			errno = failed_errno;
 			return -1;
 		}
@@ -519,6 +523,7 @@ int start_command(struct child_process *cmd)
 			close_pair(fderr);
 		else if (cmd->err)
 			close(cmd->err);
+		argv_array_clear(&cmd->args);
 		errno = failed_errno;
 		return -1;
 	}
@@ -543,7 +548,9 @@ int start_command(struct child_process *cmd)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0]);
+	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
+	argv_array_clear(&cmd->args);
+	return ret;
 }
 
 int run_command(struct child_process *cmd)

run-command.h

@@ -5,8 +5,11 @@
 #include <pthread.h>
 #endif
 
+#include "argv-array.h"
+
 struct child_process {
 	const char **argv;
+	struct argv_array args;
 	pid_t pid;
 	/*
 	 * Using .in, .out, .err:

transport-helper.c

@@ -101,7 +101,6 @@ static void do_take_over(struct transport *transport)
 static struct child_process *get_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
-	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct child_process *helper;
 	const char **refspecs = NULL;
@@ -123,10 +122,9 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->in = -1;
 	helper->out = -1;
 	helper->err = 0;
-	argv_array_pushf(&argv, "git-remote-%s", data->name);
-	argv_array_push(&argv, transport->remote->name);
-	argv_array_push(&argv, remove_ext_force(transport->url));
-	helper->argv = argv_array_detach(&argv, NULL);
+	argv_array_pushf(&helper->args, "git-remote-%s", data->name);
+	argv_array_push(&helper->args, transport->remote->name);
+	argv_array_push(&helper->args, remove_ext_force(transport->url));
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
 
@@ -245,7 +243,6 @@ static int disconnect_helper(struct transport *transport)
 		close(data->helper->out);
 		fclose(data->out);
 		res = finish_command(data->helper);
-		argv_array_free_detached(data->helper->argv);
 		free(data->helper);
 		data->helper = NULL;
 	}
@@ -397,18 +394,16 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 {
 	struct child_process *helper = get_helper(transport);
 	struct helper_data *data = transport->data;
-	struct argv_array argv = ARGV_ARRAY_INIT;
 	int cat_blob_fd, code;
 	memset(fastimport, 0, sizeof(*fastimport));
 	fastimport->in = helper->out;
-	argv_array_push(&argv, "fast-import");
-	argv_array_push(&argv, debug ? "--stats" : "--quiet");
+	argv_array_push(&fastimport->args, "fast-import");
+	argv_array_push(&fastimport->args, debug ? "--stats" : "--quiet");
 
 	if (data->bidi_import) {
 		cat_blob_fd = xdup(helper->in);
-		argv_array_pushf(&argv, "--cat-blob-fd=%d", cat_blob_fd);
+		argv_array_pushf(&fastimport->args, "--cat-blob-fd=%d", cat_blob_fd);
 	}
-	fastimport->argv = argv.argv;
 	fastimport->git_cmd = 1;
 
 	code = start_command(fastimport);
@@ -421,30 +416,24 @@ static int get_exporter(struct transport *transport,
 {
 	struct helper_data *data = transport->data;
 	struct child_process *helper = get_helper(transport);
-	int argc = 0, i;
-	struct strbuf tmp = STRBUF_INIT;
+	int i;
 
 	memset(fastexport, 0, sizeof(*fastexport));
 
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
 	fastexport->out = dup(helper->in);
-	fastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));
-	fastexport->argv[argc++] = "fast-export";
-	fastexport->argv[argc++] = "--use-done-feature";
-	fastexport->argv[argc++] = data->signed_tags ?
-		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
-	if (data->export_marks) {
-		strbuf_addf(&tmp, "--export-marks=%s.tmp", data->export_marks);
-		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
-	}
-	if (data->import_marks) {
-		strbuf_addf(&tmp, "--import-marks=%s", data->import_marks);
-		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
-	}
+	argv_array_push(&fastexport->args, "fast-export");
+	argv_array_push(&fastexport->args, "--use-done-feature");
+	argv_array_push(&fastexport->args, data->signed_tags ?
+		"--signed-tags=verbatim" : "--signed-tags=warn-strip");
+	if (data->export_marks)
+		argv_array_pushf(&fastexport->args, "--export-marks=%s.tmp", data->export_marks);
+	if (data->import_marks)
+		argv_array_pushf(&fastexport->args, "--import-marks=%s", data->import_marks);
 
 	for (i = 0; i < revlist_args->nr; i++)
-		fastexport->argv[argc++] = revlist_args->items[i].string;
+		argv_array_push(&fastexport->args, revlist_args->items[i].string);
 
 	fastexport->git_cmd = 1;
 	return start_command(fastexport);
@@ -485,7 +474,6 @@ static int fetch_with_import(struct transport *transport,
 
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
-	argv_array_free_detached(fastimport.argv);
 
 	/*
 	 * The fast-import stream of a remote helper that advertises