On Fri, 29 Apr 2005, Alexander Thomas wrote:
> 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,67 @@
>
>
> /*** Code. ***/
> +
> static svn_error_t *
> +print_blame_xml (svn_stream_t *out,
> + apr_pool_t *pool,
> + const char *rev,
> + const char *author,
> + const char *time,
> + const char *line)
Each function, even internal ones, should have a doc string explaining
its purpose. Not all old functions have this, but new functions
should.
I have some comments about the DTD structure, see below.
> @@ -70,18 +131,57 @@
> 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,
> + rev_str ? rev_str : "",
I think making the element optional is better than having an empty
string (again, see below).
> + author ? author : " -",
> + time_stdout,
This strings with padding spaces makes sense in the traditional blame
output, but not in XML, which is a structured format.
> + line ? line : "");
See Julian's comment about encodings.
> + 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);
> + if (opt_state->xml)
> + return print_blame_xml (out, pool,
> + rev_str ? rev_str : "",
> + author ? author : " -",
> + NULL,
> + line ? line : "");
I think the XML output should output all information available and
ignore the -v flag. That's what the list command does. The output will
be parsed by a machine anyway.
> + else
> + return svn_stream_printf (out, pool, "%s %10s %s\n", rev_str,
> + author ? author : " -", line);
> }
> }
> -
>
> +
> @@ -151,7 +269,20 @@
> /* Check for a peg revision. */
> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
> subpool));
> -
> + /* "<traget ...>" */
> + if (opt_state->xml)
> + {
> + sb = svn_stringbuf_create ("", pool);
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "target",
> + "path", truepath ? truepath : "",
Can truepath really be NULL here? If not, then the check is just confusing.
> + NULL);
> + SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> + }
> + else
> + SVN_ERR (svn_cmdline_printf (subpool,
> + _("\nFile: %s\n"),
> + truepath ? truepath : ""));
Same here.
> +
> err = svn_client_blame2 (truepath,
If it could be NULL, this would crash.
> &peg_revision,
> &opt_state->start_revision,
> @@ -174,8 +305,22 @@
> return err;
> }
> }
> +
> + /* "</target>" */
> + 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);
>
> + if (opt_state->xml)
> + {
> + if (! opt_state->incremental)
> + SVN_ERR (print_footer_xml (pool));
> + }
> +
Very minor and a matter of taste, but here you spoil five lines where
two would do.
if (opt_state->xml && ! opt_state->incremental)
...
> return SVN_NO_ERROR;
> }
> 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,11 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
In general, I think this should be more consistet with the list.dtd file.
> +<!-- For "svn blame" -->
> +<!ELEMENT blame (target*)>
> +<!ELEMENT target (entry*)>
> +<!ATTLIST target path CDATA #REQUIRED> <!-- file: character -->
"file:character" -> "path or URL".
> +<!ELEMENT entry (rev, author, time?, details)>
I suggest borrowing the commit element from list.dtd and having the content model
<!ELEMENT entry (commit?)>
If there is no revision, there is no author or date.
> +<!ELEMENT rev (#PCDATA)> <!-- revision -->
Should be named revision for consistency.
> +<!ELEMENT author (#PCDATA)> <!-- author -->
> +<!ELEMENT time (#PCDATA)> <!-- date and time -->
These are common between this and other DTD files. Maybe time to
create a common.mod or whatever.
> +<!ELEMENT details (#PCDATA)> <!-- deatils or content changed -->
I think this will be removed.
> Index: subversion/tests/clients/cmdline/blame_tests.py
I leave this to others to review.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 29 15:14:48 2005