Expose the mempack backend in git2 #593
Conversation
|
The part I'm uneasy about here is the |
staktrace
force-pushed
the
mempack
branch
2 times, most recently
from
7ff7575 to
24dccb4
Compare
| /// Override the object database for this repository | ||
| pub fn set_odb(&self, odb: &Odb<'_>) -> Result<(), Error> { | ||
| unsafe { | ||
| try_call!(raw::git_repository_set_odb(self.raw(), odb.raw())); |
There was a problem hiding this comment.
Can you double check the memory management here? Is it safe to drop Odb after this call?
There was a problem hiding this comment.
So here, yes, it seems safe to drop Odb after this call. libgit2 internally does refcounting on the odb objects, and will only free the odb once it is not "owned" and the refcount drops to zero. Calling git_repository_set_odb will both mark the repo as the owner and increment the refcount, so if the rust code drops the ref it will continue to live until the repo itself gets destroyed or has a different odb installed.
| self.raw, | ||
| mempack.raw(), | ||
| priority as c_int | ||
| )); |
There was a problem hiding this comment.
Like below, can you check to see if it's safe to drop Mempack after this call?
There was a problem hiding this comment.
This one is a bit weird. It looks like dropping the ref on the rust side is safe, because that won't actually free the C struct. In fact, the only thing that frees the C struct is the odb itself, here. So what could happen is if the rust code creates a mempack, doesn't attach it to an odb, and then drops it, it will get leaked. In that sense maybe it would be better to only allow the mempack to be created from an Odb object, and have it automatically added to that Odb so that even if the rust side drops the ref, it won't get leaked.
There was a problem hiding this comment.
I guess the other problem is that as-is, a rust user could create a mempack, add it to an odb, and then keep a ref to the mempack even after the odb is destroyed. That would be bad, as it would be a dangling pointer to freed memory.
I'll rework this so that the mempack can only be created from an odb and the lifetime is more tightly scoped.
This also adds a method to override the ODB on the repository, which is also exposed by libgit2. This allows the user to create a new ODB, configure it, and install it on the repository.
|
Ok, I updated the patch to do a better job with the lifetime management. I also added a test that exercises the new code, and a |
|
Looks good to me, thanks! |
This also adds a method to override the ODB on the repository, which is also
exposed by libgit2. This allows the user to create a new ODB which e.g. just
has a mempack backend, and set it on the repository.