Merge pull request nodegit#1006 from nodegit/oid-leak-fix

Oid leak fix

Author: srajko

generate/input/descriptor.json

@@ -73,6 +73,11 @@
   {
     "annotated_commit": {
       "functions": {
+        "git_annotated_commit_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        }
       }
     },
     "attr": {
@@ -1002,6 +1007,11 @@
         "git_index_add_frombuffer": {
           "ignore": true
         },
+        "git_index_checksum": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_index_clear": {
           "isAsync": true,
           "return": {
@@ -1198,6 +1208,11 @@
         "git_indexer_append": {
           "ignore": true
         },
+        "git_indexer_hash": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_indexer_new": {
           "ignore": true
         }
@@ -1277,6 +1292,11 @@
             }
           }
         },
+        "git_note_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_note_remove": {
           "isAsync": true,
           "return": {
@@ -1293,6 +1313,11 @@
     },
     "object": {
       "functions": {
+        "git_object_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_object_short_id": {
           "args": {
             "out": {
@@ -1404,6 +1429,11 @@
             "cppClassName": "Wrapper",
             "jsClassName": "Buffer"
           }
+        },
+        "git_odb_object_id": {
+          "return": {
+            "ownedByThis": true
+          }
         }
       },
       "dependencies": [
@@ -1418,6 +1448,7 @@
       "ignore": true
     },
     "oid": {
+      "selfFreeing": "true",
       "cpyFunction": "git_oid_cpy",
       "freeFunctionName": "free",
       "shouldAlloc": true,
@@ -1479,6 +1510,11 @@
         "git_packbuilder_foreach": {
           "ignore": true
         },
+        "git_packbuilder_hash": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_packbuilder_new": {
           "isAsync": false
         },
@@ -1722,6 +1758,20 @@
       "needsForwardDeclaration": false,
       "ignore": true
     },
+    "reflog_entry": {
+      "functions": {
+        "git_reflog_entry_id_new": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
+        "git_reflog_entry_id_old": {
+          "return": {
+            "ownedByThis": true
+          }
+        }
+      }
+    },
     "refspec": {
       "cType": "git_refspec",
       "functions": {
@@ -2191,6 +2241,11 @@
             "isErrorCode": true
           }
         },
+        "git_submodule_head_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_submodule_init": {
           "isAsync": true,
           "return": {
@@ -2211,6 +2266,16 @@
             "type": "int"
           }
         },
+        "git_submodule_index_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
+        "git_submodule_wd_id": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_submodule_location": {
           "isAsync": true,
           "args": {

generate/input/libgit2-supplement.json

@@ -268,6 +268,15 @@
           "git_patch_convenient_from_diff"
         ]
       ],
+      [
+        "reflog_entry",
+        [
+          "git_reflog_entry_committer",
+          "git_reflog_entry_id_new",
+          "git_reflog_entry_id_old",
+          "git_reflog_entry_message"
+        ]
+      ],
       [
         "revwalk",
         [
@@ -705,6 +714,14 @@
         "git_merge_head_id"
       ]
     },
+    "reflog": {
+      "functions": [
+        "git_reflog_entry_committer",
+        "git_reflog_entry_id_new",
+        "git_reflog_entry_id_old",
+        "git_reflog_entry_message"
+      ]
+    },
     "status": {
       "functions": [
         "git_status_list_entrycount",

generate/scripts/helpers.js

@@ -220,6 +220,7 @@ var Helpers = {
     field.jsFunctionName = utils.camelCase(field.name);
     field.cppClassName = Helpers.cTypeToCppName(field.type);
     field.jsClassName = utils.titleCase(Helpers.cTypeToJsName(field.type));
+    field.ownedByThis = true;
 
     if (Helpers.isCallbackFunction(field.cType)) {
       Helpers.processCallback(field);

test/tests/commit.js

@@ -4,6 +4,7 @@ var promisify = require("promisify-node");
 var fse = promisify(require("fs-extra"));
 
 var garbageCollect = require("../utils/garbage_collect.js");
+var leakTest = require("../utils/leak_test");
 
 var local = path.join.bind(path, __dirname);
 
@@ -367,11 +368,11 @@ describe("Commit", function() {
     }).then(function(reflog) {
       var reflogEntry = reflog.entryByIndex(0);
       assert.equal(
-        NodeGit.Reflog.entryMessage(reflogEntry),
+        reflogEntry.message(),
         customReflogMessage
       );
       assert.equal(
-        NodeGit.Reflog.entryIdNew(reflogEntry).toString(),
+        reflogEntry.idNew().toString(),
         oid
       );
       // only setTarget should have added to the entrycount
@@ -627,30 +628,9 @@ describe("Commit", function() {
   it("does not leak", function() {
     var test = this;
 
-    garbageCollect();
-    var Commit = NodeGit.Commit;
-    var startSelfFreeingCount = Commit.getSelfFreeingInstanceCount();
-    var startNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount();
-
-    var resolve;
-    var promise = new Promise(function(_resolve) { resolve = _resolve; });
-
-    NodeGit.Commit.lookup(test.repository, oid)
-      .then(function() {
-        // get out of this promise chain to help GC get rid of the commit
-        setTimeout(resolve, 0);
-      });
-
-    return promise
-      .then(function() {
-        garbageCollect();
-        var endSelfFreeingCount = Commit.getSelfFreeingInstanceCount();
-        var endNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount();
-        // any new self-freeing commits should have been freed
-        assert.equal(startSelfFreeingCount, endSelfFreeingCount);
-        // no new non-self-freeing commits should have been constructed
-        assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount);
-      });
+    return leakTest(NodeGit.Commit, function() {
+        return NodeGit.Commit.lookup(test.repository, oid);
+    });
   });
 
   it("duplicates signature", function() {

test/tests/oid.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("Oid", function() {
   var NodeGit = require("../../");
   var Oid = NodeGit.Oid;
@@ -70,4 +72,24 @@ describe("Oid", function() {
     var oid2 = Oid.fromString("13c633665257696a3800b0a39ff636b4593f918f");
     assert(!this.oid.equal(oid2));
   });
+
+  it("does not leak constructed Oid", function() {
+    return leakTest(Oid, function() {
+      return Promise.resolve(
+        Oid.fromString("13c633665257696a3800b0a39ff636b4593f918f")
+      );
+    });
+  });
+
+  it("does not leak owned Oid", function() {
+    return leakTest(Oid, function() {
+      return NodeGit.Repository.open(local("../repos/workdir"))
+        .then(function(repo) {
+          return NodeGit.Commit.lookup(repo, oid);
+        })
+        .then(function(commit) {
+          return commit.id();
+        });
+    });
+  });
 });

test/utils/leak_test.js

@@ -0,0 +1,33 @@
+var assert = require("assert");
+
+var garbageCollect = require("./garbage_collect");
+
+function leakTest(Type, getInstance) {
+  garbageCollect();
+  var startSelfFreeingCount = Type.getSelfFreeingInstanceCount();
+  var startNonSelfFreeingCount = Type.getNonSelfFreeingConstructedCount();
+
+  var resolve;
+  var promise = new Promise(function(_resolve) { resolve = _resolve; });
+
+  getInstance()
+    .then(function() {
+      var selfFreeingCount = Type.getSelfFreeingInstanceCount();
+      assert.equal(startSelfFreeingCount + 1, selfFreeingCount);
+      // get out of this promise chain to help GC get rid of the commit
+      setTimeout(resolve, 0);
+    });
+
+  return promise
+    .then(function() {
+      garbageCollect();
+      var endSelfFreeingCount = Type.getSelfFreeingInstanceCount();
+      var endNonSelfFreeingCount = Type.getNonSelfFreeingConstructedCount();
+      // any new self-freeing commits should have been freed
+      assert.equal(startSelfFreeingCount, endSelfFreeingCount);
+      // no new non-self-freeing commits should have been constructed
+      assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount);
+    });
+}
+
+module.exports = leakTest;