Re: [PATCH 1/7] sequencer: fix leaked todo_list memory

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 21, 2012, 20:57 UTC
Message-ID: <20120521205701.GD28331@burratino>
In-Reply-To: <20120521145610.1911.87598.chriscool@tuxfamily.org>

Christian Couder wrote:

Show 20 quoted lines

> --- 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