You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2004/09/27 21:47:52 UTC
[PATCH] formatting dav human-readable errors
When mod_dav_svn generates a m:human-readable error it gets sent via
Apache's mod_dav function dav_error_response_tag() like this:
ap_rprintf(r,
"<m:human-readable errcode=\"%d\">" DEBUG_CR
"%s" DEBUG_CR
"</m:human-readable>" DEBUG_CR,
err->error_id,
apr_xml_quote_string(r->pool, err->desc, 0));
and DEBUG_CR is defined in Apache's mod_dav.h like so:
#if 1
...
#define DEBUG_CR "\n"
...
#else
...
#define DEBUG_CR ""
...
#endif
The result is that the error sent to the client has additional
leading and trailing newlines, and when the client prints the error it
looks like
svn:
The version resource does not correspond to the resource within the transaction. Either the requested version resource is out of date (needs to be updated), or the requested version resource is newer than the transaction root (restart the commit).
I find it easier to parse if the leading newline is removed to give
svn: The version resource ...
Does anyone object to this:
* subversion/libsvn_ra_dav/util.c (end_err_element): Strip a leading
and trailing newline from ELEM_human_readable cdata.
Index: subversion/libsvn_ra_dav/util.c
===================================================================
--- subversion/libsvn_ra_dav/util.c (revision 11132)
+++ subversion/libsvn_ra_dav/util.c (working copy)
@@ -399,7 +399,18 @@
case ELEM_human_readable:
{
if (cdata && *err)
- (*err)->message = apr_pstrdup((*err)->pool, cdata);
+ {
+ /* dav_error_response_tag() on the server will add a leading
+ and trailing newline if DEBUG_CR is defined in mod_dav.h */
+ apr_size_t len;
+ if (*cdata == '\n')
+ ++cdata;
+ len = strlen(cdata);
+ if (cdata[len-1] == '\n')
+ --len;
+
+ (*err)->message = apr_pstrmemdup((*err)->pool, cdata, len);
+ }
break;
}
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by kf...@collab.net.
Ben Reser <be...@reser.org> writes:
> > No objection in principle, but do we consider our error output formats
> > to be patrt of the API? (I.e., is this something we want to change in
> > the 1.x line?)
>
> Umm it's XML, whitespace shouldn't be significant. Anyone parsing XML
> and depending on whitespace has a broken parser...
The error that is visibly printed out by (say) the command-line client
is not XML. It's just an error.
I'm not saying that we definitely can't change it, I'm just saying
it's not XML. (Similar bitstrings may have existed in an XML stream
somewhere along the way, somehow causally related to the error the
user sees before them, but that's an implementation detail.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Ben Reser <be...@reser.org>.
On Wed, Sep 29, 2004 at 04:38:42PM +0100, Philip Martin wrote:
> Ben Reser <be...@reser.org> writes:
>
> > Can you actually give a transcript of an example of when you see this
> > message.
>
> Commit an out-of-date file.
>
> $ svn ci foo
> Committed revision N.
> $ svn up -rN-1 foo
> $ echo x >> foo
> $ svn ci foo
>
> > I guess I'm failing to understand your explanation of the
> > problem you're solving. In your email you talk about the whitespace in
> > the XML.
>
> 1. mod_dav_svn generates an error text.
> 2. mod_dav adds whitespace when writing the XML.
> 3. The client parses the XML and extracts the error text complete with
> whitespace.
> 4. The client creates an svn_error_t and copies the error text complete
> with whitespace.
> 5. The svn_error_t gets returned back up the call stack in the client.
>
> I propose to remove the whitespace at step 4.
I don't see a problem with this. If someone is parsing our output they
probably are ignoring whitespace already or can easily start ignoring
it.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> Can you actually give a transcript of an example of when you see this
> message.
Commit an out-of-date file.
$ svn ci foo
Committed revision N.
$ svn up -rN-1 foo
$ echo x >> foo
$ svn ci foo
> I guess I'm failing to understand your explanation of the
> problem you're solving. In your email you talk about the whitespace in
> the XML.
1. mod_dav_svn generates an error text.
2. mod_dav adds whitespace when writing the XML.
3. The client parses the XML and extracts the error text complete with
whitespace.
4. The client creates an svn_error_t and copies the error text complete
with whitespace.
5. The svn_error_t gets returned back up the call stack in the client.
I propose to remove the whitespace at step 4.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Ben Reser <be...@reser.org>.
On Tue, Sep 28, 2004 at 10:41:35PM +0100, Philip Martin wrote:
> Ben Reser <be...@reser.org> writes:
>
> > On Tue, Sep 28, 2004 at 09:30:14PM +0100, Philip Martin wrote:
> >> It's not XML, it's a change to the text stored in an svn_error_t. I
> >> guess that means I need a better log message!
> >
> > The fact that it's in svn_error_t doesn't mean it's not XML. The errors
> > we return via DAV are XML and that's what this is. But that presents a
> > further argument as to why it's not part of our API...
>
> This is client side; it's all happening in the client's address space.
> The XML that comes from the server has already been parsed, that's not
> changing, all that's changing is an svn_error_t that gets returned up
> the call stack in the client.
Can you actually give a transcript of an example of when you see this
message. I guess I'm failing to understand your explanation of the
problem you're solving. In your email you talk about the whitespace in
the XML.
Now that I look at your patch more careful I guess you're skipping over
printing the whitespace that just gets acquired from the XML. Is that
the case? If so what exactly does it look like from the command line
client when you get this problem?
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> On Tue, Sep 28, 2004 at 09:30:14PM +0100, Philip Martin wrote:
>> It's not XML, it's a change to the text stored in an svn_error_t. I
>> guess that means I need a better log message!
>
> The fact that it's in svn_error_t doesn't mean it's not XML. The errors
> we return via DAV are XML and that's what this is. But that presents a
> further argument as to why it's not part of our API...
This is client side; it's all happening in the client's address space.
The XML that comes from the server has already been parsed, that's not
changing, all that's changing is an svn_error_t that gets returned up
the call stack in the client.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Ben Reser <be...@reser.org>.
On Tue, Sep 28, 2004 at 09:30:14PM +0100, Philip Martin wrote:
> It's not XML, it's a change to the text stored in an svn_error_t. I
> guess that means I need a better log message!
The fact that it's in svn_error_t doesn't mean it's not XML. The errors
we return via DAV are XML and that's what this is. But that presents a
further argument as to why it's not part of our API...
We've changed error messages before without ever thinking about if they
are creating problems for people parsing them. While this is a server
side message, client side messages are now being translated. So really
our message is opaque. It's intended to be an explanation to the user,
not something for a piece of software to parse.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> On Tue, Sep 28, 2004 at 09:52:41AM -0500, kfogel@collab.net wrote:
>> Philip Martin <ph...@codematters.co.uk> writes:
>> > Does anyone object to this:
>> >
>> > * subversion/libsvn_ra_dav/util.c (end_err_element): Strip a leading
>> > and trailing newline from ELEM_human_readable cdata.
>>
>> No objection in principle, but do we consider our error output formats
>> to be patrt of the API? (I.e., is this something we want to change in
>> the 1.x line?)
>
> Umm it's XML, whitespace shouldn't be significant. Anyone parsing XML
> and depending on whitespace has a broken parser...
It's not XML, it's a change to the text stored in an svn_error_t. I
guess that means I need a better log message!
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by Ben Reser <be...@reser.org>.
On Tue, Sep 28, 2004 at 09:52:41AM -0500, kfogel@collab.net wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
> > Does anyone object to this:
> >
> > * subversion/libsvn_ra_dav/util.c (end_err_element): Strip a leading
> > and trailing newline from ELEM_human_readable cdata.
>
> No objection in principle, but do we consider our error output formats
> to be patrt of the API? (I.e., is this something we want to change in
> the 1.x line?)
Umm it's XML, whitespace shouldn't be significant. Anyone parsing XML
and depending on whitespace has a broken parser...
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] formatting dav human-readable errors
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> Does anyone object to this:
>
> * subversion/libsvn_ra_dav/util.c (end_err_element): Strip a leading
> and trailing newline from ELEM_human_readable cdata.
No objection in principle, but do we consider our error output formats
to be patrt of the API? (I.e., is this something we want to change in
the 1.x line?)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org