Peter Davis peter@pdavis.cx writes:
[Note: I was unsure if there is a convention on whether new arguments
should always be added as the last argument -- ie., should is_attribute
come before *pool? Also, I'd appreciate if someone could run the tests
over dav, because I'm not set up to do that ATM.]
Actually, we generally try to leave the pool as the last argument and
squeeze new args in either just before it, or near other related args.
Index: subversion/include/svn_xml.h
===================================================================
--- subversion/include/svn_xml.h (revision 4632)
+++ subversion/include/svn_xml.h (working copy)
@@ -61,26 +61,30 @@
/** Create or append in @a *outstr an xml-escaped version of @a string.
*
* Create or append in @a *outstr an xml-escaped version of @a string,
- * suitable for output as character data or as an attribute value.
- * If @a *outstr is @c NULL, store a new stringbuf, else append to the
- * existing stringbuf there.
+ * suitable for output as character data. If @a is_attribute is true,
+ * then @a *outstr will additionally be suitable for output as an
+ * attribute value. If @a *outstr is @c NULL, store a new stringbuf,
+ * else append to the existing stringbuf there.
*/
Maybe If @a is_attribute is @c TRUE... ? We all-caps TRUE and FALSE
when describing values of boolean args (and of course, in the code
which tests for those values).
void svn_xml_escape_stringbuf (svn_stringbuf_t **outstr,
const svn_stringbuf_t *string,
- apr_pool_t *pool);
+ apr_pool_t *pool,
+ svn_boolean_t is_attribute);
Yeah, move is_attribute to just before the pool argument (same for the
other two escape functions).
/** Create or append in @a *outstr the unescaped version of the
Index: subversion/libsvn_subr/xml.c
===================================================================
--- subversion/libsvn_subr/xml.c (revision 4632)
+++ subversion/libsvn_subr/xml.c (working copy)
@@ -32,7 +32,8 @@
xml_escape (svn_stringbuf_t **outstr,
const char *data,
apr_size_t len,
- apr_pool_t *pool)
+ apr_pool_t *pool,
+ svn_boolean_t is_attribute)
{
const char *end = data + len;
const char *p = data, *q;
@@ -48,7 +49,9 @@
the time. */
q = p;
while (q end *q != '' *q != '' *q != ''
- *q != '' *q != '\'')
+ (!is_attribute || *q != ' ' *q != ''
+ *q != '\'' *q != '\n'
+ *q != '\r' *q != '\t'))
While this is technically correct, I'd rather see an extra set of
parens around the -ed stuff after !is_attribute || -- just makes
the reading easier to follow in moments where that part of your brain
that stores order-of-operations misfires. :-) Not crucial, mind you
(and Greg Stein is probably gritting his teeth already at my
suggestion), but it never hurts to make code super-clear.
@@ -67,6 +70,19 @@
svn_stringbuf_appendcstr (*outstr, );
else if (*q == '\'')
svn_stringbuf_appendcstr (*outstr, );
+ else if (*q == '\n')
+ svn_stringbuf_appendcstr (*outstr, );
+ else if (*q == '\r')
+ svn_stringbuf_appendcstr (*outstr, );
+ else if (*q == '\t')
+ svn_stringbuf_appendcstr (*outstr, );
+
+ /* Escaping normal spaces is technically unnecessary, but I'm
+ including this for (1) completeness, and (2) if ever support
+ is added for non-CDATA attribute types, this will become
+ necessary. */
+ else if (*q == ' ')
+ svn_stringbuf_appendcstr (*outstr, );
p = q + 1;
}
@@ -165,27 +181,30 @@
I'd really prefer *not* to escape spaces. Perhaps you could #if 0 out
that space-escaping portions, leaving your comments present in case we
do in fact need to enable that feature. Spaces are way more common
than any other whitespace character in our attributes, and the less
escaping we actually do, the better for readability.
Index: subversion/libsvn_ra_dav/commit.c
===================================================================
--- subversion/libsvn_ra_dav/commit.c (revision 4632)
+++ subversion/libsvn_ra_dav/commit.c (working copy)
@@ -515,7 +515,10 @@
if (r-prop_changes == NULL)
r-prop_changes = apr_table_make(pool, 5);
- svn_xml_escape_string(escaped, value, pool);
+ /* Not sure whether this will eventually be output as an XML
+ attribute, but it can't hurt to escape it if
+ not. --pediddle */
+ svn_xml_escape_string(escaped, value, pool, TRUE);
apr_table_set(r-prop_changes, name, escaped-data);
}
else
These property values will be CDATA. See do_proppatch() in that same
file.
Nice work. A few cleanups and it's good to go. Care to resubmit?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 14 02:22:04 2006