[PATCH] Re: Future-proofing "svn st" output

I wrote:
> I have a proto-patch that implements solution #4 as described
> above and passes "make check" (apart from the independent breakage
> introduced when "svn help --quiet" was added) and a quick eyeball
> check, but I also have a bunch of questions.
...
> Argle, what a lot of fuss about such an unimportant matter. Sorry,
> folks. I was going to leave off posting the current state of the
> patch until I'd had answers to those questions, but I've changed
> my mind. So here it is. At present it left-aligns author names,
> which is a change from the old behaviour. Is it a problem that
> "make check" doesn't catch this?
>
> The patch is against revision 2890.
...
> The thing I'm least pleased with here is the deeply nested
> structure for holding fields. The aim is to reduce duplication
> of data and thereby make it harder for anyone changing this
> to introduce inconsistencies, but it does come at a cost.
> Suggestions for improving this would be welcome.

Gosh, I'm dim.

Here's a revised patch. It offers the exact same functionality, but
status.c is now 120 lines shorter than my first patch would have
made it, and much friendlier.

-------------------- patch begins --------------------
Index: ./subversion/clients/cmdline/cl.h
===================================================================
--- ./subversion/clients/cmdline/cl.h
+++ ./subversion/clients/cmdline/cl.h Tue Aug 6 01:43:29 2002
@@ -82,6 +82,7 @@
   svn_boolean_t verbose; /* be verbose */
   svn_boolean_t update; /* contact the server for the full
story */
   svn_boolean_t strict; /* do strictly what was requested */
+ svn_boolean_t describe_only; /* describe output format, don't
stat */
   svn_stringbuf_t *filedata; /* contents of file used as option
data */
   const char *filedata_encoding; /* the locale/encoding of the
filedata*/
   svn_boolean_t help; /* print usage message */
@@ -267,6 +268,13 @@
                                 svn_boolean_t show_last_committed,
                                 svn_boolean_t skip_unrecognized,
                                 apr_pool_t *pool);
+
+/* Print descriptions of all the fields that would be output
+ by calling svn_cl__print_status_list with parameters
+ DETAILED and SHOW_LAST_COMMITTED having specified values. */
+void svn_cl__describe_status_list (svn_boolean_t detailed,
+ svn_boolean_t
show_last_committed);
+
 
 /* Print a hash that maps property names (char *) to property values
    (svn_stringbuf_t *). The names are assumed to be in UTF-8 format;
Index: ./subversion/clients/cmdline/status.c
===================================================================
--- ./subversion/clients/cmdline/status.c
+++ ./subversion/clients/cmdline/status.c Tue Aug 6 11:44:32 2002
@@ -21,6 +21,10 @@
 
  
 /*** Includes. ***/
+#include <assert.h>
+#include <stdio.h>
+#include <string.h>
+
 #include <apr_hash.h>
 #include <apr_tables.h>
 #include "svn_sorts.h"
@@ -30,196 +34,308 @@
 #include "cl.h"
 
 
-/* Fill in the first four characters of STR_STATUS with status code
- characters, based on TEXT_STATUS, PROP_STATUS, LOCKED, and COPIED.
-
- This function is also used by commit-cmd.c
-*/
-void
-svn_cl__generate_status_codes (char *str_status,
- enum svn_wc_status_kind text_status,
- enum svn_wc_status_kind prop_status,
- svn_boolean_t locked,
- svn_boolean_t copied)
+
+/* A single item to be reported by some variant of "svn st".
+ VARIANTS should be conceived as a set of characters; the item
+ will be included if and only if this set contains the character
code
+ used for the variant requested.
+ WIDTH is the number of characters in the field.
+ FILL_IN is a pointer to a function whose job it is to fill in the
+ contents of the field. It will be handed a pointer to the start of
+ the field, the width requested, and a pointer to the status object
+ being reported on. It guarantees never to write to anything
outside
+ the range 0..WIDTH-1 inclusive. It is safe to assume that the
+ field is filled with spaces to begin with.
+ TAG is a shortish string, containing no whitespace, that another
+ program can use to recognize a field it's interested in. Tags
+ must never change, so choose them wisely.
+ DESC is a string containing a brief human-readable description
+ of the field.
+ Preferably, TAG should be at most 10 characters and DESC at most
60.
+ Then when we describe the fields, everything will fit in at most
+ 2+2+2+1+10+1+60 = 77 characters.
+ Alternatively: FILL_IN, TAG and DESC may all be null pointers.
+ In this case, the action associated with the field is to do
+ nothing.
+ These will be grouped into an array. The last element in the
+ array is distinguished by having VARIANTS a null pointer. */
+struct status_item {
+ const char *variants;
+ size_t width;
+ void (*fill_in)(char *, size_t, const svn_wc_status_t *);
+ const char *tag;
+ const char *desc;
+};
+
+typedef struct status_item status_item_t;
+
+
+
+/* Some of the fill-in functions below nede to be able to write
+ a revision number into a fixed-width field of a string. This
+ is much more painful than it should be, so it's pulled out into
+ this function here. The number is right-aligned within the field;
+ if it won't fit, digits are thrown away at the left-hand end.
+ FIELD is assumed pre-filled with spaces. WIDTH is the number
+ of characters available. We never write to anywhere outside
+ the range 0..WIDTH-1. */
+static void
+fill_in__revnum (char *field, size_t width, svn_revnum_t rev)
 {
- char text_statuschar, prop_statuschar;
+ char temp[21]; /* enough space for an unsigned long, including
terminator */
 
- switch (text_status)
- {
- case svn_wc_status_none:
- text_statuschar = ' ';
- break;
- case svn_wc_status_normal:
- text_statuschar = '_';
- break;
- case svn_wc_status_added:
- text_statuschar = 'A';
- break;
- case svn_wc_status_absent:
- text_statuschar = '!';
- break;
- case svn_wc_status_deleted:
- text_statuschar = 'D';
- break;
- case svn_wc_status_replaced:
- text_statuschar = 'R';
- break;
- case svn_wc_status_modified:
- text_statuschar = 'M';
- break;
- case svn_wc_status_merged:
- text_statuschar = 'G';
- break;
- case svn_wc_status_conflicted:
- text_statuschar = 'C';
- break;
- case svn_wc_status_unversioned:
- default:
- text_statuschar = '?';
- break;
- }
+ /* If WIDTH <= 20, we must copy the last WIDTH (non-\0) characters
+ of TEMP to FIELD. If WIDTH > 20, we must copy all of TEMP to
+ the last 20 characters of FIELD. So, COPY_LENGTH is the number
+ of characters to copy across, TEMP_OFFSET is the starting offset
+ within TEMP, and FIELD_OFFSET is the starting offset within
FIELD. */
+ size_t copy_length = width <= 20 ? width : 20;
+ size_t temp_offset = 20 - copy_length;
+ size_t field_offset = width - copy_length;
 
- switch (prop_status)
+ sprintf (temp, "%20lu", (unsigned long) rev);
+
+ memcpy (field+field_offset, temp+temp_offset, copy_length);
+}
+
+/* Perform the work of a fill-in function for the status of the
+ text or properties of a node. */
+static char
+fill_in__status (enum svn_wc_status_kind status)
+{
+ switch (status)
     {
- case svn_wc_status_none:
- prop_statuschar = ' ';
- break;
- case svn_wc_status_normal:
- prop_statuschar = '_';
- break;
- case svn_wc_status_added:
- prop_statuschar = 'A';
- break;
- case svn_wc_status_absent:
- prop_statuschar = '!';
- break;
- case svn_wc_status_deleted:
- prop_statuschar = 'D';
- break;
- case svn_wc_status_replaced:
- prop_statuschar = 'R';
- break;
- case svn_wc_status_modified:
- prop_statuschar = 'M';
- break;
- case svn_wc_status_merged:
- prop_statuschar = 'G';
- break;
- case svn_wc_status_conflicted:
- prop_statuschar = 'C';
- break;
- case svn_wc_status_unversioned:
- default:
- prop_statuschar = '?';
- break;
+ case svn_wc_status_none: return ' ';
+ case svn_wc_status_normal: return '_';
+ case svn_wc_status_added: return 'A';
+ case svn_wc_status_absent: return '!';
+ case svn_wc_status_deleted: return 'D';
+ case svn_wc_status_replaced: return 'R';
+ case svn_wc_status_modified: return 'M';
+ case svn_wc_status_merged: return 'G';
+ case svn_wc_status_conflicted: return 'C';
+ case svn_wc_status_unversioned: return '?';
+ default: return '?';
     }
+}
 
- sprintf (str_status, "%c%c%c%c",
- text_statuschar,
- prop_statuschar,
- locked ? 'L' : ' ',
- copied ? '+' : ' ');
+
+
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the status of the content of a node. */
+static void
+fill_in_text_status (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ assert (width >= 1);
+ field[0] = fill_in__status (status->text_status);
 }
 
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the status of the properties of a node. */
+static void
+fill_in_prop_status (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ assert (width >= 1);
+ field[0] = fill_in__status (status->prop_status);
+}
 
-/* Print a single status structure in the short format */
-static void
-print_short_format (const char *path,
- svn_wc_status_t *status)
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the lockedness of a node. */
+static void
+fill_in_lockedness (char *field, size_t width, const svn_wc_status_t
*status)
 {
- char str_status[5];
+ assert (width >= 1);
+ if (status->locked)
+ field[0] = 'L';
+}
 
- if (! status)
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the copiedness-with-history of a node. */
+static void
+fill_in_copiedness (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ assert (width >= 1);
+ if (status->copied)
+ field[0] = '+';
+}
+
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the out-of-dateness of a node. */
+static void
+fill_in_out_of_dateness (char *field, size_t width, const
svn_wc_status_t *status)
+{
+ assert (width >= 1);
+ if ((status->repos_text_status != svn_wc_status_none)
+ || (status->repos_prop_status != svn_wc_status_none))
+ field[0] = '*';
+}
+
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the working revision of a node. */
+static void
+fill_in_working_rev (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ assert (width >= 1);
+
+ if (! status->entry)
+ return;
+
+ if (status->entry->revision == SVN_INVALID_REVNUM)
+ field[width-1] = '?';
+ else if (status->copied)
+ field[width-1] = '-';
+ else
+ fill_in__revnum (field, width, status->entry->revision);
+}
+
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the revision number in which a node last changed. */
+static void
+fill_in_changed_rev (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ assert (width >= 1);
+
+ if (! status->entry)
     return;
 
- /* Create local-mod status code block. */
- svn_cl__generate_status_codes (str_status,
- status->text_status,
- status->prop_status,
- status->locked,
- status->copied);
-
- printf ("%s %s\n", str_status, path);
+ if (status->entry->cmt_rev == SVN_INVALID_REVNUM)
+ field[width-1] = '?';
+ else
+ fill_in__revnum (field, width, status->entry->cmt_rev);
 }
 
+/* Fill-in function (see definition of STATUS_ITEM above)
+ for the author of the last change to a node. */
+static void
+fill_in_author (char *field, size_t width, const svn_wc_status_t
*status)
+{
+ const char * author = 0;
 
-/* Print a single status structure in the long format */
-static void
-print_long_format (const char *path,
- svn_boolean_t show_last_committed,
- svn_wc_status_t *status)
-{
- char str_status[5];
- char str_rev[7];
- char update_char;
- svn_revnum_t local_rev;
- char last_committed[6 + 3 + 8 + 3 + 1] = { 0 };
+ assert (width >= 1);
 
- if (! status)
+ if (! status->entry)
     return;
 
- /* Create local-mod status code block. */
- svn_cl__generate_status_codes (str_status,
- status->text_status,
- status->prop_status,
- status->locked,
- status->copied);
-
- /* Get local revision number */
- if (status->entry)
- local_rev = status->entry->revision;
+ author = status->entry->cmt_author;
+ if (! author)
+ field[0] = '?';
   else
- local_rev = SVN_INVALID_REVNUM;
+ {
+ size_t length = strlen (author);
+ size_t copy_length = length <= width ? length : width;
+
+ memcpy (field, author, copy_length);
+ }
+}
 
- if (show_last_committed && status->entry)
+static const status_item_t
+all_fields[] = {
+ { "puvw", 1, &fill_in_text_status, "textstatus",
+ "status of file/directory contents" },
+ { "puvw", 1, &fill_in_prop_status, "propstatus",
+ "status of file/directory properties" },
+ { "puvw", 1, &fill_in_lockedness, "locked",
+ "'L' if file/directory is locked by Subversion" },
+ { "puvw", 1, &fill_in_copiedness, "copied",
+ "'+' if to be added with history" },
+ { "puvw", 3, 0, 0, 0 },
+ { " u w", 1, &fill_in_out_of_dateness, "update",
+ "'*' if file/directory needs updating" },
+ { " v ", 1, 0, 0, 0 },
+ { " uvw", 3, 0, 0, 0 },
+ { " uvw", 6, &fill_in_working_rev, "workrev",
+ "working revision number" },
+ { " uvw", 3, 0, 0, 0 },
+ { " vw", 6, &fill_in_changed_rev, "changerev",
+ "revision number of last change" },
+ { " vw", 3, 0, 0, 0 },
+ { " vw", 8, &fill_in_author, "author",
+ "author of last change" },
+ { " vw", 3, 0, 0, 0 },
+ { 0, 0, 0, 0, 0 }
+};
+
+
+
+/* Run through all the items for a particular variant,
+ filling stuff in starting at BUFFER. If BUFFER is a
+ null pointer, don't bother filling anything in.
+ Return the total width we've moved past in doing this.
+ Assume BUFFER is large enough and is full of spaces already.
+ If BUFFER is non-null, STATUS must be too. If BUFFER is null,
+ STATUS is unused.*/
+static size_t
+traverse_fields (char *buffer,
+ char variant,
+ const svn_wc_status_t *status)
+{
+ size_t offset = 0;
+ const status_item_t *item;
+
+ for (item=all_fields; item->variants; ++item)
     {
- char revbuf[20];
- const char *revstr = revbuf;
- const char *author;
-
- author = status->entry->cmt_author;
- if (SVN_IS_VALID_REVNUM (status->entry->cmt_rev))
- sprintf(revbuf, "%" SVN_REVNUM_T_FMT,
status->entry->cmt_rev);
- else
- revstr = " ? ";
-
- /* ### we shouldn't clip the revstr and author, but that
implies a
- ### variable length 'last_committed' which means an
allocation,
- ### which means a pool, ... */
- sprintf (last_committed, "%6.6s %8.8s ",
- revstr,
- author ? author : " ? ");
+ if (! strchr (item->variants, variant))
+ continue;
+
+ if (buffer && item->tag)
+ item->fill_in (buffer+offset, item->width, status);
+ offset += item->width;
     }
- else
- strcpy (last_committed, " ");
 
- /* Set the update character. */
- update_char = ' ';
- if ((status->repos_text_status != svn_wc_status_none)
- || (status->repos_prop_status != svn_wc_status_none))
- update_char = '*';
+ return offset;
+}
 
- /* Determine the appropriate local revision string. */
- if (! status->entry)
- strcpy (str_rev, " ");
- else if (local_rev == SVN_INVALID_REVNUM)
- strcpy (str_rev, " ? ");
- else if (status->copied)
- strcpy (str_rev, " -");
- else
- sprintf (str_rev, "%6ld", local_rev);
+/* Output a single status line for a given field set. */
+static void
+print_status_line (const char *path,
+ char variant,
+ const svn_wc_status_t *status)
+{
+ char prefix[81];
+ size_t prefix_length = traverse_fields (0, variant, 0);
+ assert (prefix_length <= 80);
+
+ memset (prefix, ' ', prefix_length);
+ prefix[prefix_length] = '\0';
 
- /* One Printf to rule them all, one Printf to bind them..." */
- printf ("%s %c %s %s%s\n",
- str_status,
- update_char,
- str_rev,
- show_last_committed ? last_committed : "",
- path);
+ traverse_fields (prefix, variant, status);
+ printf ("%s%s\n", prefix, path);
 }
 
 
+
+/* Output field descriptions for all fields needed by the specified
variant.
+ variant. */
+static void
+describe_fields (char variant)
+{
+ size_t offset = 0;
+ const status_item_t *item;
 
+ for (item=all_fields; item->variants; ++item)
+ {
+ if (! strchr (item->variants, variant))
+ continue;
+
+ if (item->tag)
+ {
+ assert (offset+item->width < 100);
+ printf ("%2lu..%2lu %-10s %s\n",
+ (unsigned long) offset,
+ (unsigned long) offset+item->width-1,
+ item->tag, item->desc);
+ }
+ offset += item->width;
+ }
+ assert (offset < 100);
+
+ printf ("%2lu.... %-10s %s\n",
+ (unsigned long) offset,
+ "pathname",
+ "file or directory being examined");
+}
+
+
 /* Called by status-cmd.c */
 void
 svn_cl__print_status_list (apr_hash_t *statushash,
@@ -231,13 +347,20 @@
 {
   int i;
   apr_array_header_t *statusarray;
- svn_wc_status_t *status = NULL;
+ const svn_wc_status_t *status = NULL;
+ char variant = 'p';
 
   /* Convert the unordered hash to an ordered, sorted array */
   statusarray = apr_hash_sorted_keys (statushash,
                                       
svn_sort_compare_items_as_paths,
                                       pool);
 
+ /* Identify the fieldset we need */
+ if (show_last_committed)
+ variant = detailed ? 'w' : 'v';
+ else
+ variant = detailed ? 'u' : 'p';
+
   /* Loop over array, printing each name/status-structure */
   for (i = 0; i < statusarray->nelts; i++)
     {
@@ -255,16 +378,29 @@
       if (err)
         svn_handle_error (err, stderr, FALSE);
 
- if (detailed)
- print_long_format (path, show_last_committed, status);
- else
- print_short_format (path, status);
+ print_status_line (path, variant, status);
     }
 
   /* If printing in detailed format, we might have a head revision to
      print as well. */
   if (detailed && (youngest != SVN_INVALID_REVNUM))
     printf ("Head revision: %6ld\n", youngest);
+}
+
+/* Called by status-cmd.c */
+void
+svn_cl__describe_status_list (svn_boolean_t detailed,
+ svn_boolean_t show_last_committed)
+{
+ char variant = 'p';
+
+ /* Identify the fieldset we need */
+ if (show_last_committed)
+ variant = detailed ? 'w' : 'v';
+ else
+ variant = detailed ? 'u' : 'p';
+
+ describe_fields (variant);
 }
 
 
Index: ./subversion/clients/cmdline/main.c
===================================================================
--- ./subversion/clients/cmdline/main.c
+++ ./subversion/clients/cmdline/main.c Tue Aug 6 01:43:32 2002
@@ -75,6 +75,7 @@
     {"strict", svn_cl__strict_opt, 0, "use strict semantics"},
     {"no-ignore", svn_cl__no_ignore_opt, 0,
                       "disregard default and svn:ignore property
ignores"},
+ {"describe-only", 'd', 0, "describe columns; don't really do
status"},
     {0, 0, 0}
   };
 
@@ -359,6 +360,7 @@
     " With no args, print only locally modified files (no network
access).\n"
     " With -u, add out-of-date information from server.\n"
     " With -v, print excessive information on every file.\n"
+ " With -d, describe the output format given the other flags and
quit.\n"
     "\n"
     "Decoding the status output, each column is one character
wide:\n"
     " 1st column: Add or delete file or directory, or change file
contents\n"
@@ -378,7 +380,7 @@
     " _ 965 938 kfogel
./autogen.sh\n"
     " _ * 965 922 sussman
./build.conf\n"
     " M 965 687 joe
./buildcheck.sh\n",
- { 'u', 'v', 'N', 'q',
+ { 'u', 'v', 'N', 'q', 'd',
       svn_cl__auth_username_opt, svn_cl__auth_password_opt,
       svn_cl__no_ignore_opt } },
   
@@ -994,6 +996,9 @@
         break;
       case 'q':
         opt_state.quiet = TRUE;
+ break;
+ case 'd':
+ opt_state.describe_only = TRUE;
         break;
       case svn_cl__xml_file_opt:
         err = svn_utf_cstring_to_utf8 (&opt_state.xml_file, opt_arg,
Index: ./subversion/clients/cmdline/status-cmd.c
===================================================================
--- ./subversion/clients/cmdline/status-cmd.c
+++ ./subversion/clients/cmdline/status-cmd.c Tue Aug 6 01:14:18 2002
@@ -47,6 +47,13 @@
   SVN_ERR (svn_cl__args_to_target_array (&targets, os, opt_state,
                                          FALSE, pool));
 
+ if (opt_state->describe_only)
+ {
+ svn_cl__describe_status_list ((opt_state->verbose ||
opt_state->update),
+ opt_state->verbose);
+ return SVN_NO_ERROR;
+ }
+
   /* Build an authentication object to give to libsvn_client. */
   auth_baton = svn_cl__make_auth_baton (opt_state, pool);
 
--------------------- patch ends ---------------------

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