> Index: subversion/clients/cmdline/blame-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/blame-cmd.c (revision 14461)
> +++ subversion/clients/cmdline/blame-cmd.c (working copy)
> @@ -24,6 +24,7 @@
> #include "svn_path.h"
> #include "svn_pools.h"
> #include "svn_cmdline.h"
> +#include "svn_xml.h"
> #include "svn_time.h"
> #include "cl.h"
>
> @@ -37,7 +38,52 @@
>
>
> /*** Code. ***/
> +
> +/* Prints blame output to standard console in XML mode. */
To be pedantic: it's print to standard out (wich might not be a console).
> static svn_error_t *
> +print_blame_xml (svn_stream_t *out,
> + apr_pool_t *pool,
> + svn_revnum_t revision,
> + const char *author,
> + const char *date)
out is never used.
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + const char *rev_str = SVN_IS_VALID_REVNUM (revision)
> + ? apr_psprintf (pool, "%ld", revision)
> + : " -";
Don't use " -" to denote "unknown revision". Make the revision
attribute optional instead. (Or follow my suggestion in another mail
to follow the list.dtd style.
> +
...
> +}
> +
I think it is cleaner to create an blame-receiver_xml, instead of
intermixing the XML handling with the traditional output.
> @@ -70,18 +116,52 @@
> abbreviations for the month and weekday names. Else, the
> line contents will be misaligned. */
> time_stdout = " -";
> - return svn_stream_printf (out, pool, "%s %10s %s %s\n", rev_str,
> - author ? author : " -",
> - time_stdout , line);
> +
> + if (opt_state->xml)
> + return print_blame_xml (out, pool,
> + revision,
> + author ? author : NULL,
Redundant conditional. Just author will do.
> + date ? date : NULL);
Same here.
> +
> + else
> + return svn_stream_printf (out, pool, "%s %10s %s %s\n", rev_str,
> + author ? author : " -",
> + time_stdout , line);
> }
> else
> {
> - return svn_stream_printf (out, pool, "%s %10s %s\n", rev_str,
> - author ? author : " -", line);
> + return svn_stream_printf (out, pool, "%s %10s %s\n", rev_str,
> + author ? author : " -", line);
> }
> }
> -
>
> +
> +/* Prints XML header in standard console. */
"...to standrad out."
> +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);
> +
> + /* "<blame>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "blame", NULL);
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints XML footer in standard console. */
And here.
> @@ -128,16 +208,28 @@
> SVN_ERR (svn_stream_for_stdout (&out, pool));
> bl.opt_state = opt_state;
> bl.out = out;
> -
> +
> subpool = svn_pool_create (pool);
>
> + /* If output is not incremental, output the XML header and wrap
> + everything in a top-level element. This makes the output in
> + its entirety a well-formed XML document. */
> + if (opt_state->xml && ! opt_state->incremental)
> + SVN_ERR (print_header_xml (pool));
> + else
> + if (opt_state->incremental)
> + return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'incremental' option only valid in XML "
> + "mode"));
Bad indentation.
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_error_t *err;
> const char *target = ((const char **) (targets->elts))[i];
> const char *truepath;
> svn_opt_revision_t peg_revision;
> -
> + svn_stringbuf_t *sb;
> +
> svn_pool_clear (subpool);
> SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
> if (is_head_or_base)
> @@ -151,7 +243,15 @@
> /* Check for a peg revision. */
> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
> subpool));
> -
> + /* "<target ...>" */
Maybe move that comment inside the body of the if statement.
> + if (opt_state->xml)
> + {
> + sb = svn_stringbuf_create ("", pool);
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "target",
> + "path", truepath, NULL);
> + SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> + }
> +
> err = svn_client_blame2 (truepath,
> &peg_revision,
> &opt_state->start_revision,
> @@ -174,8 +274,22 @@
> return err;
> }
> }
> +
> + /* "</target>" */
If so, here too.
> + if (opt_state->xml)
> + {
> + sb = svn_stringbuf_create ("", pool);
> + svn_xml_make_close_tag (&sb, pool, "target");
> + SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> + }
> }
> svn_pool_destroy (subpool);
>
> Index: subversion/clients/cmdline/dtd/blame.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/blame.dtd (revision 0)
> +++ subversion/clients/cmdline/dtd/blame.dtd (revision 0)
> @@ -0,0 +1,10 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn blame" -->
> +<!ELEMENT blame (target*)>
> +<!ELEMENT target (entry*)>
> +<!ATTLIST target path CDATA #REQUIRED> <!-- path or URL: character -->
Does ": character" mean it is a single character? Of course not, but
it is a little confusing. The data type is clear from the declaration,
so you can just drop that.
> +<!ELEMENT entry (author?, date?)>
> +<!ELEMENT author (#PCDATA)> <!-- author -->
> +<!ELEMENT date (#PCDATA)> <!-- date and time -->
> +<!ATTLIST entry revision CDATA #REQUIRED> <!-- revision number: integer -->
I suggest the following sturctuure instead:
<entry line-number="14">
<commit revision="14789">
<author>blah</author>
<date>glah</date>
</commit>
</entry>
The point is that line-number is a proerty of the entry; everything
else are properties on the commit info. If there is no revision
number, there can't be either author or date. This means that revision
is #REQUIRED,, everything else in entry and commit is optional.
> Index: subversion/tests/clients/cmdline/blame_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/blame_tests.py (revision 14510)
> +++ subversion/tests/clients/cmdline/blame_tests.py (working copy)
> +
> + template = ["<?xml version=\"1.0\" encoding=\"utf-8\"?>\n",
> + "<blame>\n",
> + "<target\n",
> + " path=\"%s\">\n" % (file_path),
> + "<entry\n",
> + " revision=\"2\">\n",
> + "<author>jrandom</author>\n",
> + "%s" % (time_str),
> + "</entry>\n",
> + "</target>\n",
> + "</blame>\n",
> + ]
> +
This could test some more cases, for example the unknown revision
case. I don't think you can test the no author or no date cases
easily.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 4 18:08:53 2005