| Message ID | df4c21b49f86d6e1e9d2b28375ab6465ffa4339a.1722933642.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
Hi Patrick On 06/08/2024 10:00, Patrick Steinhardt wrote: > We're not releasing the `todo_list` in `sequencer_pick_revisions()` when > hitting an error path. Restructure the function to have a common exit > path such that we can easily clean up the list and thus plug this memory > leak. This looks good, I've left a couple of small formatting comments below if you do end up re-rolling. > @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r, > enum object_type type = oid_object_info(r, > &oid, > NULL); > - return error(_("%s: can't cherry-pick a %s"), > + res = error(_("%s: can't cherry-pick a %s"), > name, type_name(type)); This line needs re-indenting to match the changes above. > + goto out; > } > - } else > - return error(_("%s: bad revision"), name); > + } else { > + res = error(_("%s: bad revision"), name); > + goto out; > + } > } > > /* > @@ -5525,14 +5530,23 @@ int sequencer_pick_revisions(struct repository *r, > opts->revs->no_walk && > !opts->revs->cmdline.rev->flags) { > struct commit *cmit; > - if (prepare_revision_walk(opts->revs)) > - return error(_("revision walk setup failed")); > + This whitespace change is good as it means we now have an empty line between the variable declarations and the code, the others I'm not fussed about either way. Best Wishes Phillip > + if (prepare_revision_walk(opts->revs)) { > + res = error(_("revision walk setup failed")); > + goto out; > + } > + > cmit = get_revision(opts->revs); > - if (!cmit) > - return error(_("empty commit set passed")); > + if (!cmit) { > + res = error(_("empty commit set passed")); > + goto out; > + } > + > if (get_revision(opts->revs)) > BUG("unexpected extra commit from walk"); > - return single_pick(r, cmit, opts); > + > + res = single_pick(r, cmit, opts); > + goto out; > } > > /* > @@ -5542,16 +5556,30 @@ int sequencer_pick_revisions(struct repository *r, > */ > > if (walk_revs_populate_todo(&todo_list, opts) || > - create_seq_dir(r) < 0) > - return -1; > - if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) > - return error(_("can't revert as initial commit")); > - if (save_head(oid_to_hex(&oid))) > - return -1; > - if (save_opts(opts)) > - return -1; > + create_seq_dir(r) < 0) { > + res = -1; > + goto out; > + } > + > + if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) { > + res = error(_("can't revert as initial commit")); > + goto out; > + } > + > + if (save_head(oid_to_hex(&oid))) { > + res = -1; > + goto out; > + } > + > + if (save_opts(opts)) { > + res = -1; > + goto out; > + } > + > update_abort_safety_file(); > res = pick_commits(r, &todo_list, opts); > + > +out: > todo_list_release(&todo_list); > return res; > } > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > index 7eb52b12ed..93c725bac3 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -12,6 +12,7 @@ test_description='Test cherry-pick continuation features > > ' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > # Repeat first match 10 times
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Patrick > > On 06/08/2024 10:00, Patrick Steinhardt wrote: >> We're not releasing the `todo_list` in `sequencer_pick_revisions()` when >> hitting an error path. Restructure the function to have a common exit >> path such that we can easily clean up the list and thus plug this memory >> leak. > > This looks good, I've left a couple of small formatting comments below > if you do end up re-rolling. Oh, formatting nitpicks, my favourite ;-) >> @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r, >> enum object_type type = oid_object_info(r, >> &oid, >> NULL); Also, if we say enum object_type type; type = oid_object_info(r, &oid, NULL); the result is much easier on the eyes usign the same three lines. Yes, initializing while declaring may look nicer and in some cases it may even be necessary, but not this one. >> - return error(_("%s: can't cherry-pick a %s"), >> + res = error(_("%s: can't cherry-pick a %s"), >> name, type_name(type)); > > This line needs re-indenting to match the changes above. Thanks.
diff --git a/sequencer.c b/sequencer.c index cade9b0ca8..fec3c5e846 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5490,8 +5490,10 @@ int sequencer_pick_revisions(struct repository *r, int i, res; assert(opts->revs); - if (read_and_refresh_cache(r, opts)) - return -1; + if (read_and_refresh_cache(r, opts)) { + res = -1; + goto out; + } for (i = 0; i < opts->revs->pending.nr; i++) { struct object_id oid; @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r, enum object_type type = oid_object_info(r, &oid, NULL); - return error(_("%s: can't cherry-pick a %s"), + res = error(_("%s: can't cherry-pick a %s"), name, type_name(type)); + goto out; } - } else - return error(_("%s: bad revision"), name); + } else { + res = error(_("%s: bad revision"), name); + goto out; + } } /* @@ -5525,14 +5530,23 @@ int sequencer_pick_revisions(struct repository *r, opts->revs->no_walk && !opts->revs->cmdline.rev->flags) { struct commit *cmit; - if (prepare_revision_walk(opts->revs)) - return error(_("revision walk setup failed")); + + if (prepare_revision_walk(opts->revs)) { + res = error(_("revision walk setup failed")); + goto out; + } + cmit = get_revision(opts->revs); - if (!cmit) - return error(_("empty commit set passed")); + if (!cmit) { + res = error(_("empty commit set passed")); + goto out; + } + if (get_revision(opts->revs)) BUG("unexpected extra commit from walk"); - return single_pick(r, cmit, opts); + + res = single_pick(r, cmit, opts); + goto out; } /* @@ -5542,16 +5556,30 @@ int sequencer_pick_revisions(struct repository *r, */ if (walk_revs_populate_todo(&todo_list, opts) || - create_seq_dir(r) < 0) - return -1; - if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) - return error(_("can't revert as initial commit")); - if (save_head(oid_to_hex(&oid))) - return -1; - if (save_opts(opts)) - return -1; + create_seq_dir(r) < 0) { + res = -1; + goto out; + } + + if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) { + res = error(_("can't revert as initial commit")); + goto out; + } + + if (save_head(oid_to_hex(&oid))) { + res = -1; + goto out; + } + + if (save_opts(opts)) { + res = -1; + goto out; + } + update_abort_safety_file(); res = pick_commits(r, &todo_list, opts); + +out: todo_list_release(&todo_list); return res; } diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 7eb52b12ed..93c725bac3 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -12,6 +12,7 @@ test_description='Test cherry-pick continuation features ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Repeat first match 10 times
We're not releasing the `todo_list` in `sequencer_pick_revisions()` when hitting an error path. Restructure the function to have a common exit path such that we can easily clean up the list and thus plug this memory leak. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- sequencer.c | 64 +++++++++++++++++++++++---------- t/t3510-cherry-pick-sequence.sh | 1 + 2 files changed, 47 insertions(+), 18 deletions(-)