Alexander Thomas wrote:
> [[[
> Patch to add XML output for 'svn info'.
Sorry I took so long to get around to reviewing this.
> * 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.
> (print_footer_xml): New function.
> (kind_str): New function.
> (schedule_str): New function.
> (make_tagged_cdata): New function.
> (print_info_xml): New function.
> (info_receiver): Edited existing callback to print xml or
> traditional output depending on cmdline option.
> (svn_cl__info): Modified for XML output.
>
> * 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,47 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn info" -->
> +<!ELEMENT info (entry*)>
> +
> +<!ELEMENT entry (url?, repository?, wc-info?, last-changed, conflict?, lock?)>
For a simple schedule-add file (without history), and possibly other cases, no
last-changed info exists, so that element should be optional.
> +<!-- path: local path -->
> +<!-- type: path type -->
> +<!-- revision number of wc or repos path/url-->
> +<!ATTLIST entry
> + path CDATA #REQUIRED
> + type (file | directory) #REQUIRED
Call the type "kind", with values "dir" and "file", for consistency with list.dtd.
> + revision CDATA #REQUIRED
> +>
> +<!ELEMENT url (#PCDATA)> <!-- repository url -->
> +
> +<!ELEMENT repository (root?, uuid)>
> +<!ELEMENT root (#PCDATA)> <!-- root: path type -->
"path type"? Don't you mean "repository URL" or something?
> +<!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, revision, date, text-updated?, prop-updated?)>
> +<!ELEMENT author (#PCDATA)> <!-- last changed author -->
> +<!ELEMENT revision (#PCDATA)> <!-- last changed revision -->
> +<!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)>
In a manual test with a conflict caused by trying to merge a binary file, I
found only cur-base-file and prev-base-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 -->
> Index: subversion/clients/cmdline/info-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/info-cmd.c (revision 14784)
> +++ subversion/clients/cmdline/info-cmd.c (working copy)
[...]
> @@ -47,8 +48,267 @@
> 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>" */
This comment is wrong.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "info", NULL);
> +
> + 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>" */
And this one.
> + svn_xml_make_close_tag (&sb, pool, "info");
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +/* Return string representation of KIND */
> +static const char *
> +kind_str (svn_node_kind_t kind)
> +{
> + switch (kind)
> + {
> + case svn_node_dir:
> + return "directory";
> + case svn_node_file:
> + return "file";
> + default:
> + return "";
> + }
> +}
This is a duplicate of a function in ls-cmd.c. Don't duplicate code, share it.
Move it into "cl.h" and "util.c", named something like "svn_cl__node_kind_str".
In its usage in "svn list --xml", I see that the corresponding XML attribute is
called "kind", so that should be its name here too.
The existing plain-text "svn info" function, print_info(), should also use this
function, unless we want an unlocalised version for XML and a localised version
for plain text ... which I suppose we do. And print_info() currently also
printfs "none" and "unknown" which is really rather silly, so we'd have to make
a decision on what it really ought to be doing if it somehow encounters those
node kinds. OK, forget about print_info() for now.
[...]
> +/* If the string DATA is non-null, format it in a simple XML CDATA element
> + * named TAGNAME and append the result to the string SB.
> + * Use POOL for temporary allocations. */
> +static void
> +make_tagged_cdata (svn_stringbuf_t **sb,
> + apr_pool_t *pool,
> + enum svn_xml_open_tag_style style,
This doesn't need to take an (undocumented) "style" parameter, because the only
style that makes sense is svn_xml_protect_pcdata. Having this hard-coded would
have prevented the error of passing the "self-closing" style to it later in
your patch.
> + const char *tagname,
> + const char *data)
> +{
> + if (data)
> + {
> + svn_xml_make_open_tag (sb, pool, style, tagname, NULL);
> + svn_xml_escape_cdata_cstring (sb, data, pool);
> + svn_xml_make_close_tag (sb, pool, tagname);
> + }
> +}
> +
> +
> +/* 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);
> + const char *rev_str = (SVN_IS_VALID_REVNUM(info->rev)) ?
> + apr_psprintf (pool, "%ld", info->rev) :
> + NULL;
> +
> + /* If REV_STR is NULL, assume WC is corrupt. */
> + if (!rev_str)
The only way rev_str can be null is if your conditional assignment above set it
to null, so just move that assignment after this "if" statement and make it
unconditional, to save having two different tests for the same thing. I know
that would mean the variable would not be initialised at its point of
declaration, but that's the norm in C for non-trivial initialisations.
> + return svn_error_createf (SVN_ERR_WC_CORRUPT, NULL,
> + _("'%s' has invalid revision"),
> + svn_path_local_style (target, pool));
> +
> + str_kind = kind_str (info->kind);
> +
> + /* "<entry ...>" */
[...]
> + /* "<copy-from-url> xx </copy-from-url>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "copy-from-url", info->copyfrom_url);
> +
> + if (SVN_IS_VALID_REVNUM (info->copyfrom_rev))
> + {
> + /* "<copied-from-rev> xx </copied-from-rev>" */
element wc-info: validity error : Element wc-info content does not follow the
DTD, expecting (schedule? , copy-from-url? , copy-from-rev? , checksum?), got
(schedule copy-from-url copied-from-rev checksum)
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "copied-from-rev", apr_psprintf (pool,
> + "%" APR_OFF_T_FMT, info->copyfrom_rev));
info-cmd.c:217: warning: long long int format, different type arg (arg 3)
APR_OFF_T_FMT is not the format of a revision number. "%ld" is.
> + }
> +
> + /* "<checksum> xx </checksum>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "checksum", info->checksum);
> +
> + /* "</wc-info>" */
> + svn_xml_make_close_tag (&sb, pool, "wc-info");
> + }
> +
> + /* "<last-changed>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "last-changed", NULL);
This "last-changed" element needs to be omitted if it is inapplicable (such as
for a simple schedule-add file).
> +
> + /* "<author> xx </author>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "author", info->last_changed_author);
> +
> + if (SVN_IS_VALID_REVNUM (info->last_changed_rev))
> + {
> + /* "<revision> xx </revision>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "revision", apr_psprintf (pool,
> + "%" APR_OFF_T_FMT, info->last_changed_rev));
Same again.
> + }
> +
> + /* "<date> xx </date>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "date", svn_time_to_cstring
> + (info->last_changed_date, pool));
(I wonder if there are any cases in which last-changed info is being printed
but last_changed_date is absent (zero), which would result in
"1970-01-01T00:00:00.000000Z" being printed.)
[...]
> + /* "<comment ...> xxxx </comment>" */
> + make_tagged_cdata (&sb, pool, svn_xml_self_closing, "comment",
A self-closing element doesn't make sense in this context, as already noted.
> + info->lock->comment);
[...]
> @@ -295,5 +573,8 @@
> }
> svn_pool_destroy (subpool);
>
> + if (opt_state->xml && (! opt_state->incremental))
> + err = print_footer_xml (pool);
That's leaking an error. You probably want "SVN_ERR" instead of "err =".
> +
> return SVN_NO_ERROR;
> }
I haven't much of an opinion on the format of the XML output. It seems OK at a
quick look.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 31 19:53:19 2005