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?
Ciao,
Dscho
