On 02/03/2010 13:06, Greg Stein wrote:
On Tue, Mar 2, 2010 at 04:45, Matthew Bentham<mj...@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
