On Jul 25, 2005, at 4:14 PM, Alexander Thomas wrote:
>
> Fix issue #2291: 'svn ls -v' should return locking information.
>
> * subversion/include/svn_types.h
> (svn_dirent2_t): New struct.
> (svn_dirent_t): Deprecated and moved to group it with new struct.
>
> * subversion/libsvn_client/ls.c
> (svn_client_ls2): Gets lock information.
>
> * subversion/clients/cmdline/ls-cmd.c
> (print_dirents): Print lock information.
> (print_dirents_xml): Print lock information in xml.
>
> * subversion/clients/cmdline/main.c
> Mention locking information in the help text for 'svn ls'.
>
> * subversion/clients/cmdline/dtd/list.dtd
> Updated DTD file.
>
Log message looks good. I wouldn't exactly call this a "fix",
though. It's an enhancement, not a bug. :-)
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 15421)
> +++ subversion/include/svn_types.h (working copy)
> @@ -188,31 +188,6 @@
> svn_recursive
> };
>
> -
> -/** A general subversion directory entry. */
> -typedef struct svn_dirent_t
> -{
> - /** node kind */
> - svn_node_kind_t kind;
> -
> - /** length of file text, or 0 for directories */
> - svn_filesize_t size;
> -
> - /** does the node have props? */
> - svn_boolean_t has_props;
> -
> - /** last rev in which this node changed */
> - svn_revnum_t created_rev;
> -
> - /** time of created_rev (mod-time) */
> - apr_time_t time;
> -
> - /** author of created_rev */
> - const char *last_author;
> -
> -} svn_dirent_t;
> -
> -
>
>
> /** Keyword substitution.
> @@ -458,6 +433,67 @@
> svn_lock_t *
> svn_lock_dup (const svn_lock_t *lock, apr_pool_t *pool);
>
> +
> +/**
> + * A general subversion directory entry with lock information.
> + *
> + * @since New in 1.3.
> + */
> +typedef struct svn_dirent2_t
> +{
> + /** node kind */
> + svn_node_kind_t kind;
> +
> + /** length of file text, or 0 for directories */
> + svn_filesize_t size;
> +
> + /** does the node have props? */
> + svn_boolean_t has_props;
> +
> + /** last rev in which this node changed */
> + svn_revnum_t created_rev;
> +
> + /** time of created_rev (mod-time) */
> + apr_time_t time;
> +
> + /** author of created_rev */
> + const char *last_author;
> +
> + /** repository lock details, if present or NULL */
>
I might word this differently, perhaps something like "if not NULL,
the details of existing repository lock".
> + svn_lock_t *lock;
> +
> +} svn_dirent2_t;
> +
> +
> +/**
> + * Similar to @c svn_dirent2_t except that @svn_dirent_t give only
> + * subversion directory entry.
>
I can see that you imagine
svn_dirent2_t == svn_dirent_t + svn_lock_t
and therefore
svn_dirent2_t - svn_lock_t = svn_dirent_t
...but most people reading the code will have no idea about these
formulas and categorizations. It's not obvious that if you subtract
"only a directory entry" from a dirent2_t that a lock_t is left
behind: people think of the lock_t as part of the dirent, not some
extra thing.
So the simpler wording is just:
"Like a @c svn_dirent2_t, but without the @c svn_lock_t field."
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +typedef struct svn_dirent_t
> +{
> + /** node kind */
> + svn_node_kind_t kind;
> +
> + /** length of file text, or 0 for directories */
> + svn_filesize_t size;
> +
> + /** does the node have props? */
> + svn_boolean_t has_props;
> +
> + /** last rev in which this node changed */
> + svn_revnum_t created_rev;
> +
> + /** time of created_rev (mod-time) */
> + apr_time_t time;
> +
> + /** author of created_rev */
> + const char *last_author;
> +
> +} svn_dirent_t;
> +
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/libsvn_client/ls.c
> ===================================================================
> --- subversion/libsvn_client/ls.c (revision 15421)
> +++ subversion/libsvn_client/ls.c (working copy)
> @@ -75,7 +75,7 @@
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> - svn_boolean_t recurse,
> + svn_boolean_t recurse,
>
Spurious whitespace change here?
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -83,14 +83,18 @@
> svn_revnum_t rev;
> svn_node_kind_t url_kind;
> const char *url;
> + apr_hash_t *locks;
> + apr_hash_index_t *hi;
> + apr_hash_index_t *hl;
>
Can you use variables here whose names are less similar? I actually
had to squint for a minute, before realizing that the two hash
indices had different names! Or, better yet, maybe the variables
names could be more descriptive.
>
> +
> /* Get an RA plugin for this filesystem object. */
> SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> &url, path_or_url,
> peg_revision,
> revision, ctx, pool));
>
> /* Decide if the URL is a file or directory. */
> - SVN_ERR (svn_ra_check_path (ra_session, "", rev, &url_kind, pool));
> + svn_ra_check_path (ra_session, "", rev, &url_kind, pool);
>
>
Why are you deliberately ignoring the error here?
>
> if (url_kind == svn_node_dir)
> {
> @@ -135,6 +139,48 @@
> _("URL '%s' non-existent in that
> revision"),
> url);
>
> + /* Get Lock entries in to the hash. */
> + svn_ra_get_locks (ra_session, &locks, "", pool);
> +
> + for (hi = apr_hash_first (pool, *dirents); hi; hi =
> apr_hash_next (hi))
> + {
> + const void *key;
> + void *val;
> + const char *bname;
> + svn_dirent_t *entry;
> + svn_dirent2_t *newent;
> +
> + apr_hash_this (hi, &key, NULL, &val);
> + bname = key;
> + entry = val;
> +
> + newent = apr_pcalloc (pool, sizeof(*newent));
> + memcpy (newent, entry, sizeof (*entry));
> +
> + /* Traverse through locks hash. */
> + for (hl = apr_hash_first (pool, locks); hl; hl =
> apr_hash_next (hl))
> + {
> + const void *kkey;
> + void *vval;
> + const char *lockon;
> +
> + apr_hash_this (hl, &kkey, NULL, &vval);
> + lockon = kkey;
> +
> + if (strlen (bname) >= strlen(lockon))
> + continue;
> +
> + if (strcasecmp (&lockon[strlen(lockon)-strlen(bname)],
> bname) == 0)
> + {
> + newent->lock = vval;
> + break;
> + }
> +
> + }
>
Yikes! This is a hash! Instead of looping through the hash keys,
perhaps you should be looking up things using apr_hash_get() instead??
(By the way, strcasecmp is incorrect anyway... you're comparing
repository paths with repository paths, and repository paths are
always case-sensitive.)
So let me step back a second, and suggest we take a slightly
different route here. kfogel, lundblad and I had a conversation, and
we think that this patch puts us in a bad "halfway" place. This
patch deprecates svn_dirent_t, but not fully. It's still being used
in other places. We think that we should either do it all the way,
or not do it.
Option 1: *Fully* deprecate svn_dirent_t. That means stop using it
everywhere. It means having to rev all the public libsvn_client and
libsvn_ra APIs that make use of it. Quite a lot of work.
Option 2: Just don't bother to create svn_dirent2_t. Instead, just
rev svn_client_ls() could return a new "svn_client_dirent_t"
structure which contains the lock_t. (Or return the hash of locks to
the caller.)
We need to discuss these options, because there are problems with both.
Alexander: many apologies for bouncing the strategy of your patch
back and forth. You are a man of infinite patience. :-) Let's
discuss a bit more before you write another iteration of this patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 00:41:09 2005