Skip to content

Commit dc7e7cd

Browse files
implausibleimplausible
committed
Update FilterSource to have an async repo getter
We discovered that after the garbage collection PR, that submodules can trigger the filter with a filter_source that has a repo that NodeGit has never seen before. This causes libgit2 to free their repo, and us to free what we thought was our repo. As a temporary stopgap to allow filter writers to user repos, I've converted the repo getter to async, and opened a nodegit owned repo. This should prevent any segfaults when pulling the repo out during a filter operation at a small perf penalty.
1 parent 2e891ce commit dc7e7cd

3 files changed

Lines changed: 170 additions & 0 deletions

File tree

‎generate/input/libgit2-supplement.json‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,28 @@
176176
},
177177
"group": "filter_source"
178178
},
179+
"git_filter_source_repo": {
180+
"args": [
181+
{
182+
"name": "out",
183+
"type": "git_repository **"
184+
},
185+
{
186+
"name": "src",
187+
"type": "const git_filter_source *"
188+
}
189+
],
190+
"isManual": true,
191+
"cFile": "generate/templates/manual/filter_source/repo.cc",
192+
"isAsync": true,
193+
"isPrototypeMethod": true,
194+
"type": "function",
195+
"group": "filter_source",
196+
"return": {
197+
"type": "int",
198+
"isErrorCode": true
199+
}
200+
},
179201
"git_patch_convenient_from_diff": {
180202
"args": [
181203
{

‎generate/templates/manual/filter_source/repo.cc‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
// NOTE you may need to occasionally rebuild this method by calling the generators
2+
// if major changes are made to the templates / generator.
3+
4+
// Due to some garbage collection issues related to submodules and git_filters, we need to clone the repository
5+
// pointer before giving it to a user.
6+
7+
/*
8+
* @param Repository callback
9+
*/
10+
NAN_METHOD(GitFilterSource::Repo) {
11+
if (info.Length() == 0 || !info[0]->IsFunction()) {
12+
return Nan::ThrowError("Callback is required and must be a Function.");
13+
}
14+
15+
RepoBaton *baton = new RepoBaton;
16+
17+
baton->error_code = GIT_OK;
18+
baton->error = NULL;
19+
baton->src = Nan::ObjectWrap::Unwrap<GitFilterSource>(info.This())->GetValue();
20+
21+
Nan::Callback *callback = new Nan::Callback(v8::Local<Function>::Cast(info[0]));
22+
RepoWorker *worker = new RepoWorker(baton, callback);
23+
24+
worker->SaveToPersistent("src", info.This());
25+
26+
AsyncLibgit2QueueWorker(worker);
27+
return;
28+
}
29+
30+
void GitFilterSource::RepoWorker::Execute() {
31+
git_error_clear();
32+
33+
{
34+
LockMaster lockMaster(true, baton->src);
35+
36+
git_repository *repo = git_filter_source_repo(baton->src);
37+
baton->error_code = git_repository_open(&repo, git_repository_path(repo));
38+
39+
if (baton->error_code == GIT_OK) {
40+
baton->out = repo;
41+
} else if (git_error_last() != NULL) {
42+
baton->error = git_error_dup(git_error_last());
43+
}
44+
}
45+
}
46+
47+
void GitFilterSource::RepoWorker::HandleOKCallback() {
48+
if (baton->error_code == GIT_OK) {
49+
v8::Local<v8::Value> to;
50+
51+
if (baton->out != NULL) {
52+
to = GitRepository::New(baton->out, true);
53+
} else {
54+
to = Nan::Null();
55+
}
56+
57+
v8::Local<v8::Value> argv[2] = {Nan::Null(), to};
58+
callback->Call(2, argv, async_resource);
59+
} else {
60+
if (baton->error) {
61+
v8::Local<v8::Object> err;
62+
if (baton->error->message) {
63+
err = Nan::Error(baton->error->message)->ToObject();
64+
} else {
65+
err = Nan::Error("Method repo has thrown an error.")->ToObject();
66+
}
67+
err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code));
68+
err->Set(Nan::New("errorFunction").ToLocalChecked(),
69+
Nan::New("FilterSource.repo").ToLocalChecked());
70+
v8::Local<v8::Value> argv[1] = {err};
71+
callback->Call(1, argv, async_resource);
72+
if (baton->error->message)
73+
free((void *)baton->error->message);
74+
free((void *)baton->error);
75+
} else if (baton->error_code < 0) {
76+
v8::Local<v8::Object> err =
77+
Nan::Error("Method repo has thrown an error.")->ToObject();
78+
err->Set(Nan::New("errno").ToLocalChecked(),
79+
Nan::New(baton->error_code));
80+
err->Set(Nan::New("errorFunction").ToLocalChecked(),
81+
Nan::New("FilterSource.repo").ToLocalChecked());
82+
v8::Local<v8::Value> argv[1] = {err};
83+
callback->Call(1, argv, async_resource);
84+
} else {
85+
callback->Call(0, NULL, async_resource);
86+
}
87+
}
88+
89+
delete baton;
90+
}

‎test/tests/filter.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,4 +1076,62 @@ describe("Filter", function() {
10761076
});
10771077
});
10781078
});
1079+
1080+
describe("FilterSource", function() {
1081+
var message = "some new fancy filter";
1082+
1083+
before(function() {
1084+
var test = this;
1085+
return fse.readFile(readmePath, "utf8")
1086+
.then((function(content) {
1087+
test.originalReadmeContent = content;
1088+
}));
1089+
});
1090+
1091+
afterEach(function() {
1092+
this.timeout(15000);
1093+
return fse.writeFile(readmePath, this.originalReadmeContent);
1094+
});
1095+
1096+
it("a FilterSource has an async repo getter", function() {
1097+
var test = this;
1098+
1099+
return Registry.register(filterName, {
1100+
apply: function(to, from, source) {
1101+
return source.repo()
1102+
.then(function() {
1103+
return NodeGit.Error.CODE.PASSTHROUGH;
1104+
});
1105+
},
1106+
check: function(source) {
1107+
return source.repo()
1108+
.then(function() {
1109+
return NodeGit.Error.CODE.OK;
1110+
});
1111+
}
1112+
}, 0)
1113+
.then(function(result) {
1114+
assert.strictEqual(result, NodeGit.Error.CODE.OK);
1115+
})
1116+
.then(function() {
1117+
var readmeContent = fse.readFileSync(
1118+
packageJsonPath,
1119+
"utf-8"
1120+
);
1121+
assert.notStrictEqual(readmeContent, message);
1122+
1123+
return fse.writeFile(
1124+
packageJsonPath,
1125+
"Changing content to trigger checkout"
1126+
);
1127+
})
1128+
.then(function() {
1129+
var opts = {
1130+
checkoutStrategy: Checkout.STRATEGY.FORCE,
1131+
paths: "package.json"
1132+
};
1133+
return Checkout.head(test.repository, opts);
1134+
});
1135+
});
1136+
});
10791137
});

0 commit comments

Comments
 (0)