Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

On Wed, Feb 17, 2010 at 05:22:13PM +0000, Matthew Bentham wrote:
> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c (revision 909397)
> +++ subversion/libsvn_client/commit_util.c (working copy)
> @@ -195,19 +195,23 @@
> {
> struct add_lock_token_baton *altb = walk_baton;
> apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
> - const svn_wc_entry_t *entry;
> -
> - SVN_ERR(svn_wc__maybe_get_entry(&entry, altb->wc_ctx, local_abspath,
> - svn_node_unknown, FALSE, FALSE,
> - scratch_pool, scratch_pool));
> -
> + const char* lock_token;
> + const char* url;
> +
> /* I want every lock-token I can get my dirty hands on!
> If this entry is switched, so what. We will send an irrelevant lock
> token. */
> - if (entry && entry->url && entry->lock_token)
> - apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, entry->url),
> + SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx, local_abspath,
> + scratch_pool, scratch_pool));
> + if (!lock_token)
> + return SVN_NO_ERROR;
> +
> + SVN_ERR(svn_wc__node_get_url(&url, altb->wc_ctx, local_abspath,
> + token_pool, scratch_pool));
> + if (url)
> + apr_hash_set(altb->lock_tokens, url,
> APR_HASH_KEY_STRING,
> - apr_pstrdup(token_pool, entry->lock_token));
> + apr_pstrdup(token_pool, lock_token));

The above looks great.

>
> return SVN_NO_ERROR;
> }
> Index: subversion/libsvn_wc/node.c
> ===================================================================
> --- subversion/libsvn_wc/node.c (revision 909397)
> +++ subversion/libsvn_wc/node.c (working copy)
> @@ -310,6 +310,34 @@
> db, local_abspath,
> scratch_pool, scratch_pool));
> }
> + else if (status == svn_wc__db_status_deleted)
> + {
> + const char *current_abspath = local_abspath;

To save a (probably negligible) bit of memory,
you could create an iterpool here...

> + while (status == svn_wc__db_status_deleted)
> + {

... clear it here ...

> + svn_dirent_split(current_abspath,
> + &current_abspath,
> + NULL, scratch_pool);
> +
> + SVN_ERR(svn_wc__db_read_info(&status, NULL, NULL, &repos_relpath,
> + &repos_root_url, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + db, current_abspath,
> + scratch_pool, scratch_pool));

... and pass it as the scratch_pool for svn_wc__db_read_info() here.
See http://subversion.apache.org/docs/community-guide/conventions.html#apr-pools

> + }

Assuming that the above loop will eventually get to the WC root,
can repos_relpath and repos_root_url ever be NULL before this
call to svn_dirent_join()? I don't think so, because it's not possible
to delete the WC root itself. See [1] below.

> + repos_relpath = svn_dirent_join(repos_relpath,
> + svn_uri_is_child(current_abspath,
> + local_abspath,
> + scratch_pool),

I think you should use svn_dirent_is_child() rather than svn_uri_is_child()
here. The URI functions are for URLs, not local absolute paths.

> + scratch_pool);

[1] Which would mean that these NULL checks are probably not necessary:

> + if (repos_relpath == NULL || repos_root_url == NULL)
> + {
> + *url = NULL;
> + return SVN_NO_ERROR;
> + }
> + }
> else
> {
> *url = NULL;

Stefan
Received on 2010-02-17 18:54:22 CET