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

> Hi Alban,
> 
> On Tue, 19 Jun 2018, Alban Gruin wrote:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 7cc76332e..9aa7ddb33 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -766,6 +766,29 @@ 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,
>> +					 unsigned verbose)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	int rc;
>> +
>> +	if (verbose)
>> +		return run_command(cmd);
>> +
>> +	/* hide stderr on success */
>> +	cmd->stdout_to_stderr = 1;
> 
> This comment is a bit misleading: that line does not hide stderr on
> success, it redirects stdout to stderr instead.
> 
>> +	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 +843,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;
>> -		}
> 
> This code made sure that we *only* do this redirection, and stderr
> buffering, if `git commit` is called non-interactively. When it is called
> interactively, redirecting stdout and stderr is absolutely not what we
> want.
> 
>> +	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 +877,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;
>> -	}
>> -
>> -	return run_command(&cmd);
>> +	return run_command_silent_on_success(&cmd,
>> +					     !(is_rebase_i(opts) && !(flags & EDIT_MSG)));
> 
> It would probably make more sense to change the signature of
> `run_command_silent_on_success()` to not even take the `int verbose`
> parameter: why call it "silent on success" when we can ask it *not* to be
> silent on success?
> 
> And then you can avoid this overly-long line (as well as the quite
> convoluted Boolean logic that took me a couple of seconds to verify) very
> elegantly by this code:
> 
> 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> 		return run_command_silent_on_success(&cmd);
> 	return run_command(&cmd);
> 
> I vaguely recall that I wanted to make this an option in the `struct
> child_process` when I originally introduced this code, but I think it was
> Peff who suggested that doing it "by hand" was the more appropriate way
> here because I use it only once.
> 
> My recollection might fail me, but if it is correct, maybe that would be a
> good way forward, to make this an `int silent_on_success:1;` field?
>