You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Peter Lundblad <pl...@google.com> on 2007/01/30 14:59:10 UTC
Re: svn commit: r23282 - in trunk/subversion/svn: . schema
Hi,
hwright@tigris.org writes:
> Modified: trunk/subversion/svn/cl.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/cl.h?pathrev=23282&r1=23281&r2=23282
> ==============================================================================
> --- trunk/subversion/svn/cl.h (original)
> +++ trunk/subversion/svn/cl.h Mon Jan 29 14:45:40 2007
> @@ -282,6 +282,14 @@
> svn_boolean_t names_only,
> apr_pool_t *pool);
>
> +/* Same as svn_cl__print_prop_hash(), only output xml to SB. If SB is NULL,
> + allocate it first from pool, otherwise append the xml to it. */
Parameter name is outstr, not sb.
> +svn_error_t *
> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
> + apr_hash_t *prop_hash,
> + svn_boolean_t names_only,
> + apr_pool_t *pool);
> +
> /* Do the following things that are commonly required before accessing revision
> properties. Ensure that REVISION is specified explicitly and is not
> relative to a working-copy item. Ensure that exactly one target is
>
> Modified: trunk/subversion/svn/main.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/main.c?pathrev=23282&r1=23281&r2=23282
> ==============================================================================
> --- trunk/subversion/svn/main.c (original)
> +++ trunk/subversion/svn/main.c Mon Jan 29 14:45:40 2007
> @@ -613,7 +613,7 @@
> " 2. Lists unversioned remote props on repos revision.\n"
> " TARGET only determines which repository to access.\n"),
> {'v', 'R', 'r', 'q', svn_cl__revprop_opt, SVN_CL__AUTH_OPTIONS,
> - svn_cl__config_dir_opt} },
> + svn_cl__config_dir_opt, svn_cl__xml_opt} },
>
> { "propset", svn_cl__propset, {"pset", "ps"}, N_
> ("Set the value of a property on files, dirs, or revisions.\n"
>
> Modified: trunk/subversion/svn/proplist-cmd.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/proplist-cmd.c?pathrev=23282&r1=23281&r2=23282
> ==============================================================================
> --- trunk/subversion/svn/proplist-cmd.c (original)
> +++ trunk/subversion/svn/proplist-cmd.c Mon Jan 29 14:45:40 2007
> @@ -124,19 +162,36 @@
> /* Check for a peg revision. */
> SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
> subpool));
> -
> - SVN_ERR(svn_cl__try
> - (svn_client_proplist3(truepath, &peg_revision,
> - &(opt_state->start_revision),
> - opt_state->recursive,
> - proplist_receiver,
> - &pl_baton,
> - ctx, subpool),
> - NULL, opt_state->quiet,
> - SVN_ERR_UNVERSIONED_RESOURCE,
> - SVN_ERR_ENTRY_NOT_FOUND,
> - SVN_NO_ERROR));
> +
> + if (opt_state->xml)
> + SVN_ERR(svn_cl__try
> + (svn_client_proplist3(truepath, &peg_revision,
> + &(opt_state->start_revision),
> + opt_state->recursive,
> + proplist_receiver_xml,
> + &pl_baton,
> + ctx, subpool),
> + NULL, opt_state->quiet,
> + SVN_ERR_UNVERSIONED_RESOURCE,
> + SVN_ERR_ENTRY_NOT_FOUND,
> + SVN_NO_ERROR));
> + else
> + SVN_ERR(svn_cl__try
> + (svn_client_proplist3(truepath, &peg_revision,
> + &(opt_state->start_revision),
> + opt_state->recursive,
> + proplist_receiver,
> + &pl_baton,
> + ctx, subpool),
> + NULL, opt_state->quiet,
> + SVN_ERR_UNVERSIONED_RESOURCE,
> + SVN_ERR_ENTRY_NOT_FOUND,
> + SVN_NO_ERROR));
It looks like the only thing that distinguishes these two calls is the
receiver parameter. I think it is more readable to either put the
opt_state->xml condition inline in the function call or assign a variable
before to avoid this duplication. What do you think?
> Modified: trunk/subversion/svn/props.c
> @@ -106,3 +107,50 @@
>
> return SVN_NO_ERROR;
> }
> +
> +svn_error_t *
> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
> + apr_hash_t *prop_hash,
> + svn_boolean_t names_only,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
> +
> + if (*outstr == NULL)
> + *outstr = svn_stringbuf_create("", pool);
> +
> + for (hi = apr_hash_first(pool, prop_hash); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + void *val;
> + const char *pname;
> + svn_string_t *propval;
> +
> + apr_hash_this(hi, &key, NULL, &val);
> + pname = key;
> + propval = val;
> +
> + if (names_only)
> + {
> + svn_xml_make_open_tag(outstr, pool, svn_xml_self_closing, "property",
> + "name", pname, NULL);
> + }
> + else
> + {
> + const char *pname_out;
> +
> + if (svn_prop_needs_translation(pname))
> + SVN_ERR(svn_subst_detranslate_string(&propval, propval,
> + TRUE, pool));
> +
> + SVN_ERR(svn_cmdline_cstring_from_utf8(&pname_out, pname, pool));
> +
> + svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
> + "property", "name", pname, NULL);
> + svn_xml_escape_cdata_string(outstr, propval, pool);
If we have a property that doesn't start with "svn:", we can't assume
that it is valid UTF8, so this call will produce non-well-formed XML.
This is also the case if the value contains characters that are not legal in
XML, such as certain control characters.
So, we need to encode values that are not well-formed, in a similar way that
we do in our DAV implementation.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r23282 - in trunk/subversion/svn: . schema
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Rall wrote:
> On Tue, 30 Jan 2007, Hyrum K. Wright wrote:
>
>> Peter Lundblad wrote:
...
>>>> Modified: trunk/subversion/svn/props.c
>>>> @@ -106,3 +107,50 @@
>>>>
>>>> return SVN_NO_ERROR;
>>>> }
>>>> +
>>>> +svn_error_t *
>>>> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
>>>> + apr_hash_t *prop_hash,
>>>> + svn_boolean_t names_only,
>>>> + apr_pool_t *pool)
>>>> +{
>>>> + apr_hash_index_t *hi;
>>>> +
>>>> + if (*outstr == NULL)
>>>> + *outstr = svn_stringbuf_create("", pool);
>>>> +
>>>> + for (hi = apr_hash_first(pool, prop_hash); hi; hi = apr_hash_next(hi))
>>>> + {
>>>> + const void *key;
>>>> + void *val;
>>>> + const char *pname;
>>>> + svn_string_t *propval;
>>>> +
>>>> + apr_hash_this(hi, &key, NULL, &val);
>>>> + pname = key;
>>>> + propval = val;
>>>> +
>>>> + if (names_only)
>>>> + {
>>>> + svn_xml_make_open_tag(outstr, pool, svn_xml_self_closing, "property",
>>>> + "name", pname, NULL);
>>>> + }
>>>> + else
>>>> + {
>>>> + const char *pname_out;
>>>> +
>>>> + if (svn_prop_needs_translation(pname))
>>>> + SVN_ERR(svn_subst_detranslate_string(&propval, propval,
>>>> + TRUE, pool));
>>>> +
>>>> + SVN_ERR(svn_cmdline_cstring_from_utf8(&pname_out, pname, pool));
>>>> +
>>>> + svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
>>>> + "property", "name", pname, NULL);
>>>> + svn_xml_escape_cdata_string(outstr, propval, pool);
>>> If we have a property that doesn't start with "svn:", we can't assume
>>> that it is valid UTF8, so this call will produce non-well-formed XML.
>>> This is also the case if the value contains characters that are not legal in
>>> XML, such as certain control characters.
>>> So, we need to encode values that are not well-formed, in a similar way that
>>> we do in our DAV implementation.
>> Would something like the following patch address the problem?
>
> Seems okay to me, but I'm not particularly familiar with this area of
> the libraries, either.
After some local testing, this solution appears to work. I've committed it in r23296.
-Hyrum
Re: svn commit: r23282 - in trunk/subversion/svn: . schema
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 30 Jan 2007, Hyrum K. Wright wrote:
> Peter Lundblad wrote:
...
> >> --- trunk/subversion/svn/proplist-cmd.c (original)
> >> +++ trunk/subversion/svn/proplist-cmd.c Mon Jan 29 14:45:40 2007
> >> @@ -124,19 +162,36 @@
> >> /* Check for a peg revision. */
> >> SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
> >> subpool));
> >> -
> >> - SVN_ERR(svn_cl__try
> >> - (svn_client_proplist3(truepath, &peg_revision,
> >> - &(opt_state->start_revision),
> >> - opt_state->recursive,
> >> - proplist_receiver,
> >> - &pl_baton,
> >> - ctx, subpool),
> >> - NULL, opt_state->quiet,
> >> - SVN_ERR_UNVERSIONED_RESOURCE,
> >> - SVN_ERR_ENTRY_NOT_FOUND,
> >> - SVN_NO_ERROR));
> >> +
> >> + if (opt_state->xml)
> >> + SVN_ERR(svn_cl__try
> >> + (svn_client_proplist3(truepath, &peg_revision,
> >> + &(opt_state->start_revision),
> >> + opt_state->recursive,
> >> + proplist_receiver_xml,
> >> + &pl_baton,
> >> + ctx, subpool),
> >> + NULL, opt_state->quiet,
> >> + SVN_ERR_UNVERSIONED_RESOURCE,
> >> + SVN_ERR_ENTRY_NOT_FOUND,
> >> + SVN_NO_ERROR));
> >> + else
> >> + SVN_ERR(svn_cl__try
> >> + (svn_client_proplist3(truepath, &peg_revision,
> >> + &(opt_state->start_revision),
> >> + opt_state->recursive,
> >> + proplist_receiver,
> >> + &pl_baton,
> >> + ctx, subpool),
> >> + NULL, opt_state->quiet,
> >> + SVN_ERR_UNVERSIONED_RESOURCE,
> >> + SVN_ERR_ENTRY_NOT_FOUND,
> >> + SVN_NO_ERROR));
> >
> > It looks like the only thing that distinguishes these two calls is the
> > receiver parameter. I think it is more readable to either put the
> > opt_state->xml condition inline in the function call or assign a variable
> > before to avoid this duplication. What do you think?
>
> The thought had occurred to me. I prefer an a function pointer to the
> correct receiver, but I'm not opposed to either method. Is there a
> convention or project preference regarding such?
Oh, I noticed the same thing before I read this discussion, and used a
local svn_proplist_receiver_t variable in r23295.
Funny thing, I originally had inlined the opt_state->xml check in the
parameter list of the svn_client_proplist3() call, but moved it
outside the loop after I noticed the same check outside the loop (just
above).
AFAIK, we don't have a set pattern for this, but do whatever seems
most easily maintainable given the situation.
> >> Modified: trunk/subversion/svn/props.c
> >> @@ -106,3 +107,50 @@
> >>
> >> return SVN_NO_ERROR;
> >> }
> >> +
> >> +svn_error_t *
> >> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
> >> + apr_hash_t *prop_hash,
> >> + svn_boolean_t names_only,
> >> + apr_pool_t *pool)
> >> +{
> >> + apr_hash_index_t *hi;
> >> +
> >> + if (*outstr == NULL)
> >> + *outstr = svn_stringbuf_create("", pool);
> >> +
> >> + for (hi = apr_hash_first(pool, prop_hash); hi; hi = apr_hash_next(hi))
> >> + {
> >> + const void *key;
> >> + void *val;
> >> + const char *pname;
> >> + svn_string_t *propval;
> >> +
> >> + apr_hash_this(hi, &key, NULL, &val);
> >> + pname = key;
> >> + propval = val;
> >> +
> >> + if (names_only)
> >> + {
> >> + svn_xml_make_open_tag(outstr, pool, svn_xml_self_closing, "property",
> >> + "name", pname, NULL);
> >> + }
> >> + else
> >> + {
> >> + const char *pname_out;
> >> +
> >> + if (svn_prop_needs_translation(pname))
> >> + SVN_ERR(svn_subst_detranslate_string(&propval, propval,
> >> + TRUE, pool));
> >> +
> >> + SVN_ERR(svn_cmdline_cstring_from_utf8(&pname_out, pname, pool));
> >> +
> >> + svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
> >> + "property", "name", pname, NULL);
> >> + svn_xml_escape_cdata_string(outstr, propval, pool);
> >
> > If we have a property that doesn't start with "svn:", we can't assume
> > that it is valid UTF8, so this call will produce non-well-formed XML.
> > This is also the case if the value contains characters that are not legal in
> > XML, such as certain control characters.
> > So, we need to encode values that are not well-formed, in a similar way that
> > we do in our DAV implementation.
>
> Would something like the following patch address the problem?
Seems okay to me, but I'm not particularly familiar with this area of
the libraries, either.
- Dan
Re: svn commit: r23282 - in trunk/subversion/svn: . schema
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Peter Lundblad wrote:
> Hi,
>
> hwright@tigris.org writes:
>> Modified: trunk/subversion/svn/cl.h
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/cl.h?pathrev=23282&r1=23281&r2=23282
>> ==============================================================================
>> --- trunk/subversion/svn/cl.h (original)
>> +++ trunk/subversion/svn/cl.h Mon Jan 29 14:45:40 2007
>> @@ -282,6 +282,14 @@
>> svn_boolean_t names_only,
>> apr_pool_t *pool);
>>
>> +/* Same as svn_cl__print_prop_hash(), only output xml to SB. If SB is NULL,
>> + allocate it first from pool, otherwise append the xml to it. */
>
> Parameter name is outstr, not sb.
Thanks for catching that. Fixed in r23291.
>> +svn_error_t *
>> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
>> + apr_hash_t *prop_hash,
>> + svn_boolean_t names_only,
>> + apr_pool_t *pool);
>> +
>> /* Do the following things that are commonly required before accessing revision
>> properties. Ensure that REVISION is specified explicitly and is not
>> relative to a working-copy item. Ensure that exactly one target is
>>
>> Modified: trunk/subversion/svn/main.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/main.c?pathrev=23282&r1=23281&r2=23282
>> ==============================================================================
>> --- trunk/subversion/svn/main.c (original)
>> +++ trunk/subversion/svn/main.c Mon Jan 29 14:45:40 2007
>> @@ -613,7 +613,7 @@
>> " 2. Lists unversioned remote props on repos revision.\n"
>> " TARGET only determines which repository to access.\n"),
>> {'v', 'R', 'r', 'q', svn_cl__revprop_opt, SVN_CL__AUTH_OPTIONS,
>> - svn_cl__config_dir_opt} },
>> + svn_cl__config_dir_opt, svn_cl__xml_opt} },
>>
>> { "propset", svn_cl__propset, {"pset", "ps"}, N_
>> ("Set the value of a property on files, dirs, or revisions.\n"
>>
>> Modified: trunk/subversion/svn/proplist-cmd.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/proplist-cmd.c?pathrev=23282&r1=23281&r2=23282
>> ==============================================================================
>> --- trunk/subversion/svn/proplist-cmd.c (original)
>> +++ trunk/subversion/svn/proplist-cmd.c Mon Jan 29 14:45:40 2007
>> @@ -124,19 +162,36 @@
>> /* Check for a peg revision. */
>> SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
>> subpool));
>> -
>> - SVN_ERR(svn_cl__try
>> - (svn_client_proplist3(truepath, &peg_revision,
>> - &(opt_state->start_revision),
>> - opt_state->recursive,
>> - proplist_receiver,
>> - &pl_baton,
>> - ctx, subpool),
>> - NULL, opt_state->quiet,
>> - SVN_ERR_UNVERSIONED_RESOURCE,
>> - SVN_ERR_ENTRY_NOT_FOUND,
>> - SVN_NO_ERROR));
>> +
>> + if (opt_state->xml)
>> + SVN_ERR(svn_cl__try
>> + (svn_client_proplist3(truepath, &peg_revision,
>> + &(opt_state->start_revision),
>> + opt_state->recursive,
>> + proplist_receiver_xml,
>> + &pl_baton,
>> + ctx, subpool),
>> + NULL, opt_state->quiet,
>> + SVN_ERR_UNVERSIONED_RESOURCE,
>> + SVN_ERR_ENTRY_NOT_FOUND,
>> + SVN_NO_ERROR));
>> + else
>> + SVN_ERR(svn_cl__try
>> + (svn_client_proplist3(truepath, &peg_revision,
>> + &(opt_state->start_revision),
>> + opt_state->recursive,
>> + proplist_receiver,
>> + &pl_baton,
>> + ctx, subpool),
>> + NULL, opt_state->quiet,
>> + SVN_ERR_UNVERSIONED_RESOURCE,
>> + SVN_ERR_ENTRY_NOT_FOUND,
>> + SVN_NO_ERROR));
>
> It looks like the only thing that distinguishes these two calls is the
> receiver parameter. I think it is more readable to either put the
> opt_state->xml condition inline in the function call or assign a variable
> before to avoid this duplication. What do you think?
The thought had occurred to me. I prefer an a function pointer to the
correct receiver, but I'm not opposed to either method. Is there a
convention or project preference regarding such?
>> Modified: trunk/subversion/svn/props.c
>> @@ -106,3 +107,50 @@
>>
>> return SVN_NO_ERROR;
>> }
>> +
>> +svn_error_t *
>> +svn_cl__print_xml_prop_hash(svn_stringbuf_t **outstr,
>> + apr_hash_t *prop_hash,
>> + svn_boolean_t names_only,
>> + apr_pool_t *pool)
>> +{
>> + apr_hash_index_t *hi;
>> +
>> + if (*outstr == NULL)
>> + *outstr = svn_stringbuf_create("", pool);
>> +
>> + for (hi = apr_hash_first(pool, prop_hash); hi; hi = apr_hash_next(hi))
>> + {
>> + const void *key;
>> + void *val;
>> + const char *pname;
>> + svn_string_t *propval;
>> +
>> + apr_hash_this(hi, &key, NULL, &val);
>> + pname = key;
>> + propval = val;
>> +
>> + if (names_only)
>> + {
>> + svn_xml_make_open_tag(outstr, pool, svn_xml_self_closing, "property",
>> + "name", pname, NULL);
>> + }
>> + else
>> + {
>> + const char *pname_out;
>> +
>> + if (svn_prop_needs_translation(pname))
>> + SVN_ERR(svn_subst_detranslate_string(&propval, propval,
>> + TRUE, pool));
>> +
>> + SVN_ERR(svn_cmdline_cstring_from_utf8(&pname_out, pname, pool));
>> +
>> + svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
>> + "property", "name", pname, NULL);
>> + svn_xml_escape_cdata_string(outstr, propval, pool);
>
> If we have a property that doesn't start with "svn:", we can't assume
> that it is valid UTF8, so this call will produce non-well-formed XML.
> This is also the case if the value contains characters that are not legal in
> XML, such as certain control characters.
> So, we need to encode values that are not well-formed, in a similar way that
> we do in our DAV implementation.
Would something like the following patch address the problem?
Index: subversion/svn/props.c
===================================================================
--- subversion/svn/props.c (revision 23290)
+++ subversion/svn/props.c (working copy)
@@ -30,6 +30,7 @@
#include "svn_props.h"
#include "svn_opt.h"
#include "svn_xml.h"
+#include "svn_base64.h"
#include "cl.h"
#include "svn_private_config.h"
@@ -138,6 +139,8 @@
else
{
const char *pname_out;
+ const char *xml_safe;
+ const char *encoding = NULL;
if (svn_prop_needs_translation(pname))
SVN_ERR(svn_subst_detranslate_string(&propval, propval,
@@ -145,9 +148,30 @@
SVN_ERR(svn_cmdline_cstring_from_utf8(&pname_out, pname, pool));
- svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
- "property", "name", pname, NULL);
- svn_xml_escape_cdata_string(outstr, propval, pool);
+ if (svn_xml_is_xml_safe(propval->data, propval->len))
+ {
+ svn_stringbuf_t *xml_esc = NULL;
+ svn_xml_escape_cdata_string(&xml_esc, propval, pool);
+ xml_safe = xml_esc->data;
+ }
+ else
+ {
+ const svn_string_t *base64ed =
svn_base64_encode_string(propval,
+
pool);
+ encoding = "base64";
+ xml_safe = base64ed->data;
+ }
+
+ if (encoding)
+ svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
+ "property", "name", pname,
+ "encoding", encoding, NULL);
+ else
+ svn_xml_make_open_tag(outstr, pool, svn_xml_protect_pcdata,
+ "property", "name", pname, NULL);
+
+ svn_stringbuf_appendcstr(*outstr, xml_safe);
+
svn_xml_make_close_tag(outstr, pool, "property");
}
}
-Hyrum