Alban Gruin <alban.gruin@xxxxxxxxx> writes:
> 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."
> +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.
> + NULL, 0,
> + &buf, 0);
> +
> + if (rc)
> + fputs(buf.buf, stderr);
> + strbuf_release(&buf);
> + return rc;
> +}
> @@ -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).
> @@ -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);
