Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 6 Jul 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I thought we made this into
> >> 
> >> 	if verbose
> >> 		return run_command
> >> 	else
> >> 		return run_command_silently
> >> 
> >> to help readers in the previous round already.
> >
> > FWIW we had quite a couple of reviews in the recent years which pointed
> > out "unnecessary else" after returning or die()ing. Maybe we should make
> > up our minds, and set a consistent rule to follow.
> 
> FWIW the pattern you are referring to is
> 
> 	do something;
> 	if (error detected) {
> 		return error(...);
> 	} else {
> 		perform
> 		remaining
> 		actions
> 		the function needs
> 		to complete
> 	        its task;
> 	}
> 
> and those reviewers (including me) are absolutely right that such an
> "else" is not just unnecessary but actively harms readability of the
> flow of logic.
> 
> I am also absolutely right when I say what is quoted at the top
> makes 100% more sense than
> 
> 	if (verbose)
>         	return run_command();
> 	return run_command_silently();
> 
> as these two are about doing the same thing a bit differently.  
> 
> The way to think about the latter is that we won't have this "if
> (verbose)" if there were a variant of run_command() that took a set
> of option bits among which is a verbose bit, but instead would have
> a single call to that function that returns..  So it's not like "in
> an exceptional case, return after calling this function; otherwise
> keep going, which happens to only return after calling another".  It
> is more like "here we need to return after spawning a command, but
> depending on this bit, we spawn the command using a different
> function".
> 
> Good programers recognize the difference between these two patterns
> without being told, and mentors should teach and GSoC student should
> learn that an overly simplified rule like "when 'if' block ends with
> return, do not write 'else'" is harmful.

You recently also suggested this if...else... dance to Pratik, where it
was not at all about doing the same thing slightly differently, but rather
two different things: 1) return an error because execvp() returned an
error, 2) indicate a serious bug (and you did not even suggest using BUG()
IIRC which is also wrong).

Maybe I am the only one who finds this inconsistent and confusing. If that
is that case, I'll quiet down.

Ciao,
Johannes