From 127f04522292fc62152762405c92d6acca4dbcb5 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sun, 14 Aug 2011 10:22:04 -0500 Subject: [PATCH 1/2] 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 Signed-off-by: Junio C Hamano --- builtin/revert.c | 7 +------ t/t3503-cherry-pick-root.sh | 27 ++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 853e9e406c..a26a7c9315 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -273,12 +273,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename) static struct tree *empty_tree(void) { - struct tree *tree = xcalloc(1, sizeof(struct tree)); - - tree->object.parsed = 1; - tree->object.type = OBJ_TREE; - pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1); - return tree; + return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN); } 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 b0faa29918..472e5b80d7 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 From 03f622c81fed05bb571519beacb0eeaf595c65f4 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 16 Aug 2011 13:27:39 -0500 Subject: [PATCH 2/2] merge-recursive: take advantage of hardcoded empty tree When this code was first written (v1.4.3-rc1~174^2~4, merge-recur: if there is no common ancestor, fake empty one, 2006-08-09), everyone needing a fake empty tree had to make her own, but ever since v1.5.5-rc0~180^2~1 (2008-02-13), the object lookup machinery provides a ready-made one. Use it. This is just a simplification, though it also fixes a small leak (since the tree in the virtual common ancestor commit is never freed). Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- merge-recursive.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 206c103635..7695fe8919 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1309,12 +1309,10 @@ int merge_recursive(struct merge_options *o, merged_common_ancestors = pop_commit(&ca); if (merged_common_ancestors == NULL) { - /* if there is no common ancestor, make an empty tree */ - struct tree *tree = xcalloc(1, sizeof(struct tree)); + /* if there is no common ancestor, use an empty tree */ + struct tree *tree; - tree->object.parsed = 1; - tree->object.type = OBJ_TREE; - pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1); + tree = lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN); merged_common_ancestors = make_virtual_commit(tree, "ancestor"); }