threads / patch / 30579

patch, 7 partsFix some sequencer leaks

Subject: [PATCH 0/7] Fix some sequencer leaks

## tl;dr

23 messages between May 21, 2012 and May 22, 2012. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Christian Couder· May 21, 2012, 14:56 UTC · lore

Here is another iteration of a series to fix some leaks in the cherry-pick, revert and sequencer code.

The previous iteration only had patch 1 and 2. They haven't changed. I just added the last 5 patches.

Maybe the last 2 patches are a bit overkill. But with this series, the valgrind output with --leak-check=full is much cleaner.

There most important leaks are still there though. One of them is related to ce_entries, like this:

==2015== 522 bytes in 6 blocks are definitely lost in loss record 51 of 54 ==2015== at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2015== by 0x53113B: xcalloc (wrapper.c:119) ==2015== by 0x52BBE9: create_ce_entry (unpack-trees.c:559) ==2015== by 0x52BDC6: unpack_nondirectories (unpack-trees.c:609) ==2015== by 0x52C45B: unpack_callback (unpack-trees.c:799) ==2015== by 0x529B9B: traverse_trees (tree-walk.c:407) ==2015== by 0x52CE53: unpack_trees (unpack-trees.c:1091) ==2015== by 0x4D1B89: git_merge_trees (merge-recursive.c:241) ==2015== by 0x4D673A: merge_trees (merge-recursive.c:1819) ==2015== by 0x509521: do_recursive_merge (sequencer.c:225) ==2015== by 0x509F5B: do_pick_commit (sequencer.c:457) ==2015== by 0x50B3B4: pick_commits (sequencer.c:878)

The other one is related to the index:

==2015== 574 (400 direct, 174 indirect) bytes in 2 blocks are definitely lost in loss record 52 of 54 ==2015== at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2015== by 0x53113B: xcalloc (wrapper.c:119) ==2015== by 0x4EF8CF: read_index_from (read-cache.c:1441) ==2015== by 0x4EF316: read_index (read-cache.c:1282) ==2015== by 0x50940B: do_recursive_merge (sequencer.c:211) ==2015== by 0x509F5B: do_pick_commit (sequencer.c:457) ==2015== by 0x50B3B4: pick_commits (sequencer.c:878) ==2015== by 0x50B7C6: sequencer_pick_revisions (sequencer.c:994) ==2015== by 0x47A6D5: cmd_cherry_pick (revert.c:237) ==2015== by 0x404FEE: run_builtin (git.c:308) ==2015== by 0x405181: handle_internal_command (git.c:468) ==2015== by 0x40529B: run_argv (git.c:514)

Christian Couder (7):
  sequencer: fix leaked todo_list memory
  sequencer: release a strbuf used in save_head()
  merge-recursive: free some string lists
  revert: free opts.revs to do a bit of cleanup
  revert: free revs->cmdline.rev
  unpack-trees: record which unpack error messages should be freed
  Properly free unpack trees error messages
 builtin/checkout.c |  1 +
 builtin/merge.c    |  6 ++++--
 builtin/revert.c   |  6 ++++++
 merge-recursive.c  |  5 ++++-
 sequencer.c        | 16 +++++++++++++---
 unpack-trees.c     | 50 +++++++++++++++++++++++++++++++++++---------------
 unpack-trees.h     |  3 +++
 7 files changed, 66 insertions(+), 21 deletions(-)
-- 
1.7.10.2.555.g6528037
Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sequencer.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
Show changes to sequencer.c +12 −3
diff --git a/sequencer.c b/sequencer.c
index 3c384b9..762c527 100644
--- 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;
 }
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
@@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
+	int res;
 
 	if (opts->subcommand == REPLAY_NONE)
 		assert(opts->revs);
@@ -985,5 +990,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	}
 	save_head(sha1_to_hex(sha1));
 	save_opts(opts);
-	return pick_commits(todo_list, opts);
+	res = pick_commits(todo_list, opts);
+
+	free_commit_list(todo_list);
+
+	return res;
 }
-- 
1.7.10.2.555.g6528037
Jonathan Nieder· May 21, 2012, 20:57 UTC · re: Christian Couder · lore

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

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

Christian Couder· May 22, 2012, 20:24 UTC · re: Jonathan Nieder · lore

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

From: Jonathan Nieder <jrnieder@gmail.com>
> 
> 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").
Ok, I will do that.

Thanks, Christian.

Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 2/7] sequencer: release a strbuf used in save_head()

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to sequencer.c +1 −0
diff --git a/sequencer.c b/sequencer.c
index 762c527..ad1bbea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -741,6 +741,7 @@ static void save_head(const char *head)
 		die_errno(_("Could not write to %s"), head_file);
 	if (commit_lock_file(&head_lock) < 0)
 		die(_("Error wrapping up %s."), head_file);
+	strbuf_release(&buf);
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
-- 
1.7.10.2.555.g6528037
Ramkumar Ramachandra· May 22, 2012, 04:12 UTC · re: Christian Couder · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

Hi Christian,
Christian Couder wrote:
Show 12 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 762c527..ad1bbea 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -741,6 +741,7 @@ static void save_head(const char *head)
>                die_errno(_("Could not write to %s"), head_file);
>        if (commit_lock_file(&head_lock) < 0)
>                die(_("Error wrapping up %s."), head_file);
> +       strbuf_release(&buf);
>  }
>
>  static int reset_for_rollback(const unsigned char *sha1)

Thanks. Shouldn't we be polluting the die() pathways with this strbuf_release(&buf) the way save_todo() does it consistently?

	if (write_in_full(fd, buf.buf, buf.len) < 0) {
		strbuf_release(&buf);
		die_errno(_("Could not write to %s"), todo_file);
	}
	if (commit_lock_file(&todo_lock) < 0) {
		strbuf_release(&buf);
		die(_("Error wrapping up %s."), todo_file);
	}
	strbuf_release(&buf);
Ram
Jonathan Nieder· May 22, 2012, 04:23 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

Ramkumar Ramachandra wrote:
> Christian Couder wrote:
Show 19 quoted lines
>> diff --git a/sequencer.c b/sequencer.c
>> index 762c527..ad1bbea 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -741,6 +741,7 @@ static void save_head(const char *head)
>>                die_errno(_("Could not write to %s"), head_file);
>>        if (commit_lock_file(&head_lock) < 0)
>>                die(_("Error wrapping up %s."), head_file);
>> +       strbuf_release(&buf);
>>  }
>>
>>  static int reset_for_rollback(const unsigned char *sha1)
>
> Thanks.  Shouldn't we be polluting the die() pathways with this
> strbuf_release(&buf) the way save_todo() does it consistently?
> 
> 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> 		strbuf_release(&buf);
> 		die_errno(_("Could not write to %s"), todo_file);

I don't see why. Doesn't exit take care of freeing everything? And looking at it from the other side, doesn't using exit mean that you cannot be valgrind-clean anyway, since allocations by functions higher in the call chain do not get a chance to be freed?

Jonathan
Ramkumar Ramachandra· May 22, 2012, 05:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

Hi Jonathan,
Jonathan Nieder wrote:
Show 12 quoted lines
> Ramkumar Ramachandra wrote:
>> Christian Couder wrote:
>>> [...]
>>       if (write_in_full(fd, buf.buf, buf.len) < 0) {
>>               strbuf_release(&buf);
>>               die_errno(_("Could not write to %s"), todo_file);
>
> [...]
> And
> looking at it from the other side, doesn't using exit mean that you
> cannot be valgrind-clean anyway, since allocations by functions higher
> in the call chain do not get a chance to be freed?

Good point; save_todo() sets a bad example. For symmetry, should these two instances of strbuf_release() before die() be removed in a separate patch?

Ram
Jonathan Nieder· May 22, 2012, 05:18 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
Show 8 quoted lines
>> And
>> looking at it from the other side, doesn't using exit mean that you
>> cannot be valgrind-clean anyway, since allocations by functions higher
>> in the call chain do not get a chance to be freed?
>
> Good point; save_todo() sets a bad example.  For symmetry, should
> these two instances of strbuf_release() before die() be removed in a
> separate patch?
I can't find myself caring much either way. :)

A single free() doesn't hurt performance much, so my hunch would be to leave it alone unless some other practical reason to keep or remove the free()s comes up.

Jonathan Nieder· May 22, 2012, 14:17 UTC · re: Christian Couder · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

Hi,
Christian Couder wrote:
 
Show 14 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 762c527..ad1bbea 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -741,6 +741,7 @@ static void save_head(const char *head)
>  		die_errno(_("Could not write to %s"), head_file);
>  	if (commit_lock_file(&head_lock) < 0)
>  		die(_("Error wrapping up %s."), head_file);
> +	strbuf_release(&buf);

Makes good sense. Actually, I'm not sure why this allocation is needed in the first place. Would something like the following work?

-- >8 --
Subject: sequencer: avoid a one-time leak in save_head()

This function uses the lockfile API to make its change to the .git/sequencer/head file effectively atomic, so there is no need to gather output intended for that file in a new buffer to write it in one go.

Noticed with valgrind.
Reported-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
By the way, the lockfile API usage looks fishy.  What kind of races is
the locking meant to prevent?  What happens if someone tries "git
cherry-pick --abort" in another window after my multipick has started
and before it finishes?
 sequencer.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to diff +2 −3
diff --git i/sequencer.c w/sequencer.c
index 3c384b94..75b6a995 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -732,12 +732,11 @@ static void save_head(const char *head)
 {
 	const char *head_file = git_path(SEQ_HEAD_FILE);
 	static struct lock_file head_lock;
-	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
 	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
-	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
+	if (write_in_full(fd, head, strlen(head)) < 0 ||
+	    write_in_full(fd, "\n", 1) < 0)
 		die_errno(_("Could not write to %s"), head_file);
 	if (commit_lock_file(&head_lock) < 0)
 		die(_("Error wrapping up %s."), head_file);
Christian Couder· May 22, 2012, 20:30 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

From: Jonathan Nieder <jrnieder@gmail.com>
Show 5 quoted lines
>
>> +	strbuf_release(&buf);
> 
> Makes good sense.  Actually, I'm not sure why this allocation is
> needed in the first place.  Would something like the following work?
Ok, I will use your patch in the next iteration.

Thanks, Christian.

Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 3/7] merge-recursive: free some string lists

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to merge-recursive.c +3 −1
diff --git a/merge-recursive.c b/merge-recursive.c
index 680937c..9c3570a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1856,7 +1856,9 @@ int merge_trees(struct merge_options *o,
 		string_list_clear(re_merge, 0);
 		string_list_clear(re_head, 0);
 		string_list_clear(entries, 1);
-
+		free(re_merge);
+		free(re_head);
+		free(entries);
 	}
 	else
 		clean = 1;
-- 
1.7.10.2.555.g6528037
Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 4/7] revert: free opts.revs to do a bit of cleanup

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/revert.c +2 −0
diff --git a/builtin/revert.c b/builtin/revert.c
index 82d1bf8..5f82a84 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
+	free(opts.revs);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
+	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
1.7.10.2.555.g6528037
Ramkumar Ramachandra· May 22, 2012, 05:14 UTC · re: Christian Couder · lore

Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup

Hi Christian,
Christian Couder wrote:
Show 20 quoted lines
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 82d1bf8..5f82a84 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>        git_config(git_default_config, NULL);
>        parse_args(argc, argv, &opts);
>        res = sequencer_pick_revisions(&opts);
> +       free(opts.revs);
>        if (res < 0)
>                die(_("revert failed"));
>        return res;
> @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>        git_config(git_default_config, NULL);
>        parse_args(argc, argv, &opts);
>        res = sequencer_pick_revisions(&opts);
> +       free(opts.revs);
>        if (res < 0)
>                die(_("cherry-pick failed"));
>        return res;

Technically, memory is allocated to both opts->revs and opts->xopts in parse_args(). Why not free both?

Ram
Christian Couder· May 22, 2012, 20:03 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup

Hi Ram,
From: Ramkumar Ramachandra <artagnon@gmail.com>
Show 26 quoted lines
> Hi Christian,
> 
> Christian Couder wrote:
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 82d1bf8..5f82a84 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>>        git_config(git_default_config, NULL);
>>        parse_args(argc, argv, &opts);
>>        res = sequencer_pick_revisions(&opts);
>> +       free(opts.revs);
>>        if (res < 0)
>>                die(_("revert failed"));
>>        return res;
>> @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>>        git_config(git_default_config, NULL);
>>        parse_args(argc, argv, &opts);
>>        res = sequencer_pick_revisions(&opts);
>> +       free(opts.revs);
>>        if (res < 0)
>>                die(_("cherry-pick failed"));
>>        return res;
> 
> Technically, memory is allocated to both opts->revs and opts->xopts in
> parse_args().  Why not free both?
I didn't see any leak with opts->xopts but I will have a look.

Thanks, Christian.

Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 5/7] revert: free revs->cmdline.rev

add_rev_cmdline() in revision.c is (re)allocating an array of struct rev_cmdline_entry. This patch releases it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to builtin/revert.c +4 −0
diff --git a/builtin/revert.c b/builtin/revert.c
index 5f82a84..1d32ed5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
+	if (opts.revs)
+		free(opts.revs->cmdline.rev);
 	free(opts.revs);
 	if (res < 0)
 		die(_("revert failed"));
@@ -233,6 +235,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
+	if (opts.revs)
+		free(opts.revs->cmdline.rev);
 	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
-- 
1.7.10.2.555.g6528037
Jonathan Nieder· May 21, 2012, 20:39 UTC · re: Christian Couder · lore

Re: [PATCH 5/7] revert: free revs->cmdline.rev

Hi,
Christian Couder wrote:
> add_rev_cmdline() in revision.c is (re)allocating an array of
> struct rev_cmdline_entry. This patch releases it.
[...]
Show 11 quoted lines
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>  	git_config(git_default_config, NULL);
>  	parse_args(argc, argv, &opts);
>  	res = sequencer_pick_revisions(&opts);
> +	if (opts.revs)
> +		free(opts.revs->cmdline.rev);
>  	free(opts.revs);
>  	if (res < 0)
>  		die(_("revert failed"));
Quick thoughts:

This feels like a layering violation. Avoidable? Maybe revision.c could gain a helper to allow this to be written like so:

	free_rev_info(opts.revs);
	free(opts.revs);

Since this is a one-time allocation it is probably worth mentioning in the log message that this is a futureproofing/valgrind-cleanliness measure and is not actually fixing a leak.

Micronit: it would feel slightly more comfortable if the free() were
after the die(), even though the die() should probably be changed to
exit().  That way someone wanting to add code after the die() that
continues to assume opts.revs is valid would be able to.

Of course I'd imagine the largest leak in cherry-pick is the deliberately constantly growing object hash table. It would be very interesting to fix that --- do you know how libgit2 handles it?

Thanks much and hope that helps, Jonathan

Christian Couder· May 22, 2012, 20:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH 5/7] revert: free revs->cmdline.rev

Hi,
From: Jonathan Nieder <jrnieder@gmail.com>
Show 30 quoted lines
> Hi,
> 
> Christian Couder wrote:
> 
>> add_rev_cmdline() in revision.c is (re)allocating an array of
>> struct rev_cmdline_entry. This patch releases it.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>>  	git_config(git_default_config, NULL);
>>  	parse_args(argc, argv, &opts);
>>  	res = sequencer_pick_revisions(&opts);
>> +	if (opts.revs)
>> +		free(opts.revs->cmdline.rev);
>>  	free(opts.revs);
>>  	if (res < 0)
>>  		die(_("revert failed"));
> 
> Quick thoughts:
> 
> This feels like a layering violation.  Avoidable?  Maybe revision.c
> could gain a helper to allow this to be written like so:
> 
> 	free_rev_info(opts.revs);
> 	free(opts.revs);
> 
> Since this is a one-time allocation it is probably worth mentioning in
> the log message that this is a futureproofing/valgrind-cleanliness
> measure and is not actually fixing a leak.

Ok, I will create the free_rev_info() helper and improve the commit message.

Show 8 quoted lines
> Micronit: it would feel slightly more comfortable if the free() were
> after the die(), even though the die() should probably be changed to
> exit().  That way someone wanting to add code after the die() that
> continues to assume opts.revs is valid would be able to.
> 
> Of course I'd imagine the largest leak in cherry-pick is the
> deliberately constantly growing object hash table.  It would be very
> interesting to fix that --- do you know how libgit2 handles it?

About the constantly growing hash table, perhaps it should be taken care of in a try_to_free_routine() used by xmalloc and other such functions. And no I don't know much about libgit2.

> Thanks much and hope that helps,
> Jonathan

Thanks for your kind review, Christian.

Jonathan Nieder· May 22, 2012, 20:40 UTC · re: Christian Couder · lore

Re: [PATCH 5/7] revert: free revs->cmdline.rev

Christian Couder wrote:
> About the constantly growing hash table, perhaps it should be taken
> care of in a try_to_free_routine() used by xmalloc and other such
> functions. And no I don't know much about libgit2.

Hm. At first glance that sounds interesting (some kind of mark-and-sweep garbage collection, so at least allocation would never _fail_ due to too large a multipick). At second glance it is less exciting, since the logic would only kick in when malloc() fails, and if I have a lot of swap then my system will have slowed to a crawl long before then.

Here are a couple of alternative ideas, with no code to back them up.

A. Save a copy of obj_hash after revision traversal and before cherry-picking anything, and reset the table to that state after every (let's say) 5 commits cherry-picked. Or perhaps empty the table completely after every 5 commits or so cherry-picked.

B. Re-exec git using "git sequence --continue" so everything gets allocated anew after every 5 commits or so.

That second idea sounds pretty ugly, but it would probably be effective. On Windows it's fork(), not execve(), that is expensive, right? :)

Just musing, Jonathan

Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 6/7] unpack-trees: record which unpack error messages should be freed

"struct unpack_trees_options" has a "const char *msgs[]" field that is setup with string values in setup_unpack_trees_porcelain(). Unfortunately this field is setup with strings that are sometimes allocated on the stack, sometimes not.

The goal of this patch is to add some infrastructure to record which of the string have been allocated on the stack so they can be properly freed when not needed anymore.

We use a bitfield in struct unpack_trees_options to record that. We add a function to both setup the error messages and record wether they should be freed. And we add another function to properly free all the messages that should be freed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 unpack-trees.c | 50 +++++++++++++++++++++++++++++++++++---------------
 unpack-trees.h |  3 +++
 2 files changed, 38 insertions(+), 15 deletions(-)
Show changes to 2 files +38 −15

unpack-trees.c, unpack-trees.h

diff --git a/unpack-trees.c b/unpack-trees.c
index bcee99c..3537fea 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -50,11 +50,29 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+static void setup_unpack_error_msg(struct unpack_trees_options *opts,
+				   enum unpack_trees_error_types err_type,
+				   char *message, int to_free)
+{
+	/* Check that we can record which messages should be freed */
+	assert(NB_UNPACK_TREES_ERROR_TYPES <= 8 * sizeof(opts->msgs_to_be_freed));
+
+	opts->msgs[err_type] = message;
+	opts->msgs_to_be_freed |= (to_free ? 1u : 0) << err_type;
+}
+
+void free_unpack_error_msgs(struct unpack_trees_options *opts)
+{
+	int i;
+	for (i = 0; i < NB_UNPACK_TREES_ERROR_TYPES; i++)
+		if (opts->msgs_to_be_freed & 1u << i)
+			free((char *)opts->msgs[i]);
+}
+
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd)
 {
 	int i;
-	const char **msgs = opts->msgs;
 	const char *msg;
 	char *tmp;
 	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
@@ -65,11 +83,11 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		msg = "Your local changes to the following files would be overwritten by %s:\n%%s";
 	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
 	sprintf(tmp, msg, cmd, cmd2);
-	msgs[ERROR_WOULD_OVERWRITE] = tmp;
-	msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+	setup_unpack_error_msg(opts, ERROR_WOULD_OVERWRITE, tmp, 1);
+	setup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_FILE, tmp, 0);
 
-	msgs[ERROR_NOT_UPTODATE_DIR] =
-		"Updating the following directories would lose untracked files in it:\n%s";
+	tmp = "Updating the following directories would lose untracked files in it:\n%s";
+	setup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_DIR, tmp, 0);
 
 	if (advice_commit_before_merge)
 		msg = "The following untracked working tree files would be %s by %s:\n%%s"
@@ -78,23 +96,25 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		msg = "The following untracked working tree files would be %s by %s:\n%%s";
 	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4);
 	sprintf(tmp, msg, "removed", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
+	setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, tmp, 1);
 	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4);
 	sprintf(tmp, msg, "overwritten", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+	setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, tmp, 1);
 
 	/*
 	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
 	 * cannot easily display it as a list.
 	 */
-	msgs[ERROR_BIND_OVERLAP] = "Entry '%s' overlaps with '%s'.  Cannot bind.";
-
-	msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
-		"Cannot update sparse checkout: the following entries are not up-to-date:\n%s";
-	msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
-		"The following Working tree files would be overwritten by sparse checkout update:\n%s";
-	msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
-		"The following Working tree files would be removed by sparse checkout update:\n%s";
+	tmp = "Entry '%s' overlaps with '%s'.  Cannot bind.";
+	setup_unpack_error_msg(opts, ERROR_BIND_OVERLAP, tmp, 0);
+
+	tmp = "Cannot update sparse checkout: the following entries are not up-to-date:\n%s";
+	setup_unpack_error_msg(opts, ERROR_SPARSE_NOT_UPTODATE_FILE, tmp, 0);
+
+	tmp = "The following Working tree files would be overwritten by sparse checkout update:\n%s";
+	setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, tmp, 0);
+	tmp = "The following Working tree files would be removed by sparse checkout update:\n%s";
+	setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_REMOVED, tmp, 0);
 
 	opts->show_all_errors = 1;
 	/* rejected paths may not have a static buffer */
diff --git a/unpack-trees.h b/unpack-trees.h
index 5e432f5..9b9d6ab 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -30,6 +30,7 @@ enum unpack_trees_error_types {
  */
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd);
+void free_unpack_error_msgs(struct unpack_trees_options *opts);
 
 struct unpack_trees_options {
 	unsigned int reset,
@@ -55,6 +56,8 @@ struct unpack_trees_options {
 	struct pathspec *pathspec;
 	merge_fn_t fn;
 	const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+	/* Bit array to know which msgs must be freed */
+	unsigned int msgs_to_be_freed;
 	/*
 	 * Store error messages in an array, each case
 	 * corresponding to a error message type
-- 
1.7.10.2.555.g6528037
Jonathan Nieder· May 21, 2012, 20:43 UTC · re: Christian Couder · lore

Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed

Christian Couder wrote:
> "struct unpack_trees_options" has a "const char *msgs[]" field
> that is setup with string values in setup_unpack_trees_porcelain().

Hmm. Incidentally, should callers (e.g., the 100 single-picks involved in a large multi-pick) be reusing these strings instead of allocating them again and again?

Christian Couder· May 22, 2012, 20:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed

From: Jonathan Nieder <jrnieder@gmail.com>
Show 8 quoted lines
> Christian Couder wrote:
> 
>> "struct unpack_trees_options" has a "const char *msgs[]" field
>> that is setup with string values in setup_unpack_trees_porcelain().
> 
> Hmm.  Incidentally, should callers (e.g., the 100 single-picks
> involved in a large multi-pick) be reusing these strings instead of
> allocating them again and again?

Some strings filling the msgs field are created with sprintf() using the cmd argument passed to setup_unpack_trees_porcelain(). So they could be different if different cmd arguments are used. In practice now cmd is either "checkout" or "merge", so we could probably reuse the same strings. But if we ever add a checkout command to the sequencer for example, we might want to use different error messages.

Another possibility is to xmalloc all the strings and then free them all.

Thanks, Christian.

Christian Couder· May 21, 2012, 14:56 UTC · re: Christian Couder · lore

[PATCH 7/7] Properly free unpack trees error messages

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/checkout.c | 1 +
 builtin/merge.c    | 6 ++++--
 merge-recursive.c  | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)
Show changes to 3 files +6 −2

builtin/checkout.c, builtin/merge.c, merge-recursive.c

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3ddda34..df8fef0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -435,6 +435,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 
 		ret = unpack_trees(2, trees, &topts);
+		free_unpack_error_msgs(&topts);
 		if (ret == -1) {
 			/*
 			 * Unpack couldn't do a trivial merge; either
diff --git a/builtin/merge.c b/builtin/merge.c
index 470fc57..b96a9ae 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -768,7 +768,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	struct tree *trees[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 	struct tree_desc t[MAX_UNPACK_TREES];
-	int i, fd, nr_trees = 0;
+	int i, fd, res, nr_trees = 0;
 	struct dir_struct dir;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
@@ -805,7 +805,9 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 		parse_tree(trees[i]);
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
+	res = unpack_trees(nr_trees, t, &opts);
+	free_unpack_error_msgs(&opts);
+	if (res)
 		return -1;
 	if (write_cache(fd, active_cache, active_nr) ||
 		commit_locked_index(lock_file))
diff --git a/merge-recursive.c b/merge-recursive.c
index 9c3570a..e196c45 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -239,6 +239,7 @@ static int git_merge_trees(int index_only,
 	init_tree_desc_from_tree(t+2, merge);
 
 	rc = unpack_trees(3, t, &opts);
+	free_unpack_error_msgs(&opts);
 	cache_tree_free(&active_cache_tree);
 	return rc;
 }
-- 
1.7.10.2.555.g6528037

← back to recent threads