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

On 18/02/2010 11:08, Matthew Bentham wrote:
> 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>
> ]]]

I think this version of the patch (re-attached) crossed in the mail with
Bert Huijben's comments on the previous version: I haven't received any
feedback on this version using svn_wc__db_scan_base_repos.

Thanks,

Matthew