On Sun, Apr 18, 2010 at 08:14, Daniel Näslund <daniel_at_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
>
Received on 2010-04-19 16:43:02 CEST