Re: [PATCH] Fix python bindings for svn_client_info()

On 4/20/06, C. Michael Pilato <cmpilato@collab.net> wrote:
> Jelmer Vernooij wrote:
> > The attached patch fixes the use of the svn.client.info() function in
> > the Python bindings. Patch against trunk.

Thanks for the patch, Jelmer! I've committed this patch in r19413 with
a few improvements. Most importantly, I fixed a few memory management
problems, and added test cases to verify this. Details below.

> > [[[
> > * subversion/bindings/swig/svn_client.i: Add typemap for svn_info_receiver_t in Python
> >
> > * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> > (svn_swig_py_info_receiver): Add wrapper function for
> > svn_info_receiver_t that calls a Python function
> >
> > * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:
> > Add prototype for svn_info_receiver_t
> > ]]]
>
> Nice, clean log message. Lacks a brief summary (such as "Add support for
> svn_client_info() to the Python bindings") and an 80-column fit (solution is
> something like s/svn_client.i: /svn_client.i\n /)

Fixed. Here's the log message from r19413:

[[[
Update Python bindings to support svn_client_info. Add tests to verify that
svn_client_info works as expected.

Patch by: Jelmer Vernooij <jelmer@samba.org>
          me

* build.conf
  Update libsvn_swig_py to depend on libsvn_client.

[ In subversion/bindings/swig ]

* svn_client.i
  Add Python typemap for svn_info_receiver_t.

* python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
  (svn_swig_py_info_receiver_func): New function. Python function wrapper for
  svn_info_receiver_t.

* python/tests/client.py
  Add file. Contains new test case for svn_client library and svn_client_info
  function.

* python/tests/run_all.py
  (suite): Add new client test to list of tests.

]]]

> > +svn_error_t *svn_swig_py_info_receiver(void *baton,
> > + const char *path,
> > + const svn_info_t *info,
> > + apr_pool_t *pool)
>
> Ick. Tabstobs. Badness. (This problem is already present in the new
> header file prototype.)

Fixed.

>
> > +{
> > + PyObject *receiver = baton;
> > + PyObject *result, *py_pool, *py_info;
> > + svn_error_t *err = SVN_NO_ERROR;
> > +
> > + if ((receiver == NULL) || (receiver == Py_None))
> > + return SVN_NO_ERROR;
> >
> > + svn_swig_py_acquire_py_lock();
> > +
> > + py_pool = make_ob_pool(pool);
> > + if (py_pool == NULL) {
> > + err = callback_exception_error();
> > + goto finished;
> > + }
> > +
> > + py_info = svn_swig_NewPointerObjString(info, "svn_info_t *", py_pool);
> > + if (py_info == NULL) {
> > + Py_DECREF(py_pool);
> > + err = callback_exception_error();
> > + goto finished;
> > + }
> > +
> > + if ((result = PyObject_CallFunction(receiver,
> > + (char *)"sOO",
> > + path, py_info,
> > + py_pool)) == NULL)
> > + {
> > + err = callback_exception_error();
> > + }
>
> I expected this implementation to be more like the support for
> svn_client_status. So, add:
>
> DECLARE_SWIG_CONSTRUCTOR(info, svn_info_dup)
>
> And then instead of all the stuff above, just do:
>
> if ((result = PyObject_CallFunction(receiver,
> (char *)"sO&",
> path, make_ob_info,
> info)) == NULL)
> {
> err = callback_exception_error();
> }

Yes, it's better to use DECLARE_SWIG_CONSTRUCTOR. This technique helps
prevent memory management bugs and is also more terse. The standard
constructors, generated by DECLARE_SWIG_CONSTRUCTOR, copy provided
objects into a brand new pool so that they can be fully managed by
Python, and will still be valid after the callback exits. I used this
technique in r19413.

See http://svn.collab.net/repos/svn/trunk/subversion/bindings/swig/python/tests/client.py
for a test case.

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