Re: [PATCH] Fill in callbacks for svn_ra_callbacks2_t()

On 6/11/06, Jelmer Vernooij <jelmer@vernstok.nl> wrote:
> > Could you update our test suite with a few new tests to make sure your
> > code works? For example, you could register a few callback functions
> > in Python and make sure they get called with appropriate arguments.
> ra_local doesn't actually call these callbacks, which makes it very hard
> to write tests using the file:// RA backend. Apparently ra_dav uses them
> most often, but there are no Python tests that use it. I have used them
> successfully here using other protocols though.

Ah, OK. Jelmer, could you provide some example test scripts which use
these functions so that I can test them myself?

At some point, we should add a 'dav-autocheck-swig-py' target, similar
to the regular dav-autocheck target, which checks the 'swig-py'
bindings instead. Madan, would you be interested in working on this?

> > Thanks for your work on the Python bindings, Jelmer! If you can fix
> > these issues, and write some tests, I'll be able to commit your patch.
> Fixed.

Thanks for the fixes, Jelmer! Your new patch looks quite good. I tried
to commit it but I found a few more issues which I'd like us to fix
first. These fixes are fairly straightforward.

1. In all new functions: Use Py_XDECREF instead of Py_DECREF.
Py_XDECREF handles NULL values safely. Only use Py_DECREF if you are
absolutely sure that the variable in question will not be NULL.

2. In ra_callbacks_set_wc_prop and ra_callbacks_push_wc_prop: If
PyString_FromStringAndSize returns NULL, we should return an
appropriate exception to Python.

3. In svn_swig_py_setup_ra_callbacks: If PyObject_GetAttrString
returns NULL, we should return an appropriate exception to Python.

I've also noticed a complex scoping issue which results from your
patch. In svn_swig_py_setup_ra_callbacks, we should ideally tie the
scope of 'py_auth_baton' and 'py_callbacks' to the RA session returned
by svn_ra_open. We need to tie the scope of these two objects together
so that the Python garbage collector will not destroy these objects
while Subversion still needs them.

1. In the typemap for svn_ra_callbacks2_t in svn_ra.i, pass the
current value of _global_svn_swig_py_pool into
svn_swig_py_setup_ra_callbacks. By doing this, you tell
svn_swig_py_setup_ra_callbacks which pool you are using to initialize
the ra session. (Tricky note: The SWIG-generated wrapper for
svn_ra_open defines a local variable named _global_svn_swig_py_pool
which shadows the global variable of the same name. This local
variable can only be accessed within SWIG typemaps, and cannot be
accessed directly from swigutil_py.c.)

2. From svn_swig_py_setup_ra_callbacks, mark py_callbacks and
py_auth_baton as owned by _global_svn_swig_py_pool. You can do this by
calling PyObject_CallMethod(py_pool, addOwnedRef, objectTuple,
py_callbacks) and PyObject_CallMethod(py_pool, addOwnedRef,
objectTuple, py_auth_baton). You don't need to worry about calling
removeOwnedRef, like Walter did, because you are not replacing
anything in the session -- you are creating a brand new session
object.

3. If the calls to PyObject_CallMethod fail, don't forget to return an
appropriate exception to Python. Otherwise, all is good.

Thanks for your great work, Jelmer! If you can fix the simple issues
in your next patch, and provide a few example test scripts, I think it
will be ready to commit. If you could fix the complex scoping issues
too, that would be great, but I can still commit your patch without
fixes for them.

Cheers,

David

-- 
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org