Paul Burba wrote:
> ...Turns out that REVISION isn't just ignored if invalid, it's simply
> ignored, see 'In IRC regarding the above' in this thread,
> http://svn.haxx.se/dev/archive-2006-10/0344.shtml, where dlr and I
> originally discussed the fact that all the delete_entry
> implementations where actually ignoring the REVISION argument. FWIW,
> no conclusion was reached on what exactly the "sanity check" mentioned
> in the doc string is supposed to mean.
Well, keep in mind that the editor is used both for sending changes from
server->client and from client->server. Some bits of it are only used in
one of the directions. When committing, we always pass the base_revision to
delete_entry() so the server can make sure we're deleting what we think
we're deleting. In the reverse direction, we never needed such a check
(until the status ood stuff) because updates and related operations had
already told the server what versions of things we had. Same goes for the
set_target_revision() editor function -- used for server->client to tell the
client what revision it is really updating to; not used in client->server
because you don't know a commit's revision until after the commit is finalized.
Right now you're thinking, "... and why don't the docs say this?" I'll try
to make some changes around the editor API docs to improve this.
> Anyway, looking again today at all the svn_delta_editor_t
> delete_entry() implementations the only one I see using its REVISION
> arg is still libsvn_wc\status.c:delete_entry(). Regardless, we should
> always pass SVN_INVALID_REVNUM if the source and the target are
> unrelated. Would you agree with that and would it alleviate your
> concerns?
I think so.
> I can't summon any argument against always sending SVN_INVALID_REVNUM
> to delete_entry() in the case of a switch...Though I haven't figured
> out how to do that exactly :-\
Well, you could check 'source' and 'target' (the nodes at the root of the
delta trees) and make sure they are ancestrally related (equivalent history
points, or one is a predecessor of the other). If that's not the case, it's
a switch. Unfortunately, if it's *not* the case, you still don't know.
Why? Because you might be using switch on a working copy directory in order
to straighten it out and un-switch subdirs inside it. Consider:
$ svn sw '^/branches/1.5.x/contrib/cgi' subversion/contrib/cgi/
U subversion/contrib/cgi/mirror_dir_through_svn.cgi
Updated to revision 34646.
$ svn st subversion
S subversion/contrib/cgi
$ svn sw '^/trunk' subversion
U subversion/contrib/cgi/mirror_dir_through_svn.cgi
Updated to revision 34646.
$
That last operation is a switch, but not one detectable by simply comparing
the ancestry of the root nodes. Those nodes are identical in this case, yet
the operation is still a switch. So we'd need to look at something more
involved, perhaps by tracking information provided in the report itself,
noting the presence of link_path()'s or something.
>> I'm concerned as I look at this code that it is well-intended, but
>> attempting to answer questions that in many cases have no answers, or at the
>> very least isn't doing enough to discern those answers where they exist.
>> And I'm further concerned that code originally added for a bit of reporting
>> goodness ('svn status' out-of-dateness stuff) is now being used more
>> operationally as part of 'svn merge' to unpleasant ends in certain cases.
>
> "Now being used"? You mean in the patch I attached to issue #3324 or
> something else?
I meant something else, but I was wrong. You're patch didn't change the way
the base_rev was used (as I presumed) -- it caused the base_rev to be used
at all.
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=982285
Received on 2008-12-10 16:26:06 CET