señior ¿tyrtle? wrote:
> Thanks for the quick review.
No problem. Thanks for revising the patch.
It is easier for me to look at your patches when you attach them with a MIME
type like text/plain, like you did before, rather than application.octet-stream
which it is this time. That's just a request for next time.
> [[[
> Added support for "svn proplist" to print in xml.
>
> * subversion/subversion/clients/cmdline/cl.h:
> (svn_cl__print_prop_hash): Remove declaration.
> It was only being used in proplist-cmd.c, and
> needed to be re-factored to print different formats.
>
> * subversion/subversion/clients/cmdline/props.c:
> (svn_cl__print_prop_hash): Remove.
>
> * subversion/subversion/clients/cmdline/proplist-cmd.c:
> (svn_cl__proplist) Modified to test for opt_state->xml and acts
> accordingly.
> -verbose switches between absolute and relative paths for xml.
> (print_prop_array) Added, based on (svn_cl__print_prop_hash).
> (print_prop_hash) Added, moved out from (svn_cl__proplist).
> (print_prop_xml) Added, prints a single property in xml.
> (print_prop_cl) Added, prints a single property to command-line.
> (group_props_xml) Added, prints a group of properties to xml.
> (group_props_cl) Added, prints a group of properties to xml.
> (begin_xml_proplist) Added. Start a property list xml stringbuf.
> (end_xml_proplist) Added. End a property list xml stringbuf.
>
> * subversion/subversion/clients/cmdline/main.c:
> (svn_cl__cmd_table) Add svn_cl__xml_opt for proplist command.
> ]]]
That's better. A bit of tidying up is still needed in the log message: colons
after close-parentheses; preferably "New" instead of "Added" for consistency
with other messages; indentation should be two spaces; there's an extra space
in the last line; symbols don't need to be in parentheses when they appear
within a sentence; the first two files would be better combined into a single
entry as the same change was made to both; error in description of
"group_props_cl". Thus:
[[[
Add an XML output mode for "svn proplist".
* subversion/subversion/clients/cmdline/cl.h,
subversion/subversion/clients/cmdline/props.c
(svn_cl__print_prop_hash): Remove. It was only being used in
proplist-cmd.c, and needed to be re-factored to print different formats.
* subversion/subversion/clients/cmdline/proplist-cmd.c:
(svn_cl__proplist): Test for opt_state->xml and act accordingly. The
"verbose" option switches between absolute and relative paths for xml.
[I'm not sure that the ability to choosing absolute or relative paths is a good
idea. Just do the same as we do in the other XML outputs.]
(print_prop_array): New, based on svn_cl__print_prop_hash.
(print_prop_hash): New, moved out from svn_cl__proplist.
(print_prop_xml): New. Print a single property in xml.
(print_prop_cl): New. Print a single property to command-line.
(group_props_xml): New. Print a group of properties to xml.
(group_props_cl): New. Print a group of properties to command-line.
(begin_xml_proplist): New. Start a property list xml stringbuf.
(end_xml_proplist): New. End a property list xml stringbuf.
* subversion/subversion/clients/cmdline/main.c:
(svn_cl__cmd_table): Add svn_cl__xml_opt for proplist command.
]]]
>>What if the property value is not UTF-8? This would produce invalid XML. Do
>>you have any ideas about what we should do in that case?
>
> I'm using svn_xml_is_xml_safe to test and encode to base64 on failure.
That sounds OK.
> It's working well here with binary/non-binary props...but there is the weird
> case of binary data actually being legal xml.
What do you mean? If you mean that certain possible property values that we
humans would call "binary" are actually composed only of valid XML characters,
and thus would not be base-64-encoded, I don't see that as a problem. The
primary purpose of this XML output is to represent the property value
unambiguously, not to offer an opinion on whether the value is meant to be
human-readable.
Please could you supply an XML DTD file to go with this, modeled on
subversion/clients/cmdline/dtd/*.dtd?
The patch:
Please could you produce a patch against a recent copy of the Subversion trunk
code? Your patch failed to apply.
> +/* A function that will print the given property. The property name is in pname,
> + value in propval. */
> +typedef svn_error_t* (*prop_print_function_t) (const char* pname,
> + svn_string_t *propval,
This can be a "const svn_string_t *", can't it?
Indentation of your function argument lists and continuation lines should be
such that all arguments start in the same column, and continuation lines start
at the column where the expression started at the corresponding level of
bracket nesting: e.g.:
foo (a,
b);
If the open-parenthesis was much too far to the right, move it down and indent
by two spaces:
foo_bar
(a,
b);
Don't use tab characters.
> + if (!svn_xml_is_xml_safe (propval->data, propval->len))
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "data", NULL);
> + const svn_string_t *bin_prop = svn_base64_encode_string (propval, pool);
We write for C89/C90, so a declaration is not allowed after a statement. Move
it up one line to the top of the block.
> @@ -43,11 +265,12 @@
> svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets;
> + svn_stringbuf_t *sb;
> int i;
>
> /* Suck up all remaining args in the target array. */
> SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
> - opt_state->targets, pool));
> + opt_state->targets, pool));
Here you've broken the existing indentation.
>
> /* Add "." if user passed 0 arguments */
> svn_opt_push_implicit_dot_target (targets, pool);
> @@ -68,7 +291,7 @@
> /* Either we have a URL target, or an implicit wc-path ('.')
> which needs to be converted to a URL. */
> if (targets->nelts <= 0)
> - return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
> + return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
Here you've corrected it, but ...
> _("No URL target available"));
... you didn't correct this line to match it. (White-space tweaking is
generally discouraged as part of a functional change, but it is OK to do a very
little bit of fixing in code that you are working on or near.)
Some final unfinished thoughts:
These function typedefs and "void *baton"s ... I'm not sure if they are being
used in an appropriate manner. I can't easily see until I can apply and
compile the patch. I don't like the bit where you cast a Boolean value into a
"void *" pointer value.
The doc strings need a bit of improvement/tidying. There are some non-obvious
arguments still undocumented. The project's convention is to write the
argument names in capital letters (except for public APIs where Doxygen mark-up
is used).
XML element names should match those in the existing DTDs where possible.
Thanks for taking it this far; I hope you still have time to go through another
round of patch and review.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 23 01:58:34 2005