| Message ID | patch-v2-01.27-8fbafe61053-20220323T203149Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | revision.[ch]: add and use release_revisions() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Fix a memory leak that's been with us since f9500261e0a (fast-rebase: > write conflict state to working tree, index, and HEAD, 2021-05-20) > changed this code to move these strbuf_release() into an if/else > block. > > We'll also add to "reflog_msg" in the "else" arm of the "if" block > being modified here, and we'll append to "branch_msg" in both > cases. But after f9500261e0a only the "if" block would free these two > "struct strbuf". > > This change is needed so that a subsequent addition of a "goto > cleanup" pattern when adding release_revisions() doesn't solve this > unrelated bug while it's at it. With or without the goto-cleanup change, this fix alone is the right fix for these leaks. I would not have written the last paragraph if I were working on this topic ;-) > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/helper/test-fast-rebase.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c > index fc2d4609043..993b90eaedd 100644 > --- a/t/helper/test-fast-rebase.c > +++ b/t/helper/test-fast-rebase.c > @@ -201,8 +201,6 @@ int cmd__fast_rebase(int argc, const char **argv) > } > if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0) > die(_("unable to update HEAD")); > - strbuf_release(&reflog_msg); > - strbuf_release(&branch_name); > > prime_cache_tree(the_repository, the_repository->index, > result.tree); > @@ -221,5 +219,8 @@ int cmd__fast_rebase(int argc, const char **argv) > if (write_locked_index(&the_index, &lock, > COMMIT_LOCK | SKIP_IF_UNCHANGED)) > die(_("unable to write %s"), get_index_file()); > + > + strbuf_release(&reflog_msg); > + strbuf_release(&branch_name); > return (result.clean == 0); > }
On Wed, Mar 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Fix a memory leak that's been with us since f9500261e0a (fast-rebase: >> write conflict state to working tree, index, and HEAD, 2021-05-20) >> changed this code to move these strbuf_release() into an if/else >> block. >> >> We'll also add to "reflog_msg" in the "else" arm of the "if" block >> being modified here, and we'll append to "branch_msg" in both >> cases. But after f9500261e0a only the "if" block would free these two >> "struct strbuf". >> >> This change is needed so that a subsequent addition of a "goto >> cleanup" pattern when adding release_revisions() doesn't solve this >> unrelated bug while it's at it. > > With or without the goto-cleanup change, this fix alone is the right > fix for these leaks. I would not have written the last paragraph if > I were working on this topic ;-) To this and your 02/27 comment: Sure, I can remove that. In general I think it's bad form for individual commits to discuss the series they're in, they should be stand-alone. But I'm pretty sure if I left this out I'd have gotten questions about why these seemingly unrelated leak fixes are here as art of series implementing release_revisions(), from either you or someone else :)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But I'm pretty sure if I left this out I'd have gotten questions about > why these seemingly unrelated leak fixes are here as art of series > implementing release_revisions(), from either you or someone else :) That is an indication that these changes are outside the scope of the series, and should be done as a preliminary and separate series, isn't it?
diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c index fc2d4609043..993b90eaedd 100644 --- a/t/helper/test-fast-rebase.c +++ b/t/helper/test-fast-rebase.c @@ -201,8 +201,6 @@ int cmd__fast_rebase(int argc, const char **argv) } if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0) die(_("unable to update HEAD")); - strbuf_release(&reflog_msg); - strbuf_release(&branch_name); prime_cache_tree(the_repository, the_repository->index, result.tree); @@ -221,5 +219,8 @@ int cmd__fast_rebase(int argc, const char **argv) if (write_locked_index(&the_index, &lock, COMMIT_LOCK | SKIP_IF_UNCHANGED)) die(_("unable to write %s"), get_index_file()); + + strbuf_release(&reflog_msg); + strbuf_release(&branch_name); return (result.clean == 0); }
Fix a memory leak that's been with us since f9500261e0a (fast-rebase: write conflict state to working tree, index, and HEAD, 2021-05-20) changed this code to move these strbuf_release() into an if/else block. We'll also add to "reflog_msg" in the "else" arm of the "if" block being modified here, and we'll append to "branch_msg" in both cases. But after f9500261e0a only the "if" block would free these two "struct strbuf". This change is needed so that a subsequent addition of a "goto cleanup" pattern when adding release_revisions() doesn't solve this unrelated bug while it's at it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/helper/test-fast-rebase.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)