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

Matthew Bentham wrote:
> 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
>

Hey there, Matthew,

I've looked at this mail thread and it seems you've done a very good job. I
like how the change to node.c became a one-liner in the end :)

Which brings me to the remaining question: should
svn_wc__internal_node_get_url() be able to handle deleted paths?

* neels looks

The function has no doc string (grr!). It is currently used by only one
other caller (_wc/translate.c which substitues keywords, not applicable to
deleted nodes).

But I'm taking the goal to have generally usable functions to help us. The
function is called svn_wc__internal_node_get_url(), and not
svn_wc__internal_node_get_url_except_if_node_is_deleted(). So:

I'm running this patch through another 'make check' and will commit if
successful.

Thanks!
~Neels