You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2003/09/13 14:39:35 UTC
ne_proppatch(), issue #1520, dead-end.
Oh crud. Greg and I have been discussing a solution to issue #1520.
Basically, we send a PROPPATCH to change a revision property, and we
get back a 207 response that contains a 500 error internally: "no
revprop hook present." The user never sees the error, a
The problem is that we're using homemade proppatch() code, and it
never checks the body of a 207 response. It assumes 207 ==
SVN_NO_ERROR unconditionally, even if the 207 response contains real
error codes. This is why the 'no-revprop-hook-script' error never gets
propagated back the client, and the client sees a lie: "set svn:log
property".
The obvious thing to do, we thought, was to start using
ne_proppatch(). Presumably it search 207 repsonse bodies for errors.
(Right, Joe?)
So I rewrote svn_ra_dav__do_proppatch() to simply call ne_proppatch().
ne_proppatch() requires an array of 'ne_proppatch_operation'
structures:
typedef struct {
const ne_propname *name;
enum {
ne_propset,
ne_propremove
} type;
const char *value;
} ne_proppatch_operation;
No big deal, really. But I noticed this bit of code in do_setprop(),
which is the helper func responsible for parsing the name/value data
(and is now building operation structures):
/* If a property is XML-safe, XML-encode it. Else, base64-encode
it. */
if (svn_xml_is_xml_safe(value->data, value->len))
{
svn_stringbuf_t *xml_esc = NULL;
svn_xml_escape_cdata_string(&xml_esc, value, pool);
xml_safe = xml_esc->data;
}
else
{
const svn_string_t *base64ed = svn_base64_encode_string(value, pool);
encoding = " V:encoding=\"base64\"";
xml_safe = base64ed->data;
}
ne_buffer_concat(body, "<", xml_tag_name, encoding, ">",
xml_safe, "</", xml_tag_name, ">", NULL);
I totally forgot that we potentially have an svn-specific 'encoding'
attribute attached to our property tags:
<D:set>
<D:prop>
<C:moo V:encoding="base64">ECAE234D3234AXRCR</C:moo>
</D:prop>
</D:set>
I don't think there's a way to get this feature using ne_proppatch().
I'm cc'ing Joe -- Joe, is that correct? Is there some clever thing I
can do? We not only need the ability to attach a custom attribute,
but also to have neon declare the xml namespace in the enclosing
<D:propertyupdate> tag.
Maybe this 'encoding' tag is the whole reason we have custom proppatch
code at all? Cmpilato was the one who implemented the custom encoding
feature.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ne_proppatch(), issue #1520, dead-end.
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Sat, Sep 13, 2003 at 01:02:55PM -0500, Ben Collins-Sussman wrote:
> Joe Orton <jo...@manyfish.co.uk> writes:
>
> > > The obvious thing to do, we thought, was to start using
> > > ne_proppatch(). Presumably it search 207 repsonse bodies for errors.
> > > (Right, Joe?)
> >
> > It does, but you get all that by just using ne_simple_request() to
> > dispatch your custom request already; that's all that ne_proppatch()
> > does anyway. (the error handling is dumb, I presume you don't need to
> > know exactly what the error was in a useful way)
>
> Our current proppatch code calls ne_request_dispatch(), not
> ne_simple_request(). Maybe I should investigate switching over to the
> latter function. Will it be able to pull out a 500 error from within
> a 207 response?
Yes, exactly that - ne_simple_request() dispatches the request and
parses a 207 response if one is returned, returning NE_ERROR for either
a non-2xx response code, or a 207 response which had a non-2xx propstat
inside it anywhere.
(I notice there's an undocumented gotcha, ne_simple_request destroys the
request object too)
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ne_proppatch(), issue #1520, dead-end.
Posted by Ben Collins-Sussman <su...@collab.net>.
Joe Orton <jo...@manyfish.co.uk> writes:
> > The obvious thing to do, we thought, was to start using
> > ne_proppatch(). Presumably it search 207 repsonse bodies for errors.
> > (Right, Joe?)
>
> It does, but you get all that by just using ne_simple_request() to
> dispatch your custom request already; that's all that ne_proppatch()
> does anyway. (the error handling is dumb, I presume you don't need to
> know exactly what the error was in a useful way)
Our current proppatch code calls ne_request_dispatch(), not
ne_simple_request(). Maybe I should investigate switching over to the
latter function. Will it be able to pull out a 500 error from within
a 207 response?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ne_proppatch(), issue #1520, dead-end.
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Sat, Sep 13, 2003 at 09:39:35AM -0500, Ben Collins-Sussman wrote:
>
> Oh crud. Greg and I have been discussing a solution to issue #1520.
> Basically, we send a PROPPATCH to change a revision property, and we
> get back a 207 response that contains a 500 error internally: "no
> revprop hook present." The user never sees the error, a
>
> The problem is that we're using homemade proppatch() code, and it
> never checks the body of a 207 response. It assumes 207 ==
> SVN_NO_ERROR unconditionally, even if the 207 response contains real
> error codes. This is why the 'no-revprop-hook-script' error never gets
> propagated back the client, and the client sees a lie: "set svn:log
> property".
>
> The obvious thing to do, we thought, was to start using
> ne_proppatch(). Presumably it search 207 repsonse bodies for errors.
> (Right, Joe?)
It does, but you get all that by just using ne_simple_request() to
dispatch your custom request already; that's all that ne_proppatch()
does anyway. (the error handling is dumb, I presume you don't need to
know exactly what the error was in a useful way)
> So I rewrote svn_ra_dav__do_proppatch() to simply call ne_proppatch().
> ne_proppatch() requires an array of 'ne_proppatch_operation'
> structures:
>
> typedef struct {
> const ne_propname *name;
> enum {
> ne_propset,
> ne_propremove
> } type;
> const char *value;
> } ne_proppatch_operation;
>
>
> No big deal, really. But I noticed this bit of code in do_setprop(),
> which is the helper func responsible for parsing the name/value data
> (and is now building operation structures):
...
> I totally forgot that we potentially have an svn-specific 'encoding'
> attribute attached to our property tags:
>
> <D:set>
> <D:prop>
> <C:moo V:encoding="base64">ECAE234D3234AXRCR</C:moo>
> </D:prop>
> </D:set>
>
>
> I don't think there's a way to get this feature using ne_proppatch().
> I'm cc'ing Joe -- Joe, is that correct? Is there some clever thing I
> can do? We not only need the ability to attach a custom attribute,
> but also to have neon declare the xml namespace in the enclosing
> <D:propertyupdate> tag.
Correct, you can't do it now, but it would be trivial to change the
ne_proppatch interface to take an attributes string. Is declaring the
namespace on the element itself really not sufficient? I don't know the
rules on attribute namespaces. (It looks like mod_dav_svn ignores the
namespace anyway currently)
Regards,
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org