Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100: > Hi, > > For atomic-revprop, the client needs to know if the error was > SVN_ERR_BAD_OLD_VALUE or not. For ra_local and ra_svn this is > already done; this patch does it for DAV (with Neon). > > > With mod_dav, we only have 2 ways to affect the 207 multistatus > response from a failed PROPPATCH: > > - We can set the text that appears in <D:responsedescription>, > including injecting arbitrary XML. > - We can set the <D:status> value to a (numeric) HTTP error code.
There is also dav_svn__error_response_tag(). > > The approach that's been discussed on-list and on IRC was to > inject the error as XML inside <D:responsedescription>. I did > actually implement that, only to discover that Neon completely > rejects the XML in that case[1]. While we would obviously fix > this in the 1.7 client code, it could break compatibility between > the 1.7 server and 1.6 and older clients. The only way to handle In other words, changing that table means changing the protocol, which isn't backwards-compatible. Good call. > this would be to introduce a new client capability, so the server > could fall back to the old code for 1.6 clients. This means we'd > have two code paths for error-handling... this gets very complex. > > > However, there is a simpler way! The <D:status> element contains > the HTTP error code, usually "500 Internal Server Error". So we > could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE. > I think of old_value_p as a precondition for the operation, a bit > like "If-Modified-Since:", so I'd suggest "412 Precondition Failed". > Note that generic DAV clients won't ever see this message, because > they won't be sending old_value_p. > Fair enough. Are there currently any circumstances where HTTP_PRECONDITION_FAILED would be raised? (e.g., by mod_dav) As far as I can tell (by greping httpd sources), the answer is "No". > This patch only does mod_dav_svn and Neon. I don't want to waste > time doing both HTTP libraries if this patch is rejected. (Also, > Serf is slightly harder because it doesn't parse <D:status> yet). > > The patch is against the atomic-revprop branch, and requires > Patch 1 to be applied first. (It does apply to trunk, too). > > [[[ > Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status > code inside a 207 multi-status response. > > * subversion/mod_dav_svn/util.c: > (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412. > > * subversion/libsvn_ra_neon/util.c: > (multistatus_baton_t): New member contains_precondition_error. > (end_207_element): Check for HTTP 412 status code and create > a SVN_ERR_BAD_OLD_VALUE error if found. > > Patch by: Jon Foster <jon.fos...@cabot.co.uk> > ]]] > Tested it and it works fine: [[[ CMD: atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 0 5 value 6 violet )" neon CMD: /home/daniel/src/svn/atomic-revprop/subversion/tests/cmdline/atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 6 violet ) neon exited with 1 <TIME = 0.305840> subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002) subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002) atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008) atomic-ra-revprop-change: At least one property change failed; repository is unchanged subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014) subversion/libsvn_ra_neon/util.c:236: (apr_err=125014) atomic-ra-revprop-change: Error setting property 'flower': revprop 'flower' has unexpected value in filesystem ]]] where 125014 is SVN_ERR_BAD_OLD_VALUE. :-) I'll commit this once I have an ra_serf version too. Would you like to write the ra_serf part of the patch, too? Thanks! Daniel > Kind regards, > > Jon > > > [1] More precisely, libsvn_ra_neon/util.c:wrapper_startelm_cb() > raises a SVN_ERR_XML_MALFORMED "XML data was not well-formed" > error if <D:responsedescription> contains any XML tags. See > the ELEM_responsedescription row in multistatus_nesting_table[] > in the same file - it explicitly states that if there's any > XML tags inside <D:responsedescription> then the XML is invalid. > > Index: subversion/mod_dav_svn/util.c > =================================================================== > --- subversion/mod_dav_svn/util.c (revision 998620) > +++ subversion/mod_dav_svn/util.c (working copy) > @@ -107,6 +107,9 @@ > case SVN_ERR_FS_PATH_ALREADY_LOCKED: > status = HTTP_LOCKED; > break; > + case SVN_ERR_BAD_OLD_VALUE: > + status = HTTP_PRECONDITION_FAILED; > + break; > /* add other mappings here */ > } > > Index: subversion/libsvn_ra_neon/util.c > =================================================================== > --- subversion/libsvn_ra_neon/util.c (revision 998620) > +++ subversion/libsvn_ra_neon/util.c (working copy) > @@ -167,6 +167,7 @@ > svn_ra_neon__request_t *req; > svn_stringbuf_t *description; > svn_boolean_t contains_error; > + svn_boolean_t contains_precondition_error; > } multistatus_baton_t; > > /* Implements svn_ra_neon__startelm_cb_t. */ > @@ -231,9 +232,12 @@ > return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, > _("The request response contained at > least " > "one error")); > + else if (b->contains_precondition_error) > + return svn_error_create(SVN_ERR_BAD_OLD_VALUE, NULL, > + b->description->data); > else > return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, > - b->description->data); > + *b->description->data); ^^^ Unintentional change? (triggers compiler warning) > } > break; > > @@ -260,6 +264,10 @@ > else > b->propstat_has_error = (status.klass != 2); > > + /* Handle "412 Precondition Failed" specially */ > + if (status.code == 412) > + b->contains_precondition_error = TRUE; > + > free(status.reason_phrase); > } > else
