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

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5504
Proposed branch: lp:~vila/bzr/323111-orphan-config-option
Merge into: lp:bzr
Prerequisite: lp:~vila/bzr/323111-deprecate-get-backup-name
Diff against target: 588 lines (+275/-90) (has conflicts)
8 files modified
bzrlib/help_topics/en/conflict-types.txt (+63/-44)
bzrlib/ignores.py (+1/-0)
bzrlib/tests/per_workingtree/test_pull.py (+2/-0)
bzrlib/tests/test_bzrdir.py (+3/-0)
bzrlib/tests/test_transform.py (+59/-12)
bzrlib/transform.py (+125/-25)
doc/en/release-notes/bzr-2.3.txt (+13/-5)
doc/en/whats-new/whats-new-in-2.3.txt (+9/-4)
Text conflict in doc/en/whats-new/whats-new-in-2.3.txt
To merge this branch: bzr merge lp:~vila/bzr/323111-orphan-config-option
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+35690@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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
Revision history for this message
Andrew Bennetts (spiv) 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...)

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

Revision history for this message
Marius Kruger (amanica) wrote :

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?

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) 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*.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) 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:

<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

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/help_topics/en/conflict-types.txt'
2--- bzrlib/help_topics/en/conflict-types.txt 2010-04-28 13:15:14 +0000
3+++ bzrlib/help_topics/en/conflict-types.txt 2010-10-15 14:58:56 +0000
4@@ -26,13 +26,13 @@
5 present the conflicting versions, and it's up to you to find the correct
6 resolution.
7
8-Whatever the conflict is, resolving it is roughly done in two steps::
9-
10- - modify the working tree content so that the conflicted item is now in the
11- state you want to keep,
12-
13- - inform Bazaar that the conflict is now solved and ask to cleanup any
14- remaining generated information (``bzr resolve <item>``).
15+Whatever the conflict is, resolving it is roughly done in two steps:
16+
17+- modify the working tree content so that the conflicted item is now in the
18+ state you want to keep,
19+
20+- inform Bazaar that the conflict is now solved and ask to cleanup any
21+ remaining generated information (``bzr resolve <item>``).
22
23 For most conflict types, there are some obvious ways to modify the working
24 tree and put it into the desired state. For some types of conflicts, Bazaar
25@@ -126,9 +126,9 @@
26
27 ``bzr resolve`` recognizes the following actions:
28
29- - ``--action=take-this`` will issue ``bzr mv FILE.THIS FILE``,
30- - ``--action=take-other`` will issue ``bzr mv FILE.OTHER FILE``,
31- - ``--action=done`` will just mark the conflict as resolved.
32+* ``--action=take-this`` will issue ``bzr mv FILE.THIS FILE``,
33+* ``--action=take-other`` will issue ``bzr mv FILE.OTHER FILE``,
34+* ``--action=done`` will just mark the conflict as resolved.
35
36 Any action will also delete the previously generated ``.BASE``, ``.THIS`` and
37 ``.OTHER`` files if they are still present in the working tree.
38@@ -172,11 +172,11 @@
39 To resolve that kind of conflict, you should rebuild FILE from either version
40 or a combination of both.
41
42-``bzr resolve`` recognizes the following actions::
43+``bzr resolve`` recognizes the following actions:
44
45- - ``--action=take-this`` will issue ``bzr rm FILE ; bzr mv FILE.moved FILE``,
46- - ``--action=take-other`` will issue ``bzr rm FILE.moved``,
47- - ``--action=done`` will just mark the conflict as resolved.
48+* ``--action=take-this`` will issue ``bzr rm FILE ; bzr mv FILE.moved FILE``,
49+* ``--action=take-other`` will issue ``bzr rm FILE.moved``,
50+* ``--action=done`` will just mark the conflict as resolved.
51
52 Note that you must get rid of FILE.moved before using ``--action=done``.
53
54@@ -213,13 +213,13 @@
55 To resolve that kind of conflict, you should either remove or rename the
56 children or the directory or a combination of both.
57
58-``bzr resolve`` recognizes the following actions::
59+``bzr resolve`` recognizes the following actions:
60
61- - ``--action=take-this`` will issue ``bzr rm directory`` including the
62- children,
63- - ``--action=take-other`` will acknowledge Bazaar choice to keep the children
64- and restoring the directory,
65- - ``--action=done`` will just mark the conflict as resolved.
66+* ``--action=take-this`` will issue ``bzr rm directory`` including the
67+ children,
68+* ``--action=take-other`` will acknowledge Bazaar choice to keep the children
69+ and restoring the directory,
70+* ``--action=done`` will just mark the conflict as resolved.
71
72 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
73
74@@ -231,18 +231,37 @@
75 Conflict: can't delete DIR because it is not empty. Not deleting.
76
77 This is the opposite of "missing parent". A directory is deleted in the
78-source, but has new children in the target. Bazaar will retain the directory.
79-Resolving this issue depends very much on the particular scenario.
80+source, but has new children in the target (either because a directory
81+deletion is merged or because the merge introduce new children). Bazaar
82+will retain the directory. Resolving this issue depends very much on the
83+particular scenario.
84
85 To resolve that kind of conflict, you should either remove or rename the
86 children or the directory or a combination of both.
87
88-``bzr resolve`` recognizes the following actions::
89-
90- - ``--action=take-this`` will acknowledge Bazaar choice to keep the directory,
91- - ``--action=take-other`` will issue ``bzr rm directory`` including the
92- children,
93- - ``--action=done`` will just mark the conflict as resolved.
94+``bzr resolve`` recognizes the following actions:
95+
96+* ``--action=take-this`` will acknowledge Bazaar choice to keep the directory,
97+
98+* ``--action=take-other`` will issue ``bzr rm directory`` including the
99+ children,
100+
101+* ``--action=done`` will just mark the conflict as resolved.
102+
103+Note that when merging a directory deletion, if unversioned files are
104+present, they become potential orphans has they don't have a directory
105+parent anymore.
106+
107+Handling such orphans, *before* the conflict is created, is controlled by
108+setting the ``bzr.transform.orphan_policy`` configuration option.
109+
110+There are two possible values for this option:
111+
112+* ``conflict`` (the default): will leave the orphans in place and
113+ generate a conflicts,
114+
115+* ``move``: will move the orphans to a ``bzr-orphans`` directory at the root
116+ of the working tree with names like ``<file>.~#~``.
117
118 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
119
120@@ -259,12 +278,12 @@
121 To resolve that kind of conflict, you just have to decide what name should be
122 retained for the file involved.
123
124-``bzr resolve`` recognizes the following actions::
125+``bzr resolve`` recognizes the following actions:
126
127- - ``--action=take-this`` will revert Bazaar choice and keep ``PATH1`` by
128- issuing ``bzr mv PATH2 PATH1``,
129- - ``--action=take-other`` will acknowledge Bazaar choice of keeping ``PATH2``,
130- - ``--action=done`` will just mark the conflict as resolved.
131+* ``--action=take-this`` will revert Bazaar choice and keep ``PATH1`` by
132+ issuing ``bzr mv PATH2 PATH1``,
133+* ``--action=take-other`` will acknowledge Bazaar choice of keeping ``PATH2``,
134+* ``--action=done`` will just mark the conflict as resolved.
135
136 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
137
138@@ -294,13 +313,13 @@
139 ``black``. To resolve that kind of conflict, you just have to decide what
140 name should be retained for the directories involved.
141
142-``bzr resolve`` recognizes the following actions::
143+``bzr resolve`` recognizes the following actions:
144
145- - ``--action=take-this`` will acknowledge Bazaar choice of leaving ``white``
146- in ``black``,
147- - ``--action=take-other`` will revert Bazaar choice and move ``black`` in
148+* ``--action=take-this`` will acknowledge Bazaar choice of leaving ``white``
149+ in ``black``,
150+* ``--action=take-other`` will revert Bazaar choice and move ``black`` in
151 ``white`` by issuing ``bzr mv black/white white ; bzr mv black white``,
152- - ``--action=done`` will just mark the conflict as resolved.
153+* ``--action=done`` will just mark the conflict as resolved.
154
155 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
156
157@@ -329,13 +348,13 @@
158 To resolve that kind of conflict, you have to decide what name should be
159 retained for the file, directory or symlink involved.
160
161-``bzr resolve`` recognizes the following actions::
162+``bzr resolve`` recognizes the following actions:
163
164- - ``--action=take-this`` will issue ``bzr rm --force foo.new`` and
165- ``bzr add foo``,
166- - ``--action=take-other`` will issue ``bzr rm --force foo`` and
167- ``bzr mv foo.new foo``,
168- - ``--action=done`` will just mark the conflict as resolved.
169+* ``--action=take-this`` will issue ``bzr rm --force foo.new`` and
170+ ``bzr add foo``,
171+* ``--action=take-other`` will issue ``bzr rm --force foo`` and
172+ ``bzr mv foo.new foo``,
173+* ``--action=done`` will just mark the conflict as resolved.
174
175 Bazaar cannot auto-detect when conflicts of this kind have been resolved.
176
177
178=== modified file 'bzrlib/ignores.py'
179--- bzrlib/ignores.py 2010-09-04 15:34:10 +0000
180+++ bzrlib/ignores.py 2010-10-15 14:58:56 +0000
181@@ -42,6 +42,7 @@
182 '.#*',
183 '[#]*#',
184 '__pycache__',
185+ 'bzr-orphans',
186 ]
187
188
189
190=== modified file 'bzrlib/tests/per_workingtree/test_pull.py'
191--- bzrlib/tests/per_workingtree/test_pull.py 2010-10-15 14:58:56 +0000
192+++ bzrlib/tests/per_workingtree/test_pull.py 2010-10-15 14:58:56 +0000
193@@ -91,6 +91,8 @@
194 'WorkingTreeFormat2 does not support missing parent conflicts')
195 trunk = self.make_branch_deleting_dir('trunk')
196 work = trunk.bzrdir.sprout('work', revision_id='2').open_workingtree()
197+ work.branch.get_config().set_user_option(
198+ 'bzr.transform.orphan_policy', 'move')
199 # Add some unversioned files in dir
200 self.build_tree(['work/dir/foo',
201 'work/dir/subdir/',
202
203=== modified file 'bzrlib/tests/test_bzrdir.py'
204--- bzrlib/tests/test_bzrdir.py 2010-10-15 14:58:56 +0000
205+++ bzrlib/tests/test_bzrdir.py 2010-10-15 14:58:56 +0000
206@@ -791,6 +791,9 @@
207 self.build_tree(['tree1/subtree/file'])
208 sub_tree.add('file')
209 tree.commit('Initial commit')
210+ # The following line force the orhaning to reveal bug #634470
211+ tree.branch.get_config().set_user_option(
212+ 'bzr.transform.orphan_policy', 'move')
213 tree.bzrdir.destroy_workingtree()
214 # FIXME: subtree/.bzr is left here which allows the test to pass (or
215 # fail :-( ) -- vila 20100909
216
217=== modified file 'bzrlib/tests/test_transform.py'
218--- bzrlib/tests/test_transform.py 2010-10-15 14:58:56 +0000
219+++ bzrlib/tests/test_transform.py 2010-10-15 14:58:56 +0000
220@@ -29,6 +29,7 @@
221 rules,
222 symbol_versioning,
223 tests,
224+ trace,
225 transform,
226 urlutils,
227 )
228@@ -2363,7 +2364,7 @@
229 # parent
230 self.assertLength(2, conflicts)
231
232- def test_resolve_orphan_non_versioned_file(self):
233+ def test_non_versioned_file_create_conflict(self):
234 wt, tt = self.make_tt_with_versioned_dir()
235 dir_tid = tt.trans_id_tree_file_id('dir-id')
236 tt.new_file('file', dir_tid, 'Contents')
237@@ -2371,7 +2372,9 @@
238 tt.unversion_file(dir_tid)
239 conflicts = resolve_conflicts(tt)
240 # no conflicts or rather: orphaning 'file' resolve the 'dir' conflict
241- self.assertLength(0, conflicts)
242+ self.assertLength(1, conflicts)
243+ self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
244+ conflicts.pop())
245
246
247 A_ENTRY = ('a-id', ('a', 'a'), True, (True, True),
248@@ -3258,35 +3261,79 @@
249
250 class TestOrphan(tests.TestCaseWithTransport):
251
252- # Alternative implementations may want to test:
253- # - can't create orphan dir
254- # - orphaning forbidden
255- # - can't create orphan
256-
257 def test_no_orphan_for_transform_preview(self):
258 tree = self.make_branch_and_tree('tree')
259 tt = transform.TransformPreview(tree)
260 self.addCleanup(tt.finalize)
261 self.assertRaises(NotImplementedError, tt.new_orphan, 'foo', 'bar')
262
263- def test_new_orphan(self):
264- wt = self.make_branch_and_tree('.')
265+ def _set_orphan_policy(self, wt, policy):
266+ wt.branch.get_config().set_user_option('bzr.transform.orphan_policy',
267+ policy)
268+
269+ def _prepare_orphan(self, wt):
270 self.build_tree(['dir/', 'dir/foo'])
271 wt.add(['dir'], ['dir-id'])
272 wt.commit('add dir')
273 tt = transform.TreeTransform(wt)
274 self.addCleanup(tt.finalize)
275 dir_tid = tt.trans_id_tree_path('dir')
276- foo_tid = tt.trans_id_tree_path('dir/foo')
277+ orphan_tid = tt.trans_id_tree_path('dir/foo')
278 tt.delete_contents(dir_tid)
279 tt.unversion_file(dir_tid)
280 raw_conflicts = tt.find_conflicts()
281 self.assertLength(1, raw_conflicts)
282 self.assertEqual(('missing parent', 'new-1'), raw_conflicts[0])
283+ return tt, orphan_tid
284+
285+ def test_new_orphan_created(self):
286+ wt = self.make_branch_and_tree('.')
287+ self._set_orphan_policy(wt, 'move')
288+ tt, orphan_tid = self._prepare_orphan(wt)
289 remaining_conflicts = resolve_conflicts(tt)
290 # Yeah for resolved conflicts !
291 self.assertLength(0, remaining_conflicts)
292 # We have a new orphan
293- self.assertEquals('foo.~1~', tt.final_name(foo_tid))
294+ self.assertEquals('foo.~1~', tt.final_name(orphan_tid))
295 self.assertEquals('bzr-orphans',
296- tt.final_name(tt.final_parent(foo_tid)))
297+ tt.final_name(tt.final_parent(orphan_tid)))
298+
299+ def test_never_orphan(self):
300+ wt = self.make_branch_and_tree('.')
301+ self._set_orphan_policy(wt, 'conflict')
302+ tt, orphan_tid = self._prepare_orphan(wt)
303+ remaining_conflicts = resolve_conflicts(tt)
304+ self.assertLength(1, remaining_conflicts)
305+ self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
306+ remaining_conflicts.pop())
307+
308+ def test_orphan_error(self):
309+ def bogus_orphan(tt, orphan_id, parent_id):
310+ raise transform.OrphaningError(tt.final_name(orphan_id),
311+ tt.final_name(parent_id))
312+ transform.orphaning_registry.register('bogus', bogus_orphan,
313+ 'Raise an error when orphaning')
314+ wt = self.make_branch_and_tree('.')
315+ self._set_orphan_policy(wt, 'bogus')
316+ tt, orphan_tid = self._prepare_orphan(wt)
317+ remaining_conflicts = resolve_conflicts(tt)
318+ self.assertLength(1, remaining_conflicts)
319+ self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
320+ remaining_conflicts.pop())
321+
322+ def test_unknown_orphan_policy(self):
323+ wt = self.make_branch_and_tree('.')
324+ # Set a fictional policy nobody ever implemented
325+ self._set_orphan_policy(wt, 'donttouchmypreciouuus')
326+ tt, orphan_tid = self._prepare_orphan(wt)
327+ warnings = []
328+ def warning(*args):
329+ warnings.append(args[0] % args[1:])
330+ self.overrideAttr(trace, 'warning', warning)
331+ remaining_conflicts = resolve_conflicts(tt)
332+ # We fallback to the default policy which create a conflict
333+ self.assertLength(1, remaining_conflicts)
334+ self.assertEqual(('deleting parent', 'Not deleting', 'new-1'),
335+ remaining_conflicts.pop())
336+ self.assertLength(1, warnings)
337+ self.assertStartsWith(warnings[0], 'donttouchmypreciouuus')
338
339=== modified file 'bzrlib/transform.py'
340--- bzrlib/transform.py 2010-10-15 14:58:56 +0000
341+++ bzrlib/transform.py 2010-10-15 14:58:56 +0000
342@@ -22,6 +22,7 @@
343 from bzrlib import (
344 errors,
345 lazy_import,
346+ registry,
347 )
348 lazy_import.lazy_import(globals(), """
349 from bzrlib import (
350@@ -75,6 +76,7 @@
351 map[key] = value
352
353
354+
355 class _TransformResults(object):
356 def __init__(self, modified_paths, rename_count):
357 object.__init__(self)
358@@ -797,6 +799,32 @@
359 """
360 raise NotImplementedError(self.new_orphan)
361
362+ def _get_potential_orphans(self, dir_id):
363+ """Find the potential orphans in a directory.
364+
365+ A directory can't be safely deleted if there are versioned files in it.
366+ If all the contained files are unversioned then they can be orphaned.
367+
368+ The 'None' return value means that the directory contains at least one
369+ versioned file and should not be deleted.
370+
371+ :param dir_id: The directory trans id.
372+
373+ :return: A list of the orphan trans ids or None if at least one
374+ versioned file is present.
375+ """
376+ orphans = []
377+ # Find the potential orphans, stop if one item should be kept
378+ for c in self.by_parent()[dir_id]:
379+ if self.final_file_id(c) is None:
380+ orphans.append(c)
381+ else:
382+ # We have a versioned file here, searching for orphans is
383+ # meaningless.
384+ orphans = None
385+ break
386+ return orphans
387+
388 def _affected_ids(self):
389 """Return the set of transform ids affected by the transform"""
390 trans_ids = set(self._removed_id)
391@@ -1310,19 +1338,87 @@
392 delete_any(self._limbo_name(trans_id))
393
394 def new_orphan(self, trans_id, parent_id):
395- """See TreeTransformBase.new_orphan."""
396- # Add the orphan dir if it doesn't exist
397- orphan_dir = 'bzr-orphans'
398- od_id = self.trans_id_tree_path(orphan_dir)
399- if self.final_kind(od_id) is None:
400- self.create_directory(od_id)
401- parent_path = self._tree_id_paths[parent_id]
402- # Find a name that doesn't exist yet in the orphan dir
403- actual_name = self.final_name(trans_id)
404- new_name = self._available_backup_name(actual_name, od_id)
405- self.adjust_path(new_name, od_id, trans_id)
406- trace.warning('%s has been orphaned in %s'
407- % (joinpath(parent_path, actual_name), orphan_dir))
408+ # FIXME: There is no tree config, so we use the branch one (it's weird
409+ # to define it this way as orphaning can only occur in a working tree,
410+ # but that's all we have (for now). It will find the option in
411+ # locations.conf or bazaar.conf though) -- vila 20100916
412+ conf = self._tree.branch.get_config()
413+ conf_var_name = 'bzr.transform.orphan_policy'
414+ orphan_policy = conf.get_user_option(conf_var_name)
415+ default_policy = orphaning_registry.default_key
416+ if orphan_policy is None:
417+ orphan_policy = default_policy
418+ if orphan_policy not in orphaning_registry:
419+ trace.warning('%s (from %s) is not a known policy, defaulting to %s'
420+ % (orphan_policy, conf_var_name, default_policy))
421+ orphan_policy = default_policy
422+ handle_orphan = orphaning_registry.get(orphan_policy)
423+ handle_orphan(self, trans_id, parent_id)
424+
425+
426+class OrphaningError(errors.BzrError):
427+
428+ # Only bugs could lead to such exception being seen by the user
429+ internal_error = True
430+ _fmt = "Error while orphaning %s in %s directory"
431+
432+ def __init__(self, orphan, parent):
433+ errors.BzrError.__init__(self)
434+ self.orphan = orphan
435+ self.parent = parent
436+
437+
438+class OrphaningForbidden(OrphaningError):
439+
440+ _fmt = "Policy: %s doesn't allow creating orphans."
441+
442+ def __init__(self, policy):
443+ errors.BzrError.__init__(self)
444+ self.policy = policy
445+
446+
447+def move_orphan(tt, orphan_id, parent_id):
448+ """See TreeTransformBase.new_orphan.
449+
450+ This creates a new orphan in the `bzr-orphans` dir at the root of the
451+ `TreeTransform`.
452+
453+ :param tt: The TreeTransform orphaning `trans_id`.
454+
455+ :param orphan_id: The trans id that should be orphaned.
456+
457+ :param parent_id: The orphan parent trans id.
458+ """
459+ # Add the orphan dir if it doesn't exist
460+ orphan_dir_basename = 'bzr-orphans'
461+ od_id = tt.trans_id_tree_path(orphan_dir_basename)
462+ if tt.final_kind(od_id) is None:
463+ tt.create_directory(od_id)
464+ parent_path = tt._tree_id_paths[parent_id]
465+ # Find a name that doesn't exist yet in the orphan dir
466+ actual_name = tt.final_name(orphan_id)
467+ new_name = tt._available_backup_name(actual_name, od_id)
468+ tt.adjust_path(new_name, od_id, orphan_id)
469+ trace.warning('%s has been orphaned in %s'
470+ % (joinpath(parent_path, actual_name), orphan_dir_basename))
471+
472+
473+def refuse_orphan(tt, orphan_id, parent_id):
474+ """See TreeTransformBase.new_orphan.
475+
476+ This refuses to create orphan, letting the caller handle the conflict.
477+ """
478+ raise OrphaningForbidden('never')
479+
480+
481+orphaning_registry = registry.Registry()
482+orphaning_registry.register(
483+ 'conflict', refuse_orphan,
484+ 'Leave orphans in place and create a conflict on the directory.')
485+orphaning_registry.register(
486+ 'move', move_orphan,
487+ 'Move orphans into the bzr-orphans directory.')
488+orphaning_registry._set_default_key('conflict')
489
490
491 class TreeTransform(DiskTreeTransform):
492@@ -2859,20 +2955,24 @@
493 elif c_type == 'missing parent':
494 trans_id = conflict[1]
495 if trans_id in tt._removed_contents:
496- orphans = []
497- empty = True
498- # Find the potential orphans, stop if one item should be kept
499- for c in tt.by_parent()[trans_id]:
500- if tt.final_file_id(c) is None:
501- orphans.append(c)
502- else:
503- empty = False
504- break
505- if empty:
506+ cancel_deletion = True
507+ orphans = tt._get_potential_orphans(trans_id)
508+ if orphans:
509+ cancel_deletion = False
510 # All children are orphans
511 for o in orphans:
512- tt.new_orphan(o, trans_id)
513- else:
514+ try:
515+ tt.new_orphan(o, trans_id)
516+ except OrphaningError:
517+ # Something bad happened so we cancel the directory
518+ # deletion which will leave it in place with a
519+ # conflict. The user can deal with it from there.
520+ # Note that this also catch the case where we don't
521+ # want to create orphans and leave the directory in
522+ # place.
523+ cancel_deletion = True
524+ break
525+ if cancel_deletion:
526 # Cancel the directory deletion
527 tt.cancel_deletion(trans_id)
528 new_conflicts.add(('deleting parent', 'Not deleting',
529
530=== modified file 'doc/en/release-notes/bzr-2.3.txt'
531--- doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:58:56 +0000
532+++ doc/en/release-notes/bzr-2.3.txt 2010-10-15 14:58:56 +0000
533@@ -175,11 +175,16 @@
534 * ``unshelve --preview`` now can show diff in a non-ascii encoding.
535 (Andrej A Antonov, #518916)
536
537-* When a bzr command remove a previously versioned directory, all
538- unversioned files are moved to a 'bzr-orphans' directory at the working
539- tree root with backup names (<file>.~#~). This was previously creating
540- spurious conflicts during merge, pull or switch operations.
541- (Vincent Ladeuil, #323111)
542+* Deleting a versioned directory can leave orphans: unversioned files that
543+ were present don't have a parent anymore. The
544+ ``bzr.transform.orphan_policy`` configuration option controls the ``bzr``
545+ behaviour: ``conflict`` (the default) leave the orphans in place and
546+ create a conflict for the directory, ``move`` create orphans named
547+ ``<file>.~#~`` in a ``bzr-orphans`` directory at the root of the working
548+ tree. (Vincent Ladeuil, #323111)
549+
550+Improvements
551+************
552
553 Documentation
554 *************
555@@ -187,6 +192,9 @@
556 * Correct the documentation for setting up the smart server with Apache.
557 (Neil Martinsen-Burrell, #311518)
558
559+* Fix rst typos in bzrlib/help_topics/en/conflict-types.txt.
560+ (Vincent Ladeuil, #660943)
561+
562 * Provide more detailed help on the Launchpad plugin at "bzr help
563 plugins/launchpad". (Neil Martinsen-Burrell, #589379)
564
565
566=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
567--- doc/en/whats-new/whats-new-in-2.3.txt 2010-10-15 14:58:56 +0000
568+++ doc/en/whats-new/whats-new-in-2.3.txt 2010-10-15 14:58:56 +0000
569@@ -104,10 +104,15 @@
570 Improved conflict handling
571 **************************
572
573-* Deleting a versioned directory containing unversioned files will no
574- longer create a conflict. Instead, the unversioned files will be moved
575- into a 'bzr-orphans' directory at the root of the working tree.
576- (Vincent Ladeuil, #323111)
577+* ``pull``, ``merge`` or ``switch`` can lead to conflicts when deleting a
578+ versioned directory contains unversioned files. The cause of the conflict
579+ is that deleting the directory will orphan the unversioned files so the
580+ user needs to instruct ``bzr`` what do to do about these orpahns. This is
581+ controlled by setting the ``bzr.transform.orphan_policy`` configuration
582+ variable with a value of ``move``. In this case the unversioned files are
583+ moved to a ``bzr-orphans`` directory at the root of the working tree. The
584+ default behaviour is specified (if needed) by setting the variable to
585+ ``conflict``. (Vincent Ladeuil, #323111)
586
587 >>>>>>> MERGE-SOURCE
588 Documentation