Code review comment for lp:~vila/bzr/323111-orphan-non-versioned-files

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    >> The implementation first check the children of the conflicting
    >> parent dir. If all of them are unversioned files (leading to an
    >> empty directory) the conflict is resolved by moving all the
    >> children into the orphan directory. The directory can then be
    >> deleted without conflicts.

    > Could we just rename the directory into bzr-orphans?

Not if it already exists obviously.

    > Preserving the clustering would be nice, though I understand
    > flattening it.

bzr-orphans collect *all* the orphans, there could be more than one
directory involved, so special-casing here... in treetransform... I
don't feel like it :)

    > One concern is that directory filling up with useless files after
    > doing a 'bzr switch' 5 or so times across a
    > directory-full-of-python-files deletion/creation.

Damn, I forgot to mention that.

When we discussed it with lifeless, the idea was that the user will
suddenly get a 'bzr-orphans' in his face and will deal with it. This is
still less disturbing than a conflict since it can accumulate several
commands producing orphans. So dealing with it should be as simple as:
have a look, nothing interesting, delete the bzr-orphans directory.

Now there are certainly various ways to enhance that but the proposed
implementation gives us a way to experiment.

Should be bring the subject to the mailing list once this has landed to
ask for a large feedback:

- do you produce many orphans ?
- did you wonder where this 'bzr-orphans' directory came from ?

- would you like warnings emitted when files are orphaned (hmm, should
  we wait feedback for that ? :-)

trace.warning added.

    > ...

    >> Note that this is self-contained into new_orphan() so far.
    >> Possible enhancements includes:
    >>
    >> - making the 'bzr-orphans' directory be configurable (it's
    >> clearly tight to the working tree though),
    >>
    >> - allows various orphaning policies (always delete, delete junk
    >> files (tbd), never orphan (the behaviour before this patch),
    >> etc)

    > - I think what we really want is a hook point for determining what to do
    > with the file. Similar to the per-file merge hook, something that is a
    > per-file 'ignored file handler'.

Hehe, obviously we may want to add some .bzrjunk support which in turn
will be respected by the orphaning :)

But once junk files are out of the way, yes, such a hook sounds like the
way to go.

    > ...
    > +class TestTransformMissingParent(tests.TestCaseWithTransport):
    > +
    > + def get_tree_transform_with_unversioned_dir(self):

    > ^- I think we usually call these "make_*", since it is creating stuff.

Bah, you know you've been reading test_transform too much when... :)

Fixed.

    > + def new_orphan(self, trans_id, parent_id):
    > + """See TreeTransformBase.new_orphan."""
    > + # Add the orphan dir if it doesn't exist
    > + od_id = self.trans_id_tree_path('bzr-orphans')
    > + if self.final_kind(od_id) is None:
    > + self.create_directory(od_id)
    > + # Find a name that doesn't exist yet in the orphan dir
    > + new_name = osutils.available_backup_name(
    > + self.final_name(trans_id), lambda name: name in
    > self._tree_path_ids)
    > + self.adjust_path(new_name, od_id, trans_id)
    > +

    > ^- I don't think this check is actually correct. Since if the
    > bzr-orphans directory already exists, you'd already have a file present
    > that you have to watch out for conflicting with.
    > So you need to check *both* that the path isn't going to be
    > created (yet) and that it doesn't already exist.

*blink* Grr, trying to make the threads easier to review, I separated
the addition of _has_named_child which *does* the correct checks.

The correct check should be done here by calling _get_backup_name() not
osutils.available_backup_name().

Fixed in both threads.

    > If it is going to be that complex, I think we want a full function, and
    > not just a lambda. Plus you can't put a docstring on a lambda :)

Hence the third submission :-) But I forgot to use it in new_orphan :-/

    > elif c_type == 'missing parent':
    > trans_id = conflict[1]
    > - try:
    > - tt.cancel_deletion(trans_id)
    > - new_conflicts.add(('deleting parent', 'Not deleting',
    > - trans_id))
    > - except KeyError:
    > + if trans_id in tt._removed_contents:

    > ^- Off hand, this feels like for each orphaned child, you will end
    > up scanning against all possible removed files. (N^2).

No. The conflict is for the removed directory, the one that *contains*
the orphans, so it's done only once.

    > Does cancel deletion remove the 'missing parent' flag for all
    > children? Does 'new_orphan'? (I'm hoping the latter does)

    > Otherwise this is definitely worthy of Whats New,

Ok.

    > and deserves a section in Compatibility breaks. (Which you may
    > have done, not sure where your NEWS entry is).

Yes.