[PATCH] issue #2069 - "svn status" in xml mode

[Alexander Thomas <al...@collab.net>]

[[[
Patch to fix issue 2069 - "svn status" in xml mode

* subversion/include/svn_wc.h
  (enum svn_wc_notify_action_t): added new notification
  'svn_wc_notify_status_xml_completed', which will be
  last notification for status xml

* subversion/libsvn_client/status.c
  (svn_client_status2): calls last notification for status
  depending on --xml option request

* subversion/clients/cmdline/cl.h
  (svn_cl__print_status): added new argument 'xml_mode' to prototype

* subversion/clients/cmdline/status.c
  (print_statents_xml): added new function to print status in
  xml format to standard console
  (print_status): added new argument 'xml_mode' and
  checking this argument to decide which format for status output
  (svn_cl__print_status_xml): added new argument 'xml_mode'
   and passes it to print_status function

* subversion/clients/cmdline/notify.c
  (notify): 'svn_wc_notify_status_xml_completed' is called
  as the last status notification and prints out the 'Status
  against revision' in xml mode, only for 'svn status -u' option

* subversion/clients/cmdline/main.c
  (svn_cl__cmd_table[]): added --xml and --incremental options
   in to subcommand array for status subcommand

* subversion/clients/cmdline/status-cmd.c
  (struct status_baton): added new item 'xml_mode' in struct
  for storing xml mode is requested or not
  (print_header_xml): prints xml header
  (print_footer_xml): prints xml footer
  (print_status): calls svn_cl__print_status function with
  new parameter xml_mode
  (svn_cl__status): checks for xml option and decides
  which respective callback functions for xml or ordinary
  output. Also prints xml header and footer

* subversion/clients/cmdline/dtd/status.dtd
  added a new dtd file for validating status xml

* subversion/tests/clients/cmdline/stat_tests.py
  (status_in_xml): new function to verify success when
  svn status output in xml format
  (test_list): added status_in_xml
]]]

-Alexander Thomas(AT)

Re: [PATCH] issue #2069 - "svn status" in xml mode

[Julian Foad <ju...@btopenworld.com>]

Alexander Thomas wrote:
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h	(revision 13821)
> +++ subversion/include/svn_wc.h	(working copy)
> @@ -543,7 +543,10 @@
>    svn_wc_notify_failed_lock,
>  
>    /** @since New in 1.2.  Failed to unlock a path. */
> -  svn_wc_notify_failed_unlock
> +  svn_wc_notify_failed_unlock,
> +
> +  /** The last notification in a status xml (including status on externals). */
> +  svn_wc_notify_status_xml_completed,

Don't add a comma at the end of the list - it's invalid C'89 (though some 
compilers accept it).

>  } svn_wc_notify_action_t;
>  
>  
> Index: subversion/libsvn_client/status.c
> ===================================================================
> --- subversion/libsvn_client/status.c	(revision 13821)
> +++ subversion/libsvn_client/status.c	(working copy)
> @@ -324,8 +324,11 @@
>  
>    if (ctx->notify_func2 && update)
>      {
> -      svn_wc_notify_t *notify
> -        = svn_wc_create_notify (path, svn_wc_notify_status_completed, pool);
> +      svn_wc_notify_t *notify;
> +      if ( *((svn_boolean_t*)status_baton))

No - that cast is ugly and wrong.  This function can't assume anything about 
the content of the argument "status_baton".

(I note that the function is already creating a little confusion by using a 
type "struct status_baton" and an argument "status_baton" which is not of that 
type.)

> +        notify = svn_wc_create_notify (path, svn_wc_notify_status_xml_completed, pool);
> +      else
> +        notify = svn_wc_create_notify (path, svn_wc_notify_status_completed, pool);
>        notify->revision = edit_revision;
>        (ctx->notify_func2) (ctx->notify_baton2, notify, pool);
>      }

> Index: subversion/clients/cmdline/notify.c
> ===================================================================
> --- subversion/clients/cmdline/notify.c	(revision 13821)
> +++ subversion/clients/cmdline/notify.c	(working copy)
[...]
> @@ -55,8 +56,10 @@
>  {
>    struct notify_baton *nb = baton;
>    char statchar_buf[5] = "    ";
> +  const char *staton_rev;
>    const char *path_local;
>    svn_error_t *err;
> +  svn_stringbuf_t *sb;

Where you want variables that are local to a small block of code, please 
declare them within the small block of code.

>  
>    path_local = svn_path_local_style (n->path, pool);
>  
> @@ -364,6 +367,26 @@
>        svn_handle_error (n->err, stderr, FALSE);
>        break;
>  
> +    case svn_wc_notify_status_xml_completed:
> +      if (SVN_IS_VALID_REVNUM (n->revision))
> +          sb = svn_stringbuf_create ("", pool);

I assume you also meant the following several statements to be subject to the 
"if", so you need braces.  Then you will be able to declare the two local 
variables in this local block:

if (...)
   {
     svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
     const char *staton_rev = apr_psprintf (pool, "%ld", n->revision);

     ...
   }

> +          staton_rev = apr_psprintf (pool, "%ld", n->revision);
> +          /* "<status-against ...>" */
> +          svn_xml_make_open_tag (&sb, pool, svn_xml_normal, 
> +                                 "status-against", NULL);
> +          
> +          /* "<rev>xx</rev>" */
> +          svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> +                                 "rev", NULL);
> +          svn_xml_escape_cdata_cstring (&sb, staton_rev, pool);
> +          svn_xml_make_close_tag (&sb, pool, "rev");
> + 
> +          /* "</status-against>" */
> +          svn_xml_make_close_tag (&sb, pool, "status-against");
> +
> +          svn_cl__error_checked_fputs (sb->data, stdout);
> +      break;
> +
>      default:
>        break;
>      }
> Index: subversion/clients/cmdline/status-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/status-cmd.c	(revision 13821)
> +++ subversion/clients/cmdline/status-cmd.c	(working copy)
[...]
> @@ -127,9 +176,15 @@
>                                     opt_state->no_ignore,
>                                     opt_state->ignore_externals,
>                                     ctx, subpool));
> +

Try to avoid accidental changes to whitespace like here ...

>      }
>  
>    svn_pool_destroy (subpool);
> -  

... and here (your inserted code is unrelated to destroying the pool, so a 
separating line is good, though deleting the two space characters in it would 
be fine) ...

> +  if (opt_state->xml)
> +    {
> +      if (! opt_state->incremental)
> +        SVN_ERR (print_footer_xml (pool));
> +    }
> + 

... and that nearly-blank line contains a space.

>    return SVN_NO_ERROR;
>  }
> Index: subversion/tests/clients/cmdline/stat_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/stat_tests.py	(revision 13821)
> +++ subversion/tests/clients/cmdline/stat_tests.py	(working copy)
> @@ -817,6 +817,69 @@
>    svntest.actions.run_and_verify_status(wc_dir, expected_status)
>  
>  
> +def status_in_xml(sbox):
> +  "status output in XML format"
> +
> +
> +  output, error = svntest.actions.run_and_verify_svn (None, None, [], 
> +				'help', 'status')
> +## TODO: is it neccessary to check output for NULL before using it

No, I don't think so.

[...]
> +  # TODO: Checks wheather output of the cmd 'status --xml' is in XML format

Isn't that what the code below does?

[...]
> +  template = ["<?xml version=\"1.0\" encoding=\"utf-8\"?>\n",
> +              "<status>\n",
> +              "<entry\n",
> +              "   file=\"%s\">\n" % (file_path),
> +              "<itemstatus>M</itemstatus>\n",
> +              "<propstatus> </propstatus>\n",
> +              "<locked> </locked>\n",
> +              "<scheduled> </scheduled>\n",
> +              "<switched> </switched>\n",
> +              "<reposlock> </reposlock>\n",
> +              "</entry>\n",
> +              "</status>\n",
> +             ]
> +
> +
> +  output, error = svntest.actions.run_and_verify_svn (None, None, [], 
> +				'status', wc_dir, '--xml')
> +
> +  for i in range(0, len(output)):
> +    if output[i] != template[i]:
> +      raise svntest.Failure
> +
>  #----------------------------------------------------------------------  

I hope that helps.  I haven't reviewed all of the patch.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org