[PATCH] 'svn blame --xml' - v4

[al...@collab.net]

Modified :
  * Removed redundant conditional check on author and date
  * Removed unused variable from function 'print_blame_xml'
  * Rearranged/Changed comments
  * Added new element <commit> in dtd and XML output
  * Added new attribute <line-number> under <entry> in dtd and XML
  * Changed python test case to reflect change in XML output

[[[
Version: 4, Patch to add XML output for 'svn blame'

* subversion/clients/cmdline/blame-cmd.c
  (print_blame_xml): New function for XML output
  (blame_receiver): Modified to check for XML option and process
  accordingly
  (print_header_xml): New function to print XML header
  (print_footer_xml): New function to print XML footer
  (svn_cl__blame): Modified for XML output

* subversion/clients/cmdline/main.c
  (svn_cl__cmd_table[]): added --xml and --incremental options
  for blame

* subversion/clients/cmdline/dtd/blame.dtd
  New dtd for blame --xml

* subversion/tests/clients/cmdline/blame_tests.py
  (blame_in_xml): New test for blame --xml
  (test_list): added blame_in_xml
]]]

-Alexander Thomas (AT)

Re: [PATCH] 'svn blame --xml' - v4

["Peter N. Lundblad" <pe...@famlundblad.se>]

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