From: Phillip Wood <phillip.wood@talktalk.net>
Date: Mon, 18 Jun 2018 15:26:34 GMT
Subject: Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.
Message-ID: <b23a9e30-58bf-afde-4d77-563e0b288e70@talktalk.net>
In-Reply-To: <20180618131844.13408-2-alban.gruin@gmail.com>

Hi Alban

On 18/06/18 14:18, Alban Gruin wrote:
> This adds a new function, run_command_silent_if_successful(), 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.

s/functionnaly/functionally/

It's not quite the same though because the shell versions handles 
--verbose where as here the caller has to put that check in every call 
site. I wonder if it would simplify the callers if the C version did the 
--verbose handling it's self.


Best Wishes

Phillip
> run_git_commit() is then refactored to use of
> run_command_silent_if_successful().
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 53 +++++++++++++++++++++++++++--------------------------
>   1 file changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 7cc76332e..3437673d2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -766,6 +766,25 @@ N_("you have staged changes in your working tree\n"
>   #define VERIFY_MSG  (1<<4)
>   #define CREATE_ROOT_COMMIT (1<<5)
>   
> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int rc;
> +
> +	/* hide stderr on success */
> +	cmd->stdout_to_stderr = 1;
> +	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 we are cherry-pick, and if the merge did not result in
>    * hand-editing, we will hit this commit and inherit the original
> @@ -820,18 +839,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);
>   	}
>   
>   	argv_array_push(&cmd.args, "commit");
> @@ -861,21 +873,10 @@ 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;
> -	}
> -
> -	return run_command(&cmd);
> +	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> +		return run_command_silent_on_success(&cmd);
> +	else
> +		return run_command(&cmd);
>   }
>   
>   static int rest_is_empty(const struct strbuf *sb, int start)
>