On 3 March 2015 at 04:06, <rhuij...@apache.org> wrote: > Author: rhuijben > Date: Tue Mar 3 01:06:16 2015 > New Revision: 1663500 > > URL: http://svn.apache.org/r1663500 > Log: > In ra-serf (for issue #4557) reinstate a bit of code that retries a delete > with an altered request if the original request fails, because the server > determined that it is an invalid request, because it has too many bytes > in the headers. > > Before r1553501 we always retried DELETE requests that required lock > tokens, as the initial request didn't have an If header at all. > Hi Bert,
See my review inline. [...] > Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500&r1=1663499&r2=1663500&view=diff > ============================================================================== > --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original) > +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16 > 2015 [...] > static svn_error_t * > delete_entry(const char *path, > svn_revnum_t revision, > @@ -1412,6 +1443,7 @@ delete_entry(const char *path, > delete_context_t *delete_ctx; > svn_ra_serf__handler_t *handler; > const char *delete_target; > + svn_error_t *err; > > if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx)) > { > @@ -1446,7 +1478,23 @@ delete_entry(const char *path, > handler->method = "DELETE"; > handler->path = delete_target; > > - SVN_ERR(svn_ra_serf__context_run_one(handler, pool)); > + err = svn_ra_serf__context_run_one(handler, pool); > + if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED > + && handler->sline.code == 400) May be it's better set handler->no_fail_on_http_failure_status to TRUE and check for handler->sline.code? This will remove dependency on specific error code and simplify code a bit. > + { > + svn_error_clear(err); > + > + /* Try again with non-standard body to overcome Apache Httpd > + header limit */ > + delete_ctx->non_recursive_if = TRUE; > + handler->body_type = "text/xml"; > + handler->body_delegate = create_delete_body; > + handler->body_delegate_baton = delete_ctx; > + > + SVN_ERR(svn_ra_serf__context_run_one(handler, pool)); I'm not sure that we can reuse svn_ra_serf__handler_t instance for multiple requests. I propose the following patch. Do you have any objections? -- Ivan Zhakov
Index: subversion/libsvn_ra_serf/commit.c =================================================================== --- subversion/libsvn_ra_serf/commit.c (revision 1736882) +++ subversion/libsvn_ra_serf/commit.c (working copy) @@ -1415,7 +1415,6 @@ delete_context_t *delete_ctx; svn_ra_serf__handler_t *handler; const char *delete_target; - svn_error_t *err; if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx)) { @@ -1449,16 +1448,26 @@ handler->method = "DELETE"; handler->path = delete_target; + handler->no_fail_on_http_failure_status = TRUE; - err = svn_ra_serf__context_run_one(handler, pool); - if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED - && handler->sline.code == 400) + SVN_ERR(svn_ra_serf__context_run_one(handler, pool)); + if (handler->sline.code == 400) { - svn_error_clear(err); - /* Try again with non-standard body to overcome Apache Httpd header limit */ delete_ctx->non_recursive_if = TRUE; + + handler = svn_ra_serf__create_handler(dir->commit_ctx->session, pool); + + handler->response_handler = svn_ra_serf__expect_empty_body; + handler->response_baton = handler; + + handler->header_delegate = setup_delete_headers; + handler->header_delegate_baton = delete_ctx; + + handler->method = "DELETE"; + handler->path = delete_target; + handler->body_type = "text/xml"; handler->body_delegate = create_delete_body; handler->body_delegate_baton = delete_ctx; @@ -1465,9 +1474,10 @@ SVN_ERR(svn_ra_serf__context_run_one(handler, pool)); } - else - SVN_ERR(err); + if (handler->server_error) + return svn_ra_serf__server_error_create(handler, pool); + /* 204 No Content: item successfully deleted */ if (handler->sline.code != 204) return svn_error_trace(svn_ra_serf__unexpected_status(handler));