On 02/03/2010 13:40, Matthew Bentham wrote:
> On 02/03/2010 13:06, Greg Stein wrote:
>> On Tue, Mar 2, 2010 at 04:45, Matthew Bentham<mjb67_at_artvps.com> wrote:
>>> Trying to remove more uses of svn_wc_entry_t, it seems there is no way at
>>> the moment to get the equivalent of entry->copied using node routines. Is
>>> this the sort of thing that is needed?
>>>
>>> [[[
>>> wc-ng: work towards eliminating svn_wc_entry_t
>>>
>>> * libsvn_wc/node.c, include/private/svn_wc_private.h
>>> Add svn_wc__node_is_status_copied()
>>>
>>> * subversion/libsvn_client/copy.c
>>> (calculate_target_mergeinfo): Remove unused 'no_repos_access'
>>> parameter, replace use of svn_wc__get_entry with node routines.
>>>
>>> Patch by: Matthew Bentham<mjb67{_AT_}artvps.com>
>>> ]]]
>>
>> Hey...
>>
>> Outside of the issues around the node functions that are being
>> discussed, I kind of worry about this code. entry->copied does not
>> truly correspond to status_copied (or status_moved_here). If you look
>> in entries.c where entry->copied is *set*, then you'll see the logic
>> is rather ugly and opaque and (from a human brain's standpoint) maybe
>> even non-deterministic :-P
>>
>> When walking into this area, it is helpful to step back and ensure
>> that the merge code is really looking for the right thing. It is
>> entirely possible it was looking at entry->copied, when it should have
>> been looking for status_(copied|moved_here).
>
> I _think_ it just needs to know whether this is an add with history.
>
>> Because of the fragility around entry->copied, I can't really tell
>> whether this patch is Right from a simple inspection. I presume that
>> you've run the full test suite, and it continues to pass with this
>> change?
>
> Mmmm, I've been running a lot of the tests, but if I run the full test
> suite it grinds to a halt after about 10hrs (and doesn't finish even
> after 28hrs, the longest I've tried so far). So I haven't been running
> the whole thing. I will probably need to change development platform to
> something non-cygwin.
>
> Nevertheless, I'm worried that calculate_target_mergeinfo seems to be
> fairly under-tested:
>
> - If I force 'copied' to 'TRUE', (ie. force it to treat every addition
> as an add with history) the only test I've found to fail is
> copy_tests.py:copy_added_paths_to_URL.
>
> - If I force 'copied' to 'FALSE', (treat every addition as a local
> addition), I can't find a test that fails. I would have expected
> something in merge_tests.py probably, but those seem to be the tests
> which are murdering my computer (there's been some relevant discussion
> about this in another thread).
>
> At the moment I'm trying to write a test that passes in trunk, and fails
> if I force 'copied' to 'FALSE'.
I've set up a Linux development environment and run the whole test
suite. There are no test failures if all adds are treated as local adds
in calculate_target_mergeinfo. So I think I need to work on improving
the test coverage before I can go back to removing entry_t here.
Matthew
Received on 2010-03-02 18:19:20 CET