You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2005/08/08 13:28:26 UTC
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V6
On Jul 31, 2005, at 5:51 PM, Alexander Thomas wrote:
>
> /**
> - * Set @a *dirents to a newly allocated hash of entries for @a
> + * Set @a *dirents and *locks to a newly allocated hash of entries
> for @a
> * path_or_url at @a revision. The actual node revision selected is
> * determined by the path as it exists in @a peg_revision. If @a
> * peg_revision is @c svn_opt_revision_unspecified, then it defaults
> @@ -1810,18 +1810,36 @@
> * @a path_or_url is a file, return only the dirent for the file.
> If @a
> * path_or_url is non-existent, return @c SVN_ERR_FS_NOT_FOUND.
> *
> - * The hash maps entry names (<tt>const char *</tt>) to @c
> svn_dirent_t *'s.
> - * Do all allocation in @a pool.
> + * The dirents hash maps entry names (<tt>const char *</tt>) to
> + * @c svn_dirent_t *'s. Do all allocation in @a pool.
> *
> + * The locks hash maps entry names (<tt>const char *</tt>) to
> + * @c svn_lock_t *'s. Do all allocation in @a pool.
> + *
> * Use authentication baton cached in @a ctx to authenticate
> against the
> * repository.
> *
> * If @a recurse is true (and @a path_or_url is a directory) this
> will
> * be a recursive operation.
> *
> - * @since New in 1.2.
> + * @since New in 1.3.
> */
The svn_client_ls3() docstring needs to say that the locks hash is
optional.
> svn_error_t *
> +svn_client_ls3 (apr_hash_t **dirents,
> + apr_hash_t **locks,
> + 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);
> +
> +/**
> + * Similar to svn_client_ls3(), return only dirent's in the form
> hash.
"form" hash? I think that's a typo. I think the docstring just
needs to say, "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,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> Index: subversion/libsvn_client/ls.c
> ===================================================================
> --- subversion/libsvn_client/ls.c (revision 15448)
> +++ subversion/libsvn_client/ls.c (working copy)
> @@ -71,6 +71,79 @@
> }
>
> svn_error_t *
> +svn_client_ls3 (apr_hash_t **dirents,
> + apr_hash_t **locks,
> + 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)
> +{
> + svn_ra_session_t *ra_session;
> + const char *url;
> + const char *repos_root;
> + svn_revnum_t rev;
> + apr_hash_index_t *hi;
> + const char *rel_path;
> + svn_node_kind_t kind;
> + apr_hash_t *new_locks;
> + apr_pool_t *subpool;
> +
> + SVN_ERR (svn_client_ls2 (dirents, path_or_url, peg_revision,
> + revision, recurse, ctx, pool));
> +
Here's the big problem: you've defined the new function in terms of
the old function, which is backwards. When you deprecate a function,
it means that subversion has to stop using it absolutely everywhere.
Our standard technique for rev'ving an API is to define the
deprecated function in terms of the new function. So svn_client_ls2
() should simply be a call to svn_client_ls3() with a NULL lock
hash. And what was formerly svn_client_ls2() should be renamed to
ls3, and extra lock features then added to it.
> + if (locks == NULL)
> + return SVN_NO_ERROR;
> +
> + /* Getting repository root. */
> + SVN_ERR (svn_client_url_from_path (&repos_root, "", pool));
> +
> + /* 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));
> +
... which means there will be no need to open a new ra_session,
right? :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking
information - V7
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-08-11 at 08:11 -0500, Ben Collins-Sussman wrote:
> On Aug 11, 2005, at 7:22 AM, Alexander Thomas wrote:
>
> > On Wed, 2005-08-10 at 15:20 -0500, Ben Collins-Sussman wrote:
> >
> >> Ok, time to argue about bikeshed color. After applying a tweaked
> >> version of the patch and running 'svn ls -v', nearly every single
> >> filename wraps off the edge of my 80-column terminal. This is not
> >> good.
> >>
> >>
> > Yes I agree is not good, not bad either.
>
> For this project, it's bad. We have an 80-column convention not only
> for our source code, but for commandline output as well. Granted,
> 'svn ls' cannot ever *guarantee* that it will never go beyond 80
> columns, because it has no control over the length of filenames. But
> we're still under an obligation to try to prevent that situation as
> much possible, by providing as much space for the filename as we can.
>
Lets hope we will be successful in restricting outputs to 80-col in
feature too. If it our policy, I don't have any difference.
> Under that philosophy, the main goal of this feature is to make 'svn
> ls -v' indicate that a file *is locked* at all, not to display all
> the lock fields. ("svn info" is the best tool for showing all lock
> fields.) So I think we should have exactly one new column that
> indicates the presence of a remote lock: it could be a single
> character, or it could be one of the svn_lock_t fields. But more
> than one column is overkill; the goal is simply to let users know
> that a lock exists.
>
Here too.
> >
> > I think it pretty clear from 'svn help' the order of occurrence, So
> > why we need to distinguish between last_author and lock_author in
> > output?.
>
> Because not everybody reads the help output. The new "lock is
> present" column should stand out as something unique. For example,
> seeing
>
> 398 username1 username2 216576 Apr 27 14:31 slouch.z5
>
> ... I think users will scratch their heads, wondering why the
> username is being listed twice. They may not even bother to look at
> 'svn help ls', but just gloss over the extra information.
>
If users got confused and scratch their heads next thing they do is,
check the available docs here its 'svn help ls' :)
Thanks for investing your valuable time to explain stuffs in-depth.
Regards
--
########################
#####****######********#
####*~ *~*#####***~~***#
###*~*# *~*######*~~*###
##*~* ** *~*#####*~~*###
#***######***####****###
########################
Alexander Thomas (AT)
alexander@collab.net
Collabnet Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
On Aug 11, 2005, at 7:22 AM, Alexander Thomas wrote:
> On Wed, 2005-08-10 at 15:20 -0500, Ben Collins-Sussman wrote:
>
>> Ok, time to argue about bikeshed color. After applying a tweaked
>> version of the patch and running 'svn ls -v', nearly every single
>> filename wraps off the edge of my 80-column terminal. This is not
>> good.
>>
>>
> Yes I agree is not good, not bad either.
For this project, it's bad. We have an 80-column convention not only
for our source code, but for commandline output as well. Granted,
'svn ls' cannot ever *guarantee* that it will never go beyond 80
columns, because it has no control over the length of filenames. But
we're still under an obligation to try to prevent that situation as
much possible, by providing as much space for the filename as we can.
Under that philosophy, the main goal of this feature is to make 'svn
ls -v' indicate that a file *is locked* at all, not to display all
the lock fields. ("svn info" is the best tool for showing all lock
fields.) So I think we should have exactly one new column that
indicates the presence of a remote lock: it could be a single
character, or it could be one of the svn_lock_t fields. But more
than one column is overkill; the goal is simply to let users know
that a lock exists.
>
> I think it pretty clear from 'svn help' the order of occurrence, So
> why we need to distinguish between last_author and lock_author in
> output?.
Because not everybody reads the help output. The new "lock is
present" column should stand out as something unique. For example,
seeing
398 username1 username2 216576 Apr 27 14:31 slouch.z5
... I think users will scratch their heads, wondering why the
username is being listed twice. They may not even bother to look at
'svn help ls', but just gloss over the extra information.
But if users see
398 username1 *username2 216576 Apr 27 14:31 slouch.z5
or
398 username1 * 216576 Apr 27 14:31 slouch.z5
... then it's clearer that something is "special" about that extra
column.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking
information - V7
Posted by Alexander Thomas <al...@collab.net>.
On Wed, 2005-08-10 at 15:20 -0500, Ben Collins-Sussman wrote:
> Ok, time to argue about bikeshed color. After applying a tweaked
> version of the patch and running 'svn ls -v', nearly every single
> filename wraps off the edge of my 80-column terminal. This is not good.
>
Yes I agree is not good, not bad either. When more and more elements
comes-in, one day it will wraps off. Why don't we learn to live with it
now?. Not a great idea to remove elements to keep output with in 80-col.
> The patch does this:
>
> 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,
> + lock ? lock->owner : "",
> + lock ? utf8_lock_timestr : "",
> utf8_entryname,
> (dirent->kind == svn_node_dir) ? "/" : ""));
>
>
> Why has the 'last author' been expanded to 12?
No specific reason, You can change last_author and lock_author to 8.8.
>
> And I think I agree with lundblad here. There's just no room for the
> lock-date, it's way too wide. I think we need to add exactly *one*
> more column for the lock-author, and it should be marked in some way,
> so as to distinguish it from the 'last author' field. Perhaps
> something like:
>
> [lundblad]
>
> or
>
> *lundblad*
>
> ?
I think it pretty clear from 'svn help' the order of occurrence, So
why we need to distinguish between last_author and lock_author in
output?.
--
########################
#####****######********#
####*~ *~*#####***~~***#
###*~*# *~*######*~~*###
##*~* ** *~*#####*~~*###
#***######***####****###
########################
Alexander Thomas (AT)
alexander@collab.net
Collabnet Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 11 Aug 2005, C. Michael Pilato wrote:
> Ben Collins-Sussman <su...@collab.net> writes:
>
> > I'm now thinking that maybe that "is locked" column should be reduced
> > to just a single "L" character, like we do in 'svn status'. We'd
> > save more space that way...
>
> +1. There are other ways to find out who holds the lock.
>
Agreed. The good thing is that all info is there for GUI clients to show
in whatever tooltips they can think of or something.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by Alexander Thomas <kc...@gmail.com>.
Sussman thanks for committing the patch.
I think minor change is needed, Now 'svn ls -v' outputs as follows
14 alexande *alexand 12 Jun 29 12:56 booking.tar.gz
1 alexande *alexand 7673 Jun 19 17:28 status.sxc
23 alexande 6 Jul 31 13:20 t1
where a single character is missing at the end for lock author. MHO is
to change format string for lock author from 8.8 to 9.9.
Regards
--
########################
#####****######********#
####*~ *~*#####***~~***#
###*~*# *~*######*~~*###
##*~* ** *~*#####*~~*###
#***######***####****###
########################
Alexander Thomas (AT)
alexander@collab.net <ma...@collab.net>
Collabnet Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by "C. Michael Pilato" <cm...@collab.net>.
Ben Collins-Sussman <su...@collab.net> writes:
> I'm now thinking that maybe that "is locked" column should be reduced
> to just a single "L" character, like we do in 'svn status'. We'd
> save more space that way...
+1. There are other ways to find out who holds the lock.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by Alexander Thomas <kc...@gmail.com>.
Philip Martin wrote:
>Ben Collins-Sussman <su...@collab.net> writes:
>
>
>
>>On Aug 11, 2005, at 8:07 AM, Alexander Thomas wrote:
>>
>>
>>>>$ svn ls -v
>>>> 1 sussman *sussman 3060 Apr 27 14:31 dutchwords
>>>>
>>>>
>
>I think the '*' is wrong because
>
> a) '*' is already used by 'svn status' but has a different meaning
>
> b) Position should be enough to distinguish the two usernames, just
> like the two revisions from 'svn status'
>
> c) It's not documented in the 'svn ls' help text
>
>
>
>>>> 1 sussman 238080 Apr 27 14:31 bluechairs.z5
>>>> 1 sussman 4829 Apr 27 14:31 bookreview.txt
>>>> 6 sussman 72192 Apr 27 14:33 driver.z5
>>>>
>>>>
>>>>
>>>>
>>>>
>>> Looks good, One last question before you commit this patch.
>>>What happens if any lazy svn clients use 'svn ls -v' output (not using
>>>libsvn_client) directly and parse the result (Just a wild thought) ?.
>>>
>>>
>>That's why we document changes to output in our releasenotes. For
>>example, the output of 'svn status -u' changed in svn 1.2 when we
>>implemented locking.
>>
>>I'm now thinking that maybe that "is locked" column should be reduced
>>to just a single "L" character, like we do in 'svn status'. We'd
>>save more space that way...
>>
>>
>
>I think 'L' would be wrong, in 'svn status' it indicates an admin
>directory lock not a repository lock, and like '*' above I don't think
>we should overlaod it. 'K' is probably the closest match among the
>'svn status' characters.
>
+1. Right 'K' is probably the closest match.
-Alexander Thomas (AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by Marc Sherman <ms...@projectile.ca>.
Ben Collins-Sussman wrote:
>
> I'm now thinking that maybe that "is locked" column should be reduced
> to just a single "L" character, like we do in 'svn status'. We'd save
> more space that way...
Perhaps a different character if the lock is held by the user running
the svn ls? A lock held by someone else is semantically quite different
from a lock you hold yourself. Possible characters to use: O (for own),
or Y (for you).
- Marc
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking
information - V7
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> On Aug 11, 2005, at 8:07 AM, Alexander Thomas wrote:
>>>
>>> $ svn ls -v
>>> 1 sussman *sussman 3060 Apr 27 14:31 dutchwords
I think the '*' is wrong because
a) '*' is already used by 'svn status' but has a different meaning
b) Position should be enough to distinguish the two usernames, just
like the two revisions from 'svn status'
c) It's not documented in the 'svn ls' help text
>>> 1 sussman 238080 Apr 27 14:31 bluechairs.z5
>>> 1 sussman 4829 Apr 27 14:31 bookreview.txt
>>> 6 sussman 72192 Apr 27 14:33 driver.z5
>>>
>>>
>>>
>> Looks good, One last question before you commit this patch.
>> What happens if any lazy svn clients use 'svn ls -v' output (not using
>> libsvn_client) directly and parse the result (Just a wild thought) ?.
>
> That's why we document changes to output in our releasenotes. For
> example, the output of 'svn status -u' changed in svn 1.2 when we
> implemented locking.
>
> I'm now thinking that maybe that "is locked" column should be reduced
> to just a single "L" character, like we do in 'svn status'. We'd
> save more space that way...
I think 'L' would be wrong, in 'svn status' it indicates an admin
directory lock not a repository lock, and like '*' above I don't think
we should overlaod it. 'K' is probably the closest match among the
'svn status' characters.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
On Aug 11, 2005, at 8:07 AM, Alexander Thomas wrote:
>>
>> $ svn ls -v
>> 1 sussman *sussman 3060 Apr 27 14:31 dutchwords
>> 1 sussman 238080 Apr 27 14:31 bluechairs.z5
>> 1 sussman 4829 Apr 27 14:31 bookreview.txt
>> 6 sussman 72192 Apr 27 14:33 driver.z5
>>
>>
>>
> Looks good, One last question before you commit this patch.
> What happens if any lazy svn clients use 'svn ls -v' output (not using
> libsvn_client) directly and parse the result (Just a wild thought) ?.
That's why we document changes to output in our releasenotes. For
example, the output of 'svn status -u' changed in svn 1.2 when we
implemented locking.
I'm now thinking that maybe that "is locked" column should be reduced
to just a single "L" character, like we do in 'svn status'. We'd
save more space that way...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking
information - V7
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-08-11 at 08:03 -0500, Ben Collins-Sussman wrote:
> On Aug 10, 2005, at 3:20 PM, Ben Collins-Sussman wrote:
> I think it's best to keep the {filesize, date, filename} all next to
> each other, because it looks like the output of 'ls -l' that everyone
> is used to. IIRC, 'svn ls -v' was originally designed to mimic 'ls -
> l'. So I've removed the lock->date field altogether, and now only
> print the lock->owner field, right next to the last-author field:
>
> $ svn ls -v
> 1 sussman *sussman 3060 Apr 27 14:31 dutchwords
> 1 sussman 238080 Apr 27 14:31 bluechairs.z5
> 1 sussman 4829 Apr 27 14:31 bookreview.txt
> 6 sussman 72192 Apr 27 14:33 driver.z5
>
>
Looks good, One last question before you commit this patch.
What happens if any lazy svn clients use 'svn ls -v' output (not using
libsvn_client) directly and parse the result (Just a wild thought) ?.
> This is what I'll commit, assuming tests pass. We can tweak more
> later if we want to.
Thanks
Regards
--
########################
#####****######********#
####*~ *~*#####***~~***#
###*~*# *~*######*~~*###
##*~* ** *~*#####*~~*###
#***######***####****###
########################
Alexander Thomas (AT)
alexander@collab.net
Collabnet Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
On Aug 10, 2005, at 3:20 PM, Ben Collins-Sussman wrote:
>
> [lundblad]
>
> or
>
> *lundblad*
I think it's best to keep the {filesize, date, filename} all next to
each other, because it looks like the output of 'ls -l' that everyone
is used to. IIRC, 'svn ls -v' was originally designed to mimic 'ls -
l'. So I've removed the lock->date field altogether, and now only
print the lock->owner field, right next to the last-author field:
$ svn ls -v
1 sussman *sussman 3060 Apr 27 14:31 dutchwords
1 sussman 238080 Apr 27 14:31 bluechairs.z5
1 sussman 4829 Apr 27 14:31 bookreview.txt
6 sussman 72192 Apr 27 14:33 driver.z5
This is what I'll commit, assuming tests pass. We can tweak more
later if we want to.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
Ok, time to argue about bikeshed color. After applying a tweaked
version of the patch and running 'svn ls -v', nearly every single
filename wraps off the edge of my 80-column terminal. This is not good.
The patch does this:
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,
+ lock ? lock->owner : "",
+ lock ? utf8_lock_timestr : "",
utf8_entryname,
(dirent->kind == svn_node_dir) ? "/" : ""));
Why has the 'last author' been expanded to 12?
And I think I agree with lundblad here. There's just no room for the
lock-date, it's way too wide. I think we need to add exactly *one*
more column for the lock-author, and it should be marked in some way,
so as to distinguish it from the 'last author' field. Perhaps
something like:
[lundblad]
or
*lundblad*
?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Another clarification... (Not a full review, this just caught my
attention.)
On Wed, 10 Aug 2005, Ben Collins-Sussman wrote:
>
> On Aug 9, 2005, at 6:15 PM, Alexander Thomas wrote:
>
>
> > + /* Get lock. */
> > + SVN_ERR (svn_ra_get_locks (ra_session, locks, "", subpool));
> > +
Note which pool is used.
> > + new_locks = apr_hash_make (pool);
> > + for (hi = apr_hash_first (subpool, *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 (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);
Woops! The lock is allocated in subpool, so it will be invalid, if not
before, a the end of the function. apr_hash_set just sets a pointer, it
does not move the data pointed to.
I'm sure Ben will take care of this as well, I just wanted to let you know
for the future, alexander.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
On Aug 9, 2005, at 6:15 PM, Alexander Thomas wrote:
> + /* Get lock. */
> + SVN_ERR (svn_ra_get_locks (ra_session, locks, "", subpool));
> +
> + new_locks = apr_hash_make (pool);
> + for (hi = apr_hash_first (subpool, *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 (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_pool_destroy (subpool);
Oops, this should svn_pool_destroy().
> + *locks = new_locks;
> +
>
I'm not going to ask you to resubmit a new patch just for this, but I
want to make it clear what the point of the subpool is. I think
you're still a tiny bit unclear on the usage.
Generally, one makes a subpool when a loop is going to use temporary
memory. That means that the body of the loop should (1) allocating
all temporary objects in the subpool, (2) clearing the subpool on
every iteration.
What you're doing above doesn't match that pattern. You're doing
(1), but you're not doing (2). I understand that you're not doing
(2) because you're using the subpool for the hash iteration. Don't
do that! apr_hash_first() should be using the main pool to loop, and
the subpool should *only* be used within the body of the loop. Then
it's safe to clear it on each iteration.
In any case, I'll tweak myself... no need to resubmit.
I'm going to actually apply the patch now and play around with it. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking
information - V7
Posted by Alexander Thomas <al...@collab.net>.
Posting version 8 of the patch for 'svn ls -v' returning locking
information.
On Tue, 2005-08-09 at 10:07 -0500, Ben Collins-Sussman wrote:
> Getting better all the time. Still, a few more changes are needed. :-)
>
>
>
> On Aug 9, 2005, at 3:20 AM, Alexander Thomas wrote:
>
> >
> > + * If @a locks is not @c NULL, maps hash entry names (<tt>const
> > char *</tt>)
> > + * to @c svn_lock_t *'s doing all allocation in @a pool.
>
> This language is ambiguous. Why would locks be NULL? Is it because
> the caller is passing in a NULL pointer, or might the function return
> NULL?
>
> The wording needs to be changed in this docstring; we need make it
> clear that *if* the caller passes in a non-NULL **locks argument,
> then the function will set it to a hash which maps [blah] to [blah].
>
I am changing this to what Karl suggested. Ben I am not very good in
framing sentences, If it still not impress U plz change it to your will,
Advance Thanks. :)
>
> > +svn_client_ls3 (apr_hash_t **dirents,
> > + apr_hash_t **locks,
> > const char *path_or_url,
> > const svn_opt_revision_t *peg_revision,
> > const svn_opt_revision_t *revision,
> > @@ -83,12 +84,22 @@
> > svn_revnum_t rev;
> > svn_node_kind_t url_kind;
> > 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;
> >
> > /* 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));
> > + /* Getting repository root. */
> > + SVN_ERR (svn_client_url_from_path (&repos_root, "", pool));
> >
>
> I don't understand how the line above works. You're fetching the URL
> cached in the entries file of the current-working-directory. This
> function shouldn't be sensitive to the current-working-directory at
> all! What if I were running 'svn ls URL', where '.' isn't under
> version control?
>
> If your goal is to get the repository root URL, then you need to use
> the existing ra_session and call svn_ra_get_repos_root().
>
>
Not aware of such a function exist, changed.
> >
> > + /* Getting relative path respective to repository root. */
> > + rel_path = svn_path_is_child (repos_root, url, pool);
> > +
> > /* Decide if the URL is a file or directory. */
> > SVN_ERR (svn_ra_check_path (ra_session, "", rev, &url_kind, pool));
> >
> > @@ -107,14 +118,14 @@
> >
> > /* 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));
> > + 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,
> > @@ -127,6 +138,7 @@
> > return svn_error_createf (SVN_ERR_FS_NOT_FOUND, NULL,
> > _("URL '%s' non-existent in that
> > revision"),
> > url);
> > + svn_path_split (rel_path, &rel_path, NULL, pool);
> >
>
> Is the line above just a way of guaranteeing that rel_path is a
> directory? I'm not sure I understand.
>
No, this is to extract the dirname from the target, if the target is
a file. I make hash keys in the lock hash relative paths so they're
the same as the keys in the dirents hash.
> >
> > apr_hash_set (*dirents, base_name, APR_HASH_KEY_STRING,
> > the_ent);
> > }
> > @@ -135,10 +147,57 @@
> > _("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);
> > +
> > + apr_pool_create (&subpool, pool);
>
> We never call apr_pool functions directly; we have special wrappers
> for them. In this case, you should be calling svn_pool_create (pool)
> to generate a subpool.
>
[...]
> + if (newkey)
> > + apr_hash_set (new_locks, newkey, APR_HASH_KEY_STRING, val);
> > + }
> > +
> > + apr_pool_destroy (subpool);
>
>
> svn_pool_destroy (subpool);
>
Changed all apr_pool to svn_pool.
>
> > +
> > + /* Get lock. */
> > + SVN_ERR (svn_ra_get_locks (ra_session, locks, "", subpool));
> > +
> > + new_locks = apr_hash_make (pool);
> > + for (hi = apr_hash_first (subpool, *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 (svn_path_canonicalize (rel_path,
> > pool),
> > + svn_path_canonicalize (key, pool),
> > + pool);
>
> You've created a subpool, but you don't seem to be using it inside
> the loop at all. I understand that newkey needs to be allocated from
> the original pool, but couldn't we be using the subpool for the two
> calls to svn_path_canonicalize()? (And if so, then you could be
> calling svn_pool_clear (subpool) at the top the loop.)
>
>
I agree with svn_path_canonicalize() to use subpool. But the using
svn_pool_clear is not a good idea, Because I am storing locks hash from
svn_ra_get_locks() in to subpool and moving hash entries to a new hash
after filtering. So I am afraid, clearing the pools will lead to crash.
>
>
> > + *locks = new_locks;
> > +
> > return SVN_NO_ERROR;
> > }
> >
> > svn_error_t *
> > +svn_client_ls2 (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)
> > +{
> > +
> > + return svn_client_ls3 (dirents, NULL, path_or_url, peg_revision,
> > + revision, recurse, ctx, pool);
> > +}
> > +
> > +svn_error_t *
> > svn_client_ls (apr_hash_t **dirents,
> > const char *path_or_url,
> > svn_opt_revision_t *revision,
> > Index: subversion/clients/cmdline/ls-cmd.c
> > ===================================================================
> > --- subversion/clients/cmdline/ls-cmd.c (revision 15584)
> > +++ subversion/clients/cmdline/ls-cmd.c (working copy)
> > @@ -37,6 +37,7 @@
> >
> > static svn_error_t *
> > print_dirents (apr_hash_t *dirents,
> > + apr_hash_t *locks,
> > svn_boolean_t verbose,
> > svn_client_ctx_t *ctx,
> > apr_pool_t *pool)
> > @@ -51,6 +52,7 @@
> > {
> > const char *utf8_entryname;
> > svn_dirent_t *dirent;
> > + svn_lock_t *lock;
> > svn_sort__item_t *item;
> >
> > svn_pool_clear (subpool);
> > @@ -63,6 +65,7 @@
> > utf8_entryname = item->key;
> >
> > dirent = apr_hash_get (dirents, utf8_entryname, item->klen);
> > + lock = apr_hash_get (locks, utf8_entryname,
> > APR_HASH_KEY_STRING);
> >
>
> Are the two hashes going to have different keys? I'm wondering why
> you're using item->klen in one case, and APR_HASH_KEY_STRING in the
> other case...?
>
No it won't be different, I am using item->klen here.
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by kf...@collab.net.
Ben Collins-Sussman <su...@collab.net> writes:
> > + * If @a locks is not @c NULL, maps hash entry names (<tt>const
> > char *</tt>)
> > + * to @c svn_lock_t *'s doing all allocation in @a pool.
>
> This language is ambiguous. Why would locks be NULL? Is it because
> the caller is passing in a NULL pointer, or might the function return
> NULL?
>
> The wording needs to be changed in this docstring; we need make it
> clear that *if* the caller passes in a non-NULL **locks argument,
> then the function will set it to a hash which maps [blah] to [blah].
One important convention: you could mean "if @a locks is not @c NULL",
or you could mean "if @a *locks is not @c NULL" (note the asterisk).
In this case, I think you did write what you meant, but make it
clearer by saying precisely what the function will do:
If @a locks is not @c NULL, set @a *locks to a hash table mapping
entry names (<tt>const char *</tt>) to @c svn_lock_t *'s,
allocating both @a *locks and everything inside it in @a pool.
(Of course, if that's not the exact behavior, then change the wording
accordingly, I'm just trying to give you an idea of how to phrase it.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V7
Posted by Ben Collins-Sussman <su...@collab.net>.
Getting better all the time. Still, a few more changes are needed. :-)
On Aug 9, 2005, at 3:20 AM, Alexander Thomas wrote:
>
> + * If @a locks is not @c NULL, maps hash entry names (<tt>const
> char *</tt>)
> + * to @c svn_lock_t *'s doing all allocation in @a pool.
This language is ambiguous. Why would locks be NULL? Is it because
the caller is passing in a NULL pointer, or might the function return
NULL?
The wording needs to be changed in this docstring; we need make it
clear that *if* the caller passes in a non-NULL **locks argument,
then the function will set it to a hash which maps [blah] to [blah].
> +svn_client_ls3 (apr_hash_t **dirents,
> + apr_hash_t **locks,
> const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> @@ -83,12 +84,22 @@
> svn_revnum_t rev;
> svn_node_kind_t url_kind;
> 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;
>
> /* 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));
> + /* Getting repository root. */
> + SVN_ERR (svn_client_url_from_path (&repos_root, "", pool));
>
I don't understand how the line above works. You're fetching the URL
cached in the entries file of the current-working-directory. This
function shouldn't be sensitive to the current-working-directory at
all! What if I were running 'svn ls URL', where '.' isn't under
version control?
If your goal is to get the repository root URL, then you need to use
the existing ra_session and call svn_ra_get_repos_root().
>
> + /* Getting relative path respective to repository root. */
> + rel_path = svn_path_is_child (repos_root, url, pool);
> +
> /* Decide if the URL is a file or directory. */
> SVN_ERR (svn_ra_check_path (ra_session, "", rev, &url_kind, pool));
>
> @@ -107,14 +118,14 @@
>
> /* 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));
> + 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,
> @@ -127,6 +138,7 @@
> return svn_error_createf (SVN_ERR_FS_NOT_FOUND, NULL,
> _("URL '%s' non-existent in that
> revision"),
> url);
> + svn_path_split (rel_path, &rel_path, NULL, pool);
>
Is the line above just a way of guaranteeing that rel_path is a
directory? I'm not sure I understand.
>
> apr_hash_set (*dirents, base_name, APR_HASH_KEY_STRING,
> the_ent);
> }
> @@ -135,10 +147,57 @@
> _("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);
> +
> + apr_pool_create (&subpool, pool);
We never call apr_pool functions directly; we have special wrappers
for them. In this case, you should be calling svn_pool_create (pool)
to generate a subpool.
> +
> + /* Get lock. */
> + SVN_ERR (svn_ra_get_locks (ra_session, locks, "", subpool));
> +
> + new_locks = apr_hash_make (pool);
> + for (hi = apr_hash_first (subpool, *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 (svn_path_canonicalize (rel_path,
> pool),
> + svn_path_canonicalize (key, pool),
> + pool);
You've created a subpool, but you don't seem to be using it inside
the loop at all. I understand that newkey needs to be allocated from
the original pool, but couldn't we be using the subpool for the two
calls to svn_path_canonicalize()? (And if so, then you could be
calling svn_pool_clear (subpool) at the top the loop.)
> + if (newkey)
> + apr_hash_set (new_locks, newkey, APR_HASH_KEY_STRING, val);
> + }
> +
> + apr_pool_destroy (subpool);
svn_pool_destroy (subpool);
> + *locks = new_locks;
> +
> return SVN_NO_ERROR;
> }
>
> svn_error_t *
> +svn_client_ls2 (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)
> +{
> +
> + return svn_client_ls3 (dirents, NULL, path_or_url, peg_revision,
> + revision, recurse, ctx, pool);
> +}
> +
> +svn_error_t *
> svn_client_ls (apr_hash_t **dirents,
> const char *path_or_url,
> svn_opt_revision_t *revision,
> Index: subversion/clients/cmdline/ls-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/ls-cmd.c (revision 15584)
> +++ subversion/clients/cmdline/ls-cmd.c (working copy)
> @@ -37,6 +37,7 @@
>
> static svn_error_t *
> print_dirents (apr_hash_t *dirents,
> + apr_hash_t *locks,
> svn_boolean_t verbose,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> @@ -51,6 +52,7 @@
> {
> const char *utf8_entryname;
> svn_dirent_t *dirent;
> + svn_lock_t *lock;
> svn_sort__item_t *item;
>
> svn_pool_clear (subpool);
> @@ -63,6 +65,7 @@
> utf8_entryname = item->key;
>
> dirent = apr_hash_get (dirents, utf8_entryname, item->klen);
> + lock = apr_hash_get (locks, utf8_entryname,
> APR_HASH_KEY_STRING);
>
Are the two hashes going to have different keys? I'm wondering why
you're using item->klen in one case, and APR_HASH_KEY_STRING in the
other case...?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V7
Posted by Alexander Thomas <kc...@gmail.com>.
Version 7 of the Patch for 'svn ls -v' should return locking information
Ben Collins-Sussman wrote:
> The svn_client_ls3() docstring needs to say that the locks hash is
> optional.
>
done.
> "form" hash? I think that's a typo. I think the docstring just
> needs to say, "same as svn_client_ls3, but always passes a NULL lock
> hash."
done
> Here's the big problem: you've defined the new function in terms of
> the old function, which is backwards. When you deprecate a function,
> it means that subversion has to stop using it absolutely everywhere.
> Our standard technique for rev'ving an API is to define the
> deprecated function in terms of the new function. So svn_client_ls2
> () should simply be a call to svn_client_ls3() with a NULL lock
> hash. And what was formerly svn_client_ls2() should be renamed to
> ls3, and extra lock features then added to it.
>
[...]
>
> ... which means there will be no need to open a new ra_session,
> right? :-)
done.