From: Phillip Wood Date: Sat, 31 Dec 2022 15:03:39 GMT Subject: Re: [PATCH 08/10] sequencer.c: always free() the "msgbuf" in do_pick_commit() Message-ID: In-Reply-To: Hi Ævar On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote: > In [1] the strbuf_release(&msgbuf) was moved into this > do_pick_commit(), but didn't take into account the case of [2], where > we'd return before the strbuf_release(&msgbuf). > > Then when the "fixup" support was added in [3] this leak got worse, as > we added another place where we'd "return" before reaching the > strbuf_release(). This message makes it sound much worse that it is. The leak only happens if there is an error and in that case rebase bails out straight away. > Let's move it to a "cleanup" label, and use an appropriate "goto". It > may or may not be safe to combine the existing "leave" and "cleanup" > labels, but this change doesn't attempt to answer that question. Let's > instead avoid calling update_abort_safety_file() in these cases, as we > didn't do so before. It is safe as update_abort_safety_file() is a no-op when rebasing and you're changing code that is only run when rebasing. I feel this patch is adding complexity for no real gain, at least if we can avoid adding a second label it would be simpler. Best Wishes Phillip > 1. 452202c74b8 (sequencer: stop releasing the strbuf in > write_message(), 2016-10-21) > 2. f241ff0d0a9 (prepare the builtins for a libified merge_recursive(), > 2016-07-26) > 3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and > 'squash' commands, 2017-01-02) > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > sequencer.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 47367e66842..db8d789fa76 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2280,8 +2280,10 @@ static int do_pick_commit(struct repository *r, > reword = 1; > else if (is_fixup(command)) { > if (update_squash_messages(r, command, commit, > - opts, item->flags)) > - return -1; > + opts, item->flags)) { > + res = -1; > + goto cleanup; > + } > flags |= AMEND_MSG; > if (!final_fixup) > msg_file = rebase_path_squash_msg(); > @@ -2291,9 +2293,11 @@ static int do_pick_commit(struct repository *r, > } else { > const char *dest = git_path_squash_msg(r); > unlink(dest); > - if (copy_file(dest, rebase_path_squash_msg(), 0666)) > - return error(_("could not rename '%s' to '%s'"), > - rebase_path_squash_msg(), dest); > + if (copy_file(dest, rebase_path_squash_msg(), 0666)) { > + res = error(_("could not rename '%s' to '%s'"), > + rebase_path_squash_msg(), dest); > + goto cleanup; > + } > unlink(git_path_merge_msg(r)); > msg_file = dest; > flags |= EDIT_MSG; > @@ -2331,7 +2335,6 @@ static int do_pick_commit(struct repository *r, > free_commit_list(common); > free_commit_list(remotes); > } > - strbuf_release(&msgbuf); > > /* > * If the merge was clean or if it failed due to conflict, we write > @@ -2403,9 +2406,11 @@ static int do_pick_commit(struct repository *r, > } > > leave: > + update_abort_safety_file(); > +cleanup: > free_message(commit, &msg); > free(author); > - update_abort_safety_file(); > + strbuf_release(&msgbuf); > > return res; > }