Python bindings API confusion

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Python bindings API confusion

C. Michael Pilato-2
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


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.

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?

-- Mike
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
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.

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

but I didn't tried yet.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
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.


Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>

swig-py-fix-svn_fs_commit_txn-patch.txt (1K) Download Attachment
swig-py-fix-svn_repos_fs_commit_txn-patch.txt (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

C. Michael Pilato-2
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 <[hidden email]> 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 <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
On 2020/07/29 23:43, C. Michael Pilato wrote:
> Makes sense to me as a way to get access to all the things that can be
> returned in an errorful situation.

Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
attributes to SubversionException instance on svn_fs.commit_txn and
svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
tuple of (None, int), for a consistency.

I've tested this method on other API with dummy attribute, but I didn't
test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.

Could you please review and try it ?

Thanks, --
Yasuhito FUTATSUKI <[hidden email]>

swig-py-allow-exception-attrs-patch.txt (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:

> On 2020/07/29 23:43, C. Michael Pilato wrote:
>> Makes sense to me as a way to get access to all the things that can be
>> returned in an errorful situation.
>
> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
> attributes to SubversionException instance on svn_fs.commit_txn and
> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
> tuple of (None, int), for a consistency.
>
> I've tested this method on other API with dummy attribute, but I didn't
> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>
> Could you please review and try it ?
>
> Thanks, --
> Yasuhito FUTATSUKI <[hidden email]>

Ah, I sent the patch before including a commit message. Here it is.
[[[
swig-py: Allow SubversionException to add attributes

[in subversion/bindings/swig/]

* include/svn_types.swg
  (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
  distinct those C APIs which return value by using pointer args when
  error is occured.
  (typemap(out) svn_error_t_with_attrs *): New typemap.

* python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
  (svn_swig_py_build_svn_exception): New function.
  (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
  get the exception class and to make the instance of it from subversion
  error.

* svn_fs.i
  (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
  - Add attribute "conflict_p" and "new_rev" to SubversionException
    instance when the exception is occured.
  - Fix the conversion format to build return value on Python 3
    "z" conversion makes str when conflict_p is not NULL, although
    it is always NULL in this context. So this is foolproof.
  (svn_fs_commit_txn):
    New fake function declaration for SWIG to make a wrapper. Ignore
    the real one.

* svn_repos.i
  (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
   svn_revnum_t *new_rev)):
    New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
    and modified argment number.
  (svn_repos_fs_commit_txn):
    New fake function declaration for SWIG to make a wrapper. Ignore
    the real one.

Found by: cmpilato
]]]
 

Thanks,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

C. Michael Pilato-2
The patch seems to work pretty well (for svn_repos_fs_commit_txn), but there are two issues I have with it.

First, unless I'm mistaken, there's no way to get the conflict_p back as part of the 2-tuple return value -- if there's a conflict, svn.repos.fs_commit_txn() is going to raise an Exception.  So we're basically returning a 2-tuple whose first value is always None.  We might as well preserve the current behavior and return the newly committed revision as a singleton.  (Which also means we don't break compatibility with prior versions.)

Also, I don't love that we've used "conflict_p" and "new_rev" as the variables names within the SubversionException.  Anything we do here will be non-obvious and require documentation, so I'd rather give these things meaningful names ("conflict_path" and "new_revision").  But as a concept, I'd say this part works well.

Does this change anything about the svn_fs_commit_txn() interface?

-- Mike

On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <[hidden email]> wrote:
On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
> On 2020/07/29 23:43, C. Michael Pilato wrote:
>> Makes sense to me as a way to get access to all the things that can be
>> returned in an errorful situation.
>
> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
> attributes to SubversionException instance on svn_fs.commit_txn and
> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
> tuple of (None, int), for a consistency.
>
> I've tested this method on other API with dummy attribute, but I didn't
> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>
> Could you please review and try it ?
>
> Thanks, --
> Yasuhito FUTATSUKI <[hidden email]>

Ah, I sent the patch before including a commit message. Here it is.
[[[
swig-py: Allow SubversionException to add attributes

[in subversion/bindings/swig/]

* include/svn_types.swg
  (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
  distinct those C APIs which return value by using pointer args when
  error is occured.
  (typemap(out) svn_error_t_with_attrs *): New typemap.

* python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
  (svn_swig_py_build_svn_exception): New function.
  (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
  get the exception class and to make the instance of it from subversion
  error.

* svn_fs.i
  (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
  - Add attribute "conflict_p" and "new_rev" to SubversionException
    instance when the exception is occured.
  - Fix the conversion format to build return value on Python 3
    "z" conversion makes str when conflict_p is not NULL, although
    it is always NULL in this context. So this is foolproof.
  (svn_fs_commit_txn):
    New fake function declaration for SWIG to make a wrapper. Ignore
    the real one.

* svn_repos.i
  (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
   svn_revnum_t *new_rev)):
    New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
    and modified argment number.
  (svn_repos_fs_commit_txn):
    New fake function declaration for SWIG to make a wrapper. Ignore
    the real one.

Found by: cmpilato
]]]


Thanks,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Yasuhito FUTATSUKI-2
On 2020/07/30 22:20, C. Michael Pilato wrote:

> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
> there are two issues I have with it.
>
> First, unless I'm mistaken, there's no way to get the conflict_p back as
> part of the 2-tuple return value -- if there's a conflict,
> svn.repos.fs_commit_txn() is going to raise an Exception.  So we're
> basically returning a 2-tuple whose first value is always None.  We might
> as well preserve the current behavior and return the newly committed
> revision as a singleton.  (Which also means we don't break compatibility
> with prior versions.)

How do you think about this issue on svn.fs.commit_txn()? It is also
always (None, new_rev) if it doesn't raise Exception, since r845217,
Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
however, I took priority of compatibility. (Though I think no one
ever used this Python wrapper function in practical program....)

Putting aside this compatibility issue, my preference is:

if error is not occured:
  if *conflict_p is NULL:
     return *new_rev as a singletone
  else:
     # a foolproof. we can detect an internal error by doing this.
     return tupple (*conflict_p, *new_rev)
else:
  # error is occred
  raise exception with values of *conflict_p and *new_rev in the exception

So I prefer to make this change and announce it. Note that we have
an option that we'll change behavor only in Python 3 with preserve
the behavor in Python 2.

> Also, I don't love that we've used "conflict_p" and "new_rev" as the
> variables names within the SubversionException.  Anything we do here will
> be non-obvious and require documentation, so I'd rather give these things
> meaningful names ("conflict_path" and "new_revision").  But as a concept,
> I'd say this part works well.

Using argment names of C API have a merit that if we will implement such
special wrappers requires attirbutes of SubversionException, we just say
implement it, without attributes names. Although my patch uses
"conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
and "$2_name" (in the case of svn.fs.commit_txn).

Of course, as there is no typemaps to be applied to multiple C APIs,
we should implement them one by one, so its trouble will be only a bit
that even we will name new attribute.
> Does this change anything about the svn_fs_commit_txn() interface?

Nothing in the current patch, except the exception also have those
attribute.

>
> -- Mike
>
> On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <[hidden email]>
> wrote:
>
>> On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
>>> On 2020/07/29 23:43, C. Michael Pilato wrote:
>>>> Makes sense to me as a way to get access to all the things that can be
>>>> returned in an errorful situation.
>>>
>>> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
>>> attributes to SubversionException instance on svn_fs.commit_txn and
>>> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
>>> tuple of (None, int), for a consistency.
>>>
>>> I've tested this method on other API with dummy attribute, but I didn't
>>> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>>>
>>> Could you please review and try it ?
>>>
>>> Thanks, --
>>> Yasuhito FUTATSUKI <[hidden email]>
>>
>> Ah, I sent the patch before including a commit message. Here it is.
>> [[[
>> swig-py: Allow SubversionException to add attributes
>>
>> [in subversion/bindings/swig/]
>>
>> * include/svn_types.swg
>>   (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
>>   distinct those C APIs which return value by using pointer args when
>>   error is occured.
>>   (typemap(out) svn_error_t_with_attrs *): New typemap.
>>
>> * python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
>>   (svn_swig_py_build_svn_exception): New function.
>>   (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
>>   get the exception class and to make the instance of it from subversion
>>   error.
>>
>> * svn_fs.i
>>   (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
>>   - Add attribute "conflict_p" and "new_rev" to SubversionException
>>     instance when the exception is occured.
>>   - Fix the conversion format to build return value on Python 3
>>     "z" conversion makes str when conflict_p is not NULL, although
>>     it is always NULL in this context. So this is foolproof.
>>   (svn_fs_commit_txn):
>>     New fake function declaration for SWIG to make a wrapper. Ignore
>>     the real one.
>>
>> * svn_repos.i
>>   (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
>>    svn_revnum_t *new_rev)):
>>     New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
>>     and modified argment number.
>>   (svn_repos_fs_commit_txn):
>>     New fake function declaration for SWIG to make a wrapper. Ignore
>>     the real one.
>>
>> Found by: cmpilato
>> ]]]
>>
>>
>> Thanks,
>> --
>> Yasuhito FUTATSUKI <[hidden email]>
>>
>

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

C. Michael Pilato-2
Sending        subversion/bindings/swig/include/svn_types.swg
Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
Sending        subversion/bindings/swig/svn_fs.i
Sending        subversion/bindings/swig/svn_repos.i
Transmitting file data .....done
Committing transaction...
Committed revision 1880967.

I made only minor comment/formatting changes.

On Sun, Aug 2, 2020 at 9:36 AM Yasuhito FUTATSUKI <[hidden email]> wrote:
On 2020/07/31 10:19, Yasuhito FUTATSUKI wrote:
> On 2020/07/30 22:20, C. Michael Pilato wrote:
>> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
>> there are two issues I have with it.
>>
>> First, unless I'm mistaken, there's no way to get the conflict_p back as
>> part of the 2-tuple return value -- if there's a conflict,
>> svn.repos.fs_commit_txn() is going to raise an Exception.  So we're
>> basically returning a 2-tuple whose first value is always None.  We might
>> as well preserve the current behavior and return the newly committed
>> revision as a singleton.  (Which also means we don't break compatibility
>> with prior versions.)
>
> How do you think about this issue on svn.fs.commit_txn()? It is also
> always (None, new_rev) if it doesn't raise Exception, since r845217,
> Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
> however, I took priority of compatibility. (Though I think no one
> ever used this Python wrapper function in practical program....)
>
> Putting aside this compatibility issue, my preference is:
>
> if error is not occured:
>   if *conflict_p is NULL:
>      return *new_rev as a singletone
>   else:
>      # a foolproof. we can detect an internal error by doing this.
       return 2-tuple (*conflict_p, *new_rev)
> else:
>   # error is occred
>   raise exception with values of *conflict_p and *new_rev in the exception
>
> So I prefer to make this change and announce it. Note that we have
> an option that we'll change behavor only in Python 3 with preserve
> the behavor in Python 2.

I've revised the patch, with this applying to both of Python 2 and Python 3.

>> Also, I don't love that we've used "conflict_p" and "new_rev" as the
>> variables names within the SubversionException.  Anything we do here will
>> be non-obvious and require documentation, so I'd rather give these things
>> meaningful names ("conflict_path" and "new_revision").  But as a concept,
>> I'd say this part works well.
>
> Using argment names of C API have a merit that if we will implement such
> special wrappers requires attirbutes of SubversionException, we just say
> implement it, without attributes names. Although my patch uses
> "conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
> and "$2_name" (in the case of svn.fs.commit_txn).
>
> Of course, as there is no typemaps to be applied to multiple C APIs,
> we should implement them one by one, so its trouble will be only a bit
> that even we will name new attribute.
In this patch, I used "$1_name" yet.

Cheers,
--
Yasuhito FUTATSUKI <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

Daniel Shahaf-2
C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:

> Sending        subversion/bindings/swig/include/svn_types.swg
> Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> Sending        subversion/bindings/swig/svn_fs.i
> Sending        subversion/bindings/swig/svn_repos.i
> Transmitting file data .....done
> Committing transaction...
> Committed revision 1880967.
>
> I made only minor comment/formatting changes.

Shouldn't the change be documented somewhere for users of the
bindings?  (E.g., release notes, API errata, API documentation…)

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion

C. Michael Pilato-2
On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <[hidden email]> wrote:
C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:
> Sending        subversion/bindings/swig/include/svn_types.swg
> Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> Sending        subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> Sending        subversion/bindings/swig/svn_fs.i
> Sending        subversion/bindings/swig/svn_repos.i
> Transmitting file data .....done
> Committing transaction...
> Committed revision 1880967.
>
> I made only minor comment/formatting changes.

Shouldn't the change be documented somewhere for users of the
bindings?  (E.g., release notes, API errata, API documentation…)

Yes, I think that makes sense.  The release notes for sure.  As for API docs/errata ... do we even have such a thing for the bindings?

-- Mike 
Reply | Threaded
Open this post in threaded view
|

Re: Python bindings API confusion (+ svnbook swig-py examples not py3 compatible)

Daniel Shahaf-2
C. Michael Pilato wrote on Fri, 28 Aug 2020 13:49 -0400:

> On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <[hidden email]>
> wrote:
>
> > C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:  
> > > Sending        subversion/bindings/swig/include/svn_types.swg
> > > Sending  
> > subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c  
> > > Sending  
> > subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h  
> > > Sending        subversion/bindings/swig/svn_fs.i
> > > Sending        subversion/bindings/swig/svn_repos.i
> > > Transmitting file data .....done
> > > Committing transaction...
> > > Committed revision 1880967.
> > >
> > > I made only minor comment/formatting changes.  
> >
> > Shouldn't the change be documented somewhere for users of the
> > bindings?  (E.g., release notes, API errata, API documentation…)
> >  
>
> Yes, I think that makes sense.  The release notes for sure.  As for API
> docs/errata ... do we even have such a thing for the bindings?

We have errata in notes/api-errata/, and the release notes could point
to them.

Documentation… I think there's just the README files in the source,
release notes of past minor lines, and the example code in
#svn.developer.usingapi.codesamples in the book?

Cheers,

Daniel

P.S.  The example code in that section of the book won't work in
Python 3 (due to using py2-only "print" and "except" syntaxes).  We
should probably add some text explaining the example code is py2 but
the bindings support py3 as well (as explained in the 1.l4 release
notes), or better yet, port the examples to py3.