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

Hi David,

David James wrote:
> On 5/25/06, Jelmer Vernooij <jelmer@samba.org> wrote:
>> The attached patch allows the callbacks in svn_ra_callbacks2_t() to be
>> implemented in Python. This is one of the things required in order to
>> get svn_ra_commit() working from Python.
>
> 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.

> You're converting SVN strings into SWIG pointers, here. Instead, we
> should convert svn strings into real Python strings using
> PyString_FromStringAndSize((void *)s->data, s->len). See
> convert_svn_string_t for an example.
Fixed.

> You're converting from SWIG "apr_file_t *" pointers into C "apr_file_t
> *" pointers, here. It's good that we can handle apr_file_t * pointers,
> but we should also be able to handle regular Python file objects. See
> svn_swig_py_make_file for some example code which works with Python
> file objects.
Fixed.

> This code converts Python "svn_string_t *" pointers into C
> "svn_string_t *" pointers. It'd be much better if we let users supply
> Python strings instead of pointers. You can convert from Python
> strings into "svn_string_t *" objects using the
> make_svn_string_from_ob function.
Fixed.

> If result == Py_None, there's a memory leak here. If you're going to
> throw away a reference, you always need to run Py_DECREF, unless
> result == NULL. Py_XDECREF will do the trick here. I noticed this
> memory leak in several places, but I only commented on it here.
Fixed.

> You're converting a SWIG auth baton pointer directly into a C auth
> baton here. I see you also implicitly increased the reference count of
> auth_baton when you called PyObject_GetAttrString. Oops. You should
> Py_DECREF(py_auth_baton) unconditionally so as to prevent memory
> leaks.
That should be ok, as the functions on svn.client that return auth_batons return opaque pointers. I've fixed the
reference.

> Also, do we need to check for "-1" specifically? Other code in
> swigutil_py.c checks for nonzero values instead.
Fixed.

Here's the summary again:

[[[

Allow callbacks in svn_ra_callbacks2_t() to be implemented in Python.

 * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c

   (svn_swig_py_setup_ra_callbacks): Fill in callback function pointers
   and auth_baton

   (ra_callbacks_open_tmp_file,
    ra_callbacks_get_wc_prop,
    ra_callbacks_set_wc_prop,
    ra_callbacks_push_wc_prop,
    ra_callbacks_invalid_wc_props,
    ra_callbacks_progress_funcs): Add wrappers for Python callbacks

   (make_ob_string): Add function
  
]]]

Cheers,

Jelmer