Makes sense to me as a way to get access to all the things that can be
returned in an errorful situation.
On Wed, Jul 29, 2020 at 7:17 AM Yasuhito FUTATSUKI <futat...@yf.bsdclub.org>
wrote:
> 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 <futat...@yf.bsclub.org>
>
