Nathan Hartman wrote on Wed, Dec 23, 2020 at 00:09:46 -0500:
> On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
> <futatuki_at_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?
Received on 2020-12-25 18:00:18 CET