On 2020/12/23 14:09, Nathan Hartman wrote: > 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. > > 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 <futat...@yf.bsclub.org>
