[PATCH 0/7] Fix some sequencer leaks

[PATCH 0/7] Fix some sequencer leaks

[Christian Couder]

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

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

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sequencer.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

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

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

[Christian Couder]

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);
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
-- 
1.7.10.2.555.g6528037

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

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

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

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c | 2 ++
 1 file changed, 2 insertions(+)

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

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

[Christian Couder]

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(+)

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

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

[Christian Couder]

"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(-)

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

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

[Christian Couder]

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

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

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

[Jonathan Nieder]

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.

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

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

[Jonathan Nieder]

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?

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

[Jonathan Nieder]

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

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

[Ramkumar Ramachandra]

Hi Christian,

Christian Couder wrote:
> 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

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Christian Couder wrote:

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

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> 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

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

[Ramkumar Ramachandra]

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?

Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

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

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

[Jonathan Nieder]

Hi,

Christian Couder wrote:
 
> 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(-)

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

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

[Christian Couder]

Hi,

From: Jonathan Nieder <jrnieder@gmail.com>
> 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.

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

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

[Christian Couder]

Hi Ram,

From: Ramkumar Ramachandra <artagnon@gmail.com>
> 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.

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

[Christian Couder]

From: Jonathan Nieder <jrnieder@gmail.com>
> 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.

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

[Christian Couder]

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.

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

[Christian Couder]

From: Jonathan Nieder <jrnieder@gmail.com>
>
>> +	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.

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

[Jonathan Nieder]

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