Merge pull request #250 from nodegit/refactor-classes

tbranyen · tbranyen · commit 4113536f40a9 · 2014-10-23T17:10:57.000-04:00
Refactor classes
diff --git a/.travis.yml b/.travis.yml
@@ -22,4 +22,4 @@ git:
 branches:
   only:
     - master
-
+script: npm --expose-gc test
diff --git a/example/add-and-commit.js b/example/add-and-commit.js
@@ -6,7 +6,7 @@ var git = require('../'),
     ;
 
 /**
- * This example creates a certain file `newfile.txt`, adds it to the git index and 
+ * This example creates a certain file `newfile.txt`, adds it to the git index and
  * commits it to head. Similar to a `git add newfile.txt` followed by a `git commit`
 **/
 
@@ -56,4 +56,4 @@ git.Repo.open(path.resolve(__dirname, '../.git'), function(openReporError, repo)
       });
     });
   });
-});
+});
diff --git a/generate/descriptor.json b/generate/descriptor.json
@@ -196,8 +196,8 @@
     "functions": {
       "git_commit_create": {
         "ignore": false,
-        "jsFunctionName": "createCommit",
-        "cppFunctionName": "CreateCommit",
+        "jsFunctionName": "create",
+        "cppFunctionName": "Create",
         "isConstructorMethod": true,
         "isAsync": true,
         "return": {
diff --git a/generate/manual/include/macros.h b/generate/manual/include/macros.h
@@ -0,0 +1,12 @@
+#ifndef NODEGIT_PSUEDONAN_H_
+#if (NODE_MODULE_VERSION > 0x000B)
+// Node 0.11+ (0.11.3 and below won't compile with these)
+# define NodeGitPsueodoNanReturnEscapingValue(val) NanReturnValue(NanEscapeScope(val))
+
+#else
+// Node 0.8 and 0.10
+# define NodeGitPsueodoNanReturnEscapingValue(val) NanReturnValue(val)
+
+#endif
+
+#endif //NODEGIT_PSUEDONAN_H_
diff --git a/generate/nkallen.json b/generate/nkallen.json
@@ -12344,8 +12344,8 @@
         "isAsync": true,
         "isConstructorMethod": false,
         "isPrototypeMethod": true,
-        "jsFunctionName": "createCommit",
-        "cppFunctionName": "CreateCommit",
+        "jsFunctionName": "create",
+        "cppFunctionName": "Create",
         "return": {
           "cType": "int",
           "cppClassName": "Int32",
diff --git a/generate/partials/convert_to_v8.cc b/generate/partials/convert_to_v8.cc
@@ -1,3 +1,4 @@
+// start convert_to_v8 block
 {%if cppClassName == 'String' %}
   {%if size %}
 to = NanNew<String>({{= parsedName =}}, {{ size }});
@@ -39,9 +40,15 @@ to = tmpArray;
   {%endif%}
 
     if ({{= parsedName =}} != NULL) {
+      // {{= cppClassName }} {{= parsedName }}
+      {%if cppClassName == 'Wrapper' %}
       to = {{ cppClassName }}::New((void *){{= parsedName =}});
+      {%else%}
+      to = {{ cppClassName }}::New((void *){{= parsedName =}}, false);
+      {%endif%}
     } else {
       to = NanNull();
     }
 
 {%endif%}
+// end convert_to_v8 block
diff --git a/generate/partials/sync_function.cc b/generate/partials/sync_function.cc
@@ -1,7 +1,7 @@
 
 {%partial doc .%}
 NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) {
-  NanScope();
+  NanEscapableScope();
   {%partial guardArguments .%}
 
   {%each .|returnsInfo 'true' as _return %}
@@ -36,17 +36,6 @@ from_{{ arg.name }}
     {%if not arg.lastArg %},{%endif%}
   {%endeach%}
   );
-
-{%each args|argsInfo as arg %}
-  {%if arg.isCppClassStringOrArray %}
-    {%if arg.freeFunctionName %}
-  {{ arg.freeFunctionName }}(from_{{ arg.name }});
-    {%else%}
-  free((void *)from_{{ arg.name }});
-    {%endif%}
-  {%endif%}
-{%endeach%}
-
 {%if return.isErrorCode %}
   if (result != GIT_OK) {
   {%each args|argsInfo as arg %}
@@ -77,9 +66,9 @@ from_{{ arg.name }}
     {%endif%}
   {%endeach%}
   {%if .|returnsCount == 1 %}
-  NanReturnValue(to);
+  NodeGitPsueodoNanReturnEscapingValue(to);
   {%else%}
-  NanReturnValue(toReturn);
+  NodeGitPsueodoNanReturnEscapingValue(toReturn);
   {%endif%}
 {%endif%}
 }
diff --git a/generate/setup.js b/generate/setup.js
@@ -114,6 +114,7 @@ fileNames.forEach(function(fileName, index) {
     }
 
     file.freeFunctionName = "git_" + fileName + "_free";
+    file.createFunctionName = "git_" + fileName + "_create";
   }
 
   // TODO Obsolete this.
@@ -136,12 +137,14 @@ fileNames.forEach(function(fileName, index) {
     return fnName === initFnName;
   });
 
-  // Doesn't actually exist.
+  // Free function doesn't actually exist.
   if (cFile.functions.indexOf(file.freeFunctionName) === -1) {
     delete file.freeFunctionName;
   }
 
-
+  if (cFile.functions.indexOf(file.createFunctionName) === -1) {
+    delete file.createFunctionName;
+  }
   var legacyFile = {};
 
   if (file.jsClassName.indexOf("Git") === 0) {
@@ -394,6 +397,12 @@ fileNames.forEach(function(fileName, index) {
     }
   });
 
+  if (file.createFunctionName) {
+    file.cppCreateFunctionName = typeMap[file.createFunctionName].cpp;
+    file.jsCreateFunctionName = typeMap[file.createFunctionName].js;
+    delete file.createFunctionName;
+  }
+
   files.push(file);
 });
 
diff --git a/generate/templates/class_content.cc b/generate/templates/class_content.cc
@@ -7,6 +7,7 @@ extern "C" {
 }
 
 #include "../include/functions/copy.h"
+#include "../include/macros.h"
 #include "../include/{{ filename }}"
 
 {%each dependencies as dependency%}
@@ -17,9 +18,9 @@ using namespace v8;
 using namespace node;
 
 {%if cType%}
-{{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw) {
+{{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing) {
   this->raw = raw;
-  this->selfFreeing = true;
+  this->selfFreeing = selfFreeing;
 }
 
 {{ cppClassName }}::~{{ cppClassName }}() {
@@ -63,18 +64,23 @@ NAN_METHOD({{ cppClassName }}::New) {
   NanScope();
 
   if (args.Length() == 0 || !args[0]->IsExternal()) {
-    return NanThrowError("{{ cType }} is required.");
+    {%if createFunctionName%}
+    return NanThrowError("A new {{ cppClassName }} cannot be instantiated. Use {{ jsCreateFunctionName }} instead.");
+    {%else%}
+    return NanThrowError("A new {{ cppClassName }} cannot be instantiated.");
+    {%endif%}
   }
-  {{ cppClassName }}* object = new {{ cppClassName }}(static_cast<{{ cType }} *>(Handle<External>::Cast(args[0])->Value()));
+
+  {{ cppClassName }}* object = new {{ cppClassName }}(static_cast<{{ cType }} *>(Handle<External>::Cast(args[0])->Value()), args[1]->BooleanValue());
   object->Wrap(args.This());
 
   NanReturnValue(args.This());
 }
 
-Handle<Value> {{ cppClassName }}::New(void *raw) {
+Handle<Value> {{ cppClassName }}::New(void *raw, bool selfFreeing) {
   NanEscapableScope();
-  Handle<Value> argv[1] = { NanNew<External>((void *)raw) };
-  return NanEscapeScope(NanNew<Function>({{ cppClassName }}::constructor_template)->NewInstance(1, argv));
+  Handle<Value> argv[2] = { NanNew<External>((void *)raw), NanNew<Boolean>(selfFreeing) };
+  return NanEscapeScope(NanNew<Function>({{ cppClassName }}::constructor_template)->NewInstance(2, argv));
 }
 
 {{ cType }} *{{ cppClassName }}::GetValue() {
diff --git a/generate/templates/class_header.h b/generate/templates/class_header.h
@@ -1,6 +1,6 @@
 #ifndef {{ cppClassName|upper }}_H
 #define {{ cppClassName|upper }}_H
-
+// generated from class_header.h
 #include <nan.h>
 #include <string>
 
@@ -36,14 +36,13 @@ class {{ cppClassName }} : public ObjectWrap {
     {{ cType }} *GetValue();
     {{ cType }} **GetRefValue();
 
-    static Handle<Value> New(void *raw);
-
-    bool selfFreeing;
+    static Handle<Value> New(void *raw, bool selfFreeing);
     {%endif%}
+    bool selfFreeing;
 
   private:
     {%if cType%}
-    {{ cppClassName }}({{ cType }} *raw);
+    {{ cppClassName }}({{ cType }} *raw, bool selfFreeing);
     ~{{ cppClassName }}();
     {%endif%}
 
diff --git a/generate/templates/struct_content.cc b/generate/templates/struct_content.cc
@@ -18,7 +18,7 @@ extern "C" {
 using namespace v8;
 using namespace node;
 using namespace std;
-
+// generated from struct_content.cc
 {{ cppClassName }}::{{ cppClassName }}() {
   {{ cType }} wrappedValue = {{ cType|upper }}_INIT;
   this->raw = ({{ cType }}*) malloc(sizeof({{ cType }}));
@@ -28,10 +28,10 @@ using namespace std;
   this->selfFreeing = true;
 }
 
-{{ cppClassName }}::{{ cppClassName }}({{ cType }}* raw) {
+{{ cppClassName }}::{{ cppClassName }}({{ cType }}* raw, bool selfFreeing) {
   this->raw = raw;
   this->ConstructFields();
-  this->selfFreeing = true;
+  this->selfFreeing = selfFreeing;
 }
 
 {{ cppClassName }}::~{{ cppClassName }}() {
@@ -90,7 +90,7 @@ NAN_METHOD({{ cppClassName }}::New) {
     instance = new {{ cppClassName }}();
   }
   else {
-    instance = new {{ cppClassName }}(static_cast<{{ cType }}*>(Handle<External>::Cast(args[0])->Value()));
+    instance = new {{ cppClassName }}(static_cast<{{ cType }}*>(Handle<External>::Cast(args[0])->Value()), true);
   }
 
   instance->Wrap(args.This());
diff --git a/generate/templates/struct_header.h b/generate/templates/struct_header.h
@@ -1,6 +1,6 @@
 #ifndef {{ cppClassName|upper }}_H
 #define {{ cppClassName|upper }}_H
-
+// generated from struct_header.h
 #include <nan.h>
 #include <string>
 
@@ -17,7 +17,7 @@ using namespace v8;
 
 class {{ cppClassName }} : public ObjectWrap {
   public:
-    {{ cppClassName }}({{ cType }}* raw);
+    {{ cppClassName }}({{ cType }}* raw, bool selfFreeing);
     static Persistent<Function> constructor_template;
     static void Initialize (Handle<v8::Object> target);
 
diff --git a/generate/types.json b/generate/types.json
@@ -1877,8 +1877,8 @@
     "js": "lookup"
   },
   "git_commit_create": {
-    "cpp": "CreateCommit",
-    "js": "createCommit"
+    "cpp": "Create",
+    "js": "create"
   },
   "const git_commit **": {
     "cpp": "GitCommit",
@@ -3301,12 +3301,12 @@
     "js": "nthGenAncestor"
   },
   "git_commit_create *": {
-    "cpp": "CreateCommit",
-    "js": "createCommit"
+    "cpp": "Create",
+    "js": "create"
   },
   "const git_commit_create *": {
-    "cpp": "CreateCommit",
-    "js": "createCommit"
+    "cpp": "Create",
+    "js": "create"
   },
   "git_commit_create_v *": {
     "cpp": "CreateV",
diff --git a/lib/repository.js b/lib/repository.js
@@ -48,7 +48,7 @@ Repository.prototype.getRemotes = function(callback) {
       callback(null, remotes);
     }
 
-    return remotes; 
+    return remotes;
   }, callback);
 };
 
@@ -218,7 +218,7 @@ Repository.prototype.createCommit = function(
   var commit = this;
 
   if (tree instanceof Tree) {
-    commit = Commit.createCommit.call(
+    commit = Commit.create.call(
       this,
       updateRef,
       author,
@@ -231,7 +231,7 @@ Repository.prototype.createCommit = function(
     );
   } else {
     createCommit = this.getTree(tree).then(function(tree) {
-      return Commit.createCommit.call(
+      return Commit.create.call(
         commit,
         updateRef,
         author,
diff --git a/test/tests/commit.js b/test/tests/commit.js
@@ -91,18 +91,18 @@ describe("Commit", function() {
     history.on("commit", function(commit) {
       historyCount++;
     });
-    
+
     history.on("end", function(commits) {
       assert.equal(historyCount, expectedHistoryCount);
       assert.equal(commits.length, expectedHistoryCount);
 
       done();
     });
-    
+
     history.on("error", function(err) {
       assert.ok(false);
     });
-    
+
     history.start();
   });
 
@@ -143,7 +143,7 @@ describe("Commit", function() {
       treeWalker.on("error", function() {
         assert.ok(false);
       });
-      
+
       treeWalker.on("end", function(entries) {
         assert.equal(commitTreeEntryCount, expectedCommitTreeEntryCount);
         done();
diff --git a/test/tests/revwalk.js b/test/tests/revwalk.js
@@ -57,4 +57,32 @@ describe("Revwalk", function() {
       });
     });
   });
+
+  // This test requires forcing garbage collection, so mocha needs to be run via
+  // node rather than npm, with a la `node --expose-gc [pathtohmoca] [testglob]`
+  var testGC = (global.gc ? it : it.skip);
+
+  testGC("doesnt segfault when accessing .author() twice", function(done) {
+      this.timeout(10000);
+      return Repository.open(reposPath).then(function(repository) {
+        var walker = repository.createRevWalk();
+        repository.getMaster().then(function(master) {
+          var did = false;
+          walker.walk(master, function(error, commit) {
+            for (var i = 0; i < 1000; i++) {
+              if (true) {
+                commit.author().name();
+                commit.author().email();
+              }
+              global.gc();
+            }
+            if (!did) {
+              done();
+              did = true;
+            }
+          });
+        });
+      });
+    });
+
 });
