Thanks for attaching this patch in plain text this time. That makes it much
more likely that I will review it ... and I have.
Here are some comments from reading the patch. I haven't compiled or run or
tested it yet.
Alexander Thomas wrote:
> [[[
> Patch to add XML output for 'svn info'
Also: "Send warnings to stderr instead of stdout."
>
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table[]): added --xml and --incremental options
> for svn info
>
> * subversion/clients/cmdline/dtd/info.dtd
> New dtd for info --xml
>
> * subversion/clients/cmdline/info-cmd.c
> (print_header_xml): New function to print XML header
> (print_target_xml): New function to print open and close targets
What are "open and close targets"? Never mind; see below.
> (print_footer_xml): New function to print XML footer
> (print_info_xml): New function for XML output
> (info_receiver): Edited existing callback to call function to
> print xml or traditional output depending on cmdline option
> (svn_cl__info): Modified for XML output and redirected errors to
> stderr
I'd call them "warnings" rather than "errors".
>
> * tools/client-side/bash_completion
> (_svn): added "--incremental" and "--xml" options to "info"
> for auto-completion.
> ]]]
> Index: subversion/clients/cmdline/dtd/info.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/info.dtd (revision 0)
> +++ subversion/clients/cmdline/dtd/info.dtd (revision 0)
> @@ -0,0 +1,54 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn info" -->
> +<!ELEMENT info (target*)>
> +
> +<!ELEMENT target (entry*)>
The "info" command's output is not (logically) grouped according to how the
targets were specified on the command line. Each item for which information is
printed is a self-contained item. So there is no need for a "target" element:
<!ELEMENT info (entry*)>
> +<!-- path: target supplied in cmdline -->
> +<!ATTLIST target
> + path CDATA #REQUIRED
> +>
> +
> +<!ELEMENT entry (url?, revision, repository?, wc-info?, last-changed, conflict?, lock?)>
> +<!-- path: local path -->
> +<!-- type: path type -->
> +<!ATTLIST entry
> + path CDATA #REQUIRED
> + type (file | directory | none | unknown) "unknown"
> +>
> +<!ELEMENT url (#PCDATA)> <!-- repository url -->
> +<!ELEMENT revision (#PCDATA)> <!-- revision number of wc or repos path/url-->
> +
> +<!ELEMENT repository (root?, uuid)>
> +<!ELEMENT root (#PCDATA)> <!-- root: path type -->
> +<!ELEMENT uuid (#PCDATA)> <!-- uuid: repository uuid -->
> +
> +<!ELEMENT wc-info (schedule?, copy-from-url?, copy-from-rev?, checksum?)>
> +<!-- schedule: applies only for wc -->
> +<!ELEMENT schedule (#PCDATA | normal | add | delete | replace | none)*>
> +<!ELEMENT copy-from-url (#PCDATA)>
> +<!ELEMENT copy-from-rev (#PCDATA)>
> +<!ELEMENT checksum (#PCDATA)>
> +
> +<!ELEMENT last-changed (author, last-rev, date, text-updated?, prop-updated?)>
> +<!ELEMENT author (#PCDATA)> <!-- last changed author -->
> +<!ELEMENT last-rev (#PCDATA)> <!-- last changed revision -->
Should be just "revision": the prefix "last-" is redundant within a
"last-changed" block.
> +<!ELEMENT date (#PCDATA)> <!-- last changed date in iso format -->
> +<!ELEMENT text-updated (#PCDATA)> <!-- text last updated -->
> +<!ELEMENT prop-updated (#PCDATA)> <!-- properties last updated -->
> +
> +<!ELEMENT conflict (prev-base-file, prev-wc-file, cur-base-file, prop-file)>
> +<!ELEMENT prev-base-file (#PCDATA)> <!-- previous base file -->
> +<!ELEMENT prev-wc-file (#PCDATA)> <!-- previous wc file -->
> +<!ELEMENT cur-base-file (#PCDATA)> <!-- current base file -->
> +<!ELEMENT prop-file (#PCDATA)> <!-- current properties file -->
> +
> +
> +<!ELEMENT lock (token, owner, created, expires?, comment?)>
> +<!ELEMENT token (#PCDATA)> <!-- lock token -->
> +<!ELEMENT owner (#PCDATA)> <!-- lock owner -->
> +<!ELEMENT created (#PCDATA)> <!-- lock created: date & time in iso format -->
> +<!ELEMENT expires (#PCDATA)> <!-- lock expires: date & time in iso format -->
> +<!ELEMENT comment (#PCDATA)> <!-- lock comment -->
> +<!ATTLIST comment
> + lines CDATA #REQUIRED>
The number-of-lines attribute is not appropriate in the XML output. That was
given only to assist parsing of the plain text output.
> Index: subversion/clients/cmdline/info-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/info-cmd.c (revision 14784)
> +++ subversion/clients/cmdline/info-cmd.c (working copy)
> @@ -28,6 +28,7 @@
> #include "svn_error.h"
> #include "svn_path.h"
> #include "svn_time.h"
> +#include "svn_xml.h"
> #include "cl.h"
>
> #include "svn_private_config.h"
> @@ -47,8 +48,372 @@
> return SVN_NO_ERROR;
> }
>
> +/* Prints XML header */
> +static svn_error_t *
> +print_header_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + /* <?xml version="1.0" encoding="utf-8"?> */
> + svn_xml_make_header (&sb, pool);
>
> + /* "<status>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "info", NULL);
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints XML target */
> static svn_error_t *
> +print_target_xml (apr_pool_t *pool, const char *target,
> + svn_boolean_t to_open)
This should be two separate functions. They do not have enough in common to
justify use of a Boolean "mode" parameter. (Such a parameters is
non-descriptive at the point of call.)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + if (to_open)
> + /* "<target ...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "target",
> + "path", target, NULL);
> + else
> + /* "</target>" */
> + svn_xml_make_close_tag (&sb, pool, "target");
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints XML footer */
> +static svn_error_t *
> +print_footer_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + /* "</status>" */
> + svn_xml_make_close_tag (&sb, pool, "info");
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* prints svn info in xml mode to standard out */
> +static svn_error_t *
> +print_info_xml (const char *target,
> + const svn_info_t *info,
> + apr_pool_t *pool)
> +{
> + const char *str_kind;
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + switch (info->kind)
> + {
> + case svn_node_file:
> + str_kind = "file";
> + break;
> +
> + case svn_node_dir:
> + str_kind = "directory";
> + break;
> +
> + case svn_node_none:
> + str_kind = "none";
> + break;
> +
> + case svn_node_unknown:
> + default:
The "default" case should throw an error, I should think. (I see you just
copied this from the existing non-XML code.)
> + str_kind = "unknown";
> + break;
> + }
Would it make sense to factor out this node-kind-to-text conversion and use it
for both the XML and plain-text outputs? Also other enum-to-text conversions
below.
> + /* "<entry ...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
> + "path", svn_path_local_style (target, pool),
> + "type", str_kind,
> + NULL);
[...]
> + if (info->conflict_old || info->conflict_wrk
> + || info->conflict_new || info->prejfile )
> + {
> + /* "<conflict>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "conflict", NULL);
> +
> + if (info->conflict_old)
> + {
> + /* "<prev-basefile> xx </prev-basefile>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "prev-basefile", NULL);
> + svn_xml_escape_cdata_cstring (&sb, info->conflict_old, pool);
> + svn_xml_make_close_tag (&sb, pool, "prev-basefile");
> + }
> +
> + if (info->conflict_wrk)
> + {
> + /* "<prev-wrkfile> xx </prev-wrkfile>" */
The name of this element doesn't match the DTD.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "prev-wrkfile", NULL);
> + svn_xml_escape_cdata_cstring (&sb, info->conflict_wrk, pool);
> + svn_xml_make_close_tag (&sb, pool, "prev-wrkfile");
> + }
[...]
> @@ -225,7 +590,10 @@
> const svn_info_t *info,
> apr_pool_t *pool)
> {
> - return print_info (path, info, pool);
> + if ((((svn_cl__cmd_baton_t *) baton)->opt_state)->xml)
You don't need quite so many parentheses:
if (((svn_cl__cmd_baton_t *) baton)->opt_state->xml)
> + return print_info_xml (path, info, pool);
> + else
> + return print_info (path, info, pool);
> }
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 30 23:32:05 2005