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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html