prepare_{git,shell}_cmd: use argv_array

These functions transform an existing argv into one suitable
for exec-ing or spawning via git or a shell. We can use an
argv_array in each to avoid dealing with manual counting and
allocation.

This also makes the memory allocation more clear and fixes
some leaks. In prepare_shell_cmd, we would sometimes
allocate a new string with "$@" in it and sometimes not,
meaning the caller could not correctly free it. On the
non-Windows side, we are in a child process which will
exec() or exit() immediately, so the leak isn't a big deal.
On Windows, though, we use spawn() from the parent process,
and leak a string for each shell command we run. On top of
that, the Windows code did not free the allocated argv array
at all (but does for the prepare_git_cmd case!).

By switching both of these functions to write into an
argv_array, we can consistently free the result as
appropriate.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

3 files changed, 39 insertions, 53 deletions

diff --git a/exec_cmd.c b/exec_cmd.c
index e85f0fd8d8..cf442a97f8 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "quote.h"
+#include "argv-array.h"
 #define MAX_ARGS	32
 
 static const char *argv_exec_path;
@@ -107,32 +108,25 @@ void setup_path(void)
 	strbuf_release(&new_path);
 }
 
-const char **prepare_git_cmd(const char **argv)
+const char **prepare_git_cmd(struct argv_array *out, const char **argv)
 {
-	int argc;
-	const char **nargv;
-
-	for (argc = 0; argv[argc]; argc++)
-		; /* just counting */
-	nargv = xmalloc(sizeof(*nargv) * (argc + 2));
-
-	nargv[0] = "git";
-	for (argc = 0; argv[argc]; argc++)
-		nargv[argc + 1] = argv[argc];
-	nargv[argc + 1] = NULL;
-	return nargv;
+	argv_array_push(out, "git");
+	argv_array_pushv(out, argv);
+	return out->argv;
 }
 
 int execv_git_cmd(const char **argv) {
-	const char **nargv = prepare_git_cmd(argv);
-	trace_argv_printf(nargv, "trace: exec:");
+	struct argv_array nargv = ARGV_ARRAY_INIT;
+
+	prepare_git_cmd(&nargv, argv);
+	trace_argv_printf(nargv.argv, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
-	sane_execvp("git", (char **)nargv);
+	sane_execvp("git", (char **)nargv.argv);
 
 	trace_printf("trace: exec failed: %s\n", strerror(errno));
 
-	free(nargv);
+	argv_array_clear(&nargv);
 	return -1;
 }
 
diff --git a/exec_cmd.h b/exec_cmd.h
index 93b0c02529..1f6b43378b 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -1,11 +1,13 @@
 #ifndef GIT_EXEC_CMD_H
 #define GIT_EXEC_CMD_H
 
+struct argv_array;
+
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
 extern const char *git_exec_path(void);
 extern void setup_path(void);
-extern const char **prepare_git_cmd(const char **argv);
+extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
diff --git a/run-command.c b/run-command.c
index 13fa452e8c..171cbaa944 100644
--- a/run-command.c
+++ b/run-command.c
@@ -158,50 +158,41 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
-static const char **prepare_shell_cmd(const char **argv)
+static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
 {
-	int argc, nargc = 0;
-	const char **nargv;
-
-	for (argc = 0; argv[argc]; argc++)
-		; /* just counting */
-	/* +1 for NULL, +3 for "sh -c" plus extra $0 */
-	nargv = xmalloc(sizeof(*nargv) * (argc + 1 + 3));
-
-	if (argc < 1)
+	if (!argv[0])
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
 #ifndef GIT_WINDOWS_NATIVE
-		nargv[nargc++] = SHELL_PATH;
+		argv_array_push(out, SHELL_PATH);
 #else
-		nargv[nargc++] = "sh";
+		argv_array_push(out, "sh");
 #endif
-		nargv[nargc++] = "-c";
-
-		if (argc < 2)
-			nargv[nargc++] = argv[0];
-		else {
-			struct strbuf arg0 = STRBUF_INIT;
-			strbuf_addf(&arg0, "%s \"$@\"", argv[0]);
-			nargv[nargc++] = strbuf_detach(&arg0, NULL);
-		}
-	}
+		argv_array_push(out, "-c");
 
-	for (argc = 0; argv[argc]; argc++)
-		nargv[nargc++] = argv[argc];
-	nargv[nargc] = NULL;
+		/*
+		 * If we have no extra arguments, we do not even need to
+		 * bother with the "$@" magic.
+		 */
+		if (!argv[1])
+			argv_array_push(out, argv[0]);
+		else
+			argv_array_pushf(out, "%s \"$@\"", argv[0]);
+	}
 
-	return nargv;
+	argv_array_pushv(out, argv);
+	return out->argv;
 }
 
 #ifndef GIT_WINDOWS_NATIVE
 static int execv_shell_cmd(const char **argv)
 {
-	const char **nargv = prepare_shell_cmd(argv);
-	trace_argv_printf(nargv, "trace: exec:");
-	sane_execvp(nargv[0], (char **)nargv);
-	free(nargv);
+	struct argv_array nargv = ARGV_ARRAY_INIT;
+	prepare_shell_cmd(&nargv, argv);
+	trace_argv_printf(nargv.argv, "trace: exec:");
+	sane_execvp(nargv.argv[0], (char **)nargv.argv);
+	argv_array_clear(&nargv);
 	return -1;
 }
 #endif
@@ -455,6 +446,7 @@ fail_pipe:
 {
 	int fhin = 0, fhout = 1, fherr = 2;
 	const char **sargv = cmd->argv;
+	struct argv_array nargv = ARGV_ARRAY_INIT;
 
 	if (cmd->no_stdin)
 		fhin = open("/dev/null", O_RDWR);
@@ -480,9 +472,9 @@ fail_pipe:
 		fhout = dup(cmd->out);
 
 	if (cmd->git_cmd)
-		cmd->argv = prepare_git_cmd(cmd->argv);
+		cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
 	else if (cmd->use_shell)
-		cmd->argv = prepare_shell_cmd(cmd->argv);
+		cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, (char**) cmd->env,
 			cmd->dir, fhin, fhout, fherr);
@@ -492,9 +484,7 @@ fail_pipe:
 	if (cmd->clean_on_exit && cmd->pid >= 0)
 		mark_child_for_cleanup(cmd->pid);
 
-	if (cmd->git_cmd)
-		free(cmd->argv);
-
+	argv_array_clear(&nargv);
 	cmd->argv = sargv;
 	if (fhin != 0)
 		close(fhin);