Ben Collins-Sussman wrote:
>> ===================================================================
>> --- 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?
Yes only whitespace change.
>> @@ -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.
>
I will keep this in mind
>> /* 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?
Sorry, accidentally changed, SVN_ERR needed here
>> + /* 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.)
get_locks () seems to return hash key relative to repository root
and get_dir() hash key is relative to target. So we need to nullify the
difference between the two keys. Yes, still we can make use of
apr_hash_get() here.
>
> 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.
Yes, we need further discussion on this.
Making changes in svn_client_ls() to accommodate lock_t, feels like
hackish. MHO is to move this to ra_get_dir() like in version #4 and
return a hash containing both dirent_t and lock_t. Yes it need lot
more work, but hope will be more efficient.
> 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.
No need for many apologies, one will do :). I am still in subversion
LEARNING MODE, so never mind.
Thanks
-Alexander Thomas (AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 01:57:13 2005