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

[Alban Gruin <alban.gruin@gmail.com>]

Hi Junio,

Le 22/06/2018 à 00:03, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> 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