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
Received on Tue Aug 9 17:08:31 2005