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'.
> Assuming so, then I might request some liberal sprinking of ###
> comments to explain that the code might need further tweaking, but
> that it seems to work to expectation. That future work on the section
> may need additional review to compare against entry->copied's
> definition. Something like that.
OK.
> And thanks! I appreciate the work you're doing to eliminate the
> demonspawn known as entry_t!
:-)
Matthew
Received on 2010-03-02 14:40:56 CET