[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [Patch] Issue #2291 - 'svn ls -v' return locking information

From: Madan U Sreenivasan <madan_at_collab.net>
Date: 2005-06-30 15:03:15 CEST

Pl. find my comments embedded....
________________________________________________________________________
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 15177)
> +++ subversion/include/svn_types.h (working copy)
> @@ -188,7 +188,45 @@
> svn_recursive
> };
>
> +/** Added loc information with exiting directory entry. */
> +typedef struct svn_dirent2_t
> +{
[snip]
> +
> /** A general subversion directory entry. */
> typedef struct svn_dirent_t
> {
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15177)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1797,31 +1797,46 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> -[snip]
> svn_error_t *
> +svn_client_ls3 (apr_hash_t **dirents,
> + const char *path_or_url,
> + const svn_opt_revision_t *peg_revision,
> + const svn_opt_revision_t *revision,
> + svn_boolean_t recurse,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
We do not need this new version of the API... as the signature is still
the same.
> +/**
> + * Similar to svn_client_ls3() except that the allocated hash of entries
> + * will contain only dirent and the hash maps entry names
> + * (<tt>const char *</tt>) to @c svn_dirent_t *'s.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> svn_client_ls2 (apr_hash_t **dirents,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> Index: subversion/libsvn_client/ls.c
> ===================================================================
> --- subversion/libsvn_client/ls.c (revision 15177)
> +++ subversion/libsvn_client/ls.c (working copy)
> @@ -36,7 +36,6 @@
> apr_pool_t *pool)
> {
> apr_hash_t *tmpdirents;
> - svn_dirent_t *the_ent;
> apr_hash_index_t *hi;
>
> /* Get the directory's entries, but not its props. */
> @@ -53,16 +52,29 @@
> const char *path;
> const void *key;
> void *val;
> + svn_lock_t *lock;
> + svn_dirent2_t *lsent = apr_pcalloc (pool, sizeof(svn_dirent2_t));
I dont see the need to change the variable name???!!!!
>
> apr_hash_this (hi, &key, NULL, &val);
>
> - the_ent = val;
> + memcpy (lsent, val, sizeof (svn_dirent_t));
unnecessary?!
> [snip] recurse, ctx, pool));
> }
> @@ -71,6 +83,87 @@
> }
>
> svn_error_t *
> +svn_client_ls3 (apr_hash_t **dirents,
> + const char *path_or_url,
> + const svn_opt_revision_t *peg_revision,
> + const svn_opt_revision_t *revision,
> + svn_boolean_t recurse,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
[snip]
> + else if (url_kind == svn_node_file)
> + {
> + apr_hash_t *parent_ents;
> + const char *parent_url, *base_name;
> + svn_dirent_t *the_ent;
Do we need this anymore??
> + svn_dirent2_t *ls_ent = apr_pcalloc (pool, sizeof(svn_dirent2_t));
> + svn_lock_t *lock;
> +
> + /* Re-open the session to the file's parent instead. */
> + svn_path_split (url, &parent_url, &base_name, pool);
> + /* 'base_name' is now the last component of an URL, but we want
> + to use it as a plain file name. Therefore, we must URI-decode
> + it. */
> + base_name = svn_path_uri_decode(base_name, pool);
> + SVN_ERR (svn_client__open_ra_session_internal (&ra_session, parent_url,
> + NULL, NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> +
> + /* Get all parent's entries, no props. */
> + SVN_ERR (svn_ra_get_dir (ra_session, "", rev, &parent_ents,
> + NULL, NULL, pool));
> +
> + /* Get base name lock information. */
> + SVN_ERR (svn_ra_get_lock (ra_session, &lock, base_name, pool));
> +
> + /* Copy the relevant entry into the caller's hash. */
> + *dirents = apr_hash_make (pool);
> + the_ent = apr_hash_get (parent_ents, base_name, APR_HASH_KEY_STRING);
> + if (the_ent == NULL)
> + return svn_error_createf (SVN_ERR_FS_NOT_FOUND, NULL,
> + _("URL '%s' non-existent in that revision"),
> + url);
> + memcpy (ls_ent, the_ent, sizeof (svn_dirent_t));
This is unnecessary... why not use ls_ent directly... or better still use the old var name with the new datatype?
> + if (lock)
> + {
> + ls_ent->lock_token = lock->token;
> + ls_ent->lock_owner = lock->owner;
> + ls_ent->lock_comment = lock->comment;
> + ls_ent->lock_creation_date = lock->creation_date;
> + ls_ent->lock_expiration_date = lock->expiration_date;
> + }
The above chunk of code is repeated. the same code is also used by
get_dir_contents(). Can we isolate this code into a function?
> +
> + apr_hash_set (*dirents, base_name, APR_HASH_KEY_STRING, ls_ent);
> + }
> + else
> + return svn_error_createf (SVN_ERR_FS_NOT_FOUND, NULL,
> + _("URL '%s' non-existent in that revision"),
> + url);
> +
> + return SVN_NO_ERROR;
> +}
You should consider factorizing new versions of the API and not copying.
Anyways, as mentioned above, we dont need a new version of the API. Pl.
make changes to svn_client_ls2() (....and all calling functions
appropriately)
> +
> +svn_error_t *
> svn_client_ls2 (apr_hash_t **dirents,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> Index: subversion/clients/cmdline/ls-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/ls-cmd.c (revision 15177)
> +++ subversion/clients/cmdline/ls-cmd.c (working copy)
[snip]
> SVN_ERR (svn_cmdline_printf
> - (subpool, "%7ld %-8.8s %10s %12s %s%s\n",
> + (subpool, "%7ld %-12s %10s %12s %12s %-12s %s%s\n",
> dirent->created_rev,
> dirent->last_author ? dirent->last_author : " ? ",
> (dirent->kind == svn_node_file) ? sizestr : "",
> utf8_timestr,
> + dirent->lock_owner ? utf8_lock_timestr : "",
> + dirent->lock_owner ? dirent->lock_owner : "",
> utf8_entryname,
> (dirent->kind == svn_node_dir) ? "/" : ""));
> }
Nice... but have you considered changing the print_dirents_xml()
function to accomodate the lock info?
> [snip]
> Index: subversion/clients/cmdline/main.c
> ===================================================================
> --- subversion/clients/cmdline/main.c (revision 15177)
> +++ subversion/clients/cmdline/main.c (working copy)
> @@ -379,12 +379,14 @@
> "current\n"
> " working directory.\n"
> "\n"
> - " With --verbose, the following fields show the status of the item:\n"
> + " With --verbose, the following fields will be shown for each item:\n"
I prefer the existing version rather...
> "\n"
[snip]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 30 16:01:31 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.