From: Jonathan Nieder <jrnieder@gmail.com>
To: Christian Couder <chriscool@tuxfamily.org>
Cc: Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, Ramkumar Ramachandra <artagnon@gmail.com>,
Nick Bowler <nbowler@elliptictech.com>
Subject: Re: [PATCH 1/7] sequencer: fix leaked todo_list memory
Date: Mon, 21 May 2012 15:57:01 -0500 [thread overview]
Message-ID: <20120521205701.GD28331@burratino> (raw)
In-Reply-To: <20120521145610.1911.87598.chriscool@tuxfamily.org>
Christian Couder wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -900,6 +900,7 @@ static int continue_single_pick(void)
> static int sequencer_continue(struct replay_opts *opts)
> {
> struct commit_list *todo_list = NULL;
> + int res;
>
> if (!file_exists(git_path(SEQ_TODO_FILE)))
> return continue_single_pick();
> @@ -915,8 +916,11 @@ static int sequencer_continue(struct replay_opts *opts)
> }
> if (index_differs_from("HEAD", 0))
> return error_dirty_index(opts);
> - todo_list = todo_list->next;
> - return pick_commits(todo_list, opts);
> + res = pick_commits(todo_list->next, opts);
> +
> + free_commit_list(todo_list);
> + return res;
[...]
> @@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
[same change there]
Makes sense. This is a one-time allocation, but it is in a somewhat
public function and freeing saves human readers and automatic
debugging tools like valgrind some distraction.
Shouldn't the error paths do this too?
walk_revs_populate_todo(&todo_list, opts);
if (create_seq_dir())
goto fail;
if (get_sha1("HEAD", sha1)) {
error(opts->action == REPLAY_REVERT ?
_("Can't revert as initial commit") :
_("Can't cherry-pick into empty head"));
goto fail;
}
save_head(sha1_to_hex(sha1));
save_opts(opts);
result = pick_commits(todo_list, opts);
free_commit_list(todo_list);
return result;
fail:
free_commit_list(todo_list);
return -1;
It does add another O(n) to the running time. Some impatient person
watching a long post-multipick pause might want to add an optional
SEQUENCER_FORGET_THE_MEMORY_WE_WILL_BE_DYING_SOON_ANYWAY flag
argument, but probably that should happen as a separate change.
So for what it's worth, I like this change as long as it is done
consistently (meaning "in all code paths that return from the
function, not just the success case").
Thanks.
Jonathan
next prev parent reply other threads:[~2012-05-21 20:57 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-21 14:56 [PATCH 0/7] Fix some sequencer leaks Christian Couder
2012-05-21 14:56 ` [PATCH 1/7] sequencer: fix leaked todo_list memory Christian Couder
2012-05-21 20:57 ` Jonathan Nieder [this message]
2012-05-22 20:24 ` Christian Couder
2012-05-21 14:56 ` [PATCH 2/7] sequencer: release a strbuf used in save_head() Christian Couder
2012-05-22 4:12 ` Ramkumar Ramachandra
2012-05-22 4:23 ` Jonathan Nieder
2012-05-22 5:07 ` Ramkumar Ramachandra
2012-05-22 5:18 ` Jonathan Nieder
2012-05-22 14:17 ` Jonathan Nieder
2012-05-22 20:30 ` Christian Couder
2012-05-21 14:56 ` [PATCH 3/7] merge-recursive: free some string lists Christian Couder
2012-05-21 14:56 ` [PATCH 4/7] revert: free opts.revs to do a bit of cleanup Christian Couder
2012-05-22 5:14 ` Ramkumar Ramachandra
2012-05-22 20:03 ` Christian Couder
2012-05-21 14:56 ` [PATCH 5/7] revert: free revs->cmdline.rev Christian Couder
2012-05-21 20:39 ` Jonathan Nieder
2012-05-22 20:01 ` Christian Couder
2012-05-22 20:40 ` Jonathan Nieder
2012-05-21 14:56 ` [PATCH 6/7] unpack-trees: record which unpack error messages should be freed Christian Couder
2012-05-21 20:43 ` Jonathan Nieder
2012-05-22 20:22 ` Christian Couder
2012-05-21 14:56 ` [PATCH 7/7] Properly free unpack trees error messages Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120521205701.GD28331@burratino \
--to=jrnieder@gmail.com \
--cc=artagnon@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=nbowler@elliptictech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).