Hi Johannes,
Le 21/06/2018 à 11:37, Johannes Schindelin a écrit :
> 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?
>
I think I found it:
https://public-inbox.org/git/1e82aeabb906a35175362418b2b4957fae50c3b0.1481642927.git.johannes.schindelin@xxxxxx/
Apparently, you wanted to introduce a new RUN_ flag for
run_command_v_opt_cd_env(), but the change was qualified as a “bolted-on
feature” by Johannes Sixt.
So, I will remove the “verbose” argument in the reroll.
Cheers,
Alban
