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

On Tue, May 28, 2013 at 09:32:59PM -0500, Felipe Contreras wrote:

Junio C Hamano wrote:

quoted

Neil Horman [off-list ref] writes:

quoted

On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:

quoted

We should free objects before leaving.

Signed-off-by: Felipe Contreras <redacted>
---
 sequencer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..7eeae2f 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		rerere(opts->allow_rerere_auto);
 	} else {
 		int 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);
 	}
 
+leave:
 	free_message(&msg);
 	free(defmsg);
 

-- 
1.8.3.rc3.312.g47657de

Acked-by: Neil Horman <nhorman@tuxdriver.com>

This is better done without "goto" in general.

The other patch 2/2/ adds one more "we need to exit from the middle
of the flow" and makes it look handier to add an exit label here,
but it would be even better to express the logic of that patch as a
normal cascade of if/else if/..., which is small enough and we do
not need the "leave:" label.

Linux kernel developers would disagree. In C 'goto' is quite of then the only
sane option, and you can see 'goto' used in the Linux kernel all over the place
for that reason.

In this particular case it also makes perfect sense.

I agree with Felipe here.  Setting asside coding practice in other projects,
while its nice to follow coding convention in a project, a jump label just makes
more sense here.  To not use it either requires you to duplicate the free
statements (undesireable), or to change the sense of theif clause here and nest
your if statements (makes for ugly reading).

quoted

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?

I agree here as well.  This fixes a bug that has nothing to do with the other
patch, save for it being in the same C file.  Fix them separately.

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.

-- 
Felipe Contreras