You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jon Foster <Jo...@cabot.co.uk> on 2010/09/20 10:01:02 UTC
[PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
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.
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
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.
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 <jo...@cabot.co.uk>
]]]
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.
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Julian Reschke <ju...@gmx.de>.
On 21.09.2010 14:49, Jon Foster wrote:
> ...
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.
> c?revision=982016&view=markup#l2121
>
> This is the mod_dav function that builds the 207 response
> from a failed PROPPATCH. It doesn't allow us to add extra
> XML wherever we want; we only get to set the<D:status>
> number and insert some text inside<D:responsedescription>.
> We *could* insert XML inside the<D:responsedescription>,
> but that would break all existing SVN clients.
> ...
Oh. I see.
Bad moddav :-)
Best regards, Julian
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Jon Foster <Jo...@cabot.co.uk>.
Julian Reschke wrote:
> On 21.09.2010 14:12, Daniel Shahaf wrote:
> > Julian Reschke wrote on Tue, Sep 21, 2010 at 13:46:13 +0200:
> >> Hi,
> >>
> >> just two comments without having looked at the remainder of the
discussion.
> >>
> >> 1) If you need to augment a standard HTTP response code with
additional
> >> information, the right thing to use is DAV:error (see
> >> <http://greenbytes.de/tech/webdav/rfc4918.html#rfc.section.16>).
> >>
> >
> > What does that gain us? As Jon said elsethread, generic DAV clients
> > aren't going to be making requests with V:old-value set.
>
> I was just offering an option that may be cleaner than overloading the
> semantics of other fields.
>
> Also, if this is *purely* for SVN, another obvious solution is just to
> add an extension element to the DAV:response element (in teh SVN
> namespace...).
(Background: For this discussion, we're only talking about how we
return PROPPATCH errors inside the 207 Multi-Status envelope).
Unfortunately, we're severely constrained by mod_dav and
ra_neon. In particular, see mod_dav's dav_failed_proppatch()
function, here:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.
c?revision=982016&view=markup#l2121
This is the mod_dav function that builds the 207 response
from a failed PROPPATCH. It doesn't allow us to add extra
XML wherever we want; we only get to set the <D:status>
number and insert some text inside <D:responsedescription>.
We *could* insert XML inside the <D:responsedescription>,
but that would break all existing SVN clients.
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Julian Reschke <ju...@gmx.de>.
On 21.09.2010 14:12, Daniel Shahaf wrote:
> Julian Reschke wrote on Tue, Sep 21, 2010 at 13:46:13 +0200:
>> Hi,
>>
>> just two comments without having looked at the remainder of the discussion.
>>
>> 1) If you need to augment a standard HTTP response code with additional
>> information, the right thing to use is DAV:error (see
>> <http://greenbytes.de/tech/webdav/rfc4918.html#rfc.section.16>).
>>
>
> What does that gain us? As Jon said elsethread, generic DAV clients
> aren't going to be making requests with V:old-value set.
I was just offering an option that may be cleaner than overloading the
semantics of other fields.
Also, if this is *purely* for SVN, another obvious solution is just to
add an extension element to the DAV:response element (in teh SVN
namespace...).
> ...
Best regards, Julian
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Reschke wrote on Tue, Sep 21, 2010 at 13:46:13 +0200:
> Hi,
>
> just two comments without having looked at the remainder of the discussion.
>
> 1) If you need to augment a standard HTTP response code with additional
> information, the right thing to use is DAV:error (see
> <http://greenbytes.de/tech/webdav/rfc4918.html#rfc.section.16>).
>
What does that gain us? As Jon said elsethread, generic DAV clients
aren't going to be making requests with V:old-value set.
> 2) Do not use 412 Precondition Failed unless the request was a
> conditional request.
>
FTR, cmpilato suggested to use 409 Conflict at some point.
> Best regards, Julian
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Julian Reschke <ju...@gmx.de>.
On 21.09.2010 13:52, Jon Foster wrote:
> Hi,
>
>> just two comments without having looked at the remainder of the
> discussion.
>
> Please read my first mail in this [PATCH 3/3] thread, which explains
> why these choices were made.
> ...
That's the message I read, and I didn't see any consideration of using
DAV:error...
Best regards, Julian
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
> just two comments without having looked at the remainder of the
discussion.
Please read my first mail in this [PATCH 3/3] thread, which explains
why these choices were made.
Kind regards,
Jon
-----Original Message-----
From: Julian Reschke [mailto:julian.reschke@gmx.de]
Sent: 21 September 2010 12:46
To: Jon Foster
Cc: Daniel Shahaf; Subversion Development
Subject: Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP
status code
Hi,
just two comments without having looked at the remainder of the
discussion.
1) If you need to augment a standard HTTP response code with additional
information, the right thing to use is DAV:error (see
<http://greenbytes.de/tech/webdav/rfc4918.html#rfc.section.16>).
2) Do not use 412 Precondition Failed unless the request was a
conditional request.
Best regards, Julian
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Julian Reschke <ju...@gmx.de>.
Hi,
just two comments without having looked at the remainder of the discussion.
1) If you need to augment a standard HTTP response code with additional
information, the right thing to use is DAV:error (see
<http://greenbytes.de/tech/webdav/rfc4918.html#rfc.section.16>).
2) Do not use 412 Precondition Failed unless the request was a
conditional request.
Best regards, Julian
RE: Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> Sent: maandag 20 september 2010 12:58
> To: Bert Huijben
> Cc: 'Jon Foster'; 'Subversion Development'
> Subject: Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error
> as a HTTP status code)
>
> Bert Huijben wrote on Mon, Sep 20, 2010 at 12:17:12 +0200:
> > I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name
> though.
> > ('Old value' is only valid in the specific function context. Maybe
> extend
> > this to something more like this 'precondition failed')
>
> In the interest of moving forward I've committed it already with its
> current macro name and default message, but we could rename the error
> code or the message in a followup commit.
>
> "Nominated value"? "Prevalue"? "BASE value"? "Entry value"?
I think the error should be in the SVN_ERR_RA (or if it originates there:
FS) prefix, not just 'SVN_ERR_BAD' and specify more what happened. (The BAD
category doesn't carry any information on what failed and is used only for
argument validation errors. This makes it hard to diagnose the error from
just the error code. And with our localized error messages that is all
applications that use libsvn_client can do)
So something like:
SVN_ERR_RA_PROP_BASEVALUE_MISMATCH
(specifies the RA problem)
Or just
SVN_ERR_RA_PRECONDITION_FAILED
(generic RA precondition problem)
Bert
Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error as
a HTTP status code)
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Mon, Sep 20, 2010 at 12:17:12 +0200:
> I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name though.
> ('Old value' is only valid in the specific function context. Maybe extend
> this to something more like this 'precondition failed')
In the interest of moving forward I've committed it already with its
current macro name and default message, but we could rename the error
code or the message in a followup commit.
"Nominated value"? "Prevalue"? "BASE value"? "Entry value"?
Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error as
a HTTP status code)
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Mon, Sep 20, 2010 at 12:17:12 +0200:
> I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name though.
> ('Old value' is only valid in the specific function context. Maybe extend
> this to something more like this 'precondition failed')
In the interest of moving forward I've committed it already with its
current macro name and default message, but we could rename the error
code or the message in a followup commit.
"Nominated value"? "Prevalue"? "BASE value"? "Entry value"?
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Jon Foster [mailto:Jon.Foster@cabot.co.uk]
> Sent: maandag 20 september 2010 12:01
> To: Daniel Shahaf; Subversion Development
> Subject: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
>
> 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.
>
> 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
> 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.
Nice and simple solution to this issue :)
I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name though.
('Old value' is only valid in the specific function context. Maybe extend
this to something more like this 'precondition failed')
Bert
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jon Foster wrote on Tue, Sep 21, 2010 at 00:43:13 +0100:
> Hi,
>
> Updated patch attached.
>
> [[[
> 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.
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (svn_ra_serf__server_error_t): New member
> contains_precondition_error.
>
> * subversion/libsvn_ra_serf/util.c
> (parse_dav_status): New method.
> (start_207): Parse DAV:status XML element.
> (end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
> error code if applicable.
> (svn_ra_serf__handle_multistatus_only): Initialise new member.
>
> Patch by: Jon Foster <jo...@cabot.co.uk>
> ]]]
>
> > > Index: subversion/libsvn_ra_serf/util.c
> > > ===================================================================
> > > --- subversion/libsvn_ra_serf/util.c (revision 999102)
> > > +++ subversion/libsvn_ra_serf/util.c (working copy)
> > > @@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t
> *r
> > > + server_err->contains_precondition_error = FALSE;
> >
> > (So, this is the error handler for a function that normally discards
> > its response, and the meat of the patch is in
> > svn_ra_serf__handle_multistatus_only().)
>
> After reviewing this code again, I've dropped that chunk. I've
> convinced
> myself that the variable is never used in that code path.
>
True.
Though I'm tempted to leave it in anyway; the next person to add code to
initialize server_err might copy this initalizer...
But I speculate that the thing is allocated with apr_pCalloc() anyway,
so this whole discussion doesn't matter :-)
> > > + err = svn_cstring_atoi(status_code_out, token);
> > > + if (err)
> > > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> > > + "Malformed DAV:status CDATA");
> > > + svn_stringbuf_setempty(buf);
> > > + return SVN_NO_ERROR;
> > > +}
> > > +
> > > /*
> > > * Expat callback invoked on a start element tag for a 207
> response.
> > > */
> > > @@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> > > svn_stringbuf_setempty(ctx->cdata);
> > > ctx->collect_cdata = TRUE;
> > > }
> > > + else if (ctx->in_error &&
> > > + strcmp(name.namespace, "DAV:") == 0 &&
> > > + strcmp(name.name, "status") == 0)
> > > + {
> > > + /* Start collecting cdata. */
> > > + svn_stringbuf_setempty(ctx->cdata);
> >
> > Okay; D:responsedescription and D:status are siblings [1], so it's
> > okay to reuse CTX->cdata.
> >
> > <paranoia on>
> > Are you sure they will always be siblings? If we aren't sure that
> yes,
> > we could just use two stringbufs (instead of reusing ctx->cdata).
>
> Even with the old code, if there are any elements with CDATA nested
> inside <D:responsedescription>, Serf is going to go wrong. If we
Ah, right.
> collect CDATA into different variables, then it'll just go wrong in
> a different way (e.g. the HTTP status line might be shown as part of
> the message).
>
> I think there's a whole package of work that could be done to make
> Serf resilient against weird XML, but I think that's separate from
> this work. (It's also quite hard to test).
>
Agreed.
> Thanks for the thourough review!
You're welcome.
Content-Description: patch_atomic_revprops_dav2.txt
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 999156)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -945,6 +945,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
> return svn_error_return(err);
> }
>
> +/* Given a string like "HTTP/1.1 500 (status)", parse out the numeric status
> + code. Ignores leading whitespace. */
Good docstring, but it will be slightly clearer (and extensible) if you
called out the parameter names:
+/* Given a string like "HTTP/1.1 500 (status)" in BUF, parse out the numeric status
+ code into *STATUS_CODE_OUT. Ignores leading whitespace. */
(but yeah, I'm being lazy and dropping the "Use POOL for temporary
allocations" boilerplate)
> +static svn_error_t *
> +parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
> + apr_pool_t *scratch_pool)
> +{
> + svn_error_t *err;
> + const char *token;
> + char *tok_status;
> + svn_stringbuf_t *temp_buf = svn_stringbuf_create(buf->data, scratch_pool);
*cough* svn_stringbuf_dup()
> +
> + svn_stringbuf_strip_whitespace(temp_buf);
> + token = apr_strtok(temp_buf->data, " \t\r\n", &tok_status);
> + if (token)
> + token = apr_strtok(NULL, " \t\r\n", &tok_status);
> + if (!token)
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + "Malformed DAV:status CDATA '%s'",
> + buf->data);
> + err = svn_cstring_atoi(status_code_out, token);
> + if (err)
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> + "Malformed DAV:status CDATA '%s'",
> + buf->data);
> +
> + return SVN_NO_ERROR;
> +}
> +
> /*
> * Expat callback invoked on a start element tag for a 207 response.
> */
> @@ -968,6 +996,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> svn_stringbuf_setempty(ctx->cdata);
> ctx->collect_cdata = TRUE;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + /* Start collecting cdata. */
> + svn_stringbuf_setempty(ctx->cdata);
> + ctx->collect_cdata = TRUE;
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -993,9 +1029,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
> ctx->collect_cdata = FALSE;
> ctx->error->message = apr_pstrmemdup(ctx->error->pool, ctx->cdata->data,
> ctx->cdata->len);
> - ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> + if (ctx->contains_precondition_error)
> + ctx->error->apr_err = SVN_ERR_BAD_OLD_VALUE;
> + else
> + ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + int status_code;
>
> + ctx->collect_cdata = FALSE;
> +
> + SVN_ERR(parse_dav_status(&status_code, ctx->cdata, parser->pool));
> + if (status_code == 412)
> + ctx->contains_precondition_error = TRUE;
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -1044,6 +1095,7 @@ svn_ra_serf__handle_multistatus_only(serf_request_
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 999156)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -673,6 +673,9 @@ typedef struct svn_ra_serf__server_error_t {
> /* Have we seen an error tag? */
> svn_boolean_t in_error;
>
> + /* Have we seen a HTTP "412 Precondition Failed" error? */
> + svn_boolean_t contains_precondition_error;
> +
> /* Should we be collecting the XML cdata? */
> svn_boolean_t collect_cdata;
>
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
The error code changed while I was e-mailing the patch. Here's the
patch with the new error code.
[[[
Signal SVN_ERR_FS_PROP_BASEVALUE_MISMATCH 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_FS_PROP_BASEVALUE_MISMATCH 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_FS_PROP_BASEVALUE_MISMATCH error if
found.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c
(parse_dav_status): New method.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use
SVN_ERR_FS_PROP_BASEVALUE_MISMATCH error code if
applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster <jo...@cabot.co.uk>
]]]
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
Updated patch attached.
[[[
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.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c
(parse_dav_status): New method.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
error code if applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster <jo...@cabot.co.uk>
]]]
Daniel Shahaf wrote:
> > [[[
> > 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:
>
> No trailing colon here ----------^
Fixed.
[...]
> > Index: subversion/libsvn_ra_serf/util.c
> > ===================================================================
> > --- subversion/libsvn_ra_serf/util.c (revision 999102)
> > +++ subversion/libsvn_ra_serf/util.c (working copy)
> > @@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t
*r
> > + server_err->contains_precondition_error = FALSE;
>
> (So, this is the error handler for a function that normally discards
> its response, and the meat of the patch is in
> svn_ra_serf__handle_multistatus_only().)
After reviewing this code again, I've dropped that chunk. I've
convinced
myself that the variable is never used in that code path.
> > @@ -945,6 +946,31 @@ svn_ra_serf__handle_status_only(serf_request_t
*re
> > return svn_error_return(err);
> > }
> >
> > +/* Given a string like "HTTP/1.1 500 (status)", parse out the
numeric status
> > + code. Ignores leading whitespace. This function will overwrite
and then
> > + clear the passed buf. */
>
> Sounds like a strange semantics. Couldn't you just take a
scratch_pool
> argument and duplicate the buffer, leaving the caller's copy
unchanged?
> Changing it in-place seems like a shoot-oneself-in-the-foot move...
Changed.
> > +static svn_error_t *
> > +parse_dav_status(svn_stringbuf_t *buf, int *status_code_out)
> > +{
> > + svn_error_t * err;
> > + const char * token;
> > + char * tok_status = 0;
>
> Style nit: no space after the * in the last three lines.
>
> Also, no need to initialize TOK_STATUS (says
svn_cstring_split_append()).
Both done.
> > +
> > + svn_stringbuf_strip_whitespace(buf);
> > + token = apr_strtok(buf->data, " \t\r\n", &tok_status);
> > + if (token)
> > + token = apr_strtok(NULL, " \t\r\n", &tok_status);
> > + if (!token)
> > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> > + "Malformed DAV:status CDATA");
>
> Should the cdata be included in the error text? (using
svn_error_createf())
Can do, and in the new patch I have.
> > + err = svn_cstring_atoi(status_code_out, token);
> > + if (err)
> > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> > + "Malformed DAV:status CDATA");
> > + svn_stringbuf_setempty(buf);
> > + return SVN_NO_ERROR;
> > +}
> > +
> > /*
> > * Expat callback invoked on a start element tag for a 207
response.
> > */
> > @@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> > svn_stringbuf_setempty(ctx->cdata);
> > ctx->collect_cdata = TRUE;
> > }
> > + else if (ctx->in_error &&
> > + strcmp(name.namespace, "DAV:") == 0 &&
> > + strcmp(name.name, "status") == 0)
> > + {
> > + /* Start collecting cdata. */
> > + svn_stringbuf_setempty(ctx->cdata);
>
> Okay; D:responsedescription and D:status are siblings [1], so it's
> okay to reuse CTX->cdata.
>
> [1] http://paste.lisp.org/display/113346
>
> <paranoia on>
> Are you sure they will always be siblings? If we aren't sure that
yes,
> we could just use two stringbufs (instead of reusing ctx->cdata).
Even with the old code, if there are any elements with CDATA nested
inside <D:responsedescription>, Serf is going to go wrong. If we
collect CDATA into different variables, then it'll just go wrong in
a different way (e.g. the HTTP status line might be shown as part of
the message).
I think there's a whole package of work that could be done to make
Serf resilient against weird XML, but I think that's separate from
this work. (It's also quite hard to test).
> > + ctx->collect_cdata = TRUE;
[...]
> > + SVN_ERR(parse_dav_status(ctx->cdata, &status_code));
> Our convention is to have the output parameters first.
Changed.
Thanks for the thourough review!
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jon Foster wrote on Mon, Sep 20, 2010 at 22:59:02 +0100:
> Daniel Shahaf wrote:
> > Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
> > > 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.
> >
> > 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?
>
> Sure. See attached.
> This updated patch is against the atomic-revprop branch.
>
> [[[
> 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:
No trailing colon here ----------^
> (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.
>
> * subversion/libsvn_ra_serf/ra_serf.h:
> (svn_ra_serf__server_error_t): New member
> contains_precondition_error.
>
> * subversion/libsvn_ra_serf/util.c:
> (parse_dav_status): New method.
> (svn_ra_serf__handle_discard_body): Initialise new member.
> (start_207): Parse DAV:status XML element.
> (end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
> error code if applicable.
> (svn_ra_serf__handle_multistatus_only): Initialise new member.
>
> Patch by: Jon Foster <jo...@cabot.co.uk>
> ]]]
>
> Kind regards,
>
> Jon
>
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 999102)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t *r
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
(So, this is the error handler for a function that normally discards its
response, and the meat of the patch is in svn_ra_serf__handle_multistatus_only().)
> @@ -945,6 +946,31 @@ svn_ra_serf__handle_status_only(serf_request_t *re
> return svn_error_return(err);
> }
>
> +/* Given a string like "HTTP/1.1 500 (status)", parse out the numeric status
> + code. Ignores leading whitespace. This function will overwrite and then
> + clear the passed buf. */
Sounds like a strange semantics. Couldn't you just take a scratch_pool
argument and duplicate the buffer, leaving the caller's copy unchanged?
Changing it in-place seems like a shoot-oneself-in-the-foot move...
> +static svn_error_t *
> +parse_dav_status(svn_stringbuf_t *buf, int *status_code_out)
> +{
> + svn_error_t * err;
> + const char * token;
> + char * tok_status = 0;
Style nit: no space after the * in the last three lines.
Also, no need to initialize TOK_STATUS (says svn_cstring_split_append()).
> +
> + svn_stringbuf_strip_whitespace(buf);
> + token = apr_strtok(buf->data, " \t\r\n", &tok_status);
> + if (token)
> + token = apr_strtok(NULL, " \t\r\n", &tok_status);
> + if (!token)
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + "Malformed DAV:status CDATA");
Should the cdata be included in the error text? (using svn_error_createf())
> + err = svn_cstring_atoi(status_code_out, token);
> + if (err)
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> + "Malformed DAV:status CDATA");
> + svn_stringbuf_setempty(buf);
> + return SVN_NO_ERROR;
> +}
> +
> /*
> * Expat callback invoked on a start element tag for a 207 response.
> */
> @@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> svn_stringbuf_setempty(ctx->cdata);
> ctx->collect_cdata = TRUE;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + /* Start collecting cdata. */
> + svn_stringbuf_setempty(ctx->cdata);
Okay; D:responsedescription and D:status are siblings [1], so it's okay to
reuse CTX->cdata.
[1] http://paste.lisp.org/display/113346
<paranoia on>
Are you sure they will always be siblings? If we aren't sure that yes,
we could just use two stringbufs (instead of reusing ctx->cdata).
> + ctx->collect_cdata = TRUE;
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -993,9 +1027,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
> ctx->collect_cdata = FALSE;
> ctx->error->message = apr_pstrmemdup(ctx->error->pool, ctx->cdata->data,
> ctx->cdata->len);
> - ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> + if (ctx->contains_precondition_error)
> + ctx->error->apr_err = SVN_ERR_BAD_OLD_VALUE;
> + else
> + ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + int status_code;
>
> + ctx->collect_cdata = FALSE;
> +
> + SVN_ERR(parse_dav_status(ctx->cdata, &status_code));
Our convention is to have the output parameters first.
> + if (status_code == 412)
> + ctx->contains_precondition_error = TRUE;
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -1044,6 +1093,7 @@ svn_ra_serf__handle_multistatus_only(serf_request_
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 999102)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -673,6 +673,9 @@ typedef struct svn_ra_serf__server_error_t {
> /* Have we seen an error tag? */
> svn_boolean_t in_error;
>
> + /* Have we seen a HTTP "412 Precondition Failed" error? */
> + svn_boolean_t contains_precondition_error;
> +
> /* Should we be collecting the XML cdata? */
> svn_boolean_t collect_cdata;
>
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Jon Foster <Jo...@cabot.co.uk>.
Daniel Shahaf wrote:
> Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
> > 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".
I looked and couldn't find any. The DAV spec doesn't list it as
one of the common errors that get sent in that place. (Obviously,
it would be very bad if mod_dav started sending that error code
for some other error).
> 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?
Sure. See attached.
> > - b->description->data);
> > + *b->description->data);
> ^^^
> Unintentional change? (triggers compiler warning)
Oops, yes. Fixed in attached.
This updated patch is against the atomic-revprop branch.
[[[
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.
* subversion/libsvn_ra_serf/ra_serf.h:
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c:
(parse_dav_status): New method.
(svn_ra_serf__handle_discard_body): Initialise new member.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
error code if applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster <jo...@cabot.co.uk>
]]]
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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 <jo...@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
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
> 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.
You always have the option of discussing your plans here or on IRC
before starting to write the patches. :-) Feel free to contact me
directly, too (via either mail or IRC /msg).
Thanks for jumping in!
Daniel