On 2020/07/31 10:19, Yasuhito FUTATSUKI wrote:
> On 2020/07/30 22:20, C. Michael Pilato wrote:
>> 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.)
>
> How do you think about this issue on svn.fs.commit_txn()? It is also
> always (None, new_rev) if it doesn't raise Exception, since r845217,
> Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
> however, I took priority of compatibility. (Though I think no one
> ever used this Python wrapper function in practical program....)
>
> Putting aside this compatibility issue, my preference is:
>
> if error is not occured:
> if *conflict_p is NULL:
> return *new_rev as a singletone
> else:
> # a foolproof. we can detect an internal error by doing this.
return 2-tuple (*conflict_p, *new_rev)
> else:
> # error is occred
> raise exception with values of *conflict_p and *new_rev in the exception
>
> So I prefer to make this change and announce it. Note that we have
> an option that we'll change behavor only in Python 3 with preserve
> the behavor in Python 2.
I've revised the patch, with this applying to both of Python 2 and Python 3.
>> 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.
>
> Using argment names of C API have a merit that if we will implement such
> special wrappers requires attirbutes of SubversionException, we just say
> implement it, without attributes names. Although my patch uses
> "conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
> and "$2_name" (in the case of svn.fs.commit_txn).
>
> Of course, as there is no typemaps to be applied to multiple C APIs,
> we should implement them one by one, so its trouble will be only a bit
> that even we will name new attribute.
In this patch, I used "$1_name" yet.
Cheers,
--
Yasuhito FUTATSUKI <futat...@yf.bsclub.org>
swig-py: Allow SubversionException to add attributes
In SWIG Python bindings, when C API a function causes an error the Python
wrapper function couldn't any return value related to the error because
it raises an SubversionException. With this patch, we introduce new
set of typemaps to help adding attributes related to the error to the
exception instance object.
[in subversion/bindings/swig/]
* include/svn_types.swg
(typemap(out) svn_error_t * SVN_ERR_WITH_ATTRS,
typemap(ret) svn_error_t * SVN_ERR_WITH_ATTRS):
New typemap. Those wrapper functions use attributes on exception object
feature apply this, and should use argout typemaps only that aware
they may be executed even the C API return an error status.
With those typemaps, the argout typemaps can use the status code of the
subversion error 'arp_err' and the exception instance object 'exc_ob'.
* 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)):
Removed. This reverts the behavor of the svn.fs.commit_txn()
return value, that it returns 2-tuple which of the first
emenent is always None if exception is not occured.
(typemap(argout) (const char **conflict_p):
New custom typemap to add an attibute to exception instance object.
This is applied to svn_fs_merge(), svn_fs_commit_txn() and
svn_repos_fs_commit_txn().
(typemap(argout) (svn_revnum_t *new_rev):
New custom typemap to add an attibute to exception instance object.
This is applied to svn_fs_commit_txn() and svn_repos_fs_commit_txn().
():
Apply the typemap svn_error_t * SVN_ERR_WITH_ATTRS to svn_fs_merge() and
svn_fs_commit_txn().
* svn_repos.i
():
Apply the typemap svn_error_t * SVN_ERR_WITH_ATTRS to svn_repos_fs_merge().
Found by: cmpilato
Index: subversion/bindings/swig/include/svn_types.swg
===================================================================
--- subversion/bindings/swig/include/svn_types.swg (revision 1880475)
+++ subversion/bindings/swig/include/svn_types.swg (working copy)
@@ -438,6 +438,54 @@
Py_INCREF(Py_None);
$result = Py_None;
}
+
+%typemap(out) svn_error_t * SVN_ERR_WITH_ATTRS (apr_status_t apr_err,
+ PyObject *exc_class,
+ PyObject *exc_ob) {
+ apr_err = 0;
+ exc_class = exc_ob = NULL;
+ if ($1 != NULL)
+ {
+ apr_err = $1->apr_err;
+ if (apr_err != SVN_ERR_SWIG_PY_EXCEPTION_SET)
+ {
+ svn_swig_py_build_svn_exception(&exc_class, &exc_ob, $1);
+ if (exc_ob == NULL)
+ {
+ /* we cannot add attrs to exception instance */
+ if (exc_class != NULL)
+ {
+ /* Raise an exception without instance ... */
+ PyErr_SetNone(exc_class);
+ Py_DECREF(exc_class);
+ }
+ SWIG_fail;
+ }
+ /* pending to raise the exception */
+ }
+ else
+ {
+ svn_error_clear($1);
+ SWIG_fail;
+ }
+ }
+ else
+ {
+ Py_INCREF(Py_None);
+ $result = Py_None;
+ }
+}
+
+%typemap(ret) svn_error_t * SVN_ERR_WITH_ATTRS {
+ if (exc_ob != NULL)
+ {
+ PyErr_SetObject(exc_class, exc_ob);
+ Py_DECREF(exc_class);
+ Py_DECREF(exc_ob);
+ Py_XDECREF($result);
+ SWIG_fail;
+ }
+}
#endif
#ifdef SWIGPERL
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
(revision 1880475)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
(working copy)
@@ -411,10 +411,11 @@
/*** Custom SubversionException stuffs. ***/
-void svn_swig_py_svn_exception(svn_error_t *error_chain)
+void svn_swig_py_build_svn_exception(
+ PyObject **exc_class, PyObject **exc_ob,svn_error_t *error_chain)
{
PyObject *args_list, *args, *apr_err_ob, *message_ob, *file_ob, *line_ob;
- PyObject *svn_module, *exc_class, *exc_ob;
+ PyObject *svn_module;
svn_error_t *err;
if (error_chain == NULL)
@@ -422,7 +423,7 @@
/* Start with no references. */
args_list = args = apr_err_ob = message_ob = file_ob = line_ob = NULL;
- svn_module = exc_class = exc_ob = NULL;
+ svn_module = *exc_class = *exc_ob = NULL;
if ((args_list = PyList_New(0)) == NULL)
goto finished;
@@ -481,16 +482,13 @@
/* Create the exception object chain. */
if ((svn_module = PyImport_ImportModule((char *)"svn.core")) == NULL)
goto finished;
- if ((exc_class = PyObject_GetAttrString(svn_module,
- (char *)"SubversionException")) == NULL)
- goto finished;
- if ((exc_ob = PyObject_CallMethod(exc_class, (char *)"_new_from_err_list",
- (char *)"O", args_list)) == NULL)
- goto finished;
+ if ((*exc_class = PyObject_GetAttrString(svn_module,
+ (char *)"SubversionException")) != NULL)
+ {
+ *exc_ob = PyObject_CallMethod(*exc_class, (char *)"_new_from_err_list",
+ (char *)"O", args_list);
+ }
- /* Raise the exception. */
- PyErr_SetObject(exc_class, exc_ob);
-
finished:
/* Release any references. */
Py_XDECREF(args_list);
@@ -500,11 +498,30 @@
Py_XDECREF(file_ob);
Py_XDECREF(line_ob);
Py_XDECREF(svn_module);
- Py_XDECREF(exc_class);
- Py_XDECREF(exc_ob);
}
+void svn_swig_py_svn_exception(svn_error_t *error_chain)
+{
+ PyObject *exc_class, *exc_ob;
+ svn_swig_py_build_svn_exception(&exc_class, &exc_ob, error_chain);
+ /* Raise the exception. */
+ if (exc_class != NULL)
+ {
+ if (exc_ob != NULL)
+ {
+ PyErr_SetObject(exc_class, exc_ob);
+ Py_DECREF(exc_ob);
+ }
+ else
+ {
+ PyErr_SetNone(exc_class);
+ }
+ Py_DECREF(exc_class);
+ }
+}
+
+
/*** Helper/Conversion Routines ***/
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
(revision 1880475)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
(working copy)
@@ -101,6 +101,11 @@
/*** Functions to expose a custom SubversionException ***/
+/* get a subversion exception class object and its instance built from
+ error_chain but not raise immediately. cunsume the error. */
+void svn_swig_py_build_svn_exception(
+ PyObject **exc_class, PyObject **exc_ob, svn_error_t *error_chain);
+
/* raise a subversion exception, created from a normal subversion
error. consume the error. */
void svn_swig_py_svn_exception(svn_error_t *err);
Index: subversion/bindings/swig/svn_fs.i
===================================================================
--- subversion/bindings/swig/svn_fs.i (revision 1880475)
+++ subversion/bindings/swig/svn_fs.i (working copy)
@@ -93,23 +93,68 @@
%apply apr_hash_t *MERGEINFO { apr_hash_t *mergeinhash };
/* -----------------------------------------------------------------------
- Fix the return value for svn_fs_commit_txn(). If the conflict result is
- NULL, then %append_output() is passed Py_None, but that goofs up
- because that is *also* the marker for "I haven't started assembling a
- multi-valued return yet" which means the second return value (new_rev)
- will not cause a 2-tuple to be manufactured.
+ Tweak a SubversionException instance when it is raised in
+ svn_fs_merge(), svn_fs_commit_txn() and svn_repos_fs_commit_txn().
+ Those APIs return conflicts (and revision number on svn_fs_commit_txn
+ and svn_repos_fs_commit_txn) related to the conflict error when it
+ is occured. As Python wrapper functions report errors by raising
+ exceptions and don't return values, we use attributes of the exception
+ to pass these values to the caller.
+*/
- The answer is to explicitly create a 2-tuple return value.
-
- FIXME: Do the Perl and Ruby bindings need to do something similar?
-*/
#ifdef SWIGPYTHON
-%typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
- /* this is always Py_None */
- Py_DECREF($result);
- /* build the result tuple */
- $result = Py_BuildValue("zi", *$1, (long)*$2);
+%typemap(argout) (const char **conflict_p) (PyObject* conflict_ob) {
+ if (*$1 == NULL)
+ {
+ Py_INCREF(Py_None);
+ conflict_ob = Py_None;
+ }
+ else
+ {
+ /* Note: We can check if apr_err == SVN_ERR_FS_CONFLICT or not
+ before access to *$1 */
+ conflict_ob = PyBytes_FromString((const char *)*$1);
+ if (conflict_ob == NULL)
+ {
+ Py_XDECREF(exc_ob);
+ Py_XDECREF($result);
+ SWIG_fail;
+ }
+ }
+ if (exc_ob != NULL)
+ {
+ PyObject_SetAttrString(exc_ob, "$1_name", conflict_ob);
+ Py_DECREF(conflict_ob);
+ }
+ else
+ {
+ %append_output(conflict_ob);
+ }
}
+
+%typemap(argout) svn_revnum_t *new_rev (PyObject *rev_ob) {
+ rev_ob = PyInt_FromLong((long)*$1);
+ if (rev_ob == NULL)
+ {
+ Py_XDECREF(exc_ob);
+ Py_XDECREF($result);
+ SWIG_fail;
+ }
+ if (exc_ob != NULL)
+ {
+ PyObject_SetAttrString(exc_ob, "$1_name", rev_ob);
+ Py_DECREF(rev_ob);
+ }
+ else
+ {
+ %append_output(rev_ob);
+ }
+}
+
+%apply svn_error_t *SVN_ERR_WITH_ATTRS {
+ svn_error_t * svn_fs_commit_txn,
+ svn_error_t * svn_fs_merge
+};
#endif
/* Ruby fixups for functions not following the pool convention. */
Index: subversion/bindings/swig/svn_repos.i
===================================================================
--- subversion/bindings/swig/svn_repos.i (revision 1880475)
+++ subversion/bindings/swig/svn_repos.i (working copy)
@@ -109,6 +109,16 @@
#endif
/* -----------------------------------------------------------------------
+ Tweak a SubversionException instance (See svn_fs.i for detail).
+*/
+
+#ifdef SWIGPYTHON
+%apply svn_error_t *SVN_ERR_WITH_ATTRS {
+ svn_error_t * svn_repos_fs_commit_txn
+};
+#endif
+
+/* -----------------------------------------------------------------------
handle svn_repos_get_committed_info().
*/
#ifdef SWIGRUBY
