Merge pull request nodegit#425 from nodegit/fix-file-lock

tbranyen · tbranyen · commit 0454485e349e · 2015-02-26T01:05:46.000-05:00
Attempt to fix Windows file locking bug
diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json
@@ -1431,9 +1431,6 @@
         "git_repository_fetchhead_foreach": {
           "ignore": true
         },
-        "git_repository_free": {
-          "ignore": true
-        },
         "git_repository_hashfile": {
           "ignore": true
         },
diff --git a/generate/scripts/generateNativeCode.js b/generate/scripts/generateNativeCode.js
@@ -144,7 +144,7 @@ module.exports = function generateNativeCode() {
         });
       }
     })
-  });
+  }).catch(console.log);
 
 };
 
diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js
@@ -288,8 +288,8 @@ var Helpers = {
     // available
     if (key == typeDef.cType + "_free") {
       typeDef.freeFunctionName = key;
-      fnDef.ignore = true;
-      return;
+      //fnDef.ignore = true;
+      //return;
     }
 
     fnDef.cppFunctionName = Helpers.cTypeToCppName(key, "git_" + typeDef.typeName);
diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc
@@ -30,6 +30,12 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) {
 
 {%each args|argsInfo as arg %}
 {%endeach%}
+
+{%-- Inside a free call, if the value is already free'd don't do it again.--%}
+{% if cppFunctionName == "Free" %}
+if (ObjectWrap::Unwrap<{{ cppClassName }}>(args.This())->GetValue() != NULL) {
+{% endif %}
+
 {%if .|hasReturns %}
   {{ return.cType }} result = {%endif%}{{ cFunctionName }}(
   {%each args|argsInfo as arg %}
@@ -67,6 +73,12 @@ from_{{ arg.name }}
   }
 {%endif%}
 
+{% if cppFunctionName == "Free" %}
+  ObjectWrap::Unwrap<{{ cppClassName }}>(args.This())->ClearValue();
+}
+{% endif %}
+
+
 {%each args|argsInfo as arg %}
   {%if arg | isOid %}
   if (args[{{ arg.jsArg }}]->IsString()) {
diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc
@@ -35,6 +35,7 @@ using namespace node;
     {% if freeFunctionName %}
       if (this->selfFreeing) {
         {{ freeFunctionName }}(this->raw);
+        this->raw = NULL;
       }
     {% endif %}
 
@@ -112,7 +113,11 @@ using namespace node;
   }
 
   {{ cType }} **{{ cppClassName }}::GetRefValue() {
-    return &this->raw;
+    return this->raw == NULL ? NULL : &this->raw;
+  }
+
+  void {{ cppClassName }}::ClearValue() {
+    this->raw = NULL;
   }
 
 {% else %}
diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h
@@ -38,6 +38,7 @@ class {{ cppClassName }} : public ObjectWrap {
     {%if cType%}
     {{ cType }} *GetValue();
     {{ cType }} **GetRefValue();
+    void ClearValue();
 
     static Handle<v8::Value> New(void *raw, bool selfFreeing);
     {%endif%}
diff --git a/generate/templates/templates/struct_content.cc b/generate/templates/templates/struct_content.cc
@@ -138,7 +138,11 @@ Handle<v8::Value> {{ cppClassName }}::New(void* raw, bool selfFreeing) {
 }
 
 {{ cType }} **{{ cppClassName }}::GetRefValue() {
-  return &this->raw;
+  return this->raw == NULL ? NULL : &this->raw;
+}
+
+void {{ cppClassName }}::ClearValue() {
+  this->raw = NULL;
 }
 
 {% partial fieldAccessors . %}
diff --git a/generate/templates/templates/struct_header.h b/generate/templates/templates/struct_header.h
@@ -26,6 +26,7 @@ class {{ cppClassName }} : public ObjectWrap {
 
     {{ cType }} *GetValue();
     {{ cType }} **GetRefValue();
+    void ClearValue();
 
     static Handle<v8::Value> New(void *raw, bool selfFreeing);
 
diff --git a/test/runner.js b/test/runner.js
@@ -45,3 +45,18 @@ beforeEach(function() {
       return exec("git reset --hard", {cwd: workdirPath});
     });
 });
+
+afterEach(function(done) {
+  // In Windows if you do not clean up the repository, there may become a
+  // conflict with file locking.
+  if (this.repository && process.platform === "win32") {
+    this.repository.stateCleanup();
+    this.repository.free();
+    delete this.repository;
+  }
+
+  process.nextTick(function() {
+    global.gc();
+    done();
+  });
+});
diff --git a/test/tests/attr.js b/test/tests/attr.js
@@ -9,7 +9,7 @@ describe("Attr", function() {
 
   var reposPath = local("../repos/workdir/.git");
 
-  before(function() {
+  beforeEach(function() {
     var test = this;
 
     return Repository.open(reposPath)
diff --git a/test/tests/blob.js b/test/tests/blob.js
@@ -10,7 +10,7 @@ describe("Blob", function() {
   var reposPath = local("../repos/workdir/.git");
   var oid = "111dd657329797f6165f52f5085f61ac976dcf04";
 
-  before(function() {
+  beforeEach(function() {
     var test = this;
 
     return Repository.open(reposPath)
diff --git a/test/tests/branch.js b/test/tests/branch.js
@@ -11,24 +11,18 @@ describe("Branch", function() {
 
   var reposPath = local("../repos/workdir/.git");
 
-  before(function() {
+  beforeEach(function() {
     var test = this;
 
     return Repository.open(reposPath)
       .then(function(repository) {
-        test.repo = repository;
-      });
-  });
-
-  beforeEach(function() {
-    var test = this;
-    var repo = test.repo;
-
-    return repo.getMasterCommit()
+        test.repository = repository;
+        return repository.getMasterCommit();
+      })
       .then(function(masterCommit) {
         test.masterCommit = masterCommit;
 
-        return repo.createBranch(branchName, masterCommit, true);
+        return test.repository.createBranch(branchName, masterCommit, true);
       })
       .then(function(branch) {
         test.branch = branch;
@@ -44,7 +38,7 @@ describe("Branch", function() {
   });
 
   it("can delete a branch", function() {
-    var repo = this.repo;
+    var repo = this.repository;
 
     Branch.delete(this.branch);
 
@@ -54,7 +48,7 @@ describe("Branch", function() {
   });
 
   it("can see if the branch is pointed to by head", function() {
-    var repo = this.repo;
+    var repo = this.repository;
 
     return repo.getBranch("master")
       .then(function(branch) {
diff --git a/test/tests/checkout.js b/test/tests/checkout.js
@@ -9,21 +9,21 @@ describe("Checkout", function() {
   var packageJsonOid = "0fa56e90e096a4c24c785206b826ab914ea3de1e";
   var reposPath = local("../repos/workdir/.git");
 
-  before(function() {
+  beforeEach(function() {
     var test = this;
 
     return Repository.open(reposPath)
       .then(function(repo) {
-        test.repo = repo;
+        test.repository = repo;
       });
   });
 
   it("can checkout the head", function() {
     var test = this;
 
-    return Checkout.head(test.repo)
+    return Checkout.head(test.repository)
     .then(function() {
-      return test.repo.getBlob(packageJsonOid);
+      return test.repository.getBlob(packageJsonOid);
     })
     .then(function(blob) {
       var packageJson = blob.toString();
@@ -35,10 +35,10 @@ describe("Checkout", function() {
   it("can checkout by tree", function() {
     var test = this;
 
-    return test.repo.getTagByName("annotated-tag").then(function(tag) {
-      return Checkout.tree(test.repo, test.tag);
+    return test.repository.getTagByName("annotated-tag").then(function(tag) {
+      return Checkout.tree(test.repository, test.tag);
     }).then(function() {
-      return test.repo.getHeadCommit();
+      return test.repository.getHeadCommit();
     }).then(function(commit) {
       assert.equal(commit, "32789a79e71fbc9e04d3eff7425e1771eb595150");
     });
diff --git a/test/tests/clone.js b/test/tests/clone.js
@@ -9,29 +9,39 @@ describe("Clone", function() {
   var Clone = require(local("../../lib/clone"));
   var NodeGit = require(local("../../"));
 
-  var http = local("../repos/http");
-  var https = local("../repos/https");
-  var ssh = local("../repos/ssh");
-  var git = local("../repos/git");
-  var file = local("../repos/file");
+  var clonePath = local("../repos/clone");
 
   var sshPublicKey = local("../id_rsa.pub");
   var sshPrivateKey = local("../id_rsa");
 
   // Set a reasonable timeout here now that our repository has grown.
   this.timeout(30000);
 
+  beforeEach(function(done) {
+    // In Windows if you do not clean up the repository, there may become a
+    // conflict with file locking.
+    if (this.repository && process.platform === "win32") {
+      this.repository.stateCleanup();
+      this.repository.free();
+      delete this.repository;
+    }
+
+    process.nextTick(function() {
+      global.gc();
+      done();
+    });
+  });
+
   beforeEach(function() {
-    return NodeGit.Promise.all([
-      fse.remove(http),
-      fse.remove(https),
-      fse.remove(ssh),
-      fse.remove(git),
-      fse.remove(file)
-    ]).catch(function unhandledFunction() {});
+    return fse.remove(clonePath).catch(function(err) {
+      console.log(err);
+
+      throw err;
+    });
   });
 
   it.skip("can clone with http", function() {
+    var test = this;
     var url = "http://github.com/nodegit/test.git";
     var opts = {
       remoteCallbacks: {
@@ -41,12 +51,14 @@ describe("Clone", function() {
       }
     };
 
-    return Clone.clone(url, http, opts).then(function(repo) {
+    return Clone.clone(url, clonePath, opts).then(function(repo) {
       assert.ok(repo instanceof Repository);
+      test.repository = repo;
     });
   });
 
   it("can clone with https", function() {
+    var test = this;
     var url = "https://github.com/nodegit/test.git";
     var opts = {
       remoteCallbacks: {
@@ -56,12 +68,14 @@ describe("Clone", function() {
       }
     };
 
-    return Clone.clone(url, https, opts).then(function(repo) {
+    return Clone.clone(url, clonePath, opts).then(function(repo) {
       assert.ok(repo instanceof Repository);
+      test.repository = repo;
     });
   });
 
   it("can clone with ssh", function() {
+    var test = this;
     var url = "git@github.com:nodegit/test.git";
     var opts = {
       remoteCallbacks: {
@@ -74,12 +88,14 @@ describe("Clone", function() {
       }
     };
 
-    return Clone.clone(url, ssh, opts).then(function(repo) {
+    return Clone.clone(url, clonePath, opts).then(function(repo) {
       assert.ok(repo instanceof Repository);
+      test.repository = repo;
     });
   });
 
   it("can clone with ssh while manually loading a key", function() {
+    var test = this;
     var url = "git@github.com:nodegit/test.git";
     var opts = {
       remoteCallbacks: {
@@ -96,12 +112,14 @@ describe("Clone", function() {
       }
     };
 
-    return Clone.clone(url, ssh, opts).then(function(repo) {
+    return Clone.clone(url, clonePath, opts).then(function(repo) {
       assert.ok(repo instanceof Repository);
+      test.repository = repo;
     });
   });
 
   it("can clone with git", function() {
+    var test = this;
     var url = "git://github.com/nodegit/test.git";
     var opts = {
       remoteCallbacks: {
@@ -111,24 +129,27 @@ describe("Clone", function() {
       }
     };
 
-    return Clone.clone(url, git, opts).then(function(repo) {
+    return Clone.clone(url, clonePath, opts).then(function(repo) {
+      test.repository = repo;
       assert.ok(repo instanceof Repository);
     });
   });
 
   it("can clone with filesystem", function() {
+    var test = this;
     var prefix = process.platform === "win32" ? "" : "file://";
     var url = prefix + local("../repos/empty");
 
-    return Clone.clone(url, file).then(function(repo) {
+    return Clone.clone(url, clonePath).then(function(repo) {
       assert.ok(repo instanceof Repository);
+      test.repository = repo;
     });
   });
 
   it("will not segfault when accessing a url without username", function() {
     var url = "https://github.com/nodegit/private";
 
-    return Clone.clone(url, git, {
+    return Clone.clone(url, clonePath, {
       remoteCallbacks: {
         certificateCheck: function() {
           return 1;
diff --git a/test/tests/commit.js b/test/tests/commit.js
@@ -24,7 +24,7 @@ describe("Commit", function() {
       });
   }
 
-  before(function() {
+  beforeEach(function() {
     return reinitialize(this);
   });
 
diff --git a/test/tests/diff.js b/test/tests/diff.js
@@ -14,7 +14,7 @@ describe("Diff", function() {
   var diffFilename = "wddiff.txt";
   var diffFilepath = local("../repos/workdir", diffFilename);
 
-  before(function() {
+  beforeEach(function() {
     var test = this;
 
     return Repository.open(reposPath).then(function(repository) {
diff --git a/test/tests/index.js b/test/tests/index.js
diff --git a/test/tests/odb.js b/test/tests/odb.js
diff --git a/test/tests/remote.js b/test/tests/remote.js
diff --git a/test/tests/repository.js b/test/tests/repository.js
diff --git a/test/tests/reset.js b/test/tests/reset.js
diff --git a/test/tests/revwalk.js b/test/tests/revwalk.js
diff --git a/test/tests/tag.js b/test/tests/tag.js

Original file line number	Diff line number	Diff line change
`@@ -144,7 +144,7 @@ module.exports = function generateNativeCode() {`
`144`	`144`	`});`
`145`	`145`	`}`
`146`	`146`	`})`
`147`		`- });`
	`147`	`+ }).catch(console.log);`
`148`	`148`
`149`	`149`	`};`
`150`	`150`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,7 @@ using namespace node;`
`35`	`35`	`{% if freeFunctionName %}`
`36`	`36`	`if (this->selfFreeing) {`
`37`	`37`	`{{ freeFunctionName }}(this->raw);`
	`38`	`+ this->raw = NULL;`
`38`	`39`	`}`
`39`	`40`	`{% endif %}`
`40`	`41`
`@@ -112,7 +113,11 @@ using namespace node;`
`112`	`113`	`}`
`113`	`114`
`114`	`115`	`{{ cType }} **{{ cppClassName }}::GetRefValue() {`
`115`		`- return &this->raw;`
	`116`	`+ return this->raw == NULL ? NULL : &this->raw;`
	`117`	`+ }`
	`118`	`+`
	`119`	`+ void {{ cppClassName }}::ClearValue() {`
	`120`	`+ this->raw = NULL;`
`116`	`121`	`}`
`117`	`122`
`118`	`123`	`{% else %}`
Original file line number	Diff line number	Diff line change
`@@ -138,7 +138,11 @@ Handle<v8::Value> {{ cppClassName }}::New(void* raw, bool selfFreeing) {`
`138`	`138`	`}`
`139`	`139`
`140`	`140`	`{{ cType }} **{{ cppClassName }}::GetRefValue() {`
`141`		`- return &this->raw;`
	`141`	`+ return this->raw == NULL ? NULL : &this->raw;`
	`142`	`+}`
	`143`	`+`
	`144`	`+void {{ cppClassName }}::ClearValue() {`
	`145`	`+ this->raw = NULL;`
`142`	`146`	`}`
`143`	`147`
`144`	`148`	`{% partial fieldAccessors . %}`