You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexander Thomas <al...@collab.net> on 2005/06/30 10:41:23 UTC
[Patch] Issue #2291 - 'svn ls -v' return locking information
[[[
Fix issue #2291 - 'svn ls -v' return locking information
* subversion/include/svn_types.h
(struct svn_dirent2_t): Added new struct.
* subversion/include/svn_client.h
(svn_client_ls3): New prototype.
(svn_client_ls2): Deprecated.
* subversion/libsvn_client/ls.c
(get_dir_contents): Modified.
(svn_client_ls3): New function.
* subversion/clients/cmdline/ls-cmd.c
(print_dirents): Modified to print lock information.
(svn_cl__ls): Modified to call new function.
* subversion/clients/cmdline/main.c
Modified help text for svn ls
]]]
Re: [Patch] Issue #2291 - 'svn ls -v' return locking information
Posted by kf...@collab.net.
Madan U Sreenivasan <ma...@collab.net> writes:
> I beg to differ. lock is now as vital an info as the author or revision
> no.( two other entities of the svn_dirent_t struct). We could compare it
> with the rwx permissions of a normal unix file. I would suggest
> incorporating lock info in the svn_dirent_t structure and always
> returning the lock info from the server as if its just one more
> attribute of the file/dir (which it is!)
> I would like to place another reasoning here. Assuming, theres a new
> attribute in the future - lets say compressed ( meaning the file is
> stored in compressed format ... just an example ), using a
> svn_ra_get_compression_info() for each dirent would simply shatter the
> natural design of things. such parameters are attributes of each dirent
> and IMHO should be filled in by get_dir_contents/svn_ra_get_dir (not
> sure which one would be apt) by the server.
I'm a newcomer to this discussion, but I think Madan has a point:
If we're fetching meta-information about dirents, we might as well
fetch everything. Adding the extra bytes on the wire is not costly
(it's "meta-information", not "information" :-) ), certainly far less
costly than extra round trips to fetch it later. Any consumer is free
to ignore whatever bits of meta-information it doesn't want, but we
might as well just hand it everything up front.
Remember the X Windows libraries, back in the good old days? Every
function took a bitmask, to tell it what information the caller wanted
back, so that the X server could carefully leave out unwanted
information, thus reducing the number of bytes -- but not the number
of round trips --between client and server. Let me put it this way:
there's a reason no one designs APIs that way anymore :-).
This general philosophical note has been brought to you by:
-The Letter Q, and
-Karl Fogel
---------------------------------------------------------------------
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' return locking information
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Fri, 2005-07-01 at 01:50, Peter N. Lundblad wrote:
> On Thu, 30 Jun 2005, Ben Collins-Sussman wrote:
>
> [snip]
> Moving the lock fetching to the server would avoid the network round-trips, but isn't good
> either.
I would side with that... doesnt seem to break the natural design of the
existing code....
> Either way, we should have a way to say that we don't want the
> lock info when it isn't needed.
I beg to differ. lock is now as vital an info as the author or revision
no.( two other entities of the svn_dirent_t struct). We could compare it
with the rwx permissions of a normal unix file. I would suggest
incorporating lock info in the svn_dirent_t structure and always
returning the lock info from the server as if its just one more
attribute of the file/dir (which it is!)
I would like to place another reasoning here. Assuming, theres a new
attribute in the future - lets say compressed ( meaning the file is
stored in compressed format ... just an example ), using a
svn_ra_get_compression_info() for each dirent would simply shatter the
natural design of things. such parameters are attributes of each dirent
and IMHO should be filled in by get_dir_contents/svn_ra_get_dir (not
sure which one would be apt) by the server.
>
> Regards,
> //Peter
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
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' return locking information
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 30 Jun 2005, Ben Collins-Sussman wrote:
>
> Second, you seem to be calling svn_ra_get_lock() on EACH object,
> which is a huge number of extra network requests! Instead, why not
> just call svn_ra_get_locks() exactly once, and then pull relevant
> locks out of the hash and plunk them into the dirent objects.
>
The sad thing is that we designed us into a corner, since we only have
depth 0 and depth infinity for this operation. Fetching all locks for the
whole repository can be a lot of data that's not used in the non-recursive
case. svn status -u does it that way, but I don't like it. Moving the lock
fetching to the server would avoid the network round-trips, but isn't good
either. Either way, we should have a way to say that we don't want the
lock info when it isn't needed.
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' return locking information
Posted by Ben Collins-Sussman <su...@collab.net>.
On Jul 1, 2005, at 6:25 AM, Alexander Thomas wrote:
> Yes I agree, initially I done the same. Because svn_lock_t is
> declared
> below the new structure, we are force to use 'struct svn_lock_t*' not
> the typedef name 'svn_lock_t*'. Will I violating any svn community
> rules
> for not using typedef ? :)
How about just rearranging the structure definitions in the header
file? :-)
>
> Currently I am using only 2 out of 5 new structure elements
> (lock_owner
> & lock_creation_date), What U think of removing unused elements ?
> because some elements from svn_info_t will also peak in later, as the
> issue says
When somebody calls svn_client_ls(), why not give them access to all
the lock fields? Just because you're not printing all the fields
doesn't mean we should artifically restrict the API. Other clients
(other than our commandline client) might use more information.
---------------------------------------------------------------------
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' return locking information
Posted by Alexander Thomas <al...@collab.net>.
On Thu, 2005-06-30 at 10:22 -0500, Ben Collins-Sussman wrote:
> On Jun 30, 2005, at 5:41 AM, Alexander Thomas wrote:
>
> >
> > +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;
> > +
> > + /** lock token or NULL if path not locked in this REPOS */
> > + const char *lock_token;
> > +
> > + /** lock owner, or NULL if not locked in this REPOS */
> > + const char *lock_owner;
> > +
> > + /** lock comment or NULL if not locked in this REPOS or no
> > comment */
> > + const char *lock_comment;
> > +
> > + /** Lock creation date or 0 if not locked in this REPOS */
> > + apr_time_t lock_creation_date;
> > +
> > + /** Lock expiration date or 0 if not locked in this REPOS */
> > + apr_time_t lock_expiration_date;
> > +
> > +} svn_dirent2_t;
> > +
>
>
> OK, so here's my one big piece of feedback: you added 5 new fields
> to the svn_dirent_t structure above. You could have simply added
> one: an svn_lock_t. Wouldn't that be much simpler? If the
> svn_lock_t field is NULL, there's no repository lock on the object.
> If it's non-NULL, then all the fields of the lock_t are filled in.
>
Yes I agree, initially I done the same. Because svn_lock_t is declared
below the new structure, we are force to use 'struct svn_lock_t*' not
the typedef name 'svn_lock_t*'. Will I violating any svn community rules
for not using typedef ? :)
Currently I am using only 2 out of 5 new structure elements (lock_owner
& lock_creation_date), What U think of removing unused elements ?
because some elements from svn_info_t will also peak in later, as the
issue says
Thanks
-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' return locking information
Posted by Ben Collins-Sussman <su...@collab.net>.
On Jun 30, 2005, at 5:41 AM, Alexander Thomas wrote:
>
> +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;
> +
> + /** lock token or NULL if path not locked in this REPOS */
> + const char *lock_token;
> +
> + /** lock owner, or NULL if not locked in this REPOS */
> + const char *lock_owner;
> +
> + /** lock comment or NULL if not locked in this REPOS or no
> comment */
> + const char *lock_comment;
> +
> + /** Lock creation date or 0 if not locked in this REPOS */
> + apr_time_t lock_creation_date;
> +
> + /** Lock expiration date or 0 if not locked in this REPOS */
> + apr_time_t lock_expiration_date;
> +
> +} svn_dirent2_t;
> +
OK, so here's my one big piece of feedback: you added 5 new fields
to the svn_dirent_t structure above. You could have simply added
one: an svn_lock_t. Wouldn't that be much simpler? If the
svn_lock_t field is NULL, there's no repository lock on the object.
If it's non-NULL, then all the fields of the lock_t are filled in.
Second, you seem to be calling svn_ra_get_lock() on EACH object,
which is a huge number of extra network requests! Instead, why not
just call svn_ra_get_locks() exactly once, and then pull relevant
locks out of the hash and plunk them into the dirent objects.
---------------------------------------------------------------------
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- V4
Posted by Alexander Thomas <th...@touchtelindia.net>.
Ben Collins-Sussman wrote:
>
> Unfortunately, now we're faced with two choices, neither of which is
> optimal:
>
> * do a network request to get the dirents, then do N more network
> requests to fetch the lock on each dirent. This is what you're
> currently doing.
> or
> * do a network request to get the dirents, then do one more
> network request to fetch *all* locks below the path.
>
> After some discussion in the Chicago office, we all agree that's it's
> Less Evil to hold the entire hash of locks in memory than to make N
> network requests.
>
Here I am posting a new version of the patch which will hold the
entire hash of locks in memory and does the rest.
Regards
-Alexander Thomas (AT)
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V4
Posted by Ben Collins-Sussman <su...@collab.net>.
On Jul 25, 2005, at 9:50 AM, Peter N. Lundblad wrote:
> On Mon, 25 Jul 2005, Alexander Thomas wrote:
>
>
>
>> Ben Collins-Sussman wrote:
>>
>>
>>
>>> Unfortunately, now we're faced with two choices, neither of which is
>>> optimal:
>>>
>>> * do a network request to get the dirents, then do N more network
>>> requests to fetch the lock on each dirent. This is what you're
>>> currently doing.
>>> or
>>> * do a network request to get the dirents, then do one more network
>>> request to fetch *all* locks below the path.
>>>
>>> After some discussion in the Chicago office, we all agree that's
>>> it's
>>> Less Evil to hold the entire hash of locks in memory than to make N
>>> network requests.
>>>
>>>
>>
>>
>> Here I am posting a new version of the patch which will hold the
>> entire
>> hash of locks in memory and does the rest.
>>
>>
>>
> I agree with using get_locks, but I don't think you should move it
> into
> the RA layer. The problem is that it will do an infinitely
> recursive lock
> traversal *per directory*. You can do svn_ra_get_locks in
> svn_client_ls
> instead. That way, you'll be able to get all locks only once.
>
>
Yes, this is exactly what I meant... to do the get_locks() call form
svn_client_ls(), so that it only needs to happen exactly once.
Alexander, can you try a version 5? :-)
> When/if we get a depth parameter on svn_fs_get_locks(), we can rev
> the RA
> API to provide the locks as well, making it streamy (as earlier
> discussed).
>
Agreed, that's a good longer-term plan.
>
> On another not, isn't it more useful to display the lock owner than
> the
> lock creation date with "svn ls -v".
>
I agree. When you discover a lock, I think the first thing you want
to know is who to call on the phone, not how old the lock is.
---------------------------------------------------------------------
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 - V5
Posted by Ben Collins-Sussman <su...@collab.net>.
On Jul 27, 2005, at 3:58 PM, Peter N. Lundblad wrote:
> Yesterday I thought this wasn't good, since when we make svn_client_ls
> streamy (there's an issue for that, too), this wouldn't be good, since
> we'd need to pass the lock with the rest of the dirent. Now that I
> think
> of it again, I realize that we can take care of that when we do the
> streaminess work.
>
> So, I'm +1 for the above.
>
> But, please make the hash keys in the lock hash relative paths so
> they're
> the same as the keys in the dirents hash, and I suggest filtering
> the hash
> in the non-recursive case (so we don't return all locks under the path
> with infinite depth). Also, since you're adding another parameter
> anyway,
> I suggest allowing it to be NULL, making it possible to avoid the
> possibly
> expensive get_locks call (in the non-verbose case in the cmdline
> client,
> for example). I think this optimization is actually a good idea, since
> else, a non-recursive, non-verbose ls command in a repository with
> many
> locks could be unreasonably slow.
>
Alexander: kfogel and I are in agreement with what Peter is saying.
I think it's safe to do another patch now. :-)
---------------------------------------------------------------------
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.
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V6
Posted by Ben Collins-Sussman <su...@collab.net>.
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
- V6
Posted by Alexander Thomas <kc...@gmail.com>.
Posting version 6 of the patch for svn ls -v with locking information
Peter N. Lundblad wrote:
>But, please make the hash keys in the lock hash relative paths so they're
>the same as the keys in the dirents hash,
>
Done.
>and I suggest filtering the hash
>in the non-recursive case (so we don't return all locks under the path
>with infinite depth).
>
Not done the extra work here, I am started doing the depth parameter on
get_locks().
So hope we can live with it for a week or two.
>Also, since you're adding another parameter anyway,
>I suggest allowing it to be NULL, making it possible to avoid the possibly
>expensive get_locks call (in the non-verbose case in the cmdline client,
>for example). I think this optimization is actually a good idea, since
>else, a non-recursive, non-verbose ls command in a repository with many
>locks could be unreasonably slow.
>
>
Done.
Regards,
-Alexander Thomas (AT)
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V5
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 26 Jul 2005, Alexander Thomas wrote:
> On Tue, 2005-07-26 at 23:59 +0200, Peter N. Lundblad wrote:
>
> Thanks for explaining the problem to the utmost satisfaction.
>
> > > > 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.)
> > > >
> > I actually prefer this solution (creating svn_client_dirent_t), since it
> > will create a new struct with the lock field always valid where it is
> > used. kfogel pointed out that this will leave us with a deprecated struct
> > when (if) we eventually revise svn_dirent_t. I don't think that's a big
> > problem; we already have lots of deprecated stuff.
> >
> If we are so sure of deprecating the new struct svn_client_dirent_t in
> feature date, why we creating it now. I might be wrong here still, what
> you feel about creating new function as svn_client_ls_with_lock () or
> svn_client_ls3 () returning both dirent_t hash and lock_t hash to the
> caller ?
>
Yesterday I thought this wasn't good, since when we make svn_client_ls
streamy (there's an issue for that, too), this wouldn't be good, since
we'd need to pass the lock with the rest of the dirent. Now that I think
of it again, I realize that we can take care of that when we do the
streaminess work.
So, I'm +1 for the above.
But, please make the hash keys in the lock hash relative paths so they're
the same as the keys in the dirents hash, and I suggest filtering the hash
in the non-recursive case (so we don't return all locks under the path
with infinite depth). Also, since you're adding another parameter anyway,
I suggest allowing it to be NULL, making it possible to avoid the possibly
expensive get_locks call (in the non-verbose case in the cmdline client,
for example). I think this optimization is actually a good idea, since
else, a non-recursive, non-verbose ls command in a repository with many
locks could be unreasonably slow.
I hope others can live with this design too.
Thanks,
//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 - V5
Posted by Alexander Thomas <al...@collab.net>.
On Tue, 2005-07-26 at 23:59 +0200, Peter N. Lundblad wrote:
Thanks for explaining the problem to the utmost satisfaction.
> > > 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.)
> > >
> I actually prefer this solution (creating svn_client_dirent_t), since it
> will create a new struct with the lock field always valid where it is
> used. kfogel pointed out that this will leave us with a deprecated struct
> when (if) we eventually revise svn_dirent_t. I don't think that's a big
> problem; we already have lots of deprecated stuff.
>
If we are so sure of deprecating the new struct svn_client_dirent_t in
feature date, why we creating it now. I might be wrong here still, what
you feel about creating new function as svn_client_ls_with_lock () or
svn_client_ls3 () returning both dirent_t hash and lock_t hash to the
caller ?
Regards,
-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
- V5
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 26 Jul 2005, Alexander Thomas wrote:
> > 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.
> >
The big problem with this approach is that we'll be lying, or at least
inconsistent. If we create svn_dirent2_t with a new lock field,
svn_ra_get_dir can't in an efficient way fill that field with information
(because of infinite depth of get_locks). svn_ra_stat and svn_repos_stat
could get the lock and svn_client_ls would get it as well. I dislike this
inconsistency.
> > 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.)
> >
I actually prefer this solution (creating svn_client_dirent_t), since it
will create a new struct with the lock field always valid where it is
used. kfogel pointed out that this will leave us with a deprecated struct
when (if) we eventually revise svn_dirent_t. I don't think that's a big
problem; we already have lots of deprecated stuff.
> 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
Aggreed that it is hackish. We'd like the lock to be a "first class
citizen" among other entryprops (last author, commit time etc.)
> return a hash containing both dirent_t and lock_t. Yes it need lot
> more work, but hope will be more efficient.
>
This means get_locks with depth=1 support. As I understand it, the BDB
locking implementation makes that hard to do efficiently. I might be wrong
here.
> > 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.
>
The problem is that we currently give you very good advice, becuase we
aren't sure how to handle this situation. Technically, I think we're in
total agreement, it is just a question of least ugly API design:-(
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
- V5
Posted by Alexander Thomas <kc...@gmail.com>.
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
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V5
Posted by Ben Collins-Sussman <su...@collab.net>.
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
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V5
Posted by Alexander Thomas <kc...@gmail.com>.
Version 5 of the patch for fixing Issue #2291.
Peter N. Lundblad wrote:
>I agree with using get_locks, but I don't think you should move it into
>the RA layer. The problem is that it will do an infinitely recursive lock
>traversal *per directory*. You can do svn_ra_get_locks in svn_client_ls
>instead. That way, you'll be able to get all locks only once.
>
>
get_locks moved to svn_client_ls2()
>When/if we get a depth parameter on svn_fs_get_locks(), we can rev the RA
>API to provide the locks as well, making it streamy (as earlier
>discussed).
>
>
Working on it :-)
>On another not, isn't it more useful to display the lock owner than the
>lock creation date with "svn ls -v". Displaying everything in the XML is
>great.
>
>
IMHO, both lock owner and lock creation date are useful piece of
information. I am still
retaining both with a simple swapping on output.
Also I am doing strcasecmp on hash keys, Will there be any protability
issues ?
>I hope to do a complete review when we've decided on the implementation.
>
>
Thanks to Peter and Sussman for their valuable comments.
Regards
-Alexander Thomas (AT)
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information
- V4
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 25 Jul 2005, Alexander Thomas wrote:
> Ben Collins-Sussman wrote:
>
> > Unfortunately, now we're faced with two choices, neither of which is
> > optimal:
> >
> > * do a network request to get the dirents, then do N more network
> > requests to fetch the lock on each dirent. This is what you're
> > currently doing.
> > or
> > * do a network request to get the dirents, then do one more network
> > request to fetch *all* locks below the path.
> >
> > After some discussion in the Chicago office, we all agree that's it's
> > Less Evil to hold the entire hash of locks in memory than to make N
> > network requests.
>
>
> Here I am posting a new version of the patch which will hold the entire
> hash of locks in memory and does the rest.
>
I agree with using get_locks, but I don't think you should move it into
the RA layer. The problem is that it will do an infinitely recursive lock
traversal *per directory*. You can do svn_ra_get_locks in svn_client_ls
instead. That way, you'll be able to get all locks only once.
When/if we get a depth parameter on svn_fs_get_locks(), we can rev the RA
API to provide the locks as well, making it streamy (as earlier
discussed).
On another not, isn't it more useful to display the lock owner than the
lock creation date with "svn ls -v". Displaying everything in the XML is
great.
I hope to do a complete review when we've decided on the implementation.
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
- V4
Posted by Alexander Thomas <kc...@gmail.com>.
Ben Collins-Sussman wrote:
> Unfortunately, now we're faced with two choices, neither of which is
> optimal:
>
> * do a network request to get the dirents, then do N more network
> requests to fetch the lock on each dirent. This is what you're
> currently doing.
> or
> * do a network request to get the dirents, then do one more network
> request to fetch *all* locks below the path.
>
> After some discussion in the Chicago office, we all agree that's it's
> Less Evil to hold the entire hash of locks in memory than to make N
> network requests.
Here I am posting a new version of the patch which will hold the entire
hash of locks in memory and does the rest.
Regards
-Alexander Thomas (AT)
Re: [Patch] Issue #2291 - 'svn ls -v' should return locking information - V3
Posted by Ben Collins-Sussman <su...@collab.net>.
On Jul 6, 2005, at 6:47 AM, Alexander Thomas wrote:
> Sorry, forgotten to attach patch, resending ...
Alex, thanks for your patience!
I'm glad you've placed an svn_lock_t structure into svn_dirent2_t, it
looks good.
Unfortunately, now we're faced with two choices, neither of which is
optimal:
* do a network request to get the dirents, then do N more network
requests to fetch the lock on each dirent. This is what you're
currently doing.
or
* do a network request to get the dirents, then do one more
network request to fetch *all* locks below the path.
After some discussion in the Chicago office, we all agree that's it's
Less Evil to hold the entire hash of locks in memory than to make N
network requests.
Lundblad is right: we need to make svn_ra_get_locks() take a depth
{0, 1, infinity} argument, just like we've been saying for a number
of other APIs. It's issue #781, and this particular API should be
added to that issue, among the others.
In the meantime, svn_ra_get_locks() was designed to return a whole
hash in memory, and other functions already call it. We shouldn't be
fearful of calling the API, we should simply make it better in some
other, future patch. (Another way we can improve svn_ra_get_locks()
in the future is to make it into a streamy 'callback' interface, to
avoid ever holding all locks in memory.)
So what we'd like to see is
1. a new iteration of this patch which call svn_ra_get_locks()
instead of svn_ra_get_lock().
...then, after we've applied the patch, if you're up for it:
2. a completey new patch to
(a) make svn_ra_get_locks() use a streamy 'callback' interface,
much like svn_ra_get_log()
(b) make svn_ra_get_locks() take a boolean 'recurse' argument
that allows the caller to choose between depth 1 and depth infinity.
Does this all make sense to folks?
---------------------------------------------------------------------
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
- V3
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> Version 3 of the patch for fixing Issue #2291.
Thanks - this log message is much better.
>
> [[[
> 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_ra_local/ra_plugin.c
> (svn_ra_local__get_dir): Gets lock information and moved towards
> the end to avoid compile time warning.
The warning isn't the ultimate reason why this needed to be moved; the warning
is simply the mechanism that alerted you to the fact that it needed to be
moved. Try to say why it really needed to be moved - e.g. because it now
depends on a function that was declared later. Maybe something like, "moved to
after the locking functions because it now calls them."
[...]
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_get_dir): Gets lock information and moved towards the end
> to avoid compile time warning.
Ditto.
- Julian
---------------------------------------------------------------------
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 - V3
Posted by Alexander Thomas <al...@collab.net>.
Sorry, forgotten to attach patch, resending ...
Version 3 of the patch for fixing Issue #2291.
[[[
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_ra_local/ra_plugin.c
(svn_ra_local__get_dir): Gets lock information and moved towards
the end to avoid compile time warning.
* 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.
* subversion/libsvn_ra_svn/client.c
(ra_svn_get_dir): Gets lock information and moved towards the end
to avoid compile time warning.
* subversion/libsvn_ra_dav/fetch.c
(svn_ra_dav__get_dir): Gets lock information.
]]]
Thanks
-Alexander Thomas (AT)
[Patch] Issue #2291 - 'svn ls -v' should return locking
information - V3
Posted by Alexander Thomas <al...@collab.net>.
Version 3 of the patch for fixing Issue #2291.
[[[
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_ra_local/ra_plugin.c
(svn_ra_local__get_dir): Gets lock information and moved towards
the end to avoid compile time warning.
* 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.
* subversion/libsvn_ra_svn/client.c
(ra_svn_get_dir): Gets lock information and moved towards the end
to avoid compile time warning.
* subversion/libsvn_ra_dav/fetch.c
(svn_ra_dav__get_dir): Gets lock information.
]]]
Thanks
-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' return locking information
- V2
Posted by Julian Foad <ju...@btopenworld.com>.
Here's a review of your log message.
Alexander Thomas wrote:
> Version 2 of the patch for fixing Issue #2291 ('svn ls -v' return
> locking information).
>
> [[[
> Fix issue #2291 - 'svn ls -v' return locking information.
"return" -> "should return"
>
> * subversion/include/svn_types.h
> (struct svn_dirent2_t): Added new struct.
"(svn_dirent2_t): New struct."
By including the word "Added", you implied that the change you made to
svn_dirent2_t was adding a new struct (perhaps inside it).
> (struct svn_dirent_t): Deprecated and re-arranged.
Omit "struct"; just give its name.
How and why was it rearranged? Actually, I think maybe you mean it was moved
(whereas "rearranged" implies that parts of it were moved).
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__get_dir): Modified and re-arranged.
"Modified" isn't helpful. Obviously you modified it; that's why it is
mentioned in this log message. Instead, say briefly why and how you modified
it, and why you rearranged it. For example, "Simplified by using the new
helper function 'foo', and rearranged so that the most important results are
output first." (That is a completely made-up example.)
>
> * subversion/clients/cmdline/ls-cmd.c
> (print_dirents): Modified to print lock information.
You can just say "Print lock information."
> (print_dirents_xml): Modified to print lock information in xml.
Just say "Output lock information."
> (svn_cl__ls): Modified to call new function.
What new function? You haven't mentioned a new function. And why - for what
purpose or effect?
>
> * subversion/clients/cmdline/main.c
> Modified help text for svn ls.
"Mention locking information in the help text for 'svn ls'." would be better.
>
> * subversion/clients/cmdline/dtd/list.dtd
> Modified dtd file.
"Updated DTD file." would be better.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_get_dir): Modified and re-arranged.
Again, a better explanation is needed.
>
> * subversion/libsvn_ra_dav/fetch.c
> (svn_ra_dav__get_dir): Modified.
And here.
> ]]]
>
>
- Julian
---------------------------------------------------------------------
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' return locking information -
V2
Posted by Alexander Thomas <al...@collab.net>.
Version 2 of the patch for fixing Issue #2291 ('svn ls -v' return
locking information).
[[[
Fix issue #2291 - 'svn ls -v' return locking information.
* subversion/include/svn_types.h
(struct svn_dirent2_t): Added new struct.
(struct svn_dirent_t): Deprecated and re-arranged.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__get_dir): Modified and re-arranged.
* subversion/clients/cmdline/ls-cmd.c
(print_dirents): Modified to print lock information.
(print_dirents_xml): Modified to print lock information in xml.
(svn_cl__ls): Modified to call new function.
* subversion/clients/cmdline/main.c
Modified help text for svn ls.
* subversion/clients/cmdline/dtd/list.dtd
Modified dtd file.
* subversion/libsvn_ra_svn/client.c
(ra_svn_get_dir): Modified and re-arranged.
* subversion/libsvn_ra_dav/fetch.c
(svn_ra_dav__get_dir): Modified.
]]]
Re: [Patch] Issue #2291 - 'svn ls -v' return locking information
Posted by Madan U Sreenivasan <ma...@collab.net>.
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