Re: svn commit: r23282 - in trunk/subversion/svn: . schema

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