[PATCH 0/3] Miscellanous cleanups around start_command/run_command callers

[PATCH 0/3] Miscellanous cleanups around start_command/run_command callers

[Johannes Sixt]

This is just to flush a queue of unrelated changes before they pile up.
I found these while I reviewed start_command and run_command call sites.

Johannes Sixt (3):
  Simplify some instances of run_command() by using
    run_command_v_opt().
  diff.c: plug a memory leak in an error path
  Simplify some 'fprintf(stderr); return -1;' by using 'return error()'

 builtin-help.c         |   16 ++++++----------
 builtin-receive-pack.c |   10 +++-------
 diff.c                 |    1 +
 ll-merge.c             |   11 ++---------
 merge-index.c          |   42 +++++++++++++-----------------------------
 5 files changed, 25 insertions(+), 55 deletions(-)

[PATCH 1/3] Simplify some instances of run_command() by using run_command_v_opt().

[Johannes Sixt]

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-receive-pack.c |   10 +++-------
 ll-merge.c             |   11 ++---------
 merge-index.c          |   42 +++++++++++++-----------------------------
 3 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 0b08da9..33d345d 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -192,7 +192,6 @@ static int run_receive_hook(const char *hook_name)
 static int run_update_hook(struct command *cmd)
 {
 	static const char update_hook[] = "hooks/update";
-	struct child_process proc;
 	const char *argv[5];
 
 	if (access(update_hook, X_OK) < 0)
@@ -204,12 +203,9 @@ static int run_update_hook(struct command *cmd)
 	argv[3] = sha1_to_hex(cmd->new_sha1);
 	argv[4] = NULL;
 
-	memset(&proc, 0, sizeof(proc));
-	proc.argv = argv;
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-
-	return hook_status(run_command(&proc), update_hook);
+	return hook_status(run_command_v_opt(argv, RUN_COMMAND_NO_STDIN |
+					RUN_COMMAND_STDOUT_TO_STDERR),
+			update_hook);
 }
 
 static int is_ref_checked_out(const char *ref)
diff --git a/ll-merge.c b/ll-merge.c
index 81c02ad..31d6f0a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -175,8 +175,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 		{ "B", temp[2] },
 		{ NULL }
 	};
-	struct child_process child;
-	const char *args[20];
+	const char *args[] = { "sh", "-c", NULL, NULL };
 	int status, fd, i;
 	struct stat st;
 
@@ -191,14 +190,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 
 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
 
-	memset(&child, 0, sizeof(child));
-	child.argv = args;
-	args[0] = "sh";
-	args[1] = "-c";
 	args[2] = cmd.buf;
-	args[3] = NULL;
-
-	status = run_command(&child);
+	status = run_command_v_opt(args, 0);
 	if (status < -ERR_RUN_COMMAND_FORK)
 		; /* failure in run-command */
 	else
diff --git a/merge-index.c b/merge-index.c
index aa9cf23..19ddd03 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -3,45 +3,20 @@
 #include "exec_cmd.h"
 
 static const char *pgm;
-static const char *arguments[9];
 static int one_shot, quiet;
 static int err;
 
-static void run_program(void)
-{
-	struct child_process child;
-	memset(&child, 0, sizeof(child));
-	child.argv = arguments;
-	if (run_command(&child)) {
-		if (one_shot) {
-			err++;
-		} else {
-			if (!quiet)
-				die("merge program failed");
-			exit(1);
-		}
-	}
-}
-
 static int merge_entry(int pos, const char *path)
 {
 	int found;
+	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
+	char hexbuf[4][60];
+	char ownbuf[4][60];
 
 	if (pos >= active_nr)
 		die("git merge-index: %s not in the cache", path);
-	arguments[0] = pgm;
-	arguments[1] = "";
-	arguments[2] = "";
-	arguments[3] = "";
-	arguments[4] = path;
-	arguments[5] = "";
-	arguments[6] = "";
-	arguments[7] = "";
-	arguments[8] = NULL;
 	found = 0;
 	do {
-		static char hexbuf[4][60];
-		static char ownbuf[4][60];
 		struct cache_entry *ce = active_cache[pos];
 		int stage = ce_stage(ce);
 
@@ -55,7 +30,16 @@ static int merge_entry(int pos, const char *path)
 	} while (++pos < active_nr);
 	if (!found)
 		die("git merge-index: %s not in the cache", path);
-	run_program();
+
+	if (run_command_v_opt(arguments, 0)) {
+		if (one_shot)
+			err++;
+		else {
+			if (!quiet)
+				die("merge program failed");
+			exit(1);
+		}
+	}
 	return found;
 }
 
-- 
1.6.3.17.g1665f

[PATCH 2/3] diff.c: plug a memory leak in an error path

[Johannes Sixt]

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 diff.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 4d0a5b9..43835d7 100644
--- a/diff.c
+++ b/diff.c
@@ -3596,6 +3596,7 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	if (start_command(&child) != 0 ||
 	    strbuf_read(&buf, child.out, 0) < 0 ||
 	    finish_command(&child) != 0) {
+		strbuf_release(&buf);
 		remove_tempfile();
 		error("error running textconv command '%s'", pgm);
 		return NULL;
-- 
1.6.3.17.g1665f

[PATCH 3/3] Simplify some 'fprintf(stderr); return -1;' by using 'return error()'

[Johannes Sixt]

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-help.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin-help.c b/builtin-help.c
index af565fb..6e53b23 100644
--- a/builtin-help.c
+++ b/builtin-help.c
@@ -80,10 +80,9 @@ static int check_emacsclient_version(void)
 	ec_process.argv = argv_ec;
 	ec_process.err = -1;
 	ec_process.stdout_to_stderr = 1;
-	if (start_command(&ec_process)) {
-		fprintf(stderr, "Failed to start emacsclient.\n");
-		return -1;
-	}
+	if (start_command(&ec_process))
+		return error("Failed to start emacsclient.");
+
 	strbuf_read(&buffer, ec_process.err, 20);
 	close(ec_process.err);
 
@@ -94,20 +93,17 @@ static int check_emacsclient_version(void)
 	finish_command(&ec_process);
 
 	if (prefixcmp(buffer.buf, "emacsclient")) {
-		fprintf(stderr, "Failed to parse emacsclient version.\n");
 		strbuf_release(&buffer);
-		return -1;
+		return error("Failed to parse emacsclient version.");
 	}
 
 	strbuf_remove(&buffer, 0, strlen("emacsclient"));
 	version = atoi(buffer.buf);
 
 	if (version < 22) {
-		fprintf(stderr,
-			"emacsclient version '%d' too old (< 22).\n",
-			version);
 		strbuf_release(&buffer);
-		return -1;
+		return error("emacsclient version '%d' too old (< 22).",
+			version);
 	}
 
 	strbuf_release(&buffer);
-- 
1.6.3.17.g1665f