| Message ID | 48bcd0ac80ee0b60eeda2bcedf55003a5049f289.1717402439.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.2) | expand |
Hi Patrick On 03/06/2024 10:48, Patrick Steinhardt wrote: > @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r, > } > unuse_commit_buffer: > repo_unuse_commit_buffer(r, commit, p); > - if (res) > - return res; > + if (res) { > + ret = res; > + goto out; > + } Having 'ret' and 'res' in this block is a bit confusing - we could delete the declaration for 'res' and either replace its use with 'ret', or rename 'ret' to 'res' in this patch. Apart from that this all looks sensible to me, it is nice to see the number of leaks going down. Thanks Phillip
On Mon, Jun 03, 2024 at 02:14:20PM +0100, Phillip Wood wrote: > Hi Patrick > > On 03/06/2024 10:48, Patrick Steinhardt wrote: > > @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r, > > } > > unuse_commit_buffer: > > repo_unuse_commit_buffer(r, commit, p); > > - if (res) > > - return res; > > + if (res) { > > + ret = res; > > + goto out; > > + } > > Having 'ret' and 'res' in this block is a bit confusing - we could delete > the declaration for 'res' and either replace its use with 'ret', or rename > 'ret' to 'res' in this patch. Yeah, let's just drop the local `res` variable here and use `ret` instead. > Apart from that this all looks sensible to me, it is nice to see the number > of leaks going down. > > Thanks Thanks! Patrick
On Tue, Jun 04, 2024 at 08:45:11AM +0200, Patrick Steinhardt wrote: > On Mon, Jun 03, 2024 at 02:14:20PM +0100, Phillip Wood wrote: > > Hi Patrick > > > > On 03/06/2024 10:48, Patrick Steinhardt wrote: > > > @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r, > > > } > > > unuse_commit_buffer: > > > repo_unuse_commit_buffer(r, commit, p); > > > - if (res) > > > - return res; > > > + if (res) { > > > + ret = res; > > > + goto out; > > > + } > > > > Having 'ret' and 'res' in this block is a bit confusing - we could delete > > the declaration for 'res' and either replace its use with 'ret', or rename > > 'ret' to 'res' in this patch. > > Yeah, let's just drop the local `res` variable here and use `ret` > instead. For the record, below is the diff to address this. I'll refrain from sending out this whole patch series again for now to only address this one issue, but will include it if there are more things that need to be handled. Patrick diff --git a/sequencer.c b/sequencer.c index 9e90084692..cc57a30883 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5253,7 +5253,6 @@ static int commit_staged_changes(struct repository *r, * We need to update the squash message to skip * the latest commit message. */ - int res = 0; struct commit *commit; const char *msg; const char *path = rebase_path_squash_msg(); @@ -5266,22 +5265,22 @@ static int commit_staged_changes(struct repository *r, p = repo_logmsg_reencode(r, commit, NULL, encoding); if (!p) { - res = error(_("could not parse commit %s"), + ret = error(_("could not parse commit %s"), oid_to_hex(&commit->object.oid)); goto unuse_commit_buffer; } find_commit_subject(p, &msg); if (write_message(msg, strlen(msg), path, 0)) { - res = error(_("could not write file: " + ret = error(_("could not write file: " "'%s'"), path); goto unuse_commit_buffer; } + + ret = 0; unuse_commit_buffer: repo_unuse_commit_buffer(r, commit, p); - if (res) { - ret = res; + if (ret) goto out; - } } }
Hi Patrick On 04/06/2024 08:19, Patrick Steinhardt wrote: > On Tue, Jun 04, 2024 at 08:45:11AM +0200, Patrick Steinhardt wrote: >> On Mon, Jun 03, 2024 at 02:14:20PM +0100, Phillip Wood wrote: >>> Hi Patrick >>> >>> On 03/06/2024 10:48, Patrick Steinhardt wrote: >>>> @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r, >>>> } >>>> unuse_commit_buffer: >>>> repo_unuse_commit_buffer(r, commit, p); >>>> - if (res) >>>> - return res; >>>> + if (res) { >>>> + ret = res; >>>> + goto out; >>>> + } >>> >>> Having 'ret' and 'res' in this block is a bit confusing - we could delete >>> the declaration for 'res' and either replace its use with 'ret', or rename >>> 'ret' to 'res' in this patch. >> >> Yeah, let's just drop the local `res` variable here and use `ret` >> instead. > > For the record, below is the diff to address this. I'll refrain from > sending out this whole patch series again for now to only address this > one issue, but will include it if there are more things that need to be > handled. > > Patrick This and all the other sequencer changes in this series look good to me Thanks Phillip > diff --git a/sequencer.c b/sequencer.c > index 9e90084692..cc57a30883 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5253,7 +5253,6 @@ static int commit_staged_changes(struct repository *r, > * We need to update the squash message to skip > * the latest commit message. > */ > - int res = 0; > struct commit *commit; > const char *msg; > const char *path = rebase_path_squash_msg(); > @@ -5266,22 +5265,22 @@ static int commit_staged_changes(struct repository *r, > > p = repo_logmsg_reencode(r, commit, NULL, encoding); > if (!p) { > - res = error(_("could not parse commit %s"), > + ret = error(_("could not parse commit %s"), > oid_to_hex(&commit->object.oid)); > goto unuse_commit_buffer; > } > find_commit_subject(p, &msg); > if (write_message(msg, strlen(msg), path, 0)) { > - res = error(_("could not write file: " > + ret = error(_("could not write file: " > "'%s'"), path); > goto unuse_commit_buffer; > } > + > + ret = 0; > unuse_commit_buffer: > repo_unuse_commit_buffer(r, commit, p); > - if (res) { > - ret = res; > + if (ret) > goto out; > - } > } > } >
diff --git a/sequencer.c b/sequencer.c index 475646671a..2ce807533f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5146,33 +5146,47 @@ static int commit_staged_changes(struct repository *r, struct replay_ctx *ctx = opts->ctx; unsigned int flags = ALLOW_EMPTY | EDIT_MSG; unsigned int final_fixup = 0, is_clean; + struct strbuf rev = STRBUF_INIT; + int ret; - if (has_unstaged_changes(r, 1)) - return error(_("cannot rebase: You have unstaged changes.")); + if (has_unstaged_changes(r, 1)) { + ret = error(_("cannot rebase: You have unstaged changes.")); + goto out; + } is_clean = !has_uncommitted_changes(r, 0); if (!is_clean && !file_exists(rebase_path_message())) { const char *gpg_opt = gpg_sign_opt_quoted(opts); - - return error(_(staged_changes_advice), gpg_opt, gpg_opt); + ret = error(_(staged_changes_advice), gpg_opt, gpg_opt); + goto out; } + if (file_exists(rebase_path_amend())) { - struct strbuf rev = STRBUF_INIT; struct object_id head, to_amend; - if (repo_get_oid(r, "HEAD", &head)) - return error(_("cannot amend non-existing commit")); - if (!read_oneliner(&rev, rebase_path_amend(), 0)) - return error(_("invalid file: '%s'"), rebase_path_amend()); - if (get_oid_hex(rev.buf, &to_amend)) - return error(_("invalid contents: '%s'"), - rebase_path_amend()); - if (!is_clean && !oideq(&head, &to_amend)) - return error(_("\nYou have uncommitted changes in your " - "working tree. Please, commit them\n" - "first and then run 'git rebase " - "--continue' again.")); + if (repo_get_oid(r, "HEAD", &head)) { + ret = error(_("cannot amend non-existing commit")); + goto out; + } + + if (!read_oneliner(&rev, rebase_path_amend(), 0)) { + ret = error(_("invalid file: '%s'"), rebase_path_amend()); + goto out; + } + + if (get_oid_hex(rev.buf, &to_amend)) { + ret = error(_("invalid contents: '%s'"), + rebase_path_amend()); + goto out; + } + if (!is_clean && !oideq(&head, &to_amend)) { + ret = error(_("\nYou have uncommitted changes in your " + "working tree. Please, commit them\n" + "first and then run 'git rebase " + "--continue' again.")); + goto out; + } /* * When skipping a failed fixup/squash, we need to edit the * commit message, the current fixup list and count, and if it @@ -5204,9 +5218,11 @@ static int commit_staged_changes(struct repository *r, len--; strbuf_setlen(&ctx->current_fixups, len); if (write_message(p, len, rebase_path_current_fixups(), - 0) < 0) - return error(_("could not write file: '%s'"), - rebase_path_current_fixups()); + 0) < 0) { + ret = error(_("could not write file: '%s'"), + rebase_path_current_fixups()); + goto out; + } /* * If a fixup/squash in a fixup/squash chain failed, the @@ -5242,8 +5258,10 @@ static int commit_staged_changes(struct repository *r, const char *path = rebase_path_squash_msg(); const char *encoding = get_commit_output_encoding(); - if (parse_head(r, &commit)) - return error(_("could not parse HEAD")); + if (parse_head(r, &commit)) { + ret = error(_("could not parse HEAD")); + goto out; + } p = repo_logmsg_reencode(r, commit, NULL, encoding); if (!p) { @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r, } unuse_commit_buffer: repo_unuse_commit_buffer(r, commit, p); - if (res) - return res; + if (res) { + ret = res; + goto out; + } } } - strbuf_release(&rev); flags |= AMEND_MSG; } @@ -5272,18 +5291,29 @@ static int commit_staged_changes(struct repository *r, if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") && refs_delete_ref(get_main_ref_store(r), "", - "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF)) - return error(_("could not remove CHERRY_PICK_HEAD")); - if (unlink(git_path_merge_msg(r)) && errno != ENOENT) - return error_errno(_("could not remove '%s'"), - git_path_merge_msg(r)); - if (!final_fixup) - return 0; + "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF)) { + ret = error(_("could not remove CHERRY_PICK_HEAD")); + goto out; + } + + if (unlink(git_path_merge_msg(r)) && errno != ENOENT) { + ret = error_errno(_("could not remove '%s'"), + git_path_merge_msg(r)); + goto out; + } + + if (!final_fixup) { + ret = 0; + goto out; + } } if (run_git_commit(final_fixup ? NULL : rebase_path_message(), - opts, flags)) - return error(_("could not commit staged changes.")); + opts, flags)) { + ret = error(_("could not commit staged changes.")); + goto out; + } + unlink(rebase_path_amend()); unlink(git_path_merge_head(r)); refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE", @@ -5301,7 +5331,12 @@ static int commit_staged_changes(struct repository *r, strbuf_reset(&ctx->current_fixups); ctx->current_fixup_count = 0; } - return 0; + + ret = 0; + +out: + strbuf_release(&rev); + return ret; } int sequencer_continue(struct repository *r, struct replay_opts *opts)
We're leaking the `rev` string buffer in various call paths. Refactor the function to have a common exit path so that we can release its memory reliably. This fixes a subset of tests failing with the memory sanitizer in t3404. But as there are more failures, we cannot yet mark the whole test suite as passing. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- sequencer.c | 105 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 35 deletions(-)