Bazaar Version Control System

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

Proposed by Vincent Ladeuil on 2010-09-10
Status: Merged
Merged at revision: 5504
Proposed branch: lp:~vila/bzr/323111-orphan-non-versioned-files
Merge into: lp:bzr
Prerequisite: lp:~vila/bzr/323111-backup-names
Diff against target: 367 lines (+200/-12) 7 files modified (has conflicts)
Text conflict in doc/en/whats-new/whats-new-in-2.3.txt
To merge this branch: bzr merge lp:~vila/bzr/323111-orphan-non-versioned-files
Reviewer Review Type Date Requested Status
Andrew Bennetts 2010-09-10 Needs Information on 2010-09-16
Review via email: mp+35110@code.launchpad.net

Description of the Change

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.

To post a comment you must log in.
Vincent Ladeuil (vila) wrote :
Download full text (5.1 KiB)

>>>>> 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 ...

Read more...

5422. By Vincent Ladeuil on 2010-09-10

Take jam's review comments into account.

* doc/en/whats-new/whats-new-in-2.3.txt:
Add entry.

* bzrlib/transform.py:
(DiskTreeTransform.new_orphan): Urgh, call a valid backup name
prediction function. Add a trace.warning call too.

* bzrlib/tests/test_transform.py:
(TestTransformMissingParent): Use make_ not get_ for helper.
(TestOrphan.test_new_orphan): Check that the orphan is moved in te
right place.

Vincent Ladeuil (vila) wrote :

>>>>> 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) wrote :

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) wrote :

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) wrote :

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) wrote :

> 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.

5423. By Vincent Ladeuil on 2010-09-16

Merge backup-names into orphan-non-versioned-files

Vincent Ladeuil (vila) wrote :
Download full text (4.1 KiB)

> 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 ...

Read more...

Marius Kruger (amanica) wrote :

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) 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'.

--
Martin

Andrew Bennetts (spiv) wrote :

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) 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.

--
Martin

Andrew Bennetts (spiv) wrote :

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) wrote :

>>>>> 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 :)

5424. By Vincent Ladeuil on 2010-09-21

Merge backup-names into orphan-non-versioned-files

5425. By Vincent Ladeuil on 2010-09-21

Move NEWS entry to 2.3b2

Vincent Ladeuil (vila) wrote :

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

5426. By Vincent Ladeuil on 2010-10-15

Merge backup-names into orphan-non-versioned-files resolving conflicts

Preview Diff

1=== modified file 'bzrlib/bzrdir.py'
2--- bzrlib/bzrdir.py 2010-10-15 14:16:12 +0000
3+++ bzrlib/bzrdir.py 2010-10-15 14:16:12 +0000
4@@ -1294,7 +1294,10 @@
5 wt = self.open_workingtree(recommend_upgrade=False)
6 repository = wt.branch.repository
7 empty = repository.revision_tree(_mod_revision.NULL_REVISION)
8- wt.revert(old_tree=empty)
9+ # We ignore the conflicts returned by wt.revert since we're about to
10+ # delete the wt metadata anyway, all that should be left here are
11+ # detritus. But see bug #634470 about subtree .bzr dirs.
12+ conflicts = wt.revert(old_tree=empty)
13 self.destroy_workingtree_metadata()
14
15 def destroy_workingtree_metadata(self):
16
17=== modified file 'bzrlib/tests/per_workingtree/test_pull.py'
18--- bzrlib/tests/per_workingtree/test_pull.py 2010-09-06 10:08:36 +0000
19+++ bzrlib/tests/per_workingtree/test_pull.py 2010-10-15 14:16:12 +0000
20@@ -17,6 +17,7 @@
21
22 from cStringIO import StringIO
23
24+from bzrlib import tests
25 from bzrlib.tests import per_workingtree
26
27
28@@ -62,3 +63,39 @@
29 tree.commit('second')
30 to_tree.pull(tree.branch)
31 self.assertEqual('second_root_id', to_tree.get_root_id())
32+
33+
34+class TestPullWithOrphans(per_workingtree.TestCaseWithWorkingTree):
35+
36+ def make_branch_deleting_dir(self, relpath=None):
37+ if relpath is None:
38+ relpath = 'trunk'
39+ builder = self.make_branch_builder(relpath)
40+ builder.start_series()
41+
42+ # Create an empty trunk
43+ builder.build_snapshot('1', None, [
44+ ('add', ('', 'root-id', 'directory', ''))])
45+ builder.build_snapshot('2', ['1'], [
46+ ('add', ('dir', 'dir-id', 'directory', '')),
47+ ('add', ('file', 'file-id', 'file', 'trunk content\n')),])
48+ builder.build_snapshot('3', ['2'], [
49+ ('unversion', 'dir-id'),])
50+ builder.finish_series()
51+ return builder.get_branch()
52+
53+ def test_pull_orphans(self):
54+ from bzrlib import workingtree
55+ if isinstance(self.workingtree_format, workingtree.WorkingTreeFormat2):
56+ raise tests.TestSkipped(
57+ 'WorkingTreeFormat2 does not support missing parent conflicts')
58+ trunk = self.make_branch_deleting_dir('trunk')
59+ work = trunk.bzrdir.sprout('work', revision_id='2').open_workingtree()
60+ # Add some unversioned files in dir
61+ self.build_tree(['work/dir/foo',
62+ 'work/dir/subdir/',
63+ 'work/dir/subdir/foo'])
64+ work.pull(trunk)
65+ self.assertLength(0, work.conflicts())
66+ # The directory removal should succeed
67+ self.failIfExists('work/dir')
68
69=== modified file 'bzrlib/tests/test_bzrdir.py'
70--- bzrlib/tests/test_bzrdir.py 2010-10-15 14:16:12 +0000
71+++ bzrlib/tests/test_bzrdir.py 2010-10-15 14:16:12 +0000
72@@ -792,12 +792,23 @@
73 sub_tree.add('file')
74 tree.commit('Initial commit')
75 tree.bzrdir.destroy_workingtree()
76+ # FIXME: subtree/.bzr is left here which allows the test to pass (or
77+ # fail :-( ) -- vila 20100909
78 repo = self.make_repository('repo', shared=True,
79 format='dirstate-with-subtree')
80 repo.set_make_working_trees(False)
81- tree.bzrdir.sprout('repo/tree2')
82- self.failUnlessExists('repo/tree2/subtree')
83- self.failIfExists('repo/tree2/subtree/file')
84+ # FIXME: we just deleted the workingtree and now we want to use it ????
85+ # At a minimum, we should use tree.branch below (but this fails too
86+ # currently) or stop calling this test 'treeless'. Specifically, I've
87+ # turn the line below into an assertRaises when 'subtree/.bzr' is
88+ # orphaned and sprout tries to access the branch there (which is left
89+ # by bzrdir.BzrDirMeta1.destroy_workingtree when it ignores the
90+ # [DeletingParent('Not deleting', u'subtree', None)] conflict). See bug
91+ # #634470. -- vila 20100909
92+ self.assertRaises(errors.NotBranchError,
93+ tree.bzrdir.sprout, 'repo/tree2')
94+# self.failUnlessExists('repo/tree2/subtree')
95+# self.failIfExists('repo/tree2/subtree/file')
96
97 def make_foo_bar_baz(self):
98 foo = bzrdir.BzrDir.create_branch_convenience('foo').bzrdir
99
100=== modified file 'bzrlib/tests/test_transform.py'
101--- bzrlib/tests/test_transform.py 2010-10-15 14:16:12 +0000
102+++ bzrlib/tests/test_transform.py 2010-10-15 14:16:12 +0000
103@@ -28,6 +28,7 @@
104 revision as _mod_revision,
105 rules,
106 tests,
107+ transform,
108 urlutils,
109 )
110 from bzrlib.bzrdir import BzrDir
111@@ -2347,6 +2348,39 @@
112 self.failUnlessExists('a/b')
113
114
115+class TestTransformMissingParent(tests.TestCaseWithTransport):
116+
117+ def make_tt_with_versioned_dir(self):
118+ wt = self.make_branch_and_tree('.')
119+ self.build_tree(['dir/',])
120+ wt.add(['dir'], ['dir-id'])
121+ wt.commit('Create dir')
122+ tt = TreeTransform(wt)
123+ self.addCleanup(tt.finalize)
124+ return wt, tt
125+
126+ def test_resolve_create_parent_for_versioned_file(self):
127+ wt, tt = self.make_tt_with_versioned_dir()
128+ dir_tid = tt.trans_id_tree_file_id('dir-id')
129+ file_tid = tt.new_file('file', dir_tid, 'Contents', file_id='file-id')
130+ tt.delete_contents(dir_tid)
131+ tt.unversion_file(dir_tid)
132+ conflicts = resolve_conflicts(tt)
133+ # one conflict for the missing directory, one for the unversioned
134+ # parent
135+ self.assertLength(2, conflicts)
136+
137+ def test_resolve_orphan_non_versioned_file(self):
138+ wt, tt = self.make_tt_with_versioned_dir()
139+ dir_tid = tt.trans_id_tree_file_id('dir-id')
140+ tt.new_file('file', dir_tid, 'Contents')
141+ tt.delete_contents(dir_tid)
142+ tt.unversion_file(dir_tid)
143+ conflicts = resolve_conflicts(tt)
144+ # no conflicts or rather: orphaning 'file' resolve the 'dir' conflict
145+ self.assertLength(0, conflicts)
146+
147+
148 A_ENTRY = ('a-id', ('a', 'a'), True, (True, True),
149 ('TREE_ROOT', 'TREE_ROOT'), ('a', 'a'), ('file', 'file'),
150 (False, False))
151@@ -3227,3 +3261,39 @@
152 trans_id = tt.trans_id_tree_path('file')
153 self.assertEqual((LINES_ONE,),
154 tt._get_parents_texts(trans_id))
155+
156+
157+class TestOrphan(tests.TestCaseWithTransport):
158+
159+ # Alternative implementations may want to test:
160+ # - can't create orphan dir
161+ # - orphaning forbidden
162+ # - can't create orphan
163+
164+ def test_no_orphan_for_transform_preview(self):
165+ tree = self.make_branch_and_tree('tree')
166+ tt = transform.TransformPreview(tree)
167+ self.addCleanup(tt.finalize)
168+ self.assertRaises(NotImplementedError, tt.new_orphan, 'foo', 'bar')
169+
170+ def test_new_orphan(self):
171+ wt = self.make_branch_and_tree('.')
172+ self.build_tree(['dir/', 'dir/foo'])
173+ wt.add(['dir'], ['dir-id'])
174+ wt.commit('add dir')
175+ tt = transform.TreeTransform(wt)
176+ self.addCleanup(tt.finalize)
177+ dir_tid = tt.trans_id_tree_path('dir')
178+ foo_tid = tt.trans_id_tree_path('dir/foo')
179+ tt.delete_contents(dir_tid)
180+ tt.unversion_file(dir_tid)
181+ raw_conflicts = tt.find_conflicts()
182+ self.assertLength(1, raw_conflicts)
183+ self.assertEqual(('missing parent', 'new-1'), raw_conflicts[0])
184+ remaining_conflicts = resolve_conflicts(tt)
185+ # Yeah for resolved conflicts !
186+ self.assertLength(0, remaining_conflicts)
187+ # We have a new orphan
188+ self.assertEquals('foo.~1~', tt.final_name(foo_tid))
189+ self.assertEquals('bzr-orphans',
190+ tt.final_name(tt.final_parent(foo_tid)))
191
192=== modified file 'bzrlib/transform.py'
193--- bzrlib/transform.py 2010-10-15 14:16:12 +0000
194+++ bzrlib/transform.py 2010-10-15 14:16:12 +0000
195@@ -19,8 +19,11 @@
196 from stat import S_ISREG, S_IEXEC
197 import time
198
199-from bzrlib.lazy_import import lazy_import
200-lazy_import(globals(), """
201+from bzrlib import (
202+ errors,
203+ lazy_import,
204+ )
205+lazy_import.lazy_import(globals(), """
206 from bzrlib import (
207 annotate,
208 bencode,
209@@ -32,6 +35,7 @@
210 multiparent,
211 osutils,
212 revision as _mod_revision,
213+ trace,
214 ui,
215 )
216 """)
217@@ -64,7 +68,6 @@
218
219 ROOT_PARENT = "root-parent"
220
221-
222 def unique_add(map, key, value):
223 if key in map:
224 raise DuplicateKey(key=key)
225@@ -759,6 +762,17 @@
226 self.create_symlink(target, trans_id)
227 return trans_id
228
229+ def new_orphan(self, trans_id, parent_id):
230+ """Schedule an item to be orphaned.
231+
232+ When a directory is about to be removed, its children, if they are not
233+ versioned are moved out of the way: they don't have a parent anymore.
234+
235+ :param trans_id: The trans_id of the existing item.
236+ :param parent_id: The parent trans_id of the item.
237+ """
238+ raise NotImplementedError(self.new_orphan)
239+
240 def _affected_ids(self):
241 """Return the set of transform ids affected by the transform"""
242 trans_ids = set(self._removed_id)
243@@ -1271,6 +1285,21 @@
244 del self._limbo_children_names[trans_id]
245 delete_any(self._limbo_name(trans_id))
246
247+ def new_orphan(self, trans_id, parent_id):
248+ """See TreeTransformBase.new_orphan."""
249+ # Add the orphan dir if it doesn't exist
250+ orphan_dir = 'bzr-orphans'
251+ od_id = self.trans_id_tree_path(orphan_dir)
252+ if self.final_kind(od_id) is None:
253+ self.create_directory(od_id)
254+ parent_path = self._tree_id_paths[parent_id]
255+ # Find a name that doesn't exist yet in the orphan dir
256+ actual_name = self.final_name(trans_id)
257+ new_name = _get_backup_name(actual_name, self.by_parent(), od_id, self)
258+ self.adjust_path(new_name, od_id, trans_id)
259+ trace.warning('%s has been orphaned in %s'
260+ % (joinpath(parent_path, actual_name), orphan_dir))
261+
262
263 class TreeTransform(DiskTreeTransform):
264 """Represent a tree transformation.
265@@ -1727,6 +1756,9 @@
266 childpath = joinpath(path, child)
267 yield self.trans_id_tree_path(childpath)
268
269+ def new_orphan(self, trans_id, parent_id):
270+ raise NotImplementedError(self.new_orphan)
271+
272
273 class _PreviewTree(tree.Tree):
274 """Partial implementation of Tree to support show_diff_trees"""
275@@ -2427,12 +2459,14 @@
276 for child in tt.iter_tree_children(old_parent):
277 tt.adjust_path(tt.final_name(child), new_parent, child)
278
279+
280 def _reparent_transform_children(tt, old_parent, new_parent):
281 by_parent = tt.by_parent()
282 for child in by_parent[old_parent]:
283 tt.adjust_path(tt.final_name(child), new_parent, child)
284 return by_parent[old_parent]
285
286+
287 def _content_match(tree, entry, file_id, kind, target_path):
288 if entry.kind != kind:
289 return False
290@@ -2799,11 +2833,26 @@
291
292 elif c_type == 'missing parent':
293 trans_id = conflict[1]
294- try:
295- tt.cancel_deletion(trans_id)
296- new_conflicts.add(('deleting parent', 'Not deleting',
297- trans_id))
298- except KeyError:
299+ if trans_id in tt._removed_contents:
300+ orphans = []
301+ empty = True
302+ # Find the potential orphans, stop if one item should be kept
303+ for c in tt.by_parent()[trans_id]:
304+ if tt.final_file_id(c) is None:
305+ orphans.append(c)
306+ else:
307+ empty = False
308+ break
309+ if empty:
310+ # All children are orphans
311+ for o in orphans:
312+ tt.new_orphan(o, trans_id)
313+ else:
314+ # Cancel the directory deletion
315+ tt.cancel_deletion(trans_id)
316+ new_conflicts.add(('deleting parent', 'Not deleting',
317+ trans_id))
318+ else:
319 create = True
320 try:
321 tt.final_name(trans_id)
322
323=== modified file 'doc/en/release-notes/bzr-2.3.txt'
324--- doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:16:12 +0000
325+++ doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:16:12 +0000
326@@ -171,6 +171,12 @@
327 * ``unshelve --preview`` now can show diff in a non-ascii encoding.
328 (Andrej A Antonov, #518916)
329
330+* When a bzr command remove a previously versioned directory, all
331+ unversioned files are moved to a 'bzr-orphans' directory at the working
332+ tree root with backup names (<file>.~#~). This was previously creating
333+ spurious conflicts during merge, pull or switch operations.
334+ (Vincent Ladeuil, #323111)
335+
336 Documentation
337 *************
338
339
340=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
341--- doc/en/whats-new/whats-new-in-2.3.txt 2010-10-15 11:30:54 +0000
342+++ doc/en/whats-new/whats-new-in-2.3.txt 2010-10-15 14:16:12 +0000
343@@ -90,6 +90,7 @@
344 ``bzr log -vp -r mainline:annotate:bzrlib/transform.py:500``
345 (Aaron Bentley)
346
347+<<<<<<< TREE
348 Testing/Bug reporting
349 *********************
350
351@@ -98,6 +99,17 @@
352 to bug reports. (Vincent Ladeuil)
353
354
355+=======
356+
357+Improved conflict handling
358+**************************
359+
360+* Deleting a versioned directory containing unversioned files will no
361+ longer create a conflict. Instead, the unversioned files will be moved
362+ into a 'bzr-orphans' directory at the root of the working tree.
363+ (Vincent Ladeuil, #323111)
364+
365+>>>>>>> MERGE-SOURCE
366 Documentation
367 *************
368