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

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

Version 6 of the patch for svn status --xml is attached.

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

* subversion/clients/cmdline/cl.h
  (svn_cl__print_status_xml): Added new function prototype.

* subversion/clients/cmdline/status.c
  (generate_status_desc): New function.
  (svn_cl__print_status_xml): New function to handle xml output.

* subversion/clients/cmdline/main.c
  (svn_cl__cmd_table): Added --incremental and --xml options to
  subcommand array.

* subversion/clients/cmdline/status-cmd.c
  (struct status_baton): Added new boolean field.
  (print_start_target_xml): New function.
  (print_header_xml): New function.
  (print_footer_xml): New function.
  (print_finish_target_xml): New function.
  (print_status): Edited to check --xml option.

* subversion/clients/cmdline/dtd/status.dtd
  New file.

* subversion/tests/clients/cmdline/stat_tests.py
  (status_in_xml): New test.
  (test_list): Added status_in_xml.

* tools/client-side/bash_completion
  (_svn):  Add "--incremental" and "--xml" options to "status"
  for auto-completion.
]]]

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

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

On Thu, 2 Jun 2005, Julian Foad wrote:

> Some comments on the DTD.
>

I'm replying, since I wrote most of this DTD (without the comments:-).
> Alexander Thomas wrote:
> > Index: subversion/clients/cmdline/dtd/status.dtd
> > ===================================================================
> > --- subversion/clients/cmdline/dtd/status.dtd	(revision 0)
> > +++ subversion/clients/cmdline/dtd/status.dtd	(revision 0)
> > @@ -0,0 +1,47 @@
> > +<!-- XML DTD for Subversion command-line client output. -->
> > +
> > +<!-- For "svn status" -->
> > +<!ENTITY % BOOL '(true | false) "false"'>
>
> It doesn't seem appropriate to have a default value.  You are always
> generating these attributes when they are applicable, aren't you?

Nope. Since copied and switches are normally uncommon, it seemed silly to
output them for every entry. So the default value makes sense.

>
> > +<!ATTLIST wc-status
> > +  item (added | conflicted | deleted | merged | ignored | modified |
> > +  replaced | external | unversioned | incomplete | obstructed |
> > +  normal | none) #REQUIRED
>
> "merged" is not possible.

Correct. I remember it was recently removed from the help output, which I
partly used when modelling this DTD.

> I don't think "none" is possible either.
>
Maybe not. The status code is somewhat complicated, so you never know:-)
I'll see if I can figure this out. Else, I leave the "none" value to be
safe...


> > +  props (conflicted | modified | normal | none) #REQUIRED
>
> I don't think "none" is possible.
>
Yes, when there are no props.

I'll take another look at the comments.

Thanks,
//Peter

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

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

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

Some comments on the DTD.

Alexander Thomas wrote:
> Index: subversion/clients/cmdline/dtd/status.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/status.dtd	(revision 0)
> +++ subversion/clients/cmdline/dtd/status.dtd	(revision 0)
> @@ -0,0 +1,47 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn status" -->
> +<!ENTITY % BOOL '(true | false) "false"'>

It doesn't seem appropriate to have a default value.  You are always generating 
these attributes when they are applicable, aren't you?

> +<!ELEMENT status (target*)>
> +
> +<!ELEMENT target (entry*, against?)>
> +<!-- target path -->

<!-- path: target path -->

> +<!ATTLIST target
> +  path CDATA #REQUIRED>
> +
> +<!ELEMENT entry (wc-status, repos-status?)>

<!-- path: entry path -->

> +<!ATTLIST entry
> +  path CDATA #REQUIRED>
> +
> +<!ELEMENT wc-status (lock?)>
> +<!-- WC dir locked? -->
> +<!-- Add with history? -->
> +<!-- Item switched -->

This comment would be better:
<!-- item: the schedule status of the item -->
<!-- props: the schedule status of the item's properties -->
<!-- wc-locked: WC dir locked? -->
<!-- copied: add with history? -->
<!-- switched: item is switched relative to its parent? -->

> +<!ATTLIST wc-status
> +  item (added | conflicted | deleted | merged | ignored | modified |
> +  replaced | external | unversioned | incomplete | obstructed |
> +  normal | none) #REQUIRED

"merged" is not possible.
I don't think "none" is possible either.

> +  props (conflicted | modified | normal | none) #REQUIRED

I don't think "none" is possible.

> +  wc-locked %BOOL;
> +  copied %BOOL;
> +  switched %BOOL;
> +>
> +
> +<!ELEMENT repos-status (lock?)>

Comment:
<!-- item: the repository status of the item -->
<!-- props: the repository status of the item's properties -->

> +<!ATTLIST repos-status
> +  item (added | deleted | modified | replaced | none) #REQUIRED
> +  props (modified | none) #REQUIRED
> +>
> +
> +<!-- Lock info stored in WC or repos. -->
> +<!ELEMENT lock (token, owner, comment?, created, expires?)>
> +
> +<!ELEMENT token (#PCDATA)> <!-- Lock token URI. -->
> +<!ELEMENT owner (#PCDATA)> <!-- Lock owner. -->
> +<!ELEMENT comment (#PCDATA)>  <!-- Lock Comment. -->
> +<!ELEMENT created (#PCDATA)>  <!-- Creation date in ISO format. -->
> +<!ELEMENT expires (#PCDATA)>  <!-- Expiration date in ISO format. -->
> +
> +<!ELEMENT against EMPTY>

Comment:
<!-- against: the revision number at which the repository information was 
obtained -->

> +<!ATTLIST against revision CDATA #REQUIRED>


- Julian

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

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

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

On Wed, 1 Jun 2005, Alexander Thomas wrote:

> Version 6 of the patch for svn status --xml is attached.
>
Below are some review comments for further patches. I fixed them, since
they were only stylistic and comment stuff. Committed in r 14921.

Thanks,
//Peter

Index: subversion/clients/cmdline/status.c
===================================================================
--- subversion/clients/cmdline/status.c	(revision 14774)
+++ subversion/clients/cmdline/status.c	(working copy)
@@ -24,8 +24,10 @@
 #include "svn_cmdline.h"
 #include "svn_wc.h"
 #include "svn_path.h"
+#include "svn_xml.h"
+#include "svn_time.h"
 #include "cl.h"
-
+#include "svn_private_config.h"

 /* Return the single character representation of STATUS */
 static char
@@ -51,6 +53,32 @@
     }
 }

+
+/* Return the detailed string representation of STATUS */
+static const char*

Space before *.

+generate_status_desc (enum svn_wc_status_kind status)
+{


+
+svn_error_t *
+svn_cl__print_status_xml (const char *path,
+                          svn_wc_status2_t *status,
+                          apr_pool_t *pool)
+{
+  svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
+
+  if (status->text_status == svn_wc_status_none
+      && status->repos_text_status == svn_wc_status_none)
+    return SVN_NO_ERROR;
+
+  svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
+                         "path", svn_path_local_style (path, pool), NULL);
+
+  apr_hash_t *att_hash = svn_xml_make_att_hash (NULL, pool);

C89 doesn't allow declarations in the middle of a block.  GCC does
(for example), so it is easy to introduce these kinds of portability
errors.  I'm also replacing svn_xml_make_att_hash with
apr_hash_make, since the former is meant to be used when you have a
list of attributes to populate the hash with.

+      /* If lock_owner is NULL, assume WC is corrupt. */
+      if (status->entry->lock_owner)
+        {
+          svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
+                                 "owner", NULL);
+          svn_xml_escape_cdata_cstring (&sb, status->entry->lock_owner,
+                                        pool);
+          svn_xml_make_close_tag (&sb, pool, "owner");
+        }
+      else
+          return svn_error_createf (SVN_ERR_WC_CORRUPT, 0,
+                        _("'%s' has lock token, but no lock owner\n"),
+                        svn_path_local_style (path, pool));
+

INdentation, and error messages should not be terminated by a newline.

+          if (status->repos_lock->expiration_date != 0)
+            {
+              svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
+                                     "expires", NULL);
+              svn_xml_escape_cdata_cstring (&sb,
+                  svn_time_to_cstring (status->repos_lock->expiration_date,
+                                       pool), pool);

Indentation.

+/* Prints XML target */
+static svn_error_t *
+print_start_target_xml (apr_pool_t *pool, const char *target)

pool argument should go last (exceptions are mostly variadic
functions).  (I expanded the docstring.)

+/* Prints XML header */

All arguments should be mentioned in the docstring, even pool.

+static svn_error_t *
+print_header_xml (apr_pool_t *pool)

+/* Prints XML footer */

Same.

+static svn_error_t *
+print_footer_xml (apr_pool_t *pool)
+{
+  svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
+
+  svn_xml_make_close_tag (&sb, pool, "status");
+
+  return svn_cl__error_checked_fputs (sb->data, stdout);
+}
+
+/* Prints </target> and the revision in repository against which
+ * status was checked */

Mention arguments by name and the REPOS_REV is only printed if it is
valid.  See the rewritten docstring in the commit.

+      if (opt_state->xml)
+        SVN_ERR (print_start_target_xml (

Misplaced paren.

+                        pool, svn_path_local_style (target, pool)));
+
+      SVN_ERR (svn_client_status2 (&repos_rev, target, &rev, print_status, &sb,
                                    opt_state->nonrecursive ? FALSE : TRUE,
                                    opt_state->verbose,
                                    opt_state->update,
                                    opt_state->no_ignore,
                                    opt_state->ignore_externals,
                                    ctx, subpool));
+      if (opt_state->xml)
+        SVN_ERR(print_finish_target_xml (repos_rev, pool));

Missing space before paren.



+  output, error = svntest.actions.run_and_verify_svn (None, None, [],
+                                                      'status', file_path, '--xml', '-u')

Long line.

+

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