diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 48eb6968d..724bc9951 100644 --- 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,7 +2293,7 @@ "functions": { "git_tree_entry_byid": { "return": { - "shouldDuplicate": true + "ownedByThis": true } }, "git_tree_entry_byindex": { @@ -2289,12 +2301,12 @@ }, "git_tree_entry_byname": { "return": { - "shouldDuplicate": true + "ownedByThis": true } }, "git_tree_entry_id": { "return": { - "shouldDuplicate": true + "ownedByThis": true } }, "git_tree_entrycount": { @@ -2302,7 +2314,7 @@ }, "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 index 299d07fea..69c733464 100644 --- 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 index 48d20ce38..d885f0ea4 100644 --- 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 index 4c669ac76..d9b1d8830 100644 --- 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 index 312c2ef28..720c0d1e2 100644 --- 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 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::Cast(info[0])->Value()), Nan::To(info[1]).FromJust(), - info.Length() >= 3 ? Nan::To(info[2]).FromJust() : false + info.Length() >= 3 && !info[2].IsEmpty() && info[2]->IsObject() ? info[2]->ToObject() : Local() ); object->Wrap(info.This()); info.GetReturnValue().Set(info.This()); } - Local {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) { + Local {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, Local owner) { Nan::EscapableHandleScope scope; - Local argv[3] = { Nan::New((void *)raw), Nan::New(selfFreeing), Nan::New(shouldDuplicate) }; - return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 3, argv).ToLocalChecked()); + Local argv[3] = { Nan::New((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 index cc77f52b3..63abef7b9 100644 --- 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 New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false); + static Local New(const {{ cType }} *raw, bool selfFreeing, Local owner = Local()); {%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 > owner; + {%endif%} {%if cType%} - {{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false); + {{ cppClassName }}({{ cType }} *raw, bool selfFreeing, Local owner = Local()); ~{{ cppClassName }}(); {%endif%} diff --git a/test/tests/commit.js b/test/tests/commit.js index e770a0c1e..2a0f27595 100644 --- a/test/tests/commit.js +++ b/test/tests/commit.js @@ -2,6 +2,9 @@ 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. @@ -9,25 +12,6 @@ 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 index ff7948cad..1490df229 100644 --- 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 index 85d708303..2fc704e27 100644 --- 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 index 002b4ef12..b8c9c32ec 100644 --- 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(); diff --git a/test/utils/garbage_collect.js b/test/utils/garbage_collect.js new file mode 100644 index 000000000..a288b99c8 --- /dev/null +++ b/test/utils/garbage_collect.js @@ -0,0 +1,20 @@ +// 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; + } +} + +module.exports = garbageCollect;