Merge lp:~vila/bzr/323111-orphan-non-versioned-files into lp:bzr

[Vincent Ladeuil]

When a bzr command remove a previously versioned directory, it
sometimes fails because the directory contains unversioned files.

This is a pain.

The idea to address this problem is to 'orphan' such unversioned
files. This is implemented by moving them out of the way into a
directory called 'bzr-orphans' at the root of the working tree
and name them <file>.~#~, <file> being the `basename` of the file
and # being an incremented number.

The initial idea discussed with lifeless on IRC long ago was to
have an even simpler policy for orphaning: keep the original file
name and if one is already present, just erase it.

I went a bit further using backup names since a single bzr
operation could produce several orphans with the same base name
otherwise and that would have meant losing all but the last
orphan with this name.

The implementation first check the children of the conflicting
parent dir. If all of them are unversioned files (leading to an
empty directory) the conflict is resolved by moving all the
children into the orphan directory. The directory can then be
deleted without conflicts.

This is implemented for DiskTreeTransform and forbidden for
TransformPreview where it's not supposed to occur anyway since
there should be no unversioned files in the input tree.

Not that since we rely on TreeTransform, I don't think the
'bzr-orphans' can be outside of the working tree (IMBW) but I
don't think it's a problem for a first implementation.

This triggers bug #634470 revealing a latent bug in subtree
support. Since this is an experimental format so far, poolie
suggested to turn the failing test into an expected failure
instead.

Given that this fixes a behaviour that wasn't exercised by the
test suite, I'd appreciate feedback on real life scenarios that
are fixed or additional test ideas.

Note that this is self-contained into new_orphan() so far.
Possible enhancements includes:

- making the 'bzr-orphans' directory be configurable (it's
  clearly tight to the working tree though),

- allows various orphaning policies (always delete, delete junk
  files (tbd), never orphan (the behaviour before this patch),
  etc)

There is one more submission coming with more cleanups.

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    >> The implementation first check the children of the conflicting
    >> parent dir. If all of them are unversioned files (leading to an
    >> empty directory) the conflict is resolved by moving all the
    >> children into the orphan directory. The directory can then be
    >> deleted without conflicts.

    > Could we just rename the directory into bzr-orphans?

Not if it already exists obviously.

    > Preserving the clustering would be nice, though I understand
    > flattening it.

bzr-orphans collect *all* the orphans, there could be more than one
directory involved, so special-casing here... in treetransform... I
don't feel like it :)

    > One concern is that directory filling up with useless files after
    > doing a 'bzr switch' 5 or so times across a
    > directory-full-of-python-files deletion/creation.

Damn, I forgot to mention that.

When we discussed it with lifeless, the idea was that the user will
suddenly get a 'bzr-orphans' in his face and will deal with it. This is
still less disturbing than a conflict since it can accumulate several
commands producing orphans. So dealing with it should be as simple as:
have a look, nothing interesting, delete the bzr-orphans directory.

Now there are certainly various ways to enhance that but the proposed
implementation gives us a way to experiment.

Should be bring the subject to the mailing list once this has landed to
ask for a large feedback:

- do you produce many orphans ?
- did you wonder where this 'bzr-orphans' directory came from ?

- would you like warnings emitted when files are orphaned (hmm, should
  we wait feedback for that ? :-)

trace.warning added.

    > ...

    >> Note that this is self-contained into new_orphan() so far.
    >> Possible enhancements includes:
    >>
    >> - making the 'bzr-orphans' directory be configurable (it's
    >> clearly tight to the working tree though),
    >>
    >> - allows various orphaning policies (always delete, delete junk
    >> files (tbd), never orphan (the behaviour before this patch),
    >> etc)

    > - I think what we really want is a hook point for determining what to do
    > with the file. Similar to the per-file merge hook, something that is a
    > per-file 'ignored file handler'.

Hehe, obviously we may want to add some .bzrjunk support which in turn
will be respected by the orphaning :)

But once junk files are out of the way, yes, such a hook sounds like the
way to go.

    > ...
    > +class TestTransformMissingParent(tests.TestCaseWithTransport):
    > +
    > + def get_tree_transform_with_unversioned_dir(self):

    > ^- I think we usually call these "make_*", since it is creating stuff.

Bah, you know you've been reading test_transform too much when... :)

Fixed.

    > + def new_orphan(self, trans_id, parent_id):
    > + """See TreeTransformBase.new_orphan."""
    > + # Add the orphan dir if it doesn't exist
    > + od_id = self.trans_id_tree_path('bzr-orphans')
    > + if self.final_kind(od_id) is None:
    > + self.create_directory(od_id)
    > + # Find a name that doesn't exist ...

[Vincent Ladeuil (vila)]

>>>>> Vincent Ladeuil <email address hidden> writes:

<snip/>

    >> ^- I don't think this check is actually correct. Since if the
    >> bzr-orphans directory already exists, you'd already have a file present
    >> that you have to watch out for conflicting with.
    >> So you need to check *both* that the path isn't going to be
    >> created (yet) and that it doesn't already exist.

    > *blink* Grr, trying to make the threads easier to review, I separated
    > the addition of _has_named_child which *does* the correct checks.

    > The correct check should be done here by calling _get_backup_name() not
    > osutils.available_backup_name().

    > Fixed in both threads.

Clarification: The code presented in the first submission wasn't
correct. When I realized that, I found _get_backup_name and from there
realized that we had some duplication and some dead code so I create the
3 threads.

So I've now fixed *this* thread to call _get_backup_name and the
following thread will deprecate it introducing tt._available_backup_name
both of them doing the right checks.

So 'fixed in both threads' means: this thread has now the correct fix
and the third one always had the correct fix.

Sorry for the confusion :-/

[Martin Pool (mbp)]

I think I would like to see a configuration option that will let you retain the old option, so that anyone who thinks it's a step backwards can use it.

http://bazaarvcs.wordpress.com/?p=189

[Andrew Bennetts (spiv)]

I agree a configuration option would be good to add. As discussed on IRC this would mean both that users that prefer the old behaviour can keep it, and also would be a good sign that the code is modular enough that we could allow plugins to define new policies without needing to significantly rework it.

From John's review:

> > Could we just rename the directory into bzr-orphans?
>
> Not if it already exists obviously.

Well, we can't rename files into bzr-orphans if the name already exists, either. But you could rename it somedir.~2~ or whatever. This would make restoring precious files *much* easier than a flat directory of *.~?~ files. I'd suspect that preserving the directory structure as much as possible will greatly reduce the number of users that will prefer the old behaviour.

Also, it would be nice to write out a file into bzr-orphans describing the original paths and the new name in bzr-orphans, to make it possible to automatically put the tree back exactly as it was. (Even cooler might be if 'bzr revert' could use that file? That's clearly way out of scope for this patch though...)

I'll follow-up separately with thoughts on the code itself.

[Andrew Bennetts (spiv)]

The code looks pretty straightforward. Nice!

Here are some issues, though:

174 +class TestOrphan(tests.TestCaseWithTransport):
175 +
176 + # Alternative implementations may want to test:
177 + # - can't create orphan dir
178 + # - orphaning forbidden
179 + # - can't create orphan

I don't quite follow this comment. Which alternative implementations — these aren't per-transform tests? Is this a list of hypothetical alternative orphan-handling policies, or scenarios alternative implementations of orphan handling ought to consider, or something else?

I guess basically I don't understand who the intended audience is for this comment.

222 +lazy_import.lazy_import(globals(), """

Why the change to "from bzrlib import lazy_import", rather than "from bzrlib.lazy_import import lazy_import". We overwhelmingly prefer the latter elsewhere in our code base.

316 + if trans_id in tt._removed_contents:
317 + orphans = []
318 + empty = True
319 + # Find the potential orphans, stop if one item should be kept
320 + for c in tt.by_parent()[trans_id]:
321 + if tt.final_file_id(c) is None:
322 + orphans.append(c)
323 + else:
324 + empty = False
325 + break
326 + if empty:

The 'empty' variable seems redundant. You could just as easily test 'if orphans' (or 'if len(orphans) == 0' if you prefer).

That's all the comments I have so far. I don't have a verdict yet, as I have few too many questions, so I'll mark my review Needs Information for now.

review: Needs Information

[Vincent Ladeuil (vila)]

> Well, we can't rename files into bzr-orphans if the name already exists,
> either. But you could rename it somedir.~2~ or whatever.

You mean creating bzr-orphans.~1~, bzr-orphans.~2~ ?

> This would make
> restoring precious files *much* easier than a flat directory of *.~?~ files.

This is not exactly a flat structure... directories can be orphaned too.

> I'd suspect that preserving the directory structure as much as possible will
> greatly reduce the number of users that will prefer the old behaviour.

The counter-argument was that if you re-create all the directories leading to the orphan,
it becomes harder to inspect bzr-orphans as you have to drill down so see which files
have been orphaned.

>
> Also, it would be nice to write out a file into bzr-orphans describing the
> original paths and the new name in bzr-orphans, to make it possible to
> automatically put the tree back exactly as it was.

Yup, I thought about that, but as you said below, out-of-scope.

> (Even cooler might be if
> 'bzr revert' could use that file?

That would certainly lead to very complex code: the orphans are not versioned, the user
didn't ask bzr to version them, so we know little about them and we are not prepared
to deal with them (ditto for shelves...)

> That's clearly way out of scope for this
> patch though...)
>
> I'll follow-up separately with thoughts on the code itself.

[Vincent Ladeuil (vila)]

> The code looks pretty straightforward. Nice!
>
> Here are some issues, though:
>
> 174 +class TestOrphan(tests.TestCaseWithTransport):
> 175 +
> 176 + # Alternative implementations may want to test:
> 177 + # - can't create orphan dir
> 178 + # - orphaning forbidden
> 179 + # - can't create orphan
>
> I don't quite follow this comment. Which alternative implementations — these
> aren't per-transform tests?

No :)

> Is this a list of hypothetical alternative
> orphan-handling policies, or scenarios alternative implementations of orphan
> handling ought to consider,

Yes.

> or something else?
>
> I guess basically I don't understand who the intended audience is for this
> comment.

People implementing new orphaning policies and wondering how to test them :)

Which I'm currently doing ;)

So the next submission, addressing the config
variable (https://code.edge.launchpad.net/~vila/bzr/323111-orphan-config-option/+merge/35690)
take care of these comments which were, obviously, addressed to
me as stealth way to avoid implementing it :-D

>
> 222 +lazy_import.lazy_import(globals(), """
>
> Why the change to "from bzrlib import lazy_import", rather than "from
> bzrlib.lazy_import import lazy_import". We overwhelmingly prefer the latter
> elsewhere in our code base.

Except it has been said several times that we shouldn't import
symbols from modules but prefer importing modules and prefix
symbols and that we should migrate to this form progressively.

I've been doing many such cleanups, some addressing weird and
subtle aliasing bugs, including ones related to lazy
imports (lazy imports more or less requires using
<module>.<symbol> in some cases).

There are a few exceptions to this rule:
- symbol_versioning but even there I've commented that we may
  want to rework this module too,
- bzrlib.tests aliasing TestCase, etc symbols from
  bzrlib.tests.TestUtils (but isn't it a sign that they should be
  define into bzrlib.tests ?)

I also found that simplifying the import list at the beginning of
the module makes it clearer and avoid leaving cruft there.

It generally makes it easier to move code from one module to the
others since there are fewer dependencies to update.

It makes the code easier to read (you don't have to go back to
the beginning of the module to know where this apparently global
symbol is coming from).

Another reason from the leaking tests saga:

,----
| I did so by wrapping transport.get_transport() only to realise
| that far too many tests were not using it. I also uncovered a
| nasty reason why we should always use the 'import <module> ;
| <module>.<function>' idiom instead of 'from <module> import
| <function> ; <function>': the later doesn't allow
| wrappers... Obvious a posteriori, but I was quite puzzled before
| I realised that. Those were worth fixing even if we decide to replace
| the wrapping by a hook as mentioned below.
`----

Now, we can make lazy_import another exception... but why ?

And why do I keep bumping into people that don't want to use the
'import module ; module.symbol' given that we laready encounter
several related problems.

The only pro I can find for doing 'from ...

[Marius Kruger (amanica)]

this bzr-orphans dir must be ignored by bzr by default,
i.e. it should not be accidental be added with an unqualified `bzr add` .

we used to have this problem with upgrading, so now the first backup
dir we make is
backup.bzr.~1~ which is ignored by default, and you can have more than one.

so either call it bzr-orphans~ or bzr-orphans~1~ or add bzr-orphans to
the default ignore list. (I think its fine if thats done a followup)

[Martin Pool (mbp)]

Regarding 'import module' - Vincent, if you want this done
consistently you should put up a proposal to add it to the coding
standards, together with the rationale and the exceptions. Then we
can settle it clearly and there won't be case-by-case arguments or
questions. We could even possibly add rules to pyflakes or something
to warn about violations.

To me 'lazy_import' actually is a reasonable case for importing the
particular symbol, because it's clear where it comes from and it seems
pointlessly redundant to say 'lazy_import.lazy_import'.

--
Martin

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> Regarding 'import module' - Vincent, if you want this done
> consistently you should put up a proposal to add it to the coding
> standards, together with the rationale and the exceptions. Then we
> can settle it clearly and there won't be case-by-case arguments or
> questions. We could even possibly add rules to pyflakes or something
> to warn about violations.
>
> To me 'lazy_import' actually is a reasonable case for importing the
> particular symbol, because it's clear where it comes from and it seems
> pointlessly redundant to say 'lazy_import.lazy_import'.

Another thing about lazy_import is it usually doesn't make sense to
include it in the main "from bzrlib import (....)" lines, because it is
often used before that point. So it will always tend to have its own
import statement.

I'm also unaware of any actual problems caused by importing the
lazy_import function directly rather than the module, so changing this
convention does seem to have no significant benefit.

[Martin Pool (mbp)]

On 17 September 2010 14:43, Andrew Bennetts
<email address hidden> wrote:
> I'm also unaware of any actual problems caused by importing the
> lazy_import function directly rather than the module, so changing this
> convention does seem to have no significant benefit.

I'm ok with saying "normally use 'import module'" but I think this
should be one of the exceptions.

--
Martin

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> On 17 September 2010 14:43, Andrew Bennetts
> <email address hidden> wrote:
> > I'm also unaware of any actual problems caused by importing the
> > lazy_import function directly rather than the module, so changing this
> > convention does seem to have no significant benefit.
>
> I'm ok with saying "normally use 'import module'" but I think this
> should be one of the exceptions.

I agree.

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > Martin Pool wrote:
    >> On 17 September 2010 14:43, Andrew Bennetts
    >> <email address hidden> wrote:
    >> > I'm also unaware of any actual problems caused by importing the
    >> > lazy_import function directly rather than the module, so changing this
    >> > convention does seem to have no significant benefit.
    >>
    >> I'm ok with saying "normally use 'import module'" but I think this
    >> should be one of the exceptions.

    > I agree.

Pfew, I thought I had to fight for that.
Of course I'm ok with the exception for lazy_import :)

[Vincent Ladeuil (vila)]

Updated, please review. I'll make another submission for the import policy discussion.

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-10-15 14:16:12 +0000
+++ bzrlib/bzrdir.py	2010-10-15 14:16:12 +0000
@@ -1294,7 +1294,10 @@
         wt = self.open_workingtree(recommend_upgrade=False)
         repository = wt.branch.repository
         empty = repository.revision_tree(_mod_revision.NULL_REVISION)
-        wt.revert(old_tree=empty)
+        # We ignore the conflicts returned by wt.revert since we're about to
+        # delete the wt metadata anyway, all that should be left here are
+        # detritus. But see bug #634470 about subtree .bzr dirs.
+        conflicts = wt.revert(old_tree=empty)
         self.destroy_workingtree_metadata()
 
     def destroy_workingtree_metadata(self):
 
=== modified file 'bzrlib/tests/per_workingtree/test_pull.py'
--- bzrlib/tests/per_workingtree/test_pull.py	2010-09-06 10:08:36 +0000
+++ bzrlib/tests/per_workingtree/test_pull.py	2010-10-15 14:16:12 +0000
@@ -17,6 +17,7 @@
 
 from cStringIO import StringIO
 
+from bzrlib import tests
 from bzrlib.tests import per_workingtree
 
 
@@ -62,3 +63,39 @@
         tree.commit('second')
         to_tree.pull(tree.branch)
         self.assertEqual('second_root_id', to_tree.get_root_id())
+
+
+class TestPullWithOrphans(per_workingtree.TestCaseWithWorkingTree):
+
+    def make_branch_deleting_dir(self, relpath=None):
+        if relpath is None:
+            relpath = 'trunk'
+        builder = self.make_branch_builder(relpath)
+        builder.start_series()
+
+        # Create an empty trunk
+        builder.build_snapshot('1', None, [
+                ('add', ('', 'root-id', 'directory', ''))])
+        builder.build_snapshot('2', ['1'], [
+                ('add', ('dir', 'dir-id', 'directory', '')),
+                ('add', ('file', 'file-id', 'file', 'trunk content\n')),])
+        builder.build_snapshot('3', ['2'], [
+                ('unversion', 'dir-id'),])
+        builder.finish_series()
+        return builder.get_branch()
+
+    def test_pull_orphans(self):
+        from bzrlib import workingtree
+        if isinstance(self.workingtree_format, workingtree.WorkingTreeFormat2):
+            raise tests.TestSkipped(
+                'WorkingTreeFormat2 does not support missing parent conflicts')
+        trunk = self.make_branch_deleting_dir('trunk')
+        work = trunk.bzrdir.sprout('work', revision_id='2').open_workingtree()
+        # Add some unversioned files in dir
+        self.build_tree(['work/dir/foo',
+                         'work/dir/subdir/',
+                         'work/dir/subdir/foo'])
+        work.pull(trunk)
+        self.assertLength(0, work.conflicts())
+        # The directory removal should succeed
+        self.failIfExists('work/dir')
 
=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-10-15 14:16:12 +0000
+++ bzrlib/tests/test_bzrdir.py	2010-10-15 14:16:12 +0000
@@ -792,12 +792,23 @@
         sub_tree.add('file')
         tree.commit('Initial commit')
         tree.bzrdir.destroy_workingtree()
+        # FIXME: subtree/.bzr is left here which allows the test to pass (or
+        # fail :-( ) -- vila 20100909
         repo = self.make_repository('repo', shared=True,
             format='dirstate-with-subtree')
         repo.set_make_working_trees(False)
-        tree.bzrdir.sprout('repo/tree2')
-        self.failUnlessExists('repo/tree2/subtree')
-        self.failIfExists('repo/tree2/subtree/file')
+        # FIXME: we just deleted the workingtree and now we want to use it ????
+        # At a minimum, we should use tree.branch below (but this fails too
+        # currently) or stop calling this test 'treeless'. Specifically, I've
+        # turn the line below into an assertRaises when 'subtree/.bzr' is
+        # orphaned and sprout tries to access the branch there (which is left
+        # by bzrdir.BzrDirMeta1.destroy_workingtree when it ignores the
+        # [DeletingParent('Not deleting', u'subtree', None)] conflict). See bug
+        # #634470.  -- vila 20100909
+        self.assertRaises(errors.NotBranchError,
+                          tree.bzrdir.sprout, 'repo/tree2')
+#        self.failUnlessExists('repo/tree2/subtree')
+#        self.failIfExists('repo/tree2/subtree/file')
 
     def make_foo_bar_baz(self):
         foo = bzrdir.BzrDir.create_branch_convenience('foo').bzrdir
 
=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-10-15 14:16:12 +0000
+++ bzrlib/tests/test_transform.py	2010-10-15 14:16:12 +0000
@@ -28,6 +28,7 @@
     revision as _mod_revision,
     rules,
     tests,
+    transform,
     urlutils,
     )
 from bzrlib.bzrdir import BzrDir
@@ -2347,6 +2348,39 @@
         self.failUnlessExists('a/b')
 
 
+class TestTransformMissingParent(tests.TestCaseWithTransport):
+
+    def make_tt_with_versioned_dir(self):
+        wt = self.make_branch_and_tree('.')
+        self.build_tree(['dir/',])
+        wt.add(['dir'], ['dir-id'])
+        wt.commit('Create dir')
+        tt = TreeTransform(wt)
+        self.addCleanup(tt.finalize)
+        return wt, tt
+
+    def test_resolve_create_parent_for_versioned_file(self):
+        wt, tt = self.make_tt_with_versioned_dir()
+        dir_tid = tt.trans_id_tree_file_id('dir-id')
+        file_tid = tt.new_file('file', dir_tid, 'Contents', file_id='file-id')
+        tt.delete_contents(dir_tid)
+        tt.unversion_file(dir_tid)
+        conflicts = resolve_conflicts(tt)
+        # one conflict for the missing directory, one for the unversioned
+        # parent
+        self.assertLength(2, conflicts)
+
+    def test_resolve_orphan_non_versioned_file(self):
+        wt, tt = self.make_tt_with_versioned_dir()
+        dir_tid = tt.trans_id_tree_file_id('dir-id')
+        tt.new_file('file', dir_tid, 'Contents')
+        tt.delete_contents(dir_tid)
+        tt.unversion_file(dir_tid)
+        conflicts = resolve_conflicts(tt)
+        # no conflicts or rather: orphaning 'file' resolve the 'dir' conflict
+        self.assertLength(0, conflicts)
+
+
 A_ENTRY = ('a-id', ('a', 'a'), True, (True, True),
                   ('TREE_ROOT', 'TREE_ROOT'), ('a', 'a'), ('file', 'file'),
                   (False, False))
@@ -3227,3 +3261,39 @@
         trans_id = tt.trans_id_tree_path('file')
         self.assertEqual((LINES_ONE,),
             tt._get_parents_texts(trans_id))
+
+
+class TestOrphan(tests.TestCaseWithTransport):
+
+    # Alternative implementations may want to test:
+    # - can't create orphan dir
+    # - orphaning forbidden
+    # - can't create orphan
+
+    def test_no_orphan_for_transform_preview(self):
+        tree = self.make_branch_and_tree('tree')
+        tt = transform.TransformPreview(tree)
+        self.addCleanup(tt.finalize)
+        self.assertRaises(NotImplementedError, tt.new_orphan, 'foo', 'bar')
+
+    def test_new_orphan(self):
+        wt = self.make_branch_and_tree('.')
+        self.build_tree(['dir/', 'dir/foo'])
+        wt.add(['dir'], ['dir-id'])
+        wt.commit('add dir')
+        tt = transform.TreeTransform(wt)
+        self.addCleanup(tt.finalize)
+        dir_tid = tt.trans_id_tree_path('dir')
+        foo_tid = tt.trans_id_tree_path('dir/foo')
+        tt.delete_contents(dir_tid)
+        tt.unversion_file(dir_tid)
+        raw_conflicts = tt.find_conflicts()
+        self.assertLength(1, raw_conflicts)
+        self.assertEqual(('missing parent', 'new-1'), raw_conflicts[0])
+        remaining_conflicts = resolve_conflicts(tt)
+        # Yeah for resolved conflicts !
+        self.assertLength(0, remaining_conflicts)
+        # We have a new orphan
+        self.assertEquals('foo.~1~', tt.final_name(foo_tid))
+        self.assertEquals('bzr-orphans',
+                          tt.final_name(tt.final_parent(foo_tid)))
 
=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-10-15 14:16:12 +0000
+++ bzrlib/transform.py	2010-10-15 14:16:12 +0000
@@ -19,8 +19,11 @@
 from stat import S_ISREG, S_IEXEC
 import time
 
-from bzrlib.lazy_import import lazy_import
-lazy_import(globals(), """
+from bzrlib import (
+    errors,
+    lazy_import,
+    )
+lazy_import.lazy_import(globals(), """
 from bzrlib import (
     annotate,
     bencode,
@@ -32,6 +35,7 @@
     multiparent,
     osutils,
     revision as _mod_revision,
+    trace,
     ui,
     )
 """)
@@ -64,7 +68,6 @@
 
 ROOT_PARENT = "root-parent"
 
-
 def unique_add(map, key, value):
     if key in map:
         raise DuplicateKey(key=key)
@@ -759,6 +762,17 @@
         self.create_symlink(target, trans_id)
         return trans_id
 
+    def new_orphan(self, trans_id, parent_id):
+        """Schedule an item to be orphaned.
+
+        When a directory is about to be removed, its children, if they are not
+        versioned are moved out of the way: they don't have a parent anymore.
+
+        :param trans_id: The trans_id of the existing item.
+        :param parent_id: The parent trans_id of the item.
+        """
+        raise NotImplementedError(self.new_orphan)
+
     def _affected_ids(self):
         """Return the set of transform ids affected by the transform"""
         trans_ids = set(self._removed_id)
@@ -1271,6 +1285,21 @@
             del self._limbo_children_names[trans_id]
         delete_any(self._limbo_name(trans_id))
 
+    def new_orphan(self, trans_id, parent_id):
+        """See TreeTransformBase.new_orphan."""
+        # Add the orphan dir if it doesn't exist
+        orphan_dir = 'bzr-orphans'
+        od_id = self.trans_id_tree_path(orphan_dir)
+        if self.final_kind(od_id) is None:
+            self.create_directory(od_id)
+        parent_path = self._tree_id_paths[parent_id]
+        # Find a name that doesn't exist yet in the orphan dir
+        actual_name = self.final_name(trans_id)
+        new_name = _get_backup_name(actual_name, self.by_parent(), od_id, self)
+        self.adjust_path(new_name, od_id, trans_id)
+        trace.warning('%s has been orphaned in %s'
+                      % (joinpath(parent_path, actual_name), orphan_dir))
+
 
 class TreeTransform(DiskTreeTransform):
     """Represent a tree transformation.
@@ -1727,6 +1756,9 @@
             childpath = joinpath(path, child)
             yield self.trans_id_tree_path(childpath)
 
+    def new_orphan(self, trans_id, parent_id):
+        raise NotImplementedError(self.new_orphan)
+
 
 class _PreviewTree(tree.Tree):
     """Partial implementation of Tree to support show_diff_trees"""
@@ -2427,12 +2459,14 @@
     for child in tt.iter_tree_children(old_parent):
         tt.adjust_path(tt.final_name(child), new_parent, child)
 
+
 def _reparent_transform_children(tt, old_parent, new_parent):
     by_parent = tt.by_parent()
     for child in by_parent[old_parent]:
         tt.adjust_path(tt.final_name(child), new_parent, child)
     return by_parent[old_parent]
 
+
 def _content_match(tree, entry, file_id, kind, target_path):
     if entry.kind != kind:
         return False
@@ -2799,11 +2833,26 @@
 
         elif c_type == 'missing parent':
             trans_id = conflict[1]
-            try:
-                tt.cancel_deletion(trans_id)
-                new_conflicts.add(('deleting parent', 'Not deleting',
-                                   trans_id))
-            except KeyError:
+            if trans_id in tt._removed_contents:
+                orphans = []
+                empty = True
+                # Find the potential orphans, stop if one item should be kept
+                for c in tt.by_parent()[trans_id]:
+                    if tt.final_file_id(c) is None:
+                        orphans.append(c)
+                    else:
+                        empty = False
+                        break
+                if empty:
+                    # All children are orphans
+                    for o in orphans:
+                        tt.new_orphan(o, trans_id)
+                else:
+                    # Cancel the directory deletion
+                    tt.cancel_deletion(trans_id)
+                    new_conflicts.add(('deleting parent', 'Not deleting',
+                                       trans_id))
+            else:
                 create = True
                 try:
                     tt.final_name(trans_id)
 
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-10-15 14:16:12 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-10-15 14:16:12 +0000
@@ -171,6 +171,12 @@
 * ``unshelve --preview`` now can show diff in a non-ascii encoding.
   (Andrej A Antonov, #518916)
 
+* When a bzr command remove a previously versioned directory, all
+  unversioned files are moved to a 'bzr-orphans' directory at the working
+  tree root with backup names (<file>.~#~). This was previously creating
+  spurious conflicts during merge, pull or switch operations.
+  (Vincent Ladeuil, #323111)
+
 Documentation
 *************
 
 
=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-10-15 11:30:54 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-10-15 14:16:12 +0000
@@ -90,6 +90,7 @@
   ``bzr log -vp -r mainline:annotate:bzrlib/transform.py:500``
   (Aaron Bentley)
 
+<<<<<<< TREE
 Testing/Bug reporting
 *********************
 
@@ -98,6 +99,17 @@
   to bug reports. (Vincent Ladeuil)
 
 
+=======
+
+Improved conflict handling
+**************************
+
+* Deleting a versioned directory containing unversioned files will no
+  longer create a conflict. Instead, the unversioned files will be moved
+  into a 'bzr-orphans' directory at the root of the working tree.
+  (Vincent Ladeuil, #323111)
+
+>>>>>>> MERGE-SOURCE
 Documentation
 *************