nodegit · johnhaley81 · Apr 7, 2016 · Apr 1, 2016 · Apr 1, 2016 · Apr 7, 2016
diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json
@@ -143,7 +143,7 @@
         },
         "git_blob_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_blob_rawcontent": {
@@ -389,12 +389,12 @@
         },
         "git_commit_author": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_commit_committer": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_commit_create": {
@@ -424,17 +424,17 @@
         },
         "git_commit_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_commit_parent_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_commit_tree_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         }
       }
@@ -1594,12 +1594,12 @@
         },
         "git_reference_target": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_reference_target_peel": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         }
       }
@@ -1627,6 +1627,7 @@
     },
     "remote": {
       "cType": "git_remote",
+      "selfFreeing": true,
       "functions": {
         "git_remote_create": {
           "isAsync": false
@@ -1711,6 +1712,11 @@
           },
           "isAsync": true
         },
+        "git_remote_get_refspec": {
+          "return": {
+            "ownedByThis": true
+          }
+        },
         "git_remote_list": {
           "args": {
             "out": {
@@ -1748,6 +1754,9 @@
         },
         "git_remote_set_push_refspecs": {
           "ignore": true
+        },
+        "git_remote_stats": {
+          "ownedByThis": true
         }
       }
     },
@@ -2182,7 +2191,7 @@
         },
         "git_tag_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tag_create_lightweight": {
@@ -2222,7 +2231,7 @@
         },
         "git_tag_tagger": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tag_target": {
@@ -2234,7 +2243,7 @@
         },
         "git_tag_target_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tag_delete": {
@@ -2252,6 +2261,9 @@
         }
       }
     },
+    "transfer_progress": {
+      "dupFunction": "git_transfer_progress_dup"
+    },
     "transport": {
       "cType": "git_transport",
       "needsForwardDeclaration": false,
@@ -2281,28 +2293,28 @@
       "functions": {
         "git_tree_entry_byid": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tree_entry_byindex": {
           "jsFunctionName": "_entryByIndex"
         },
         "git_tree_entry_byname": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tree_entry_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tree_entrycount": {
           "jsFunctionName": "entryCount"
         },
         "git_tree_id": {
           "return": {
-            "shouldDuplicate": true
+            "ownedByThis": true
           }
         },
         "git_tree_walk": {

diff --git a/generate/templates/manual/include/functions/copy.h b/generate/templates/manual/include/functions/copy.h
@@ -15,4 +15,6 @@ const git_time *git_time_dup(const git_time *arg);
 const git_diff_delta *git_diff_delta_dup(const git_diff_delta *arg);
 const git_diff_file *git_diff_file_dup(const git_diff_file *arg);
 
+void git_transfer_progress_dup(git_transfer_progress **out, const git_transfer_progress *arg);
+
 #endif
diff --git a/generate/templates/manual/src/functions/copy.cc b/generate/templates/manual/src/functions/copy.cc
@@ -10,3 +10,8 @@ const git_error *git_error_dup(const git_error *arg) {
   result->message = strdup(arg->message);
   return result;
 }
+
+void git_transfer_progress_dup(git_transfer_progress **out, const git_transfer_progress *arg) {
+  *out = (git_transfer_progress *)malloc(sizeof(git_transfer_progress));
+  memcpy(*out, arg, sizeof(git_transfer_progress));
+}
diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc
@@ -61,7 +61,7 @@
     {% if cppClassName == 'Wrapper' %}
       to = {{ cppClassName }}::New({{= parsedName =}});
     {% else %}
-      to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }} {% if shouldDuplicate %}, true{% endif %});
+      to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }} {% if ownedByThis %}, info.This(){% endif %});
     {% endif %}
   }
   else {

diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc
@@ -24,15 +24,24 @@ using namespace v8;
 using namespace node;
 
 {% if cType %}
-  {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) {
-    if (shouldDuplicate) {
-      {% if shouldAlloc %}
-    this->raw = ({{ cType }} *)malloc(sizeof({{ cType }}));
-    {{ dupFunction }}(this->raw, raw);
+  {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing, Local<v8::Object> owner) {
+    if (!owner.IsEmpty()) {
+      // if we have an owner, there are two options - either we duplicate the raw object
+      // (so we own the duplicate, and can self-free it)
+      // or we keep a handle on the owner so it doesn't get garbage collected
+      // while this wrapper is accessible
+      {% if dupFunction %}
+        {% if shouldAlloc %}
+      this->raw = ({{ cType }} *)malloc(sizeof({{ cType }}));
+      {{ dupFunction }}(this->raw, raw);
+        {% else %}
+      {{ dupFunction }}(&this->raw, raw);
+        {% endif %}
+      selfFreeing = true;
       {% else %}
-    {{ dupFunction }}(&this->raw, raw);
+      this->owner.Reset(owner);
+      this->raw = raw;
       {% endif %}
-      selfFreeing = true;
     } else {
       this->raw = raw;
     }
@@ -117,17 +126,22 @@ using namespace node;
     {{ cppClassName }}* object = new {{ cppClassName }}(static_cast<{{ cType }} *>(
       Local<External>::Cast(info[0])->Value()),
       Nan::To<bool>(info[1]).FromJust(),
-      info.Length() >= 3 ? Nan::To<bool>(info[2]).FromJust() : false
+      info.Length() >= 3 && !info[2].IsEmpty() && info[2]->IsObject() ? info[2]->ToObject() : Local<v8::Object>()
     );
     object->Wrap(info.This());
 
     info.GetReturnValue().Set(info.This());
   }
 
-  Local<v8::Value> {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) {
+  Local<v8::Value> {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, Local<v8::Object> owner) {
     Nan::EscapableHandleScope scope;
-    Local<v8::Value> argv[3] = { Nan::New<External>((void *)raw), Nan::New(selfFreeing), Nan::New(shouldDuplicate) };
-    return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 3, argv).ToLocalChecked());
+    Local<v8::Value> argv[3] = { Nan::New<External>((void *)raw), Nan::New(selfFreeing), owner };
+    return scope.Escape(
+      Nan::NewInstance(
+        Nan::New({{ cppClassName }}::constructor_template),
+        owner.IsEmpty() ? 2 : 3, // passing an empty handle as part of the arguments causes a crash
+        argv
+      ).ToLocalChecked());
   }
 
   NAN_METHOD({{ cppClassName }}::GetSelfFreeingInstanceCount) {

diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h
@@ -49,7 +49,7 @@ class {{ cppClassName }} : public Nan::ObjectWrap {
     {{ cType }} *GetValue();
     void ClearValue();
 
-    static Local<v8::Value> New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false);
+    static Local<v8::Value> New(const {{ cType }} *raw, bool selfFreeing, Local<v8::Object> owner = Local<v8::Object>());
     {%endif%}
     bool selfFreeing;
 
@@ -82,10 +82,15 @@ class {{ cppClassName }} : public Nan::ObjectWrap {
 
 
   private:
-
+    // owner of the object, in the memory management sense. only populated
+    // when using ownedByThis, and the type doesn't have a dupFunction
+    // CopyablePersistentTraits are used to get the reset-on-destruct behavior.
+    {%if not dupFunction %}
+    Nan::Persistent<Object, Nan::CopyablePersistentTraits<Object> > owner;
+    {%endif%}
 
     {%if cType%}
-    {{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false);
+    {{ cppClassName }}({{ cType }} *raw, bool selfFreeing, Local<v8::Object> owner = Local<v8::Object>());
     ~{{ cppClassName }}();
     {%endif%}
 

diff --git a/test/tests/commit.js b/test/tests/commit.js
@@ -2,32 +2,16 @@ var assert = require("assert");
 var path = require("path");
 var promisify = require("promisify-node");
 var fse = promisify(require("fs-extra"));
+
+var garbageCollect = require("../utils/garbage_collect.js");
+
 var local = path.join.bind(path, __dirname);
 
 // Have to wrap exec, since it has a weird callback signature.
 var exec = promisify(function(command, opts, callback) {
   return require("child_process").exec(command, opts, callback);
 });
 
-// aggressively collects garbage until we fail to improve terminatingIterations
-// times.
-function garbageCollect() {
-  var terminatingIterations = 3;
-  var usedBeforeGC = Number.MAX_VALUE;
-  var nondecreasingIterations = 0;
-  for ( ; ; ) {
-    global.gc();
-    var usedAfterGC = process.memoryUsage().heapUsed;
-    if (usedAfterGC >= usedBeforeGC) {
-      nondecreasingIterations++;
-      if (nondecreasingIterations >= terminatingIterations) {
-        break;
-      }
-    }
-    usedBeforeGC = usedAfterGC;
-  }
-}
-
 describe("Commit", function() {
   var NodeGit = require("../../");
   var Repository = NodeGit.Repository;

diff --git a/test/tests/remote.js b/test/tests/remote.js
@@ -2,6 +2,8 @@ var assert = require("assert");
 var path = require("path");
 var local = path.join.bind(path, __dirname);
 
+var garbageCollect = require("../utils/garbage_collect.js");
+
 describe("Remote", function() {
   var NodeGit = require("../../");
   var Repository = NodeGit.Repository;
@@ -379,4 +381,48 @@ describe("Remote", function() {
         }
       });
   });
+
+  it("is kept alive by refspec", function() {
+    var repo = this.repository;
+    var Remote = NodeGit.Remote;
+
+    garbageCollect();
+    var startSelfFreeingCount = Remote.getSelfFreeingInstanceCount();
+    var startNonSelfFreeingCount = Remote.getNonSelfFreeingConstructedCount();
+
+    var resolve;
+    var promise = new Promise(function(_resolve) { resolve = _resolve; });
+
+    var remote;
+
+    repo.getRemote("origin")
+      .then(function(_remote) {
+        remote = _remote;
+        setTimeout(resolve, 0);
+      });
+
+    return promise
+      .then(function() {
+        // make sure we have created one self-freeing remote
+        assert.equal(startSelfFreeingCount + 1,
+          Remote.getSelfFreeingInstanceCount());
+        assert.equal(startNonSelfFreeingCount,
+          Remote.getNonSelfFreeingConstructedCount());
+        var refspec = remote.getRefspec(0);
+        assert.equal("refs/heads/*", refspec.src());
+        remote = null;
+        garbageCollect();
+        // the refspec should be holding on to the remote
+        assert.equal(startSelfFreeingCount + 1,
+          Remote.getSelfFreeingInstanceCount());
+
+        assert.equal("refs/heads/*", refspec.src());
+
+        refspec = null;
+        garbageCollect();
+        // the remote should be freed now
+        assert.equal(startSelfFreeingCount,
+          Remote.getSelfFreeingInstanceCount());
+      });
+  });
 });
diff --git a/test/tests/revwalk.js b/test/tests/revwalk.js
@@ -315,7 +315,7 @@ describe("Revwalk", function() {
       var walker = repository.createRevWalk();
 
       repository.getMasterCommit().then(function(firstCommitOnMaster) {
-        walker.walk(firstCommitOnMaster, function(err, commit) {
+        walker.walk(firstCommitOnMaster.id(), function(err, commit) {
           if (err && err.errno === NodeGit.Error.CODE.ITEROVER) {
             return done();
           }

diff --git a/test/tests/submodule.js b/test/tests/submodule.js
@@ -107,9 +107,11 @@ describe("Submodule", function() {
   });
 
   it("can setup and finalize submodule add", function() {
+    this.timeout(30000);
+
     var repo = this.repository;
-    var submodulePath = "hellogitworld";
-    var submoduleUrl = "https://github.com/githubtraining/hellogitworld.git";
+    var submodulePath = "nodegittest";
+    var submoduleUrl = "https://github.com/nodegit/test.git";
 
     var submodule;
     var submoduleRepo;
@@ -134,7 +136,7 @@ describe("Submodule", function() {
         return reference.peel(NodeGit.Object.TYPE.COMMIT);
       })
       .then(function(commit) {
-        return submoduleRepo.createBranch("master", commit);
+        return submoduleRepo.createBranch("master", commit.id());
       })
       .then(function() {
         return submodule.addFinalize();