Merge lp:~vila/bzr/323111-orphan-config-option into lp:bzr

[Vincent Ladeuil]

Provides a ``bzr.transform.orphan_policy`` option to control orphan handling when merging directory deletion.

This introduces a ``bzrlib.transform.orphan_policy`` configuration
variable that control whether orphans are created or not during a
tree transform and how.

The accepted values are:

- never: The previous behaviour, no orphans are ever created
  leaving the directory in a conflicted state that the user has
  to resolve.

- always: The new behaviour, unversioned files are moved into a
  ``bzr-orphans`` directory at the root of the working tree. The
  user is expected to look at the content and remove the
  bzr-orphans directory (the orphans will accumulate otherwise).

There is now an ``orphaning_registry`` in bzrlib.transform that
is used to centralise the various ways to manage orphans and as
the source of valid values for the config variable.

The default value (``always``) is also the fallback if a bogus
value is provided (and a warning emitted in this case).

I made the factory accept a simple callable as requiring a
TreeTransform method would require using a getattr (since we will
have to use the name in the registry to get the bound method
later) for no good reasons. This also leave more room for exotic
implementations (as in alien to TreeTransform, requiring more
house-keeping, etc).

[Martin Pool (mbp)]

You should add something to the user guide: aside from being needed, reading how we describe it to users will help us know if we've got it right. Starting from the text for whatsnew will be ok.

I think the always/never distinction is not the right dimension though, because it seems to assume there is one thing that we want to have either on or off. But I think what we want is, well, actually different policies, which might include:

 * conflict: mark the directory conflicted and don't remove it
 * unversion: mark the containing directory unversioned, but leave it in the tree
 * move: move them to a bzr-orphans directory in the root of the tree
 * delete: just immediately delete them

> I made the factory accept a simple callable as requiring a TreeTransform method

I think, generally, I'm against any interface taking a pure callable. (I was just regretting one I added myself in Launchpad feature flags.) So often it turns out that you also want to ask the thing passed in for a description of itself, or something like that. Passing an object with a well-known method name gives a bit more room to move.

I agree it shouldn't be bound to treetransform details.

188 + # FIXME: There is no tree config, so we use the branch one (it's weird
189 + # to define it this way as orphaning can only occur in a working tree,
190 + # but that's all we have (for now). It will find the option in
191 + # locations,conf or bazaar.conf though) -- vila 20100916
192 + conf = self._tree.branch.get_config()

Perhaps there should be a bug for that.

review: Needs Fixing

[Andrew Bennetts (spiv)]

Martin Pool wrote:
[...]
> I think the always/never distinction is not the right dimension though,
> because it seems to assume there is one thing that we want to have either on
> or off. But I think what we want is, well, actually different policies, which
> might include:
>
> * conflict: mark the directory conflicted and don't remove it
> * unversion: mark the containing directory unversioned, but leave it in the tree
> * move: move them to a bzr-orphans directory in the root of the tree
> * delete: just immediately delete them

This is a great way to put it, that's what I was thinking too.

For example, I'm imagining plugins adding support for:

orphans = vary_by_file_ext
orphan_policies = pyc:delete,*:move

(as a not very carefully designed strawman...)

Similarly, it would be lovely if support for 'precious' files could be done
initially by a plugin...

> > I made the factory accept a simple callable as requiring a TreeTransform method
>
> I think, generally, I'm against any interface taking a pure callable. (I was
> just regretting one I added myself in Launchpad feature flags.) So often it
> turns out that you also want to ask the thing passed in for a description of
> itself, or something like that. Passing an object with a well-known method
> name gives a bit more room to move.

See also our hook registration API, which asks for a description for its
callables :)

[Marius Kruger (amanica)]

On 17 September 2010 10:01, Andrew Bennetts For example, I'm imagining
plugins adding support for:
>
>
> orphans = vary_by_file_ext
> orphan_policies = pyc:delete,*:move
>

wouldn't it be more suitable to go in ~/.bazaar/rules then?

[Martin Pool (mbp)]

On 17 September 2010 18:01, Andrew Bennetts
<email address hidden> wrote:
> Martin Pool wrote:
> [...]
>> I think the always/never distinction is not the right dimension though,
>> because it seems to assume there is one thing that we want to have either on
>> or off.  But I think what we want is, well, actually different policies, which
>> might include:
>>
>>  * conflict: mark the directory conflicted and don't remove it
>>  * unversion: mark the containing directory unversioned, but leave it in the tree
>>  * move: move them to a bzr-orphans directory in the root of the tree
>>  * delete: just immediately delete them
>
> This is a great way to put it, that's what I was thinking too.
>
> For example, I'm imagining plugins adding support for:
>
> orphans = vary_by_file_ext
> orphan_policies = pyc:delete,*:move
>
> (as a not very carefully designed strawman...)

Mm, or better I'd like to see the rules file group them in to classes
of 'precious', 'junk', etc, then perhaps different orphan policies
depending on class.
--
Martin

[Vincent Ladeuil (vila)]

>>>>> Martin Pool <email address hidden> writes:

<snip/>

    > Mm, or better I'd like to see the rules file group them in to classes
    > of 'precious', 'junk', etc, then perhaps different orphan policies
    > depending on class.

That exactly what I feared, you're trying to fit precious/junk handling
into this bug fix which, while related, will not allow addressing the
whole problem.

I am *not* trying to handle precious files here not delete junk files,
I'm just putting unversioned file away when they block deleting a
directory.

That's only *one* aspect and people are suffering from this *today*.

[Vincent Ladeuil (vila)]

>>>>> Marius Kruger <email address hidden> writes:

    > On 17 September 2010 10:01, Andrew Bennetts For example, I'm imagining
    > plugins adding support for:
    >>
    >>
    >> orphans = vary_by_file_ext
    >> orphan_policies = pyc:delete,*:move
    >>

    > wouldn't it be more suitable to go in ~/.bazaar/rules then?

It would be even more suitable to mirror what is done for ignored files
(junk file, .bzrjunk files and so on) and modify all the commands that
should take care of that.

Then we can have:
- versioned files
- unversioned files
  - junk
  - the rest, which include precious files and maybe nothing else

But again, that vastly out-of-scope with this bugfix.

[Martin Pool (mbp)]

On 17 September 2010 22:37, Vincent Ladeuil <email address hidden> wrote:
>>>>>> Martin Pool <email address hidden> writes:
>
> <snip/>
>
>    > Mm, or better I'd like to see the rules file group them in to classes
>    > of 'precious', 'junk', etc, then perhaps different orphan policies
>    > depending on class.
>
> That exactly what I feared, you're trying to fit precious/junk handling
> into this bug fix which, while related, will not allow addressing the
> whole problem.
>
> I am *not* trying to handle precious files here not delete junk files,
> I'm just putting unversioned file away when they block deleting a
> directory.
>
> That's only *one* aspect and people are suffering from this *today*.

Sorry, I didn't mean to imply this is part of this bug, it was a
discussion of follow-on changes. I think you are quite right to
change just this before bringing in a junk category. However, I do
think you should change the option values.

--
Martin

[Vincent Ladeuil (vila)]

Forwarding to <email address hidden> to get a larger feedback.
>>>>> Martin Pool <email address hidden> writes:

    > On 17 September 2010 22:37, Vincent Ladeuil <email address hidden> wrote:
    >>>>>>> Martin Pool <email address hidden> writes:
    >>
    >> <snip/>
    >>
    >>    > Mm, or better I'd like to see the rules file group them in to classes
    >>    > of 'precious', 'junk', etc, then perhaps different orphan policies
    >>    > depending on class.
    >>
    >> That exactly what I feared, you're trying to fit precious/junk handling
    >> into this bug fix which, while related, will not allow addressing the
    >> whole problem.
    >>
    >> I am *not* trying to handle precious files here not delete junk files,
    >> I'm just putting unversioned file away when they block deleting a
    >> directory.
    >>
    >> That's only *one* aspect and people are suffering from this *today*.

    > Sorry, I didn't mean to imply this is part of this bug, it was a
    > discussion of follow-on changes.

Ha ok, right, I completely misunderstood then.

    > I think you are quite right to change just this before bringing in
    > a junk category. However, I do think you should change the option
    > values.

So, for this submission, and trying to take all applicable remarks into
account, I will:

- rename 'always' to 'move'
- rename 'never' to 'conflict'
- add 'bzr-orphans' to the default ignore list

- delay turning callables into full-fledged ones until the impact of the
  junk/repcious files handling make it clearer what this fix is really
  suppposed to provide (I kind of suspect it won't have to make a
  decision anymore if the unversioned files are handled higher in the
  stack).

and:

- make 'conflict' the default, so people impacted by this bug can turn
  it to 'move' and all the other will remained un-impacted until the
  follow-on changes are implemented properly.

For the follow-on changes, we need a bit more analysis to:

- check which commands should now take the junk files into account (at
  least pull/merge/switch should delete them even before the orphaning
  triggered, clean-tree should handle them appropriately, but they may
  be other. Thoughts ?)

- discuss a bit more what a 'precious' file is.

I'm not clear about what a precious but *unversioned* file is... either
it's really precious and it's versioned or it can be generated but I
dont quite see why it should be called precious then or... what ?

What kind of file should bzr never delete, yet, don't handle (or even
ignore) ? A project settings file ? But in this case such a file
shouldn't be in a directory that don't exist anymore for a given tree
revision no ?

         Vincent

[lp:~vila/bzr/323111-orphan-config-option]

5427. By Vincent Ladeuil on 2010-09-21

Merge deprecate-get-backup-name into orphan-config-option

5428. By Vincent Ladeuil on 2010-09-21

Move NEWS entry to 2.3b2

5429. By Vincent Ladeuil on 2010-09-21

Revert to 'conflict' being the default orphaning policy and fix fallouts.

5430. By Vincent Ladeuil on 2010-09-22

Add 'bzr-orphans' to USER_DEFAULTS in bzrlib/ignores.py.

[Martin Pool (mbp)]

On 22 September 2010 18:57, Vincent Ladeuil <email address hidden> wrote:

> What kind of file should bzr never delete, yet, don't handle (or even
> ignore) ? A project settings file ? But in this case such a file
> shouldn't be in a directory that don't exist anymore for a given tree
> revision no ?

Let's talk about it on the list?

I could guess:
 * project settings
 * build products that are super expensive to produce
 * per-tree configuration (similar things seem to come up a lot from
people running apps within a source tree)
 * for that matter the database produced by a running app

--
Martin

[Vincent Ladeuil (vila)]

> On 22 September 2010 18:57, Vincent Ladeuil <email address hidden> wrote:
>
> > What kind of file should bzr never delete, yet, don't handle (or even
> > ignore) ? A project settings file ? But in this case such a file
> > shouldn't be in a directory that don't exist anymore for a given tree
> > revision no ?
>
> Let's talk about it on the list?
>
> I could guess:
> * project settings
> * build products that are super expensive to produce
> * per-tree configuration (similar things seem to come up a lot from
> people running apps within a source tree)
> * for that matter the database produced by a running app

Clear enough for me, thanks. This hints at 'precious' being clearly: unversioned but not junk.

[Vincent Ladeuil (vila)]

Hmm, someone mentioned an interesting twist: when switching a tree from one branch to another, you want *some* precious files to follow and *some* other to stay:

<zyga_> vila, I just noticed bzr switch-pipe does not store local changes
<zyga_> vila, perhaps the version I'm using is not compatible with bzr (on 10.10)
<vila> zyga_: by store local changes you mean shelve ?
<vila> zyga_: keeping uncommitted changes in the working tree while switching is a feature, I often need to commit code I just wrote while not being in the "right" thread (or pipe)
<zyga> vila, no I mean bzr switch-pipeline
<zyga> vila, I found the issue
<zyga> vila, bzr-pipeline is _fantastic_
<zyga> vila, the issue was, untracked files are _not_ stored/restored by the pipeline
<zyga> vila, so if you add a new file and start hacking on it (but not bzr add it) it will not be hidden by bzr switch-pipeline
<vila> zyga: that's a bzr issue then, there are still unversioned files that you want to keep with you (even if there are unversioned files that you want to hide)
<vila> zyga: examples at https://code.edge.launchpad.net/~vila/bzr/323111-orphan-config-option/+merge/35690
<vila> zyga: comments or bugs with your use case welcome

[Martin Pool (mbp)]

On 4 October 2010 21:53, Vincent Ladeuil <email address hidden> wrote:
> Hmm, someone mentioned an interesting twist: when switching a tree from one branch to another, you want *some* precious files to follow and *some* other to stay:

Well, the obvious answer there is that he should add but not commit
them, and then they'll dtrt.

Perhaps user feedback comments would be better kept on a bug than
here... (but here's better than nowhere.)

--
Martin

[Martin Pool (mbp)]

> orhaning
typo

Perhaps there should be user docs for this?

The NEWS entry ought to explain what an orphan is

+orphaning_registry = registry.Registry()
+orphaning_registry.register('conflict', refuse_orphan,
+ 'Never create orphans.')
+orphaning_registry.register('move', move_orphan,
+ 'Move orphans into the bzr-orphans directory.')
+orphaning_registry._set_default_key('conflict')

I think in explaining 'conflict' it's not so much never create them - the transform situtation forces the existence of orphans. Rather, we'll leave them in place and mark the directory conflicted.

         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 = self._available_backup_name(actual_name, od_id)
- self.adjust_path(new_name, od_id, trans_id)
- trace.warning('%s has been orphaned in %s'
- % (joinpath(parent_path, actual_name), orphan_dir))
+ # FIXME: There is no tree config, so we use the branch one (it's weird
+ # to define it this way as orphaning can only occur in a working tree,
+ # but that's all we have (for now). It will find the option in
+ # locations,conf or bazaar.conf though) -- vila 20100916
+ conf = self._tree.branch.get_config()
+ conf_var_name = 'bzrlib.transform.orphan_policy'
+ orphan_policy = conf.get_user_option(conf_var_name)
+ default_policy = orphaning_registry.default_key
+ if orphan_policy is None:
+ orphan_policy = default_policy
+ if orphan_policy not in orphaning_registry:
+ trace.warning('%s (from %s) is not a known policy, defaulting to %s'
+ % (orphan_policy, conf_var_name, default_policy))
+ orphan_policy = default_policy
+ handle_orphan = orphaning_registry.get(orphan_policy)
+ handle_orphan(self, trans_id, parent_id)
+

You have a comma in 'locations,conf'.

Won't checking this here mean that you get one warning per orphaned file? Perhaps not necessarily a big deal but it suggests you should check this somewhere earlier, and then hold onto the handler?

otherwise I think this is ok. Perhaps we should file some followon bugs.

I might like a 'just kill them' policy.

[lp:~vila/bzr/323111-orphan-config-option]

5431. By Vincent Ladeuil on 2010-10-15

Merge deprecate-get-backup-name into orphan-config-option resolving conflicts

5432. By Vincent Ladeuil on 2010-10-15

Add more doc and fix rst typos

5433. By Vincent Ladeuil on 2010-10-15

Rename bzrlib.transform.orphan_policy to bzr.transform.orphan_policy.

[Vincent Ladeuil (vila)]

> Perhaps there should be user docs for this?

Added in bzrlib/help_topics/en/conflict-types.txt

>
> The NEWS entry ought to explain what an orphan is

Done

>
> +orphaning_registry = registry.Registry()
> +orphaning_registry.register('conflict', refuse_orphan,
> + 'Never create orphans.')
> +orphaning_registry.register('move', move_orphan,
> + 'Move orphans into the bzr-orphans directory.')
> +orphaning_registry._set_default_key('conflict')
>
> I think in explaining 'conflict' it's not so much never create them - the
> transform situtation forces the existence of orphans. Rather, we'll leave
> them in place and mark the directory conflicted.

Fixed.

> You have a comma in 'locations,conf'.

Fixed.

>
> Won't checking this here mean that you get one warning per orphaned file?

Yes, but only if the user set a wrong value so I think it's ok.

> Perhaps not necessarily a big deal but it suggests you should check this
> somewhere earlier, and then hold onto the handler?

That would be overkill just avoid multiple valid warnings that the user should fixed.

>
> otherwise I think this is ok. Perhaps we should file some followon bugs.
>

I think we have some for junk/precious handling no ?

> I might like a 'just kill them' policy.

That's what junk file handling should render useless.

I'll land with the fixes mentioned above.

[lp:~vila/bzr/323111-orphan-config-option]

5434. By Vincent Ladeuil on 2010-10-15

Better docs.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/help_topics/en/conflict-types.txt'
--- bzrlib/help_topics/en/conflict-types.txt	2010-04-28 13:15:14 +0000
+++ bzrlib/help_topics/en/conflict-types.txt	2010-10-15 14:58:56 +0000
@@ -26,13 +26,13 @@
 present the conflicting versions, and it's up to you to find the correct
 resolution.
 
-Whatever the conflict is, resolving it is roughly done in two steps::
-
- - modify the working tree content so that the conflicted item is now in the
-   state you want to keep,
-
- - inform Bazaar that the conflict is now solved and ask to cleanup any
-   remaining generated information (``bzr resolve <item>``).
+Whatever the conflict is, resolving it is roughly done in two steps:
+
+- modify the working tree content so that the conflicted item is now in the
+  state you want to keep,
+
+- inform Bazaar that the conflict is now solved and ask to cleanup any
+  remaining generated information (``bzr resolve <item>``).
 
 For most conflict types, there are some obvious ways to modify the working
 tree and put it into the desired state. For some types of conflicts, Bazaar
@@ -126,9 +126,9 @@
 
 ``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will issue ``bzr mv FILE.THIS FILE``,
- - ``--action=take-other`` will issue ``bzr mv FILE.OTHER FILE``,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=take-this`` will issue ``bzr mv FILE.THIS FILE``,
+* ``--action=take-other`` will issue ``bzr mv FILE.OTHER FILE``,
+* ``--action=done`` will just mark the conflict as resolved.
 
 Any action will also delete the previously generated ``.BASE``, ``.THIS`` and
 ``.OTHER`` files if they are still present in the working tree.
@@ -172,11 +172,11 @@
 To resolve that kind of conflict, you should rebuild FILE from either version
 or a combination of both.
 
-``bzr resolve`` recognizes the following actions::
+``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will issue ``bzr rm FILE ; bzr mv FILE.moved FILE``,
- - ``--action=take-other`` will issue ``bzr rm FILE.moved``,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=take-this`` will issue ``bzr rm FILE ; bzr mv FILE.moved FILE``,
+* ``--action=take-other`` will issue ``bzr rm FILE.moved``,
+* ``--action=done`` will just mark the conflict as resolved.
 
 Note that you must get rid of FILE.moved before using ``--action=done``.
 
@@ -213,13 +213,13 @@
 To resolve that kind of conflict, you should either remove or rename the
 children or the directory or a combination of both.
 
-``bzr resolve`` recognizes the following actions::
+``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will issue ``bzr rm directory`` including the
-   children,
- - ``--action=take-other`` will acknowledge Bazaar choice to keep the children
-   and restoring the directory,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=take-this`` will issue ``bzr rm directory`` including the
+  children,
+* ``--action=take-other`` will acknowledge Bazaar choice to keep the children
+  and restoring the directory,
+* ``--action=done`` will just mark the conflict as resolved.
 
 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
 
@@ -231,18 +231,37 @@
   Conflict: can't delete DIR because it is not empty.  Not deleting.
 
 This is the opposite of "missing parent".  A directory is deleted in the
-source, but has new children in the target.  Bazaar will retain the directory.
-Resolving this issue depends very much on the particular scenario.
+source, but has new children in the target (either because a directory
+deletion is merged or because the merge introduce new children).  Bazaar
+will retain the directory.  Resolving this issue depends very much on the
+particular scenario.
 
 To resolve that kind of conflict, you should either remove or rename the
 children or the directory or a combination of both.
 
-``bzr resolve`` recognizes the following actions::
-
- - ``--action=take-this`` will acknowledge Bazaar choice to keep the directory,
- - ``--action=take-other`` will issue ``bzr rm directory`` including the 
-   children,
- - ``--action=done`` will just mark the conflict as resolved.
+``bzr resolve`` recognizes the following actions:
+
+* ``--action=take-this`` will acknowledge Bazaar choice to keep the directory,
+
+* ``--action=take-other`` will issue ``bzr rm directory`` including the 
+  children,
+
+* ``--action=done`` will just mark the conflict as resolved.
+
+Note that when merging a directory deletion, if unversioned files are
+present, they become potential orphans has they don't have a directory
+parent anymore.
+
+Handling such orphans, *before* the conflict is created, is controlled by
+setting the ``bzr.transform.orphan_policy`` configuration option.
+
+There are two possible values for this option:
+
+* ``conflict`` (the default): will leave the orphans in place and
+  generate a conflicts,
+
+* ``move``: will move the orphans to a ``bzr-orphans`` directory at the root
+  of the working tree with names like ``<file>.~#~``.
 
 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
 
@@ -259,12 +278,12 @@
 To resolve that kind of conflict, you just have to decide what name should be
 retained for the file involved.
 
-``bzr resolve`` recognizes the following actions::
+``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will revert Bazaar choice and keep ``PATH1`` by
-   issuing ``bzr mv PATH2 PATH1``,
- - ``--action=take-other`` will acknowledge Bazaar choice of keeping ``PATH2``,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=take-this`` will revert Bazaar choice and keep ``PATH1`` by
+  issuing ``bzr mv PATH2 PATH1``,
+* ``--action=take-other`` will acknowledge Bazaar choice of keeping ``PATH2``,
+* ``--action=done`` will just mark the conflict as resolved.
 
 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
 
@@ -294,13 +313,13 @@
 ``black``.  To resolve that kind of conflict, you just have to decide what
 name should be retained for the directories involved.
 
-``bzr resolve`` recognizes the following actions::
+``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will acknowledge Bazaar choice of leaving ``white`` 
-   in ``black``,
- - ``--action=take-other`` will revert Bazaar choice and move ``black`` in
+* ``--action=take-this`` will acknowledge Bazaar choice of leaving ``white`` 
+  in ``black``,
+* ``--action=take-other`` will revert Bazaar choice and move ``black`` in
    ``white`` by issuing ``bzr mv black/white white ; bzr mv black white``,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=done`` will just mark the conflict as resolved.
 
 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
 
@@ -329,13 +348,13 @@
 To resolve that kind of conflict, you have to decide what name should be
 retained for the file, directory or symlink involved.
 
-``bzr resolve`` recognizes the following actions::
+``bzr resolve`` recognizes the following actions:
 
- - ``--action=take-this`` will issue ``bzr rm --force foo.new`` and 
-   ``bzr add foo``,
- - ``--action=take-other`` will issue ``bzr rm --force foo`` and 
-   ``bzr mv foo.new foo``,
- - ``--action=done`` will just mark the conflict as resolved.
+* ``--action=take-this`` will issue ``bzr rm --force foo.new`` and 
+  ``bzr add foo``,
+* ``--action=take-other`` will issue ``bzr rm --force foo`` and 
+  ``bzr mv foo.new foo``,
+* ``--action=done`` will just mark the conflict as resolved.
 
 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
 

=== modified file 'bzrlib/ignores.py'
--- bzrlib/ignores.py	2010-09-04 15:34:10 +0000
+++ bzrlib/ignores.py	2010-10-15 14:58:56 +0000
@@ -42,6 +42,7 @@
     '.#*',
     '[#]*#',
     '__pycache__',
+    'bzr-orphans',
 ]
 
 

=== modified file 'bzrlib/tests/per_workingtree/test_pull.py'
--- bzrlib/tests/per_workingtree/test_pull.py	2010-10-15 14:58:56 +0000
+++ bzrlib/tests/per_workingtree/test_pull.py	2010-10-15 14:58:56 +0000
@@ -91,6 +91,8 @@
                 'WorkingTreeFormat2 does not support missing parent conflicts')
         trunk = self.make_branch_deleting_dir('trunk')
         work = trunk.bzrdir.sprout('work', revision_id='2').open_workingtree()
+        work.branch.get_config().set_user_option(
+            'bzr.transform.orphan_policy', 'move')
         # Add some unversioned files in dir
         self.build_tree(['work/dir/foo',
                          'work/dir/subdir/',

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-10-15 14:58:56 +0000
+++ bzrlib/tests/test_bzrdir.py	2010-10-15 14:58:56 +0000
@@ -791,6 +791,9 @@
         self.build_tree(['tree1/subtree/file'])
         sub_tree.add('file')
         tree.commit('Initial commit')
+        # The following line force the orhaning to reveal bug #634470
+        tree.branch.get_config().set_user_option(
+            'bzr.transform.orphan_policy', 'move')
         tree.bzrdir.destroy_workingtree()
         # FIXME: subtree/.bzr is left here which allows the test to pass (or
         # fail :-( ) -- vila 20100909

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-10-15 14:58:56 +0000
+++ bzrlib/tests/test_transform.py	2010-10-15 14:58:56 +0000
@@ -29,6 +29,7 @@
     rules,
     symbol_versioning,
     tests,
+    trace,
     transform,
     urlutils,
     )
@@ -2363,7 +2364,7 @@
         # parent
         self.assertLength(2, conflicts)
 
-    def test_resolve_orphan_non_versioned_file(self):
+    def test_non_versioned_file_create_conflict(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')
@@ -2371,7 +2372,9 @@
         tt.unversion_file(dir_tid)
         conflicts = resolve_conflicts(tt)
         # no conflicts or rather: orphaning 'file' resolve the 'dir' conflict
-        self.assertLength(0, conflicts)
+        self.assertLength(1, conflicts)
+        self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
+                         conflicts.pop())
 
 
 A_ENTRY = ('a-id', ('a', 'a'), True, (True, True),
@@ -3258,35 +3261,79 @@
 
 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('.')
+    def _set_orphan_policy(self, wt, policy):
+        wt.branch.get_config().set_user_option('bzr.transform.orphan_policy',
+                                               policy)
+
+    def _prepare_orphan(self, wt):
         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')
+        orphan_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])
+        return tt, orphan_tid
+
+    def test_new_orphan_created(self):
+        wt = self.make_branch_and_tree('.')
+        self._set_orphan_policy(wt, 'move')
+        tt, orphan_tid = self._prepare_orphan(wt)
         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('foo.~1~', tt.final_name(orphan_tid))
         self.assertEquals('bzr-orphans',
-                          tt.final_name(tt.final_parent(foo_tid)))
+                          tt.final_name(tt.final_parent(orphan_tid)))
+
+    def test_never_orphan(self):
+        wt = self.make_branch_and_tree('.')
+        self._set_orphan_policy(wt, 'conflict')
+        tt, orphan_tid = self._prepare_orphan(wt)
+        remaining_conflicts = resolve_conflicts(tt)
+        self.assertLength(1, remaining_conflicts)
+        self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
+                         remaining_conflicts.pop())
+
+    def test_orphan_error(self):
+        def bogus_orphan(tt, orphan_id, parent_id):
+            raise transform.OrphaningError(tt.final_name(orphan_id),
+                                           tt.final_name(parent_id))
+        transform.orphaning_registry.register('bogus', bogus_orphan,
+                                              'Raise an error when orphaning')
+        wt = self.make_branch_and_tree('.')
+        self._set_orphan_policy(wt, 'bogus')
+        tt, orphan_tid = self._prepare_orphan(wt)
+        remaining_conflicts = resolve_conflicts(tt)
+        self.assertLength(1, remaining_conflicts)
+        self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
+                         remaining_conflicts.pop())
+
+    def test_unknown_orphan_policy(self):
+        wt = self.make_branch_and_tree('.')
+        # Set a fictional policy nobody ever implemented
+        self._set_orphan_policy(wt, 'donttouchmypreciouuus')
+        tt, orphan_tid = self._prepare_orphan(wt)
+        warnings = []
+        def warning(*args):
+            warnings.append(args[0] % args[1:])
+        self.overrideAttr(trace, 'warning', warning)
+        remaining_conflicts = resolve_conflicts(tt)
+        # We fallback to the default policy which create a conflict
+        self.assertLength(1, remaining_conflicts)
+        self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
+                         remaining_conflicts.pop())
+        self.assertLength(1, warnings)
+        self.assertStartsWith(warnings[0], 'donttouchmypreciouuus')

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-10-15 14:58:56 +0000
+++ bzrlib/transform.py	2010-10-15 14:58:56 +0000
@@ -22,6 +22,7 @@
 from bzrlib import (
     errors,
     lazy_import,
+    registry,
     )
 lazy_import.lazy_import(globals(), """
 from bzrlib import (
@@ -75,6 +76,7 @@
     map[key] = value
 
 
+
 class _TransformResults(object):
     def __init__(self, modified_paths, rename_count):
         object.__init__(self)
@@ -797,6 +799,32 @@
         """
         raise NotImplementedError(self.new_orphan)
 
+    def _get_potential_orphans(self, dir_id):
+        """Find the potential orphans in a directory.
+
+        A directory can't be safely deleted if there are versioned files in it.
+        If all the contained files are unversioned then they can be orphaned.
+
+        The 'None' return value means that the directory contains at least one
+        versioned file and should not be deleted.
+
+        :param dir_id: The directory trans id.
+
+        :return: A list of the orphan trans ids or None if at least one
+             versioned file is present.
+        """
+        orphans = []
+        # Find the potential orphans, stop if one item should be kept
+        for c in self.by_parent()[dir_id]:
+            if self.final_file_id(c) is None:
+                orphans.append(c)
+            else:
+                # We have a versioned file here, searching for orphans is
+                # meaningless.
+                orphans = None
+                break
+        return orphans
+
     def _affected_ids(self):
         """Return the set of transform ids affected by the transform"""
         trans_ids = set(self._removed_id)
@@ -1310,19 +1338,87 @@
         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 = self._available_backup_name(actual_name, od_id)
-        self.adjust_path(new_name, od_id, trans_id)
-        trace.warning('%s has been orphaned in %s'
-                      % (joinpath(parent_path, actual_name), orphan_dir))
+        # FIXME: There is no tree config, so we use the branch one (it's weird
+        # to define it this way as orphaning can only occur in a working tree,
+        # but that's all we have (for now). It will find the option in
+        # locations.conf or bazaar.conf though) -- vila 20100916
+        conf = self._tree.branch.get_config()
+        conf_var_name = 'bzr.transform.orphan_policy'
+        orphan_policy = conf.get_user_option(conf_var_name)
+        default_policy = orphaning_registry.default_key
+        if orphan_policy is None:
+            orphan_policy = default_policy
+        if orphan_policy not in orphaning_registry:
+            trace.warning('%s (from %s) is not a known policy, defaulting to %s'
+                          % (orphan_policy, conf_var_name, default_policy))
+            orphan_policy = default_policy
+        handle_orphan = orphaning_registry.get(orphan_policy)
+        handle_orphan(self, trans_id, parent_id)
+
+
+class OrphaningError(errors.BzrError):
+
+    # Only bugs could lead to such exception being seen by the user
+    internal_error = True
+    _fmt = "Error while orphaning %s in %s directory"
+
+    def __init__(self, orphan, parent):
+        errors.BzrError.__init__(self)
+        self.orphan = orphan
+        self.parent = parent
+
+
+class OrphaningForbidden(OrphaningError):
+
+    _fmt = "Policy: %s doesn't allow creating orphans."
+
+    def __init__(self, policy):
+        errors.BzrError.__init__(self)
+        self.policy = policy
+
+
+def move_orphan(tt, orphan_id, parent_id):
+    """See TreeTransformBase.new_orphan.
+
+    This creates a new orphan in the `bzr-orphans` dir at the root of the
+    `TreeTransform`.
+
+    :param tt: The TreeTransform orphaning `trans_id`.
+
+    :param orphan_id: The trans id that should be orphaned.
+
+    :param parent_id: The orphan parent trans id.
+    """
+    # Add the orphan dir if it doesn't exist
+    orphan_dir_basename = 'bzr-orphans'
+    od_id = tt.trans_id_tree_path(orphan_dir_basename)
+    if tt.final_kind(od_id) is None:
+        tt.create_directory(od_id)
+    parent_path = tt._tree_id_paths[parent_id]
+    # Find a name that doesn't exist yet in the orphan dir
+    actual_name = tt.final_name(orphan_id)
+    new_name = tt._available_backup_name(actual_name, od_id)
+    tt.adjust_path(new_name, od_id, orphan_id)
+    trace.warning('%s has been orphaned in %s'
+                  % (joinpath(parent_path, actual_name), orphan_dir_basename))
+
+
+def refuse_orphan(tt, orphan_id, parent_id):
+    """See TreeTransformBase.new_orphan.
+
+    This refuses to create orphan, letting the caller handle the conflict.
+    """
+    raise OrphaningForbidden('never')
+
+
+orphaning_registry = registry.Registry()
+orphaning_registry.register(
+    'conflict', refuse_orphan,
+    'Leave orphans in place and create a conflict on the directory.')
+orphaning_registry.register(
+    'move', move_orphan,
+    'Move orphans into the bzr-orphans directory.')
+orphaning_registry._set_default_key('conflict')
 
 
 class TreeTransform(DiskTreeTransform):
@@ -2859,20 +2955,24 @@
         elif c_type == 'missing parent':
             trans_id = conflict[1]
             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:
+                cancel_deletion = True
+                orphans = tt._get_potential_orphans(trans_id)
+                if orphans:
+                    cancel_deletion = False
                     # All children are orphans
                     for o in orphans:
-                        tt.new_orphan(o, trans_id)
-                else:
+                        try:
+                            tt.new_orphan(o, trans_id)
+                        except OrphaningError:
+                            # Something bad happened so we cancel the directory
+                            # deletion which will leave it in place with a
+                            # conflict. The user can deal with it from there.
+                            # Note that this also catch the case where we don't
+                            # want to create orphans and leave the directory in
+                            # place.
+                            cancel_deletion = True
+                            break
+                if cancel_deletion:
                     # Cancel the directory deletion
                     tt.cancel_deletion(trans_id)
                     new_conflicts.add(('deleting parent', 'Not deleting',

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-10-15 14:58:56 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-10-15 14:58:56 +0000
@@ -175,11 +175,16 @@
 * ``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)
+* Deleting a versioned directory can leave orphans: unversioned files that
+  were present don't have a parent anymore. The
+  ``bzr.transform.orphan_policy`` configuration option controls the ``bzr``
+  behaviour: ``conflict`` (the default) leave the orphans in place and
+  create a conflict for the directory, ``move`` create orphans named
+  ``<file>.~#~`` in a ``bzr-orphans`` directory at the root of the working
+  tree.  (Vincent Ladeuil, #323111)
+
+Improvements
+************
 
 Documentation
 *************
@@ -187,6 +192,9 @@
 * Correct the documentation for setting up the smart server with Apache.
   (Neil Martinsen-Burrell, #311518)
 
+* Fix rst typos in bzrlib/help_topics/en/conflict-types.txt.
+  (Vincent Ladeuil, #660943)
+
 * Provide more detailed help on the Launchpad plugin at "bzr help
   plugins/launchpad".  (Neil Martinsen-Burrell, #589379)
 

=== 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 14:58:56 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-10-15 14:58:56 +0000
@@ -104,10 +104,15 @@
 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)
+* ``pull``, ``merge`` or ``switch`` can lead to conflicts when deleting a
+  versioned directory contains unversioned files. The cause of the conflict
+  is that deleting the directory will orphan the unversioned files so the
+  user needs to instruct ``bzr`` what do to do about these orpahns. This is
+  controlled by setting the ``bzr.transform.orphan_policy`` configuration
+  variable with a value of ``move``. In this case the unversioned files are
+  moved to a ``bzr-orphans`` directory at the root of the working tree. The
+  default behaviour is specified (if needed) by setting the variable to
+  ``conflict``.  (Vincent Ladeuil, #323111)
 
 >>>>>>> MERGE-SOURCE
 Documentation