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 <futat...@yf.bsclub.org>