Skip to content

Check parameters before performing reset#1603

Merged
implausible merged 1 commit into
nodegit:masterfrom
rcjsuen:reset-check
Dec 20, 2018
Merged

Check parameters before performing reset#1603
implausible merged 1 commit into
nodegit:masterfrom
rcjsuen:reset-check

Conversation

@rcjsuen

@rcjsuen rcjsuen commented Dec 18, 2018

Copy link
Copy Markdown
Member

If a promise is passed down to the native C++ layer, v8 will crash. To ensure that this does not happen, a check will be performed in the JavaScript layer to verify that the repository is the same as the repository of the commit object.

While logically the commit of one repository instance is the same as a commit of another repository instance (if they point to the same .git folder), programmatically they are not equivalent as a check is done
on libgit2's side to ensure that they come from the same thing. As a result, the check on the JavaScript side purely compares the pointers over some other logical piece of information (such as its path or working directory).

This is not a general fix for #1127 but is simply meant to help the case of #1596. I have not put much thought into it but perhaps the generator could be changed so that at least Repository and Commit objects could be verified?

If a promise is passed down to the native C++ layer, v8 will crash. To
ensure that this does not happen, a check will be performed in the
JavaScript layer to verify that the repository is the same as the
repository of the commit object.

While logically the commit of one repository instance is the same as a
commit of another repository instance (if they point to the same .git
folder), programmatically they are not equivalent as a check is done
on libgit2's side to ensure that they come from the same thing. As a
result, the check on the JavaScript side purely compares the pointers
over some other logical piece of information (such as its path or
working directory).

Signed-off-by: Remy Suen <remy.suen@gmail.com>

@implausible implausible left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rcjsuen

@implausible implausible merged commit 7e64250 into nodegit:master Dec 20, 2018
@rcjsuen rcjsuen deleted the reset-check branch December 21, 2018 11:42
@rcjsuen

rcjsuen commented Dec 21, 2018

Copy link
Copy Markdown
Member Author

Thanks for reviewing and merging, @implausible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants