Re: Python bindings API confusion

On 2020/07/29 7:45, Yasuhito FUTATSUKI wrote:
> On 2020/07/29 1:02, Yasuhito FUTATSUKI wrote:
>> On 2020/07/28 23:34, C. Michael Pilato wrote:
>>> Hey, all.
>>>
>>> I'm writing some code that performs commits via the Subversion Python
>>> bindings, and I'm struggling to understand some things I see there.
>>>
>>> In the svn_fs.i interface file, there's this block of code:
>>>
>>> /* -----------------------------------------------------------------------
>>> Fix the return value for svn_fs_commit_txn(). If the conflict result is
>>> NULL, then %append_output() is passed Py_None, but that goofs up
>>> because that is *also* the marker for "I haven't started assembling a
>>> multi-valued return yet" which means the second return value (new_rev)
>>> will not cause a 2-tuple to be manufactured.
>>>
>>> The answer is to explicitly create a 2-tuple return value.
>>>
>>> FIXME: Do the Perl and Ruby bindings need to do something similar?
>>> */
>>> #ifdef SWIGPYTHON
>>> %typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
>>> /* this is always Py_None */
>>> Py_DECREF($result);
>>> /* build the result tuple */
>>> $result = Py_BuildValue("zi", *$1, (long)*$2);
>>> }
>>> #endif
>>
>> Ah, it returns a tuple of str and int, not a tuple of bytes and int.
>
> I made a patch addressing to it, as attached
> swig-py-fix-svn_fs_commit-patch.txt.
> (Only affect on Python 3, no function change on Python 2.7)
>
>>> This reads and claims to behave exactly as I'd expect, given the dual
>>> return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which wraps
>>> it). And since this interface file is included from svn_repos.i, I would
>>> expect those typemaps to apply also to svn_repos_fs_commit_txn, which has
>>> matching parameter types and names.
>>
>> It seems it isn't match because svn_repos_fs_commit_txn have extra
>> argument "svn_repos_t *repos" between them.
>>
>>> But this isn't how the code appears to work in practice. A successful
>>> commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just the
>>> newly created revision number. Moreover, if my commit succeeds but the
>>> post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
>>> masking the return of the newly created revision number altogether. Now,
>>> the masked revision number thing I can understand -- it's hard to do in
>>> Python what we promise to do in C (which is to return an svn_error_t but
>>> still set the new_rev return value). But the 2-tuple thing I have no
>>> explanation for. Any ideas?
>>
>> I think it is need one more typemap for it in svn_repos.i like
>>
>> [[[
>> #ifdef SWIGPYTHON
>> %typemap(argout) (const char **conflict_p, svn_repos_t *repos, svn_revnum_t *new_rev) {
>> /* this is always Py_None */
>> Py_DECREF($result);
>> /* build the result tuple */
>> $result = Py_BuildValue("zi", *$1, (long)*$3);
>> }
>> #endif
>> ]]]
>
> Also I made a patch for it, based on fixed typemap for svn_fs_commit_txn,
> as attached swig-py-fix-svn_repos_fs_commit-patch.txt
>
> Difference on SWIG generated subversion/bindings/swig/python/svn_repos.c
> between before and after this patch is below. (SWIG 3.0.12, for Python 3)
> [[[
> --- subversion/bindings/swig/python/svn_repos.c.before_patch 2020-07-29 05:45:35.626521000 +0900
> +++ subversion/bindings/swig/python/svn_repos.c 2020-07-29 06:41:51.220682000 +0900
> @@ -12126,23 +12126,16 @@
> resultobj = Py_None;
> }
> {
> - PyObject *s;
> - if (*arg1 == NULL) {
> - Py_INCREF(Py_None);
> - s = Py_None;
> - }
> - else {
> - s = PyBytes_FromString(*arg1);
> - if (s == NULL)
> - SWIG_fail;
> - }
> - resultobj = SWIG_Python_AppendOutput(resultobj, s);
> - }
> - if (SWIG_IsTmpObj(res3)) {
> - resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_long((*arg3)));
> - } else {
> - int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN | 0 ) : 0 ;
> - resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_long, new_flags));
> + /* this is always Py_None */
> + Py_DECREF(resultobj);
> + /* build the result tuple */
> + resultobj = Py_BuildValue(
> + #if PY_VERSION_HEX >= 0x03000000
> + "yi",
> + #else
> + "zi",
> + #endif
> + *arg1, (long)*arg3);
> }
> {
> Py_XDECREF(_global_py_pool);
> ]]]
>
> I couldn't make test cases for them, so I didn't test, yet.

I looked over svn_fs_commit_txn() C API, then I reconsidered how the
Python wapper function should be.
 
If svn_fs_commit_txn() returns NULL (i.e. the Python wrapper function
can return without exception), the *conflict_p is always NULL.
So I think it is no mean except compatibility that Python wrapper
returns it as None in a tuple.

It is better that when an exception is occur, the wrapper API returns
conflict_p and new_rev as attributes of SubversionException object,
if we can make it so.

Any thoughts?

Cheers,

-- 
Yasuhito FUTATSUKI <futatuki_at_yf.bsclub.org>