Implement remotes via CFFI #360
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This moves enough code into python with CFFI to pass the test_remotes unit tests. There is no credentials support yet. There is a small change in the return value of Remote.fetch() in that we now return a TransferProgress object instead of extracting a few values into a dictionary.
Now that it has the features of the old implementation, let's add documentation on how to use it.
This code has been obsoleted by the CFFI-using code. Some credentials code remains in C due to the clone functionality making use of it.
This lets us get rid of the last piece of C for anything related to remotes and credentials.
Pass the contents of the buffer containing the git_oid to bytes() so build a raw representation of its contents. Using bytes(ffi.buffer(...)) returns the representation of the buffer.
Let both pip and Travis know what we need.
The documentation recommends adding the ffi code as an extension so it gets built at the right time. Make use of the LIBGIT2 environment variable to build and link the ffi module the same way the C extension does so the user doesn't have to export CFLAGS.
Casting a pointer to a non-pointer type is something which you should never do. Instead, do something a bit more convoluted, but which is guaranteed to give us the right pointer, as well as making sure that the memory we exchange between python/cffi and the C extension is of the right pointer size. While we're touching this code, fix which object we pass to the Remote constructor to keep alive. We need to pass the Repository object to stop it from becoming unreferenced (and thus freeing memory the remote needs) instead of the git_repository pointer.
Make to_str() accept None as well as ffi.NULL to return as a negative value, and grab the version in a more compatible way.
The C API expects a non-NULL new name and raises an assertion if we don't protect against such input, so let's guard against falsy values, which also takes care of the empty string, which is also not valid input.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the cffi module allows us to use python instead of C, which reduces the line count and boilerplate (not to mention python > C in general), let's start integrating this library by using it for the remotes. Remotes are IO-bound operations and essentially all of the work is done by libgit2, so the reduction in performance which can be seen in the revwalk PoC would be minimal here.
Unfortunately due to the need to copy the C declarations, which are provided by libgit2's headers for the C extension, the reduction in line count isn't as large as I would have hoped, but consider that there's ~300 lines which don't really add anything, and there's even extra documentation for the callbacks. Adding more functionality will be able to re-use a bunch of these declarations, so we should see even better results there.
As cffi cannot carry exceptions across the C layer, the re-raised exception would now have a wrong stacktrace. I'm not sure if there is a way to re-raise an exception in a way that keeps the old stack trace.EDIT: there is a way to raise an exception with an old stack trace, but the python 2 version (three-argument raise) causes a syntax error in python 3 even if we're never going to call the code.
I've reworked some of the common code from the PoC so that we keep compatibility with Python 2.6 as it didn't take much effort.
This PR does introduce a slight change.
Remote.fetch()now returns aTransferProgressobject instead of a dict with a few values from said struct. AFAIU thefetch()method came before we exposed TransferProgress, which is why it uses its own type. We would want to make this change at some point soon anyway, but I can switch to the one in master if we want to wait for a non-patch release to change that.