Re: [PATCH] Allow "cease invocation" return from Python callback for repos.svn_repos_history2()

[Роман Донченко]

Alexey Neyman <sti...@att.net> писал в своём письме Thu, 18 Mar 2010  
21:51:53 +0300:

On Wednesday 17 March 2010 04:59:34 pm Роман Донченко wrote:
Alexey Neyman <sti...@att.net> писал в своём письме Wed, 17 Mar 2010
00:05:01 +0300:
> Hi all,
>
> The svn_repos_history2() function allows the history_func() to return
> a special error, SVN_ERR_CEASE_INVOCATION, to stop the search. This
> is not supported in Python bindings, though: attempt to return
> core.SVN_ERR_CEASE_INVOCATION from the history receiver results in an
> exception:
>
>   def history_lookup(path, rev, pool):
>     return core.SVN_ERR_CEASE_INVOCATION
>
>   repos.svn_repos_history2(..., history_lookup, ...)
>
> svn.core.SubversionException: ('Python callback returned an invalid
> object', 20014)
>
> Indeed, svn_swig_py_repos_history_func() only expects a return of
> None. This patch allows the callback to return
> core.SVN_ERR_CEASE_INVOCATION. Currently, this is only used in
> svn_repos_history2() - apparently, it's the only function that
> supports SVN_ERR_CEASE_INVOCATION. However, there is a FIXME at least
> in copyfrom_info_receiver() in libsvn_client/log.c, stating that
> other callbacks may eventually be able to return
> SVN_ERR_CEASE_INVOCATION as well.

Good idea, but I think that the callback should signal an error the
same way the Subversion functions do it - namely, by throwing
core.SubversionException. The wrapper would then translate the
exception's fields into an svn_error_t. It looks like tweaking
callback_exception_error is the most obvious way to do that. You don't
even have to special-case SVN_ERR_CEASE_INVOCATION, just copy whatever
code the exception had into the error.

I think that may be beyond my current knowledge of Python's C API... I
tried that:

/* Return a Subversion error about a failed callback. */
static svn_error_t *callback_exception_error(void)
{
  PyObject *err = PyErr_Occurred();
  PyObject *svn_module, *exc_class;
  PyObject *message, *apr_err;

  if ((svn_module = PyImport_ImportModule((char *)"svn.core")) == NULL)
    goto finished;
  if ((exc_class = PyObject_GetAttrString(svn_module,
                (char *)"SubversionException")) == NULL)
    goto finished;
  if (PyErr_GivenExceptionMatches(exc_class, err))
    {
      message = PyObject_GetAttrString(err, (char *)"message");
      apr_err = PyObject_GetAttrString(err, (char *)"apr_err");
      if (message && apr_err && PyString_Check(message)
           && PyInt_Check(apr_err))
        {
          return svn_error_create(PyInt_AsLong(apr_err), NULL,
                          PyString_AsString(message));
        }
    }

finished:
  return svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
                          "Python callback raised an exception");
}

First, it cannot find "apr_err" in the instance of
core.Subversion_Exception; PyObject_GetAttrString(err, (char *)"apr_err")
returns NULL.

And, it produces the following error:

SystemError: NULL result without error in PyObject_Call

Can you help? What's wrong with that code?

Hmm. How are you testing this? I'd write a test script, but I'm short on  
time, and you probably already have one. 8=]

It would also be great if you added a relevant test to the testsuite.

I thought about it, that would be the next step once the API is settled
and implemented.

Okay.


Regards,
Alexey.

Ditto,
Roman.