Hi Junio,
Le 22/06/2018 à 00:03, Junio C Hamano a écrit :
> 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."
>
I like your wording. It’s not a difference of point of view, I’m just bad at
writing commit messages ;)
> > +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.
>
Yeah, you’re right.
> > @@ -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);
> >
> > }
>
> 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);
Okay.
Cheers,
Alban
