Code review comment for lp:~vila/bzr/323111-orphan-config-option

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