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

Neels J Hofmeyr wrote:
> 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.

Committed in r911846.

You should soon find yourself on
http://www.red-bean.com/svnproject/contribulyzer/ :)

Thanks again,
~Neels