On 17/02/2010 18:40, Greg Stein wrote:
> On Wed, Feb 17, 2010 at 12:22, Matthew Bentham<mjb67_at_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>
]]]
Received on 2010-02-18 12:09:23 CET