[PATCH 4 of 6] revlog: add public CPython function to get parent revisions

[Georges Racinet]

On 12/9/18 6:02 AM, Yuya Nishihara wrote:
> On Wed, 05 Dec 2018 22:43:35 +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya at tcha.org>
>> # Date 1543756237 -32400
>> #      Sun Dec 02 22:10:37 2018 +0900
>> # Node ID 716a73bab79be30c20c75e13324c44205d5e2120
>> # Parent  3842abba948cd7f4bb3fad6805265a35fb94a083
>> revlog: add public CPython function to get parent revisions
>> +/*
>> + * Get parents of the given rev.
>> + *
>> + * If the specified rev is out of range, IndexError will be raised. If the
>> + * revlog entry is corrupted, ValueError may be raised.
>> + *
>> + * Returns 0 on success or -1 on failure.
>> + */
>> +int HgRevlogIndex_GetParents(PyObject *op, int rev, int *ps)
> This is based on the idea that the Rust module will be statically linked with
> the cext objects. I thought that would be easier for our use case, package-local
> visibility, but I'm not certain. If that makes things complicated, maybe we
> should choose dynamic linking and wrap the function with PyCapsule, as you did
> in the PoC code.

Yes, it's true in the direct-ffi code, I passed the function pointer
around because I was wary of a loop in dependencies (that's a bit silly
since we can't avoid linking the Rust extension within the parsers
module anyway), but also I didn't want to touch the existing C code too
much for my first patch set. This version you're proposing feels simpler
to me.

Using a capsule in that context wouldn't be much complicated either, all
we'd need in hg-direct-ffi would be to declare and call PyCapsule_Import
(it's obviously designed to use very few CPython API concepts, only
needs a char * for the name). The advantage it'd have then would be that
the same capsule could be used for all types of FFI, and we could do the
same for other functions we could need later on (maybe in other
packages). Adding a new capsule seems less risky for people like me, who
aren't as familiar with mercurial's C code as you are or, if you prefer
to see it that way, will require less review.

To be more explicit for other mailing-list subscribers, here's the
change in revlog.c I'm doing in my PoC CPython code (this is inside the
module init function) :

@@ -2846,6 +2846,12 @@
        if (nullentry)
                PyObject_GC_UnTrack(nullentry);
 
+       void *caps = PyCapsule_New(
+           index_get_parents,
"mercurial.cext.parsers.index_get_parents_CAPI",
+           NULL);
+       if (caps != NULL)
+               PyModule_AddObject(mod, "index_get_parents_CAPI", caps);
+
 #ifdef WITH_RUST
        rustlazyancestorsType.tp_new = PyType_GenericNew;
        if (PyType_Ready(&rustlazyancestorsType) < 0)

So, to summarize, I think we should maybe promote capsules as the
preferred way to interface Rust code with inner C code. It wouldn't be
hard to document either. What do you think ?

And yes, I should submit formally those rust-cpython bindings sooner
than later.

Regards,


-- 
Georges Racinet
Anybox SAS, http://anybox.fr
Téléphone: +33 6 51 32 07 27
GPG: B59E 22AB B842 CAED 77F7 7A7F C34F A519 33AB 0A35, sur serveurs publics