Re: [PATCH 1/2] sequencer: trivial fix

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 29, 2013, 10:51 UTC
Message-ID: <CAMP44s0vARKGsn2noBEAxSVHD1bkU9pR7nPCvFJwp5epwidkQw@mail.gmail.com>
In-Reply-To: <ko4jf7$e4d$1@ger.gmane.org>

On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz <jojo@schmitz-digital.de> wrote:

> Felipe Contreras wrote:
>>
>> Junio C Hamano wrote:

Show 17 quoted lines

>>> It probably is better to fold this patch into the other one when it
>>> is rerolled to correct the option name gotcha "on the tin".
>>
>>
>> Why? This patch is standalone and fixes an issue that is independent
>> of the other patch. Why squash two patches that do *two* different
>> things?
>>
>> Anyway, I'll happily drop this patch if you want this memory leak to
>> remain. But then I'll do the same in the other patch.
>>
>> This mantra of avodiing 'goto' is not helping anybody.
>
>
> adding 5 letters (to change the next "if" into an "else if") versus your
> addition of several lines and some 15 additional letters (ignoring the
> whitsspace)  is IMHO enough to see what is better?

This has nothing to do with what Junio said. Junio said it is better to squash the two changes into one, which is not clearly better.

As for your suggestion, what happens the next time somebody needs to add something else to this chunk of code? Another if, and then another, and soon enough you end up with five levels of indentation, and in some of those patches you have to change the indentation of existing code.

If only there was much bigger and successful software project that had hashed all these questions and came up with a code-style to last the ages. Oh, but there is, it's called Linux, and the answer is to use goto's.

If the code used a goto in the first place.. BAM:

--- a/sequencer.c
+++ b/sequencer.c
 <at>  <at>  -628,8 +628,10  <at>  <at>  static int
do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}

 	allow = allow_empty(opts, commit);
-	if (allow < 0)
-		return allow;
+	if (allow < 0) {
+		res = allow;
+		goto leave;
+	}
 	if (!opts->no_commit)
 		res = run_git_commit(defmsg, opts, allow);

And every time you need to add more code you just do it, and stop
worrying about increasing indentation, or re-indenting.

Problem solved.

-- 
Felipe Contreras