señior ¿tyrtle? wrote:
> [[[
> Added support for "svn proplist" to print in xml.
>
> * subversion/subversion/clients/cmdline/cl.h:
> Removed declaration of svn_cl__print_prop_hash.
Please put the name of each file-level symbol that you modify in parentheses, thus:
(svn_cl__print_prop_hash): Remove.
The same throughout the log message...
> 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:
> Removed svn_cl__print_prop_hash.
>
> * subversion/subversion/clients/cmdline/proplist-cmd.c:
> Added support for printing properties in xml.
> Moved the old extern svn_cl__print_prop_hash function
> to static svn__print_prop_hash.
>
> * subversion/subversion/clients/cmdline/main.c:
> Allow "--xml" for proplist command.
> ]]]
>
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/subversion/clients/cmdline/props.c
> ===================================================================
[...]
> -}
> +}
> \ No newline at end of file
Please avoid removing the newline at the end of the last line of the file.
> Index: subversion/subversion/clients/cmdline/proplist-cmd.c
> ===================================================================
> --- subversion/subversion/clients/cmdline/proplist-cmd.c (revision 17)
> +++ subversion/subversion/clients/cmdline/proplist-cmd.c (working copy)
[...]
> @@ -34,6 +36,178 @@
>
> /*** Code. ***/
>
> +typedef svn_error_t* (*prop_print_function_t)(const char* pname,
Space before parenthesis?
This needs its own documentation comment. Please provide one for each new
function (etc.) that you write. It would also be very nice if you could
provide doc strings for the functions that you have moved from elsewhere, but
you don't have to do that. I have noted where comments are missing below,
without regard to whether the functions are new or moved.
Indentation of following lines.
> + svn_string_t *propval,
> + void *refcon,
> + apr_pool_t *pool);
> +
> +/* Print a hash that maps property names (char *) to property values
> + (svn_string_t *). The names are assumed to be in UTF-8 format;
> + the values are either in UTF-8 (the special Subversion props) or
> + plain binary values.
> +
> + Pass the property name and a (possibly de-translated) value
> + to the print function. */
> +
> +static svn_error_t *
> +svn__print_prop_hash (apr_hash_t *prop_hash,
Local functions don't need a prefix ("svn__").
Indentation of following lines.
> + prop_print_function_t print_func,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
> +
> + 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 (svn_prop_needs_translation (pname))
> + SVN_ERR (svn_subst_detranslate_string (&propval, propval,
> + TRUE, pool));
> +
> + SVN_ERR (print_func(pname, propval, baton, pool));
Need a space before the second opening parenthesis.
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Iterate an array of props, where each element is a hash that maps
> + property names (char *) to property values (svn_string_t *).
This comment looks like it belongs to svn__print_props, not to
prop_path_print_function_t. Put it just before the thing that it applies to.
> +
> + Pass one hash and the current property's path to the receiving print-function.
> + */
If you put the close-comment marker on its own line, please align the asterisk
with the asterisk of the opening marker.
> +
> +typedef svn_error_t* (*prop_path_print_function_t)(apr_hash_t *prop_hash,
This needs its own documentation comment.
Space before parenthesis?
Indentation of following lines.
> + const char *path,
> + void *baton,
> + apr_pool_t *pool);
> +
> +static svn_error_t*
> +svn__print_props(apr_array_header_t *props,
Bring doc string from above. No prefix needed. Space before paren.
Indentation of following lines.
> + svn_boolean_t is_url,
> + prop_path_print_function_t print_func,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + int j;
> + for (j = 0; j < props->nelts; ++j)
> + {
> + svn_client_proplist_item_t *item
> + = ((svn_client_proplist_item_t **)props->elts)[j];
> + const char *name_local;
> +
> + if (! is_url)
> + name_local = svn_path_local_style (item->node_name->data,
> + pool);
> + else
> + name_local = item->node_name->data;
> +
> + SVN_ERR (print_func(item->prop_hash, name_local, baton, pool));
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +svn__do_print_prop_xml (const char *pname,
Doc string? Prefix. Indentation of following lines.
> + svn_string_t *propval,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = (svn_stringbuf_t*) baton;
Indentation.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "key",
> + NULL);
Indentation.
> + svn_xml_escape_cdata_cstring (&sb, pname, pool);
> + svn_xml_make_close_tag (&sb, pool, "key");
> +
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "value",
> + NULL);
Indentation.
> + svn_xml_escape_cdata_cstring (&sb, propval->data, pool);
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?
> + svn_xml_make_close_tag (&sb, pool, "value");
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Print a bunch of properties in xml.
> +
> + Basically just a wrapper to get all the properties within
> + a <properties></properties tag */
Missing ">" and ".". Wrapper for what? Ideally, say how the arguments are
used (e.g. does it append to any string that might already be in "sb"?).
> +
> +static svn_error_t *
> +svn__print_prop_hash_xml (apr_hash_t *prop_hash,
Doc string? Prefix. Indentation of following lines.
> + svn_stringbuf_t *sb,
> + apr_pool_t *pool)
> +{
> + /* <properties> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "properties",
> + NULL);
> +
> + SVN_ERR (svn__print_prop_hash(prop_hash, svn__do_print_prop_xml, sb, pool));
Space before paren.
> +
> + /* </properties> */
> + svn_xml_make_close_tag (&sb, pool, "properties");
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +svn__do_print_prop_cl (const char *pname,
Doc string? Prefix. Indentation of following lines.
> + svn_string_t *propval,
> + void *names_only,
> + apr_pool_t *pool)
> +{
> + const char *pname_stdout;
> + SVN_ERR (svn_cmdline_cstring_from_utf8 (&pname_stdout, pname, pool));
> + /* ### We leave these printfs for now, since if propval wasn't translated
> + * above, we don't know anything about its encoding. In fact, it
> + * might be binary data... */
(This comment is ugly, but I see you just moved it from elsewhere, so you don't
need to change it.)
> + if (names_only)
> + printf (" %s\n", pname_stdout);
> + else
> + printf (" %s : %s\n", pname_stdout, propval->data);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* Print a property as xml */
> +
> +static svn_error_t*
> +svn__do_print_props_xml(apr_hash_t *prop_hash,
Doc string? Prefix. Indentation of following lines.
> + const char *path,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = (svn_stringbuf_t*) baton;
> + /* <path> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "path", NULL);
> + svn_xml_escape_cdata_cstring (&sb, path, pool);
> + svn_xml_make_close_tag (&sb, pool, "path");
> + /* </path> */
> +
> + SVN_ERR (svn__print_prop_hash_xml(prop_hash, sb, pool));
> +}
> +
> +/* Print a property. */
> +
> +static svn_error_t*
> +svn__do_print_props_cl(apr_hash_t *prop_hash,
Doc string? Prefix. Indentation of following lines.
> + const char *path,
> + void *baton,
> + apr_pool_t *pool)
> + {
> + SVN_ERR (svn_cmdline_printf(pool, "Properties on '%s':\n", path));
> + SVN_ERR (svn__print_prop_hash(prop_hash, svn__do_print_prop_cl, baton, pool));
Space before paren.
[...]
> @@ -81,14 +256,22 @@
> SVN_ERR (svn_client_revprop_list (&proplist,
> URL, &(opt_state->start_revision),
> &rev, ctx, pool));
> -
> - SVN_ERR
> - (svn_cmdline_printf (pool,
> +
> + if (opt_state->xml)
> + {
> + sb = svn_stringbuf_create ("", pool);
> + SVN_ERR (svn__print_prop_hash_xml(proplist, sb, pool));
> + SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> + }
> + else
> + {
> + SVN_ERR (svn_cmdline_printf (pool,
> _("Unversioned properties on revision %ld:\n"),
Indentation.
> rev));
>
> - SVN_ERR (svn_cl__print_prop_hash
> - (proplist, (! opt_state->verbose), pool));
> + SVN_ERR (svn__print_prop_hash
> + (proplist, svn__do_print_prop_cl, (void*)(! opt_state->verbose), pool));
Indentation.
> + }
> }
> else /* operate on normal, versioned properties (not revprops) */
> {
> @@ -98,7 +281,6 @@
> {
> const char *target = ((const char **) (targets->elts))[i];
> apr_array_header_t *props;
> - int j;
> svn_error_t *err;
> svn_boolean_t is_url = svn_path_is_url (target);
> const char *truepath;
> @@ -129,23 +311,15 @@
> return err;
> }
>
> - for (j = 0; j < props->nelts; ++j)
> + if (opt_state->xml)
> {
> - svn_client_proplist_item_t *item
> - = ((svn_client_proplist_item_t **)props->elts)[j];
> - const char *name_local;
> + sb = svn_stringbuf_create ("", pool);
> + SVN_ERR (svn__print_props(props, is_url, svn__do_print_props_xml, sb, pool));
Space before paren.
> + SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> + }
> + else
> + SVN_ERR (svn__print_props(props, is_url, svn__do_print_props_cl, (void*)opt_state->verbose, pool));
Space before paren. Line too long.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 22 19:39:23 2005