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 <avarab@xxxxxxxxx>
---
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;
}
