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

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