[PATCH] Support svn.ra.get_file_revs()

[Jelmer Vernooij <je...@samba.org>]

Hi,

[[[
Fix Python bindings for svn.ra.get_file_revs().

 * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.[ch]
   (svn_swig_py_ra_file_rev_handler_func): Add Python wrapper for 
    svn_ra_file_rev_handler_t.
 * subversion/bindings/swig/python/tests/ra.py
   (test_get_file_revs): Add test for svn.ra.get_file_revs()
]]]

Cheers,

Jelmer
-- 
Jelmer Vernooij <je...@samba.org> - http://samba.org/~jelmer/

Re: [PATCH] Support svn.ra.get_file_revs()

[David James <dj...@collab.net>]

On 6/18/06, Jelmer Vernooij <je...@samba.org> wrote:
> On Sun, 2006-06-18 at 01:25 -0700, David James wrote:
> > On 6/17/06, Jelmer Vernooij <je...@samba.org> wrote:
> > 2) In svn_swig_py_ra_file_rev_handler_func, we should pass in
> > py_prop_diffs to the callback function instead of the regular
> > "prop_diffs". (prop_diffs is not a Python object.)
> Well spotted, thanks.
>
> > Once these issues are fixed, this is ready to commit, I think.
> Fixed version is attached. Is this ok to commit?

Looks great! Thanks for adding the extra NULL checks to
svn_swig_py_ra_file_rev_handler_func. These checks make our code much
more robust.

A few more review comments:

1. If you add Py_None to an array, you should Py_INCREF it first.
Otherwise, the Py_None variable might be destroyed by the Python
garbage collector when your array is destroyed, causing some mayhem.
2. PyDict_New() can return NULL. We should check for this explicitly.
3. The new svn_swig_py_proparray_to_dict function is not declared in
swigutil_py.h (or used elsewhere in the Python bindings), so it should
be marked as 'static', and given an appropriate name. I renamed the
function to 'proparray_to_dict'.

I also have a few style suggestions:

4. In svn_swig_py_proparray_to_dict, py_key won't be NULL, because you
check for it explicitly. So you can use Py_DECREF here instead of
Py_XDECREF for clarity.
5. There's a few places where the braces are off in your patch, there
are trailing spaces, or the lines are longer than 80 chars.
6. It's nice to put some whitespace between the different files in
your log message.

Since the above comments are only minor, I fixed all of them and
committed in r20158.

Cheers,

David

P.S. NOTE: There are many places in the Python bindings where we don't
check whether the return value of PyList_New are NULL. These are all
segfault bugs which will be sensitized if you run out of memory.


-- 
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

Re: [PATCH] Support svn.ra.get_file_revs()

[Jelmer Vernooij <je...@samba.org>]

On Sun, 2006-06-18 at 01:25 -0700, David James wrote:
> On 6/17/06, Jelmer Vernooij <je...@samba.org> wrote:
> 2) In svn_swig_py_ra_file_rev_handler_func, we should pass in
> py_prop_diffs to the callback function instead of the regular
> "prop_diffs". (prop_diffs is not a Python object.)
Well spotted, thanks.

> Once these issues are fixed, this is ready to commit, I think.
Fixed version is attached. Is this ok to commit?

[[[
Fix Python bindings for svn.ra.get_file_revs().

 * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.[ch]
   (svn_swig_py_ra_file_rev_handler_func): Add Python wrapper for
    svn_ra_file_rev_handler_t.
   (svn_swig_py_proparray_to_dict): New function.
 * subversion/bindings/swig/svn_ra.i: Add typemap for 
    svn_ra_file_rev_handler_t.
 * subversion/bindings/swig/python/tests/ra.py
   (test_get_file_revs): Add test for svn.ra.get_file_revs().
]]]

Since Python has the ability to store entries with value None in
dictionaries that are different from nonexisting entries (unlike
apr_hash_t) I chose to use a dictionary rather than a list of
(name,value) tuples for prop_diffs.

Cheers,

Jelmer

-- 
Jelmer Vernooij <je...@samba.org> - http://samba.org/~jelmer/

Re: [PATCH] Support svn.ra.get_file_revs()

[David James <dj...@collab.net>]

On 6/17/06, Jelmer Vernooij <je...@samba.org> wrote:
> [[[
> Fix Python bindings for svn.ra.get_file_revs().
>
>  * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.[ch]
>    (svn_swig_py_ra_file_rev_handler_func): Add Python wrapper for
>     svn_ra_file_rev_handler_t.
>  * subversion/bindings/swig/python/tests/ra.py
>    (test_get_file_revs): Add test for svn.ra.get_file_revs()
> ]]]

Great work, Jelmer! I read through this patch carefully, and
everything looks quite good except for a few small details. I
especially appreciate your attention to detail in using Py_XDECREF for
py_rev_props and py_prop_diffs, which might be NULL.

Some details to fix:
1) We need to create a new function 'proparray_to_list' to convert an
array of svn_prop_t * items into a Python list. You used
svn_swig_py_array_to_list to do the job, but it won't work.
2) In svn_swig_py_ra_file_rev_handler_func, we should pass in
py_prop_diffs to the callback function instead of the regular
"prop_diffs". (prop_diffs is not a Python object.)
3) In ra.py, we should check the value of "prop_diffs" to make sure it
is correct.

Once these issues are fixed, this is ready to commit, I think.

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