Re: [PATCH 6/7] sequencer: Expose API to cherry-picking machinery

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:

>> git_path() calls vsnprintf which clobbers errno, so depending on the
>> platform this can print messages like
>>
>>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
[...]
> Ugh, yet another "bugfix patch" to queue near the beginning of the
> series.  Thanks for catching this.

It's not a bug you introduced, and ideally it would be a separate
patch on its own, against the js/cherry-pick-usability branch.

>> Ramkumar Ramachandra wrote:

>>> +static struct tree *empty_tree(void)
>>> [...]
>>
>> This tree is leaked (for example if you cherry-pick a sequence of
>> root commits).
>
> This is not something I introduced -- it can wait until later, no?

Yep, it's Christian's fault (for introducing multiple-cherry-pick).

Patch below.  The things I do... :)

[...]
> It is a simple code movement.  Is there something I can do to help?

Part of the review was about examples where it was not simple code
movement.  Maybe if there is another round the thing to do would be to
send a patch generated with -B -M to allow others to more easily check
it.

Thanks.

-- >8 --
Subject: revert: plug memory leak in "cherry-pick root commit" codepath

The empty tree passed as common ancestor to merge_trees() when
cherry-picking a parentless commit is allocated on the heap and never
freed.  Leaking such a small one-time allocation is not a very big
problem, but now that "git cherry-pick" can cherry-pick multiple
commits it can start to add up.

Avoid the leak by storing the fake tree exactly once in the BSS
section (i.e., use a static).  While at it, let's add a test to make
sure cherry-picking multiple parentless commits continues to work.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Patch is against cc/cherry-pick-series (86c7bb47).

 builtin/revert.c            |   10 +++++-----
 t/t3503-cherry-pick-root.sh |   27 ++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 853e9e40..c1b0fb3d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -273,12 +273,12 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
 
 static struct tree *empty_tree(void)
 {
-	struct tree *tree = xcalloc(1, sizeof(struct tree));
+	static struct tree tree;
 
-	tree->object.parsed = 1;
-	tree->object.type = OBJ_TREE;
-	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
-	return tree;
+	tree.object.parsed = 1;
+	tree.object.type = OBJ_TREE;
+	pretend_sha1_file(NULL, 0, OBJ_TREE, tree.object.sha1);
+	return &tree;
 }
 
 static NORETURN void die_dirty_index(const char *me)
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index b0faa299..472e5b80 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -16,15 +16,40 @@ test_expect_success setup '
 	echo second > file2 &&
 	git add file2 &&
 	test_tick &&
-	git commit -m "second"
+	git commit -m "second" &&
+
+	git symbolic-ref HEAD refs/heads/third &&
+	rm .git/index file2 &&
+	echo third > file3 &&
+	git add file3 &&
+	test_tick &&
+	git commit -m "third"
 
 '
 
 test_expect_success 'cherry-pick a root commit' '
 
+	git checkout second^0 &&
 	git cherry-pick master &&
 	test first = $(cat file1)
 
 '
 
+test_expect_success 'cherry-pick two root commits' '
+
+	echo first >expect.file1 &&
+	echo second >expect.file2 &&
+	echo third >expect.file3 &&
+
+	git checkout second^0 &&
+	git cherry-pick master third &&
+
+	test_cmp expect.file1 file1 &&
+	test_cmp expect.file2 file2 &&
+	test_cmp expect.file3 file3 &&
+	git rev-parse --verify HEAD^^ &&
+	test_must_fail git rev-parse --verify HEAD^^^
+
+'
+
 test_done
-- 
1.7.6