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.
Fix issue #2291: 'svn ls -v' should return locking information.
* subversion/include/svn_client.h
(svn_client_ls3): New prototype.
(svn_client_ls2): Deprecated.
* subversion/libsvn_client/ls.c
(svn_client_ls3): Gets lock information and returns it as hash.
(svn_client_ls2): Modified to call new function.
* 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 10 14:16:48 2005