You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org> on 2020/08/02 13:34:30 UTC
Re: Python bindings API confusion
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 <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
I'm sorry, but I'd like to make a correction.
On 2020/12/26 20:02, Yasuhito FUTATSUKI wrote:
> [[[
> Index: subversion/bindings/swig/python/svn/fs.py
> ===================================================================
> --- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
> +++ subversion/bindings/swig/python/svn/fs.py (working copy)
> @@ -24,6 +24,14 @@
> ######################################################################
>
> from libsvn.fs import *
> +
> +# For API compatibility we should replace wrapper function entity before
> +# adding alternative names.
> +_svn_fs_commit_txn = svn_fs_commit_txn
> +def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
To keep Python 2 compatibility, it is better to remove annotation.
This function definition was originally taken from SWIG generated module
file for Python 3, subversion/bindings/swig/python/fs.py, and then
I modified to adjust return value. It was a mistake.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/31 8:59, Yasuhito FUTATSUKI wrote:
<snip>
> During writing the test above I also found that on swig-py for Python 3
> wrapper functions using svn_string_t * or svn_stringbuf_t * for in
> input arguments don't accept str objects for their values. This was
> my overlook, so I'll fix it. However, this is another topic.
Fix it in 1885370.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/31 8:59, Yasuhito FUTATSUKI wrote:
> On 2020/12/28 19:10, Daniel Shahaf wrote:
>> Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
>>> On 2020/12/26 3:38, Nathan Hartman wrote:
>>>
>>>> But the solution you suggest above, adding a new_rev attribute to the
>>>> exception and leaving the return value as it was, sounds like a good way to
>>>> avoid the incompatibility and also avoid the API bump.
>>>
>>> I had written such a patch on typemaps at first because it was straight
>>> forward. Although I removed it after r1880967 is commited, I can write
>>> such a code again and I'll do if it is needed.
>>>
>>> Alternately, here is an easier way to do it. The patch below is a code
>>> that replace wrapper function entity on top of the svn.fs module
>>> (not tested, and more appropriate comment is needed in the code).
>
> <snip>
>
>>> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
>>> in above patch, then it will satisfy the Daniel's proposal.
>>
>> I agree with Nathan that it'd be confusing for a Python
>> svn_fs_commit_txn2() to exist when a C function of the same name does
>> not, even if the C API skips a revv number in consideration of swig-py.
>> So, if we'd like to introduce a simplified API, I'd vote to name it
>> something else.
>
> Then I'll commit patch amended only for restore Python 2 support,
> without renaming to svn_fs_commit_txn2, if Mike agrees because he
> already wrote some code using svn.fs.commit_txn().
> (attached swig-py-fs_commit_txn-revert-return-value-type-patch.txt)
I got a reply from Mike off the list with non negative answer. So I think
there is no objection to restore the behavior on return value type of
svn.fs.commit_txn(), yet, at least.
However as a result of reconsideration of how it should be done, now
I don't think this patch wasn't sufficieant. If C API svn_fs_commit_txn()
returns SVN_NO_ERROR but conflict_p is not NULL by accident,
svn.fs.commit_txn() with this patch returns (None, (conflict, new_revision))
and this is obiously incorrect. So I added the check for it.
Also, I rerote the comment, then commit it in 1885007.
> I also wrote tests for svn.fs.commit_txn, both for return value and
> attributes in SubversionException, by porting tests for C API into Python.
> (attached patch swig-py-fs_commit_txn-test-patch.txt; tested on Python
> 2.7.18 and 3.9.0 on FreeBSD 12)
I added to more assertion to check if attributes exist in exception,
and commit it in 1885008.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/28 19:10, Daniel Shahaf wrote:
> Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
>> On 2020/12/26 3:38, Nathan Hartman wrote:
>>
>>> But the solution you suggest above, adding a new_rev attribute to the
>>> exception and leaving the return value as it was, sounds like a good way to
>>> avoid the incompatibility and also avoid the API bump.
>>
>> I had written such a patch on typemaps at first because it was straight
>> forward. Although I removed it after r1880967 is commited, I can write
>> such a code again and I'll do if it is needed.
>>
>> Alternately, here is an easier way to do it. The patch below is a code
>> that replace wrapper function entity on top of the svn.fs module
>> (not tested, and more appropriate comment is needed in the code).
<snip>
>> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
>> in above patch, then it will satisfy the Daniel's proposal.
>
> I agree with Nathan that it'd be confusing for a Python
> svn_fs_commit_txn2() to exist when a C function of the same name does
> not, even if the C API skips a revv number in consideration of swig-py.
> So, if we'd like to introduce a simplified API, I'd vote to name it
> something else.
Then I'll commit patch amended only for restore Python 2 support,
without renaming to svn_fs_commit_txn2, if Mike agrees because he
already wrote some code using svn.fs.commit_txn().
(attached swig-py-fs_commit_txn-revert-return-value-type-patch.txt)
I also wrote tests for svn.fs.commit_txn, both for return value and
attributes in SubversionException, by porting tests for C API into Python.
(attached patch swig-py-fs_commit_txn-test-patch.txt; tested on Python
2.7.18 and 3.9.0 on FreeBSD 12)
During writing the test above I also found that on swig-py for Python 3
wrapper functions using svn_string_t * or svn_stringbuf_t * for in
input arguments don't accept str objects for their values. This was
my overlook, so I'll fix it. However, this is another topic.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
> On 2020/12/26 3:38, Nathan Hartman wrote:
>
> > But the solution you suggest above, adding a new_rev attribute to the
> > exception and leaving the return value as it was, sounds like a good way to
> > avoid the incompatibility and also avoid the API bump.
>
> I had written such a patch on typemaps at first because it was straight
> forward. Although I removed it after r1880967 is commited, I can write
> such a code again and I'll do if it is needed.
>
> Alternately, here is an easier way to do it. The patch below is a code
> that replace wrapper function entity on top of the svn.fs module
> (not tested, and more appropriate comment is needed in the code).
>
> [[[
> Index: subversion/bindings/swig/python/svn/fs.py
> ===================================================================
> --- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
> +++ subversion/bindings/swig/python/svn/fs.py (working copy)
> @@ -24,6 +24,14 @@
> ######################################################################
>
> from libsvn.fs import *
> +
> +# For API compatibility we should replace wrapper function entity before
> +# adding alternative names.
> +_svn_fs_commit_txn = svn_fs_commit_txn
> +def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
> + r"""svn_fs_commit_txn(svn_fs_txn_t txn, apr_pool_t pool) -> svn_error_t"""
> + return None, _svn_fs_commit_txn(*args)
> +
> from svn.core import _unprefix_names, Pool, _as_list
> _unprefix_names(locals(), 'svn_fs_')
> _unprefix_names(locals(), 'SVN_FS_')
>
> ]]]
>
> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
> in above patch, then it will satisfy the Daniel's proposal.
I agree with Nathan that it'd be confusing for a Python
svn_fs_commit_txn2() to exist when a C function of the same name does
not, even if the C API skips a revv number in consideration of swig-py.
So, if we'd like to introduce a simplified API, I'd vote to name it
something else.
Cheers,
Daniel
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/26 3:38, Nathan Hartman wrote:
> But the solution you suggest above, adding a new_rev attribute to the
> exception and leaving the return value as it was, sounds like a good way to
> avoid the incompatibility and also avoid the API bump.
I had written such a patch on typemaps at first because it was straight
forward. Although I removed it after r1880967 is commited, I can write
such a code again and I'll do if it is needed.
Alternately, here is an easier way to do it. The patch below is a code
that replace wrapper function entity on top of the svn.fs module
(not tested, and more appropriate comment is needed in the code).
[[[
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -24,6 +24,14 @@
######################################################################
from libsvn.fs import *
+
+# For API compatibility we should replace wrapper function entity before
+# adding alternative names.
+_svn_fs_commit_txn = svn_fs_commit_txn
+def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
+ r"""svn_fs_commit_txn(svn_fs_txn_t txn, apr_pool_t pool) -> svn_error_t"""
+ return None, _svn_fs_commit_txn(*args)
+
from svn.core import _unprefix_names, Pool, _as_list
_unprefix_names(locals(), 'svn_fs_')
_unprefix_names(locals(), 'SVN_FS_')
]]]
Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
in above patch, then it will satisfy the Daniel's proposal.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Dec 25, 2020 at 12:00 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
>
> I may be more than a bit late for this, but can we avoid the
> incompatibility in the first place? I.e., avoid requiring API users to
> check svn.core.SVN_VER_MINOR before interpreting an API's return value?
> Adding a new_rev attribute to the exception is a welcome change — it
> addresses Nathan's point (1) — but I question whether changing «return
> (None, foo)» to «return foo» (plus or minus a singleton tuple around it)
> justifies breaking compatibility. This change sounds like it should be
> part of a proper API bump, namely, provide a svn.fs.commit_txn2(),
> keeping svn.fs.commit_txn() as-is.
>
> (Implementation-wise, it may be easier to implement svn.fs.commit_txn2()
> as a wrapper around .commit_txn() than the other way around. When we
> next revv the C API, we can skip a revv number in consideration of the
> bindings, and call the revved API svn_fs_commit_txn3(), deprecating
> svn_fs_commit_txn().)
I thought about suggesting an API bump for the Python wrapper but I didn't
suggest it because I thought it might be confusing to have
svn.fs.commit_txn2() be a wrapper for svn_fs_commit_txn(), especially if
there's ever a svn_fs_commit_txn2().
But the solution you suggest above, adding a new_rev attribute to the
exception and leaving the return value as it was, sounds like a good way to
avoid the incompatibility and also avoid the API bump.
Cheers,
Nathan
Re: Python bindings API confusion
Posted by Branko Čibej <br...@apache.org>.
On 25.12.2020 18:00, Daniel Shahaf wrote:
>> In addition, we introduce a framework to add attributes to
>> 'SubversionException' to return additional values to the caller in
>> situations such as above. (This new feature is mentioned for
>> completeness but is not part of the errata.)
> "the erratum"
Using the plural is correct in this case, as there is more than one
erratum being discussed.
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Wed, Dec 23, 2020 at 00:09:46 -0500:
> On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
> >
> > On 2020/08/29 2:49, C. Michael Pilato wrote:
> > >>> 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?
> >
> > I'm very sorry for my late reply.
> >
> > This change affects all programs using return value of svn.fs.commit_txn.
> > So I think api-errata is needed.
> >
> > As you all and I know my English is too bad, however I wrote it as a
> > starting point. Please refine or rewrite if the errata is needed.
>
> Thank you for remembering to do this.
>
> I rewrote it differently, because I was not familiar with the context
> of r1880967, so I think this was helpful as an exercise for me to
> learn more about how the C APIs and Python bindings interact.
>
> Please review and tell me what you think... (Maybe some of the facts
> are wrong!)
>
> [[[
> svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
>
> Like other C APIs, svn_fs_commit_txn uses its return value to indicate
> success or error. In addition, it returns two more values by pointer
> arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
> either the commit succeeds, indicated by a valid revision number in
> 'new_rev' or the commit fails, with details in 'conflict'. The API's
> return value, taken together with these two additional pieces of
> information, tell the full story of the function's success or failure.
>
> In particular, if a commit succeeds but an error occurs in post-commit
> processing, the API will return an error but still indicate the
> successful commit by returning a valid revision number in 'new_rev'.
>
> When the C API returns successfully, the Python wrapper returns the
> two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
> an error, the Python wrapper raises an exception.
>
> Unfortunately, this suffers the following problems:
>
> (1) An exception prevents the wrapper from returning the 2-tuple, so
> the caller cannot know whether a successful commit occurred or not.
>
> (2) When there is no exception, 'conflict' is always None, defeating
> the purpose of trying to return it in a 2-tuple.
>
> (3) This is inconsistent with svn.repos.commit_txn, which only returns
> a 'new_rev' singleton on success.
>
> For the 1.15 release, svn.fs.commit_txn is made consistent with
> svn.repos.commit_txn in returning a 'new_rev' singleton on success.
"returning a singleton" sounds like it returns a one-element tuple, as in
«return tuple([foo])», as opposed to just «return foo».
> In addition, we introduce a framework to add attributes to
> 'SubversionException' to return additional values to the caller in
> situations such as above. (This new feature is mentioned for
> completeness but is not part of the errata.)
"the erratum"
> API users who relied on the previous 2-tuple return value should
> adjust their code to process the 'new_rev' singleton on successful
> return.
I may be more than a bit late for this, but can we avoid the
incompatibility in the first place? I.e., avoid requiring API users to
check svn.core.SVN_VER_MINOR before interpreting an API's return value?
Adding a new_rev attribute to the exception is a welcome change — it
addresses Nathan's point (1) — but I question whether changing «return
(None, foo)» to «return foo» (plus or minus a singleton tuple around it)
justifies breaking compatibility. This change sounds like it should be
part of a proper API bump, namely, provide a svn.fs.commit_txn2(),
keeping svn.fs.commit_txn() as-is.
(Implementation-wise, it may be easier to implement svn.fs.commit_txn2()
as a wrapper around .commit_txn() than the other way around. When we
next revv the C API, we can skip a revv number in consideration of the
bindings, and call the revved API svn_fs_commit_txn3(), deprecating
svn_fs_commit_txn().)
Unrelated: could the log message of r1880967 be extended to make it
clear (in the top summary) that the revision comprises not only internal
infrastructure changes but also user-visible changes?
Thanks,
Daniel
> ]]]
>
> Thoughts?
Re: Python bindings API confusion (+ svnbook swig-py examples not
py3 compatible)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, 28 Aug 2020 13:49 -0400:
> On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
> 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.
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Dec 23, 2020 at 1:02 PM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
> The last paragraph is for "== Impact on API Users ==" section, I
> think. (Yes, my draft also missed this mark of the section.)
>
> I think the error in this erratum is (3) only, and the fix might
> be a change type of return value of svn.repos.commit_txn. This was
> caused by r845217, which fixed the return type of svn.fs.commit_txn
> to 2-tuple and wasn't applied to svn.repos.commit_txn. So the
> explanation of these API is only for justification of the fix.
> Also as the target of this api-errata are users of those API,
> I think that so detailed explanation of APIs itself does not need.
>
> However, as a document for these Python wrapper APIs, this
> explanation is very useful.
Thanks.
Feel free to cut out the parts that are excessive for the errata.
Maybe one of us will add the longer explanation to the API docs later.
Cheers,
Nathan
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/23 14:09, Nathan Hartman wrote:
> On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
>>
>> On 2020/08/29 2:49, C. Michael Pilato wrote:
>>>>> 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?
>>
>> I'm very sorry for my late reply.
>>
>> This change affects all programs using return value of svn.fs.commit_txn.
>> So I think api-errata is needed.
>>
>> As you all and I know my English is too bad, however I wrote it as a
>> starting point. Please refine or rewrite if the errata is needed.
>
> Thank you for remembering to do this.
>
> I rewrote it differently, because I was not familiar with the context
> of r1880967, so I think this was helpful as an exercise for me to
> learn more about how the C APIs and Python bindings interact.
>
> Please review and tell me what you think... (Maybe some of the facts
> are wrong!)
>
> [[[
>
> svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
>
> Like other C APIs, svn_fs_commit_txn uses its return value to indicate
> success or error. In addition, it returns two more values by pointer
> arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
> either the commit succeeds, indicated by a valid revision number in
> 'new_rev' or the commit fails, with details in 'conflict'. The API's
> return value, taken together with these two additional pieces of
> information, tell the full story of the function's success or failure.
>
> In particular, if a commit succeeds but an error occurs in post-commit
> processing, the API will return an error but still indicate the
> successful commit by returning a valid revision number in 'new_rev'.
>
> When the C API returns successfully, the Python wrapper returns the
> two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
> an error, the Python wrapper raises an exception.
>
> Unfortunately, this suffers the following problems:
>
> (1) An exception prevents the wrapper from returning the 2-tuple, so
> the caller cannot know whether a successful commit occurred or not.
>
> (2) When there is no exception, 'conflict' is always None, defeating
> the purpose of trying to return it in a 2-tuple.
>
> (3) This is inconsistent with svn.repos.commit_txn, which only returns
> a 'new_rev' singleton on success.
>
> For the 1.15 release, svn.fs.commit_txn is made consistent with
> svn.repos.commit_txn in returning a 'new_rev' singleton on success.
>
> In addition, we introduce a framework to add attributes to
> 'SubversionException' to return additional values to the caller in
> situations such as above. (This new feature is mentioned for
> completeness but is not part of the errata.)
>
> API users who relied on the previous 2-tuple return value should
> adjust their code to process the 'new_rev' singleton on successful
> return.
>
> ]]]
>
> Thoughts?
The last paragraph is for "== Impact on API Users ==" section, I
think. (Yes, my draft also missed this mark of the section.)
I think the error in this erratum is (3) only, and the fix might
be a change type of return value of svn.repos.commit_txn. This was
caused by r845217, which fixed the return type of svn.fs.commit_txn
to 2-tuple and wasn't applied to svn.repos.commit_txn. So the
explanation of these API is only for justification of the fix.
Also as the target of this api-errata are users of those API,
I think that so detailed explanation of APIs itself does not need.
However, as a document for these Python wrapper APIs, this
explanation is very useful.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> On 2020/08/29 2:49, C. Michael Pilato wrote:
> >>> 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?
>
> I'm very sorry for my late reply.
>
> This change affects all programs using return value of svn.fs.commit_txn.
> So I think api-errata is needed.
>
> As you all and I know my English is too bad, however I wrote it as a
> starting point. Please refine or rewrite if the errata is needed.
Thank you for remembering to do this.
I rewrote it differently, because I was not familiar with the context
of r1880967, so I think this was helpful as an exercise for me to
learn more about how the C APIs and Python bindings interact.
Please review and tell me what you think... (Maybe some of the facts
are wrong!)
[[[
svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
Like other C APIs, svn_fs_commit_txn uses its return value to indicate
success or error. In addition, it returns two more values by pointer
arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
either the commit succeeds, indicated by a valid revision number in
'new_rev' or the commit fails, with details in 'conflict'. The API's
return value, taken together with these two additional pieces of
information, tell the full story of the function's success or failure.
In particular, if a commit succeeds but an error occurs in post-commit
processing, the API will return an error but still indicate the
successful commit by returning a valid revision number in 'new_rev'.
When the C API returns successfully, the Python wrapper returns the
two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
an error, the Python wrapper raises an exception.
Unfortunately, this suffers the following problems:
(1) An exception prevents the wrapper from returning the 2-tuple, so
the caller cannot know whether a successful commit occurred or not.
(2) When there is no exception, 'conflict' is always None, defeating
the purpose of trying to return it in a 2-tuple.
(3) This is inconsistent with svn.repos.commit_txn, which only returns
a 'new_rev' singleton on success.
For the 1.15 release, svn.fs.commit_txn is made consistent with
svn.repos.commit_txn in returning a 'new_rev' singleton on success.
In addition, we introduce a framework to add attributes to
'SubversionException' to return additional values to the caller in
situations such as above. (This new feature is mentioned for
completeness but is not part of the errata.)
API users who relied on the previous 2-tuple return value should
adjust their code to process the 'new_rev' singleton on successful
return.
]]]
Thoughts?
Nathan
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/08/29 2:49, C. Michael Pilato wrote:
> On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
> 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?
I'm very sorry for my late reply.
This change affects all programs using return value of svn.fs.commit_txn.
So I think api-errata is needed.
As you all and I know my English is too bad, however I wrote it as a
starting point. Please refine or rewrite if the errata is needed.
[[[
API ERRATA -- $Id$
Root Cause of Errata: design error of wrapper function
Library(s) Affected: (SWIG Python bindings)svn.fs
Function(s) Affected: (SWIG Python bindings)svn.fs.commit_txn
New Behavior in: 1.15
Related Issues: n/a
== Details ==
The C API svn_fs_commit_txn has two pointer arguments to return values,
'conflict' and 'new_rev'. However 'conflict' is always NULL unless
svn_fs_commit_txn cause error, and as Python Wrapper function raises
error if C API returns error state, the return value of Python Wrapper
function svn.fs.commit_txn returned 2-tupple of None and 'new_rev' value
since 1.14.
This behavior is quite useless and inconsistent with the behavior
of svn.repos.commit_txn, the wrapper of svn_repos_commit_txn, which
has also two pointer arguments 'conflict' and 'new_rev' but only
returns 'new_rev' singleton.
For the 1.15 release, svn.fs.commit_txn returns 'new_rev' singleton
if it doesn't raise exception. In addtion, we introduce framework
to add attributes to 'SubversionException' to return values to caller
if argments for output in C API are useful when the API returns an error
(however this new feature actually is not a part of the errata).
If an API user expects that it returns a 2-tuple will get some exceptions
when try to retrieve its element.
All API users should review their treatments of return value of the API.
]]]
I also worry but couldn't wrote description of this change for API docs.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
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
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
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 <fu...@yf.bsdclub.org>
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 <fu...@yf.bsclub.org>
>