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)