Re: [PATCH v2 28/34] run_command_opt(): optionally hide stderr when the command succeeds

[Jeff King <peff@peff.net>]

On Wed, Dec 14, 2016 at 07:53:23AM -0500, Jeff King wrote:

> On Wed, Dec 14, 2016 at 09:34:20AM +0100, Johannes Sixt wrote:
> 
> > I wanted to see what it would look like if we make it the caller's
> > responsibility to throw away stderr. The patch is below, as fixup
> > of patch 29/34. The change is gross, but the end result is not that
> > bad, though not really a delightful read, either, mostly due to the
> > strange cleanup semantics of the start_command/finish_command combo,
> > so... I dunno.
> 
> I don't have a strong opinion on the patches under discussion, but here
> are a few pointers on the run-command interface:
> [...]

And here is a patch representing my suggestions, on top of yours. Not
tested beyond "make test".

I think read_author_script() could be simplified even more by appending
to the env array in the first loop, but I didn't want to refactor the
quote handling.

---
 sequencer.c | 65 +++++++++++------------------------
 1 file changed, 20 insertions(+), 45 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index f375880bd..27a9eaf15 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -591,18 +591,17 @@ static int write_author_script(const char *message)
 }
 
 /*
- * Read the author-script file into an environment block, ready for use in
- * run_command(), that can be free()d afterwards.
+ * Read the author-script file into an environment block. Returns -1
+ * on error, 0 otherwise.
  */
-static char **read_author_script(void)
+static int read_author_script(struct argv_array *env)
 {
 	struct strbuf script = STRBUF_INIT;
 	int i, count = 0;
-	char *p, *p2, **env;
-	size_t env_size;
+	char *p, *p2;
 
 	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
-		return NULL;
+		return -1;
 
 	for (p = script.buf; *p; p++)
 		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
@@ -614,19 +613,12 @@ static char **read_author_script(void)
 			count++;
 		}
 
-	env_size = (count + 1) * sizeof(*env);
-	strbuf_grow(&script, env_size);
-	memmove(script.buf + env_size, script.buf, script.len);
-	p = script.buf + env_size;
-	env = (char **)strbuf_detach(&script, NULL);
-
 	for (i = 0; i < count; i++) {
-		env[i] = p;
+		argv_array_push(env, p);
 		p += strlen(p) + 1;
 	}
-	env[count] = NULL;
 
-	return env;
+	return 0;
 }
 
 static const char staged_changes_advice[] =
@@ -659,20 +651,18 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 			  int allow_empty, int edit, int amend,
 			  int cleanup_commit_message)
 {
-	char **env = NULL;
-	int rc;
 	const char *value;
-	struct strbuf errout = STRBUF_INIT;
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts)) {
-		if (!edit)
+		if (!edit) {
 			cmd.stdout_to_stderr = 1;
+			cmd.err = -1;
+		}
 
-		env = read_author_script();
-		if (!env) {
+		if (read_author_script(&cmd.env_array)) {
 			const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
 			return error(_(staged_changes_advice),
@@ -706,35 +696,20 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (opts->allow_empty_message)
 		argv_array_push(&cmd.args, "--allow-empty-message");
 
-	cmd.env = (const char *const *)env;
-
-	if (cmd.stdout_to_stderr) {
+	if (cmd.err == -1) {
 		/* hide stderr on success */
-		cmd.err = -1;
-		rc = -1;
-		if (start_command(&cmd) < 0)
-			goto cleanup;
-
-		if (strbuf_read(&errout, cmd.err, 0) < 0) {
-			close(cmd.err);
-			finish_command(&cmd); /* throw away exit code */
-			goto cleanup;
-		}
-
-		close(cmd.err);
-		rc = finish_command(&cmd);
+		struct strbuf errout = STRBUF_INIT;
+		int rc = pipe_command(&cmd,
+				      NULL, 0, /* stdin */
+				      NULL, 0, /* stdout */
+				      &errout, 0);
 		if (rc)
 			fputs(errout.buf, stderr);
-	} else {
-		rc = run_command(&cmd);
+		strbuf_release(&errout);
+		return rc;
 	}
 
-cleanup:
-	child_process_clear(&cmd);
-	strbuf_release(&errout);
-	free(env);
-
-	return rc;
+	return run_command(&cmd);
 }
 
 static int is_original_commit_empty(struct commit *commit)