Nathan Hartman wrote on Wed, Dec 23, 2020 at 00:09:46 -0500: > On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI > <futat...@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?
