From 8f911102f347e4e686de30ca4f0f5098fd0bba50 Mon Sep 17 00:00:00 2001 From: Remy Suen Date: Tue, 31 Jan 2017 19:11:52 +0900 Subject: [PATCH 1/2] Expose libgit2 error code to clients when a promise fails When an API function of libgit2 fails within a promise, the promise is rejected with a JavaScript Error object that wraps the recorded error message in giterr_last(). However, the original non-zero return code of the API function is not exposed to the caller of the NodeGit API. This means that NodeGit clients cannot easily identify what the cause of an error is outside of parsing the Error object's message. This is an extremely volatile way of determining the cause as libgit2 may choose to alter the wording of the message at any time. To solve the problem above, the returned JavaScript Error object now esposes the libgit2 error code via its `errno` property. --- .../manual/patches/convenient_patches.cc | 9 +++++++- .../templates/manual/revwalk/fast_walk.cc | 9 +++++++- .../manual/revwalk/file_history_walk.cc | 9 +++++++- generate/templates/partials/async_function.cc | 9 +++++++- test/tests/merge.js | 22 +++++++++++++++++++ 5 files changed, 54 insertions(+), 4 deletions(-) diff --git a/generate/templates/manual/patches/convenient_patches.cc b/generate/templates/manual/patches/convenient_patches.cc index 79823e37a..27de54fcf 100644 --- a/generate/templates/manual/patches/convenient_patches.cc +++ b/generate/templates/manual/patches/convenient_patches.cc @@ -95,8 +95,15 @@ void GitPatch::ConvenientFromDiffWorker::HandleOKCallback() { } if (baton->error) { + Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method convenientFromDiff has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); Local argv[1] = { - Nan::Error(baton->error->message) + err }; callback->Call(1, argv); if (baton->error->message) diff --git a/generate/templates/manual/revwalk/fast_walk.cc b/generate/templates/manual/revwalk/fast_walk.cc index 2f263f83f..ec3a10cd7 100644 --- a/generate/templates/manual/revwalk/fast_walk.cc +++ b/generate/templates/manual/revwalk/fast_walk.cc @@ -88,8 +88,15 @@ void GitRevwalk::FastWalkWorker::HandleOKCallback() { if (baton->error) { + Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method fastWalk has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); Local argv[1] = { - Nan::Error(baton->error->message) + err }; callback->Call(1, argv); if (baton->error->message) diff --git a/generate/templates/manual/revwalk/file_history_walk.cc b/generate/templates/manual/revwalk/file_history_walk.cc index 0f511969f..220482679 100644 --- a/generate/templates/manual/revwalk/file_history_walk.cc +++ b/generate/templates/manual/revwalk/file_history_walk.cc @@ -289,8 +289,15 @@ void GitRevwalk::FileHistoryWalkWorker::HandleOKCallback() } if (baton->error) { + Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method fileHistoryWalk has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); Local argv[1] = { - Nan::Error(baton->error->message) + err }; callback->Call(1, argv); if (baton->error->message) diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index 3d3227d4a..e47e5b77b 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -155,8 +155,15 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { callback->Call(2, argv); } else { if (baton->error) { + Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method {{ jsFunctionName }} has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); Local argv[1] = { - Nan::Error(baton->error->message) + err }; callback->Call(1, argv); if (baton->error->message) diff --git a/test/tests/merge.js b/test/tests/merge.js index 3a63b0ced..05a719d20 100644 --- a/test/tests/merge.js +++ b/test/tests/merge.js @@ -1298,4 +1298,26 @@ describe("Merge", function() { assert.ok(repository.isDefaultState()); }); }); + + it("can retrieve error code on if common merge base not found", function() { + var repo; + return NodeGit.Repository.open(local("../repos/workdir")) + .then(function(r) { + repo = r; + return repo.getCommit("4bd806114ce26503c103c85dcc985021951bbc18"); + }) + .then(function(commit) { + return commit.getParents(commit.parentcount()); + }) + .then(function(parents) { + return NodeGit.Merge.base(repo, parents[0], parents[1]) + .then(function() { + return Promise.reject(new Error( + "should not be able to retrieve common merge base")); + }, function(err) { + assert.equal("No merge base found", err.message); + assert.equal(NodeGit.Error.CODE.ENOTFOUND, err.errno); + }); + }); + }); }); From b40ee03663c05f314669fbefe056675b9e6ba94a Mon Sep 17 00:00:00 2001 From: Remy Suen Date: Sun, 26 Feb 2017 10:58:12 +0900 Subject: [PATCH 2/2] Fix error message assertion to match libgit2 changes libgit2 has changed all their error messages to start with a lowercased character. Updated the test to reflect this change. --- test/tests/merge.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tests/merge.js b/test/tests/merge.js index 05a719d20..85e659166 100644 --- a/test/tests/merge.js +++ b/test/tests/merge.js @@ -1315,7 +1315,7 @@ describe("Merge", function() { return Promise.reject(new Error( "should not be able to retrieve common merge base")); }, function(err) { - assert.equal("No merge base found", err.message); + assert.equal("no merge base found", err.message); assert.equal(NodeGit.Error.CODE.ENOTFOUND, err.errno); }); });