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

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.

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

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?

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

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

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

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

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

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

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

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

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

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

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

sent to pqm by email