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:
> + struct child_process cmd = CHILD_PROCESS_INIT;
> [...]
> env = read_author_script();
> if (!env) {
The child_process struct comes with its own internal env array. So you
can do:
read_author_script(&cmd.env_array);
if (!cmd.env_array.argc)
...
and then you don't have to worry about free-ing env, as it happens
automatically as part of the child cleanup (I suspect the refactoring
may also reduce some of the confusing memory handling in
read_author_script()).
> + if (cmd.stdout_to_stderr) {
> + /* 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);
> + if (rc)
> + fputs(errout.buf, stderr);
> + } else {
> + rc = run_command(&cmd);
> + }
We have a helper function for capturing output, so I think you can write
this as:
if (cmd.err == -1) {
struct strbuf errout = STRBUF_INIT;
int rc = pipe_command(&cmd,
NULL, 0, /* stdin */
NULL, 0, /* stdout */
&errout, 0);
if (rc)
fputs(errout.buf, stderr);
strbuf_release(&errout);
} else
rc = run_command(&cmd);
and drop the cleanup goto entirely (if you do the "env" thing above, you
could even drop "rc" and just return directly from each branch of the
conditional).
> - rc = run_command_v_opt_cd_env(array.argv, opt, NULL,
> - (const char *const *)env);
> - argv_array_clear(&array);
> +cleanup:
> + child_process_clear(&cmd);
> + strbuf_release(&errout);
> free(env);
Even if you do keep the goto here, I think this child_process_clear() is
unnecessary. It should be done automatically either by finish_command(),
or by start_command() when it returns an error.
-Peff