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

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.

« Back to merge proposal