Alexander, ping! I've addressed most of Peter's concerns. Can you comment on
the rest?
Peter N. Lundblad wrote:
> On Thu, 11 Aug 2005 sussman@tigris.org wrote:
> r15678 | sussman | 2005-08-11 15:02:04 +0100 (Thu, 11 Aug 2005) | 25 lines
>
> Fix issue #2291: 'svn ls -v' should show the existence of repository locks.
> Slightly tweaked by sussman.
>
> Patch by: Alexander Thomas <alexander@collab.net>
> Review by: sussman
> lundblad
>>Modified: trunk/subversion/clients/cmdline/ls-cmd.c
[...]
>>+ svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
>>+ "token", NULL);
>>+ svn_xml_escape_cdata_cstring (&sb, lock->token, pool);
>>+ svn_xml_make_close_tag (&sb, pool, "token");
>
> The above is now handled by svn_cl__xml_tagged_cdata. That's rather new.
> Same for the other lock fields below.
Fixed.
I notice also that it should use "subpool"; also fixed.
[...]
>>+ /* "<lock>" */
>
> I think such comments are redundant, but if you keep it, it should be
> "</lock>".
Fixed.
>>@@ -292,14 +345,15 @@
>> /* Get peg revisions. */
>> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
>> subpool));
>>- SVN_ERR (svn_client_ls2 (&dirents, truepath, &peg_revision,
>>+
>>+ SVN_ERR (svn_client_ls3 (&dirents, &locks, truepath, &peg_revision,
>> &(opt_state->start_revision),
>> opt_state->recursive, ctx, subpool));
>
> An improvement would be to pass NULL for the locks argument if
> !opt_state->verbose. It avoids downloading all the locks if they aren't
> displayed.
Done.
>>Modified: trunk/subversion/include/svn_client.h
[...]
>>+/**
>>+ * Same as svn_client_ls3(), but always passes a NULL lock hash.
>>+ *
>>+ * @deprecated Provided for backward compatibility with the 1.2 API.
>> */
>> svn_error_t *
>> svn_client_ls2 (apr_hash_t **dirents,
>
> This remoes the @since from svn_ls_client2().
Fixed.
>>Modified: trunk/subversion/libsvn_client/ls.c
[...]
>>+ if (locks == NULL)
>>+ return SVN_NO_ERROR;
>>+
>>+ if (rel_path == NULL || rel_path[0] == 0)
>>+ rel_path = "/";
>>+ else
>>+ rel_path = apr_psprintf (pool, "/%s/", rel_path);
>
> Hmmm... I don't understand what this above code is doing. When can
> rel_path be NULL, and why are slashes added at each end of the string
> unconditionally?
rel_path is null when you list the root of the repository. On the other hand,
it is never the empty string. I am updating the doc string of
svn_path_is_child() to make that explicit.
The trailing slash is removed by the "canonicalize" below, so that was redundant.
The leading slash is necessary to make this path match the the beginning of the
absolute paths returned by svn_ra_get_locks().
>>+ subpool = svn_pool_create (pool);
>>+
>>+ /* Get lock. */
>
> s/lock/locks/
Fixed. (It's a fairly redundant comment but it does provide a quick pointer to
where the real work happens among all the variable manipulations and error
handling, so I left it in.)
I also encapsulated the locks part of the function in its own code block so it
doesn't get messier next time a new feature is added to the function.
>>+ SVN_ERR (svn_ra_get_locks (ra_session, locks, "", pool));
>>+
>>+ new_locks = apr_hash_make (pool);
>>+ for (hi = apr_hash_first (pool, *locks); hi; hi = apr_hash_next (hi))
>>+ {
>>+ const void *key;
>>+ void *val;
>>+ const char *newkey;
>>+
>>+ svn_pool_clear (subpool);
>>+
>>+ apr_hash_this (hi, &key, NULL, &val);
>>+ newkey = svn_path_is_child (svn_path_canonicalize (rel_path, subpool),
>>+ svn_path_canonicalize (key, subpool),
>>+ pool);
>
> Why canonicalize here? If rel_path needs that, it could be done outside of
> the loop, but why are we creating non-canonical paths in the first place?
> And regarding key, doesn't the API guarantee that that is canonicalized?
The trailing slash and the canonicalisation appear to be unnecessary.
Alexander, can you comment?
I attach a patch which addresses everything here (and a bit more). Is it correct?
- Julian
Follow-up tweaks to r15678, 15717, 15884 which added lock information to "svn
ls -v" (issue #2291). Improve efficiency by not fetching locks from the
repository if they aren't going to be displayed. Clean up code and comments.
Suggested by: lundblad
me
* subversion/clients/cmdline/ls-cmd.c
(print_dirents): Localise the code that handles locks; remove an unused bit.
(print_dirents_xml): Simplify code by using svn_cl__xml_tagged_cdata.
(svn_cl__ls): Avoid fetching lock info if it is not required.
* subversion/libsvn_client/ls.c
(svn_client_ls3): Localise the code that handles locks; remove needless bits.
* subversion/include/svn_client.h
(svn_client_ls2): Restore its "@since" tag.
Index: subversion/clients/cmdline/ls-cmd.c
===================================================================
--- subversion/clients/cmdline/ls-cmd.c (revision 16034)
+++ subversion/clients/cmdline/ls-cmd.c (working copy)
@@ -52,7 +52,6 @@
{
const char *utf8_entryname;
svn_dirent_t *dirent;
- svn_lock_t *lock;
svn_sort__item_t *item;
svn_pool_clear (subpool);
@@ -65,7 +64,6 @@
utf8_entryname = item->key;
dirent = apr_hash_get (dirents, utf8_entryname, item->klen);
- lock = apr_hash_get (locks, utf8_entryname, item->klen);
if (verbose)
{
@@ -74,8 +72,9 @@
apr_status_t apr_err;
apr_size_t size;
char timestr[20];
- const char *sizestr, *utf8_timestr, *lock_owner;
-
+ const char *sizestr, *utf8_timestr;
+ svn_lock_t *lock;
+
/* svn_time_to_human_cstring gives us something *way* too long
to use for this, so we have to roll our own. We include
the year if the entry's time is not within half a year. */
@@ -101,8 +100,7 @@
sizestr = apr_psprintf (subpool, "%" SVN_FILESIZE_T_FMT,
dirent->size);
- if (lock)
- lock_owner = apr_psprintf (subpool, "*%s", lock->owner);
+ lock = apr_hash_get (locks, utf8_entryname, item->klen);
SVN_ERR (svn_cmdline_printf
(subpool, "%7ld %-8.8s %c %10s %12s %s%s\n",
@@ -217,47 +215,27 @@
/* "</commit>" */
svn_xml_make_close_tag (&sb, subpool, "commit");
- /* "<lock>" */
if (lock)
{
- svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
+ /* "<lock>" */
+ svn_xml_make_open_tag (&sb, subpool, svn_xml_normal, "lock", NULL);
- svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
- "token", NULL);
- svn_xml_escape_cdata_cstring (&sb, lock->token, pool);
- svn_xml_make_close_tag (&sb, pool, "token");
-
- svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
- "owner", NULL);
- svn_xml_escape_cdata_cstring (&sb, lock->owner, pool);
- svn_xml_make_close_tag (&sb, pool, "owner");
+ svn_cl__xml_tagged_cdata (&sb, subpool, "token", lock->token);
- if (lock->comment)
- {
- svn_xml_make_open_tag (&sb, pool, svn_xml_normal,
- "comment", NULL);
- svn_xml_escape_cdata_cstring (&sb, lock->comment, pool);
- svn_xml_make_close_tag (&sb, pool, "comment");
- }
+ svn_cl__xml_tagged_cdata (&sb, subpool, "owner", lock->owner);
- svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
- "created", NULL);
- svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring
- (lock->creation_date, pool),
- pool);
- svn_xml_make_close_tag (&sb, pool, "created");
+ svn_cl__xml_tagged_cdata (&sb, subpool, "comment", lock->comment);
+
+ svn_cl__xml_tagged_cdata (&sb, subpool, "created",
+ svn_time_to_cstring (lock->creation_date,
+ subpool));
if (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
- (lock->expiration_date, pool),
- pool);
- svn_xml_make_close_tag (&sb, pool, "expires");
- }
+ svn_cl__xml_tagged_cdata (&sb, subpool, "expires",
+ svn_time_to_cstring
+ (lock->expiration_date, subpool));
- /* "<lock>" */
+ /* "</lock>" */
svn_xml_make_close_tag (&sb, subpool, "lock");
}
/* "</entry>" */
@@ -346,7 +324,10 @@
SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
subpool));
- SVN_ERR (svn_client_ls3 (&dirents, &locks, truepath, &peg_revision,
+ SVN_ERR (svn_client_ls3 (&dirents,
+ (opt_state->xml || opt_state->verbose)
+ ? &locks : NULL,
+ truepath, &peg_revision,
&(opt_state->start_revision),
opt_state->recursive, ctx, subpool));
Index: subversion/libsvn_client/ls.c
===================================================================
--- subversion/libsvn_client/ls.c (revision 16034)
+++ subversion/libsvn_client/ls.c (working copy)
@@ -87,10 +87,6 @@
const char *url;
const char *repos_root;
const char *rel_path;
- apr_pool_t *subpool;
- apr_hash_t *new_locks;
- apr_hash_index_t *hi;
- svn_error_t *err;
/* Get an RA plugin for this filesystem object. */
SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
@@ -149,46 +145,41 @@
_("URL '%s' non-existent in that revision"),
url);
- if (locks == NULL)
- return SVN_NO_ERROR;
-
- if (rel_path == NULL || rel_path[0] == 0)
- rel_path = "/";
- else
- rel_path = apr_psprintf (pool, "/%s/", rel_path);
-
- subpool = svn_pool_create (pool);
-
- /* Get lock. */
- err = svn_ra_get_locks (ra_session, locks, "", pool);
-
- if (err && err->apr_err == SVN_ERR_RA_NOT_IMPLEMENTED)
+ if (locks)
{
- svn_error_clear (err);
- *locks = apr_hash_make (pool);
- }
- else if (err)
- return err;
-
- new_locks = apr_hash_make (pool);
- for (hi = apr_hash_first (pool, *locks); hi; hi = apr_hash_next (hi))
- {
- const void *key;
- void *val;
- const char *newkey;
-
- svn_pool_clear (subpool);
-
- apr_hash_this (hi, &key, NULL, &val);
- newkey = svn_path_is_child (svn_path_canonicalize (rel_path, subpool),
- svn_path_canonicalize (key, subpool),
- pool);
- if (newkey)
- apr_hash_set (new_locks, newkey, APR_HASH_KEY_STRING, val);
- }
+ apr_hash_t *new_locks;
+ apr_hash_index_t *hi;
+ svn_error_t *err;
+
+ /* Add a leading slash to match the paths from svn_ra_get_locks(). */
+ rel_path = apr_psprintf (pool, "/%s", rel_path ? rel_path : "");
+
+ /* Get locks. */
+ err = svn_ra_get_locks (ra_session, locks, "", pool);
+
+ if (err && err->apr_err == SVN_ERR_RA_NOT_IMPLEMENTED)
+ {
+ svn_error_clear (err);
+ *locks = apr_hash_make (pool);
+ }
+ else if (err)
+ return err;
+
+ new_locks = apr_hash_make (pool);
+ for (hi = apr_hash_first (pool, *locks); hi; hi = apr_hash_next (hi))
+ {
+ const void *key;
+ void *val;
+ const char *newkey;
+
+ apr_hash_this (hi, &key, NULL, &val);
+ newkey = svn_path_is_child (rel_path, key, pool);
+ if (newkey)
+ apr_hash_set (new_locks, newkey, APR_HASH_KEY_STRING, val);
+ }
- svn_pool_destroy (subpool);
- *locks = new_locks;
+ *locks = new_locks;
+ }
return SVN_NO_ERROR;
}
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 16034)
+++ subversion/include/svn_client.h (working copy)
@@ -2060,6 +2060,8 @@
/**
* Same as svn_client_ls3(), but always passes a NULL lock hash.
*
+ * @since New in 1.2.
+ *
* @deprecated Provided for backward compatibility with the 1.2 API.
*/
svn_error_t *
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Sep 3 19:30:08 2005