Can you repost this patch with a log message? Thanks,
-Karl
Gareth McCaughan <Gareth.McCaughan@pobox.com> writes:
> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 27 20:52:22 2002