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.)
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.
Does this change anything about the svn_fs_commit_txn() interface?
-- Mike
On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <futat...@yf.bsdclub.org>
wrote:
> On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
> > On 2020/07/29 23:43, C. Michael Pilato wrote:
> >> Makes sense to me as a way to get access to all the things that can be
> >> returned in an errorful situation.
> >
> > Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
> > attributes to SubversionException instance on svn_fs.commit_txn and
> > svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
> > tuple of (None, int), for a consistency.
> >
> > I've tested this method on other API with dummy attribute, but I didn't
> > test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
> >
> > Could you please review and try it ?
> >
> > Thanks, --
> > Yasuhito FUTATSUKI <futat...@yf.bsclub.org>
>
> Ah, I sent the patch before including a commit message. Here it is.
> [[[
> swig-py: Allow SubversionException to add attributes
>
> [in subversion/bindings/swig/]
>
> * include/svn_types.swg
> (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
> distinct those C APIs which return value by using pointer args when
> error is occured.
> (typemap(out) svn_error_t_with_attrs *): New typemap.
>
> * python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
> (svn_swig_py_build_svn_exception): New function.
> (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
> get the exception class and to make the instance of it from subversion
> error.
>
> * svn_fs.i
> (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
> - Add attribute "conflict_p" and "new_rev" to SubversionException
> instance when the exception is occured.
> - Fix the conversion format to build return value on Python 3
> "z" conversion makes str when conflict_p is not NULL, although
> it is always NULL in this context. So this is foolproof.
> (svn_fs_commit_txn):
> New fake function declaration for SWIG to make a wrapper. Ignore
> the real one.
>
> * svn_repos.i
> (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
> svn_revnum_t *new_rev)):
> New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
> and modified argment number.
> (svn_repos_fs_commit_txn):
> New fake function declaration for SWIG to make a wrapper. Ignore
> the real one.
>
> Found by: cmpilato
> ]]]
>
>
> Thanks,
> --
> Yasuhito FUTATSUKI <futat...@yf.bsclub.org>
>
