Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2019, 03:20 UTC
Message-ID: <xmqqftjda745.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqeeyxbqc6.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> So I'd probably just send a "fixup!"to be queued on top of the
> series to fix a leak in 'next' for now, remind the maintainer not to
> merge it to 'master' before the release, and once the upcoming
> release is made, send another reminder to the maintainer to squash
> the "fixup!" in before rebuilding 'next', if I owned this series.

This is based on René's suggestion, but seeing the pre-context of this fix, many of the error code path, after an error return from edit_todo_list() has been handled, can follow the pattern of this fix to jump to the cleanup label after losing their own calls to todo_list_release(). An obvious advantage of doing so is that it future-proofs the codepath---the todo-list structure may not stay to be the only thing that holds resources that need cleaned up.

 sequencer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b395dd6e11..ec0b793fc5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5140,14 +5140,18 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return error_errno(_("could not write '%s'"), todo_file);
 	}
 
+	res = -1;
+
 	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
-		return -1;
+		goto cleanup;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
-		return -1;
+		goto cleanup;
 
 	todo_list_write_total_nr(&new_todo);
 	res = pick_commits(r, &new_todo, opts);
+
+cleanup:
 	todo_list_release(&new_todo);
 
 	return res;