From 62f5be606b11cca87a856a3a6164990be5aa6ffd Mon Sep 17 00:00:00 2001 From: Kurt Berglund Date: Wed, 26 Jul 2017 12:28:37 -0700 Subject: [PATCH 1/4] Change tree_entry to not be selfFreeing The libgit2 use cases all are instances where the git_tree_entry is owned by the object that returned it. As such this marks the tree_entry as not self freeing. And also updates functions returning one of these to set ownedByThis. This will keep the owning object alive should someone have a reference to a tree_entry. The git_treebuilder_insert method needed to be marked as not async in order to have access to the owning object. This seems like a case where the generator could be improved should this method need to be async. --- generate/input/descriptor.json | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index d33298c30..256987e8c 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -2562,6 +2562,19 @@ "git_treebuilder_filter": { "ignore": true }, + "git_treebuilder_get": { + "return": { + "ownedByThis": true + } + }, + "git_treebuilder_insert": { + "isAsync": false, + "args": { + "out": { + "ownedByThis": true + } + } + }, "git_treebuilder_write": { "args": { "id": { @@ -2582,7 +2595,7 @@ } }, "tree_entry": { - "selfFreeing": true, + "selfFreeing": false, "dupFunction": "git_tree_entry_dup", "freeFunctionName": "git_tree_entry_free", "functions": { From fb26b08a7ee30f9347c5b6de767c128e8ffe4933 Mon Sep 17 00:00:00 2001 From: Kurt Berglund Date: Wed, 26 Jul 2017 14:04:05 -0700 Subject: [PATCH 2/4] Remove leak test from tree_entry since it's no longer selfFreeing --- test/tests/tree_entry.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/tests/tree_entry.js b/test/tests/tree_entry.js index 13093fdaa..16bb6b3fd 100644 --- a/test/tests/tree_entry.js +++ b/test/tests/tree_entry.js @@ -2,8 +2,6 @@ var assert = require("assert"); var path = require("path"); var local = path.join.bind(path, __dirname); -var leakTest = require("../utils/leak_test"); - describe("TreeEntry", function() { var NodeGit = require("../../"); var Repository = NodeGit.Repository; @@ -177,15 +175,4 @@ describe("TreeEntry", function() { assert.equal(object.isTree(), true); }); }); - - it("does not leak", function() { - var test = this; - - return leakTest(NodeGit.TreeEntry, function() { - return test.commit.getTree() - .then(function(tree) { - return tree.entryByPath("example"); - }); - }); - }); }); From f1408478c89cf5986947d08ce1575c40267bbf59 Mon Sep 17 00:00:00 2001 From: kurtb Date: Tue, 15 Aug 2017 18:03:10 -0700 Subject: [PATCH 3/4] Pull request feedback Fix up ownership on all tree_entry related calls --- generate/input/descriptor.json | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 256987e8c..bccbbb841 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -2529,19 +2529,22 @@ "functions": { "git_tree_entry_byid": { "return": { - "ownedByThis": true + "ownedByThis": true, + "selfFreeing": false } }, "git_tree_entry_byindex": { "jsFunctionName": "_entryByIndex", "return": { - "ownedByThis": true + "ownedByThis": true, + "selfFreeing": false } }, "git_tree_entry_byname": { "jsFunctionName": "_entryByName", "return": { - "ownedByThis": true + "ownedByThis": true, + "selfFreeing": false } }, "git_tree_entrycount": { @@ -2564,6 +2567,7 @@ }, "git_treebuilder_get": { "return": { + "selfFreeing": false, "ownedByThis": true } }, @@ -2571,6 +2575,7 @@ "isAsync": false, "args": { "out": { + "selfFreeing": false, "ownedByThis": true } } @@ -2595,7 +2600,7 @@ } }, "tree_entry": { - "selfFreeing": false, + "selfFreeing": true, "dupFunction": "git_tree_entry_dup", "freeFunctionName": "git_tree_entry_free", "functions": { From 67fc71da5ec9c5c2689f8825a707dbacab5e9cff Mon Sep 17 00:00:00 2001 From: Kurt Berglund Date: Wed, 16 Aug 2017 15:52:39 -0700 Subject: [PATCH 4/4] Test for treebuilder insert memory management --- test/tests/tree_entry.js | 13 +++++++++++++ test/tests/treebuilder.js | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/test/tests/tree_entry.js b/test/tests/tree_entry.js index 16bb6b3fd..13093fdaa 100644 --- a/test/tests/tree_entry.js +++ b/test/tests/tree_entry.js @@ -2,6 +2,8 @@ var assert = require("assert"); var path = require("path"); var local = path.join.bind(path, __dirname); +var leakTest = require("../utils/leak_test"); + describe("TreeEntry", function() { var NodeGit = require("../../"); var Repository = NodeGit.Repository; @@ -175,4 +177,15 @@ describe("TreeEntry", function() { assert.equal(object.isTree(), true); }); }); + + it("does not leak", function() { + var test = this; + + return leakTest(NodeGit.TreeEntry, function() { + return test.commit.getTree() + .then(function(tree) { + return tree.entryByPath("example"); + }); + }); + }); }); diff --git a/test/tests/treebuilder.js b/test/tests/treebuilder.js index 5e1ce6263..c7ef01f43 100644 --- a/test/tests/treebuilder.js +++ b/test/tests/treebuilder.js @@ -5,10 +5,14 @@ var promisify = require("promisify-node"); var readDir = promisify(fs.readdir); var local = path.join.bind(path, __dirname); +var leakTest = require("../utils/leak_test"); + describe("TreeBuilder", function(){ var Git = require("../../"); var reposPath = local("../repos/workdir"); + var oid = "111dd657329797f6165f52f5085f61ac976dcf04"; + //setup test repo each test beforeEach(function() { var test = this; @@ -76,4 +80,19 @@ describe("TreeBuilder", function(){ return Git.Tree.lookup(test.repo, newTreeEntry.oid()); }); }); + + it("does not leak inserts", function() { + var test = this; + + // The underlying C++ git_tree_entry is owned by the treebuilder that + // creates it. But since git_tree_entry is duplicable the generator will + // duplicate it and mark it as self freeing. + // Validate this with the leakTest. + return leakTest(Git.TreeEntry, function() { + return Git.Treebuilder.create(test.repo, null) + .then(function(builder) { + return builder.insert("test", oid, Git.TreeEntry.FILEMODE.BLOB); + }); + }); + }); });