On 17/02/2010 18:40, Greg Stein wrote:
On Wed, Feb 17, 2010 at 12:22, Matthew Bentham<mj...@artvps.com> wrote:
...
Revised patch attached fixes the unit test (and hopefully the indentation,
if I've understood the style correctly), but I have no real instinct as to
whether I should be modifying a libsvn_wc routine in this way. Should I be
able to implement the client using functionality libsvn_wc already exports?
...
The changes to add_lock_token() look fine, but we need something
different for get_url().
Let's step back a moment, and look at what we're really trying to
accomplish. The add_lock_token() function is being called to look for
nodes that have locks associated with them, *in a deleted subtree*.
Further note that locks are associated with repository nodes, not the
nodes in a working copy. This means nodes from the BASE tree rather
than WORKING.
Thus, if a lock token is present, then a BASE node should be present.
That means svn_wc__db_scan_base_repos() will fetch the URL information
for the lock.
Ah yes that's much easier.
It would be nice to just compute the relative/child path and use that,
but it doesn't take into consideration switched subtrees. (tho
deletions across a switch are certainly interesting, and maybe not
even possible...)
Mmm! I made my head hurt trying to decide what the expected behaviour
would be with this :-) You're right, if we don't do any path mangling
then we've got a pretty good chance of doing something consistent.
I think the right answer is to craft a new node function here.
Something with a signature like:
svn_wc__node_get_lock_info(&lock_token,&url, wc_ctx, local_abspath,
result_pool, scratch_pool);
The implementation would be similar to add_lock_token(): examine the
lock token, then get the URL. In this case, if a lock token is
present, then you can just use scan_base_repos() to get the URL
information. Even better, the code can use base_get_info() (ignore
errors about the node not being present, and return "no lock"!). That
might return the repos info in one shot, without a need to do the
scan.
How does that sound?
I don't know about this, I think you're saying that 'add_lock_token'
would just be a call to 'svn_wc__node_get_lock_info' (followed by
whacking the info into the hash table). If we do that, we can remove
the existing 'svn_wc__node_get_lock_token', as it only has one other
caller, which could ignore the returned URL if necessary.
My question is, _should_ svn_wc__node_get_url be able to return the URL
of nodes which are deleted in WORKING? If it should, or if there's no
reason why it shouldn't, then add_lock_token can be implemented with
these two existing node routines, rather than creating a new one.
See whether you like the look of it this way, I think it's quite compact
and neat. If 'svn_wc__db_scan_base_repos' can fail to find the URL of a
node which is 'svn_wc__db_status_deleted', then this code isn't actually
_correct_ :-), but my understanding is that 'svn_wc__db_status_deleted'
means that it exists in BASE, so this is fine.
[[[
wc-ng: work towards eliminating svn_wc_entry_t
* subversion/libsvn_client/commit_util.c
(add_lock_token): Replace a use of svn_wc__maybe_get_entry with
use of svn_wc__node_get_*
* subversion/libsvn_wc/node.c
(svn_wc__internal_node_get_url): Find the URL of deleted nodes
Patch by: Matthew Bentham<mjb67{_AT_}artvps.com>
]]]
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));
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/node.c
===================================================================
--- subversion/libsvn_wc/node.c (revision 909397)
+++ subversion/libsvn_wc/node.c (working copy)
@@ -294,7 +294,8 @@
if (repos_relpath == NULL)
{
if (status == svn_wc__db_status_normal
- || status == svn_wc__db_status_incomplete)
+ || status == svn_wc__db_status_incomplete
+ || status == svn_wc__db_status_deleted)
{
SVN_ERR(svn_wc__db_scan_base_repos(&repos_relpath, &repos_root_url,
NULL,
