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

[Yuya Nishihara]

On Mon, 10 Dec 2018 18:54:56 +0100, Georges Racinet wrote:
> 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 ?

My only concern about using PyCapsule is, we would have to get around some
"static mut" variables in Rust if the cost of the name resolution matters.
That's what I noticed while reading your rust/hg-cpython code.

To be clear, I'm not against using PyCapsule. It's documented as the "right"
way to interface C-level APIs across modules. I wrote this patch before
reading your upcoming series. That's the only reason I made the GetParents()
function public.