On Fri, Dec 5, 2008 at 10:51 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> I'm looking off and on at issue #3324[1] (Path moved by merge but source not
> deleted). Looking at Paul's initial solution (which he says breaks more
> things than it fixes) has drawn my eye to an area of code that's changed a
> bit since I last looked at it. The code in question is the delta driver in
> libsvn_repos/reporter.c, which has grown at some point in the past the
> ability to transmit a base-revision with the editor->delete_entry() call. I
> *think* this was added when Paul was doing the detailed out-of-date
> information tweaks for 'svn status'.
That is correct, that change was from the ood status work, see:
svn log http://svn.collab.net/repos/svn/branches/ood-status-info@22753 -r22116
r22116 makes a similar change in delta_dirs() in r22116.
> (So, not a recent change.)
>
> However, now that I'm reading the code, I find I have some questions. For
> example, starting in reporter.c:798, we see:
>
> /* If there's a source and it's not related to the target, nuke it. */
> if (s_entry && !related)
> {
> svn_revnum_t deleted_rev;
>
> SVN_ERR(svn_repos_deleted_rev(svn_fs_root_fs(b->t_root), t_path,
> s_rev, b->t_rev, &deleted_rev,
> pool));
> SVN_ERR(b->editor->delete_entry(e_path, deleted_rev, dir_baton,
> pool));
> s_path = NULL;
> }
>
> At first glance, it makes sense. If there's something in the source tree
> that's not the same object as is in the target, delete the one (and then
> later, out of the above context, add the other). (By the way, related may
> also be TRUE if the "ignore_ancestry" option is set for this operation, not
> because things really are related, but because the code should behave as if
> they are.) Anyway, what I don't really understand is the use of the
> svn_repos_delete_rev() function, and the passing of its return value through
> delete_entry(). Here's why: if we know the source and target aren't
> related at all, and we're merely manufacturing a deletion in the editor
> drive, then why would we expect to find a real deletion of the target in the
> repository?
Agreed, if the source and target are unrelated then the call to
svn_repos_delete_rev(), regardless of what it returns, doesn't make
much sense and we should just call the svn_delta_editor_t delete_entry
callback with revision == SVN_INVALID_REVNUM, in which case it is
(supposed to be) ignored:
/** Remove the directory entry named @a path, a child of the directory
* represented by @a parent_baton. If @a revision is a valid
* revision number, it is used as a sanity check to ensure that you
* are really removing the revision of @a path that you think you are.
*
* All allocations should be performed in @a pool.
*/
svn_error_t *(*delete_entry)(const char *path,
svn_revnum_t revision,
void *parent_baton,
apr_pool_t *pool);
I'm embarrassed to say I spent a lot of time thinking about this and
looking at the code before I remembered that some very relevant
conversations about this have already occurred...
...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.
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?
> Actually, we can even ask similar questions at a higher level, too. This
> code is used to transform trees not just for 'svn update', but also for 'svn
> switch', where the source and target trees might be two different branches.
> What does it even mean to try to identify a deletion-rev in such a
> scenario? How do you bound the search?
>
> It looks to me like the source revision and target revision are the bounds,
> which makes total sense if the source and target trees are the same tree at
> different snapshots, such as with 'svn update'. But what about 'svn
> switch', where the source might be /trunk in r1234, and the target
> /branches/foo in r1234. You might still need to delete something to make
> that transformation, but with a revision range bounding of 1234:1234, you
> aren't gonna find a real deleted-rev. And again, what is the semantic
> meaning of this anyway? I can certainly construct an algorithm for finding
> this answer (it involves searching backward from source to common-ancestor
> for the addition of the missing thing and, failing that, searching forward
> from common-ancestor to target for its deletion). But as this code can also
> be used to compare completely unrelated trees, even that isn't a foolproof
> deleted-rev search mechanism.
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 :-\
> 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?
> Paul, can you help me understand this better? Is there something key that
> I'm missing?
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=981921
Received on 2008-12-10 00:41:49 CET