On Sun, Apr 18, 2010 at 08:14, Daniel Näslund <dan...@longitudo.com> wrote: > Hi! > > [[[ > As part of WC-NG, remove a use of svn_wc_entry_t. > > * subversion/libsvn_client/status.c > (svn_client_status5): Use svn_wc__node_is_status_added() instead of > checking entry->schedule. The check is meant to be used for > correctly determining when a node has been deleted in the repo > when running 'svn status -u'. > ]]] > > make check passes.
Great. That is an indicator that you aren't doing something terribly wrong. But our test suite doesn't cover everything, so this is not a "guarantee". > Since I've heard that all the easy replacements of svn_wc_entry_t has > already been done, I submit this patch for review. Am I missing > someting? Is there some cases when svn_wc_schedule_add does not equal > the output of svn_wc__node_status_is_added()? We've got a bit more supporting code to do things like this, such as these _is_status_* function and the is_file_external function (just added a week or two ago). But no... take a look at the implementation for status_is_added. It doesn't even test for db_status_obstructed_add. (and the delete case similarly). The status_added function also does not eliminate replacements from its computation. If BASE_SHADOWED is True, then it is a replacement instead of a simple add (or copy or move). But if that BASE node is not_present, then it is an add again. Even worse: the parent stub could have a BASE node marked as not_present, while the "real" node has a valid BASE node. IOW, to truly check for the not-present case which turns a replacement back into an add, you must look at the parent stub. And when I mentioned obstructed_add? Well... that is what wc_db reports. But technically entry->schedule should be made "normal" for that case. Unfortunately, I see five uses of that function in libsvn_client. :-( ... I was worried when these were added, that they'd be too simplistic, and that they'd preserve bad questions, but wasn't in the middle of coding and never reviewed them well. The node_is_status_* functions are generally misleading. And as you can see, for at least the status_added case, what is happening NOW is not a proper conversion of the old concept. Maybe the code that is working now does so because it actually wants to ask a simpler question than what I described above. Or maybe they have simplifying assumptions. That is why these node functions are also problematic: we want to ask the *real* question. Not frame it in terms of the old, grotesque definitions. In your scenario, you absolutely must consider the BASE node (whereas is_status_added is only examining WORKING). In fact, that is *only* what you want to check. You don't care about a WORKING node. You're merely trying to set sb.deleted_in_repos. So that means looking for the presence of the BASE node (in real and stub!), and using that to determine the value. > I can think of the case of a replacement. That would be > svn_wc_schedule_replace for entry->schedule. But _is_status_deleted() > just returns that the node is added. Not a problem though, since we're > interrested in the cases where a path exists in a previous revision, if > we're doing a replace, the path must exist (e.g. be in BASE) in our wc. > > Doing a test with two wc's from the same rev: > > wc1>svn rm A/D/G/pi > wc1>svn ci -m "" > > wc2>svn rm A/D/G/pi > wc2>touch A/D/G/pi > wc2>svn add A/D/G/pi > wc2>svn st > R A/D/G/pi > wc2>svn st -u > R * 1 A/D/G/pi I don't think this is testing the .deleted_in_repos situation. The cmdline just looks for a change in the repos, rather than a specific test for repos_text_status == svn_wc_status_deleted. So you're probably not getting repos_text_status set properly, unless you fix status_added(), or move to a different node query function. Cheers, -g > > cheers, > Daniel >
