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

From: Jeff King <peff@peff.net>
Date: Dec 14, 2016, 12:53 UTC
Message-ID: <20161214125322.o3naglvyuzgk2pri@sigill.intra.peff.net>
In-Reply-To: <2637bed1-c36f-32f6-b255-ea32da76d792@kdbg.org>

On Wed, Dec 14, 2016 at 09:34:20AM +0100, Johannes Sixt wrote:

Show 6 quoted lines

> 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()).

Show 20 quoted lines

> +	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).

Show 7 quoted lines

> -	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