Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2018, 22:03 UTC
Message-ID: <xmqqk1qrre4w.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180621141732.19952-2-alban.gruin@gmail.com>

Alban Gruin <alban.gruin@gmail.com> writes:

Show 7 quoted lines

> This adds a new function, run_command_silent_on_success(), to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.
>
> run_git_commit() is then refactored to use of
> run_command_silent_on_success().

It might be just a difference in viewpoint, but I think it is more customary in this project (hence it will be easier to understand and accept by readers of the patch) if a change like this is explained NOT as "introducing a new function and then rewrite an existing code to use it", and instead as "extract a helper function from an existing code so that future callers can reuse it."

Show 9 quoted lines

> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	cmd->stdout_to_stderr = 1;
> +	rc = pipe_command(cmd,
> +			  NULL, 0,
> +			  /* stdout is already redirected */

I can see that this comment was inherited from the original place this code was lifted from (and that is why I say this is not "adding a new helper and rewriting an existing piece of code to use it" but is "extracting this function out of an existing code for future reuse"), but does it still make sense in the new context to keep the same comment?

The original in run_git_commit() made the .stdout_to_stderr=1 assignment many lines before it called pipe_command(), and it made sense to remind readers why we are passing NULL to the out buffer and only passing the err buffer here. But in the context of this new helper function, the redirection that may make such a reminder necessary sits immediately before the pipe_command() call for everybody to see.

Show 8 quoted lines

> +			  NULL, 0,
> +			  &buf, 0);
> +
> +	if (rc)
> +		fputs(buf.buf, stderr);
> +	strbuf_release(&buf);
> +	return rc;
> +}

Show 21 quoted lines

> @@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  
>  	cmd.git_cmd = 1;
>  
> -	if (is_rebase_i(opts)) {
> -		if (!(flags & EDIT_MSG)) {
> -			cmd.stdout_to_stderr = 1;
> -			cmd.err = -1;
> -		}
> +	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> +		const char *gpg_opt = gpg_sign_opt_quoted(opts);
>  
> -		if (read_env_script(&cmd.env_array)) {
> -			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> -			return error(_(staged_changes_advice),
> -				     gpg_opt, gpg_opt);
> -		}
> +		return error(_(staged_changes_advice),
> +			     gpg_opt, gpg_opt);
>  	}

The original in this hunk is criminally incomprehensible ;-) The assignment of .stdout_to_stderr=1 does not have to happen this early in the code, and also using .err being -1 to communicate that the "capture and show error only upon error" magic is going to be used to the later code is unnecessarily opaque. The new code gets rid of that ugliness from this earlier hunk and instead makes this part only about reading the various state files (and reporting failure to do so).

Show 22 quoted lines

> @@ -861,20 +872,8 @@ 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");
>  
> -	if (cmd.err == -1) {
> -		/* hide stderr on success */
> -		struct strbuf buf = STRBUF_INIT;
> -		int rc = pipe_command(&cmd,
> -				      NULL, 0,
> -				      /* stdout is already redirected */
> -				      NULL, 0,
> -				      &buf, 0);
> -		if (rc)
> -			fputs(buf.buf, stderr);
> -		strbuf_release(&buf);
> -		return rc;
> -	}
> -
> +	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> +		return run_command_silent_on_success(&cmd);
>  	return run_command(&cmd);
>  }

And then, the updated code in this hunk checks the EDIT_MSG bit instead. It probably is easier to understand the code if you added an "else" after, to highlight the fact that this is choosing one out of two possible ways to run "cmd", i.e.

	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
		return run_command_silent_on_success(&cmd);
	else
		return run_command(&cmd);