Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PackBuilder methods #1048

Merged
merged 9 commits into from
Nov 17, 2020
Merged

Conversation

jeremywestwood
Copy link
Contributor

A (very much WIP) attempt to add support for the Packbuilder methods from Libgit2 to create .pack and .idx files for a repository.

I just wanted to get initial input on whether the approach taken here is correct, or if I should be structuring it a different way?

I based the overall strategy on what was done for Libgit2Sharp: libgit2/libgit2sharp#1183

TODOs:

  • Handle the "owned" parameter correctly (if its needed?)
  • Write tests

Once I've got some feedback and done the TODOs then I'll write up a proper pull request.

@jeremywestwood jeremywestwood changed the title WIP: Add support PackBuilder methods WIP: Add support for PackBuilder methods Nov 13, 2020
@jdavid
Copy link
Member

jdavid commented Nov 14, 2020

I don't think you need the owned parameter. Also I think you could do this only with cffi, and avoid the C code in src/, it should be much simpler; you can look at pygit2/index.py for an example of a feature (Index) done entirely in cffi.
Thanks

@jdavid
Copy link
Member

jdavid commented Nov 16, 2020

Looks good. Just don't see _from_c used anywhere, maybe you don't need it. I'll do a more detailed review when the tests will be available.

@jeremywestwood jeremywestwood changed the title WIP: Add support for PackBuilder methods Add support for PackBuilder methods Nov 16, 2020
@jeremywestwood jeremywestwood marked this pull request as ready for review November 16, 2020 13:15
@jeremywestwood
Copy link
Contributor Author

jeremywestwood commented Nov 16, 2020

Thanks for the comments. I've updated based on them and added the tests.

Please let me know any other feedback.

EDIT: I see some of my tests are not working on windows, am investigating
UPDATE: Should be resolved now

Copy link
Member

@jdavid jdavid left a comment

Choose a reason for hiding this comment

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

Looks good, just made a few comments.

Comment on lines 74 to 75
def write(self, path):
err = C.git_packbuilder_write(self._packbuilder, to_bytes(path), 0, ffi.NULL, ffi.NULL)
Copy link
Member

Choose a reason for hiding this comment

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

git_packbuilder_write supports NULL for default location, so we should support None here:

def write(self, path=None):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated to support NULL

@@ -83,6 +84,39 @@ def write(self, *args, **kwargs):
object."""
return self.odb.write(*args, **kwargs)

def pack(self, path, pack_delegate=None, max_threads=None):
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, path=None should be supported

pack_delegate = pack_delegate or pack_all_objects

builder = PackBuilder(self)
if max_threads:
Copy link
Member

Choose a reason for hiding this comment

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

libgit2 docs say: when set to 0, libgit2 will autodetect the number of CPUs

But here if the user passes zero set_max_threads won't be called. Instead the test should be:

if max_threads is not None:

Please update the documentation string as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have updated to support 0 and renamed the method and argument

err = C.git_packbuilder_insert_recur(self._packbuilder, oid, ffi.NULL)
check_error(err)

def set_max_threads(self, n_threads):
Copy link
Member

Choose a reason for hiding this comment

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

Docs for git_packbuilder_set_threads says Set number of threads to spawn

It doesn't say anything about maximum, so please rename this method to set_threads

Comment on lines 56 to 67
def convert_object_to_oid(git_object):
oid = ffi.new('git_oid *')
ffi.buffer(oid)[:] = git_object.raw[:]
return oid

def add(self, git_object):
oid = self.convert_object_to_oid(git_object)
err = C.git_packbuilder_insert(self._packbuilder, oid, ffi.NULL)
check_error(err)

def add_recur(self, git_object):
oid = self.convert_object_to_oid(git_object)
Copy link
Member

Choose a reason for hiding this comment

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

You are not expecting a Git object here, but an Object id, so the argument git_object is misleading. I would call it oid but then you had to rename the local variable with the same name, maybe git_oid?

def convert_object_to_oid(oid):
    git_oid = ffi.new('git_oid *')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed it here and the other methods which call it

@jdavid jdavid merged commit 5d775fb into libgit2:master Nov 17, 2020
@jdavid
Copy link
Member

jdavid commented Nov 17, 2020

Merged, thanks!

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