Hi, Alexander,
Here's my review of your last patch. I think you're getting closer... :-)
As an aside, would you please chage you mail clients to not send patches
as application/octet-stream. It is easier for people to review if it is
sent with a text mime type, since many mail clients will show it inline
then.
> 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,65 @@
>
>
> /*** Code. ***/
> +
> +/* Prints blame output to standard out in XML mode. */
> static svn_error_t *
> +print_blame_xml (apr_pool_t *pool,
> + svn_revnum_t revision,
> + const char *author,
> + const char *date,
> + apr_int64_t line_no)
(See my comment in the last mail)
Change to:
static svn_error_t *
blame_receiver_xml (void *baton, apr_int64_t line_no, svn_revnum_t revision,
const char *author, const char *date, cosnt char *line,
apr_pool_t *pool)
and leave blame_receiver alone.
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + const char *rev_str = SVN_IS_VALID_REVNUM (revision)
> + ? apr_psprintf (pool, "%ld", revision)
> + : NULL;
> + const char *lno_str = line_no >=0
> + ? apr_psprintf (pool, "%lld", line_no)
> + : NULL;
> +
There is a missing space after >=, but that check is redundant
anyway. svn_client_blame_receiver_t isn't called with negative line numbers.
> + /* "<entry ...>" */
> + if (lno_str)
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
> + "line-number", lno_str, NULL);
> + else
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry", NULL);
> +
So this if statement also becomes redundant.
> + /* "<commit ...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "commit",
> + "revision", rev_str, NULL);
> +
This passes a NULL attribute value to svn_xml_make_open_tag, which
> isn't allowed by the API. There is no point in having a commit
> element if the revision is invalid.
> + /* "<author>xx</author>" */
> + if (author)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "author", NULL);
> + svn_xml_escape_cdata_cstring (&sb, author, pool);
> + svn_xml_make_close_tag (&sb, pool, "author");
Use two spaces for indentation.
> + }
> +
> + /* "<date>xx</date>" */
> + if (date)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "date", NULL);
> + svn_xml_escape_cdata_cstring (&sb, date, pool);
> + svn_xml_make_close_tag (&sb, pool, "date");
> + }
> +
Same here.
> @@ -128,16 +218,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"));
Still 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 +253,15 @@
> /* Check for a peg revision. */
> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
> subpool));
> -
> + if (opt_state->xml)
> + {
> + /* "<target ...>" */
> + 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,
And some lines below, you would choose between blame_receiver and
blame_receiver_xml depending on opt_state->xml.
> 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,12 @@
> +<!-- 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 -->
> +<!ELEMENT entry (commit)>
(commit?)
> +<!ELEMENT commit (author?, date?)>
> +<!ELEMENT author (#PCDATA)> <!-- author -->
> +<!ELEMENT date (#PCDATA)> <!-- date and time -->
> +<!ATTLIST entry line-number CDATA #IMPLIED> <!-- line number: integer -->
#REQUIRED.
> +<!ATTLIST commit revision CDATA #REQUIRED> <!-- revision number: integer -->
#REQUIRED is correct, but the code doesn't current match that (see
above).
To test blame when the revision is not known, blame a file with a
revision range starting after the file was created, i.e.
svn blame -r10000:HEAD http://svn.collab.net/repos/svn/trunk/HACKING
(Not sure if you know when there will be not commit info.)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu May 5 22:29:31 2005