Re: svn commit: r38105 - trunk/subversion/libsvn_ra_serf

On Fri, Jun 19, 2009 at 3:08 PM, Bert Huijben <rhuijben_at_sharpsvn.net> wrote:
> Author: rhuijben
> Date: Fri Jun 19 13:08:39 2009
> New Revision: 38105
>
> Log:
> Make all response handlers in svn_ra_serf return svn_error_t* for
> a much cleaner error handling. This fixes issue #3375, but some further
> refactoring would be welcome.
>
> * subversion/libsvn_ra_serf/commit.c
>  (handle_checkout): Updated for new handler prototype and replace abort()
>    with normal SVN_ERR_MALFUNCTION()
>  (post_response_handler): Updated for changed prototype.
>
> * subversion/libsvn_ra_serf/locks.c
>  (handle_lock): Updated for new prototype. Return errors where possible.
>
> * subversion/libsvn_ra_serf/options.c
>  (options_response_handler): Updated for new prototype.
>
> * subversion/libsvn_ra_serf/ra_serf.h
>  (svn_ra_serf__response_handler_t): Updated to return svn_error_t * and
>    remove the temporary session argument.
>  (svn_ra_serf__handle_status_only, svn_ra_serf__handle_discard_body,
>   svn_ra_serf__handle_server_error, svn_ra_serf__handle_multistatus_only,
>   svn_ra_serf__handle_xml_parser): Updated to follow new prototype.
>  (SVN_SESSION_ERR): Removed macro.
>
> * subversion/libsvn_ra_serf/update.c
>  (error_fetch): Return svn_error_t *
>  (handle_fetch, handle_stream): Update error handling for new prototype.
>
> * subversion/libsvn_ra_serf/util.c
>  (svn_ra_serf__handle_discard_body,
>   svn_ra_serf__handle_status_only,
>   svn_ra_serf__handle_multistatus_only):
>    Implement new prototype.
>  (svn_ra_serf__handle_xml_parser):
>    Implement new prototype. Return errors instead of adding them to
>    the session. And remove an obsolete test.
>  (svn_ra_serf__handle_server_error):
>    Updated for new prototype. Clear ignored errors.
>  (handle_response):
>    Use apr_status_t handler for discarding data, clear ignored
>    authentication errors. Compose session errors instead of overwriting
>    existing values. Add specific handling for serfs error handling.
>
> Modified:
>   trunk/subversion/libsvn_ra_serf/commit.c
>   trunk/subversion/libsvn_ra_serf/locks.c
>   trunk/subversion/libsvn_ra_serf/options.c
>   trunk/subversion/libsvn_ra_serf/ra_serf.h
>   trunk/subversion/libsvn_ra_serf/update.c
>   trunk/subversion/libsvn_ra_serf/util.c

Hi Bert,

Just an fyi:

In addition to the error handling problem I already fixed in r40377,
is seems this change is also responsible for triggering an assert in
update_tests.py 57 'verify update of deleted locked files'. This test
was not added until after your change, but on a hunch I checked if the
test would pass with ra_serf back to r38104. It does, and then fails
with the introduction of r38105.

The test is marked as XFail so assert has gone unnoticed. I've
attached the test run output for the test using ra_neon, where it
fails "as expected" and from ra_serf where the assert occurs.

This assert can quickly be replicated from the command line by locking
a file, deleting the files containing directory and the updating.

I haven't yet been able to figure out what abour r38105 is causing
this. I'm out of the office today and will look at it more on Monday,
but in the meantime if you have anything to add...

Thanks,

Paul

Paul

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415110