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

The code looks pretty straightforward. Nice!

Here are some issues, though:

174 +class TestOrphan(tests.TestCaseWithTransport):
175 +
176 + # Alternative implementations may want to test:
177 + # - can't create orphan dir
178 + # - orphaning forbidden
179 + # - can't create orphan

I don't quite follow this comment. Which alternative implementations — these aren't per-transform tests? Is this a list of hypothetical alternative orphan-handling policies, or scenarios alternative implementations of orphan handling ought to consider, or something else?

I guess basically I don't understand who the intended audience is for this comment.

222 +lazy_import.lazy_import(globals(), """

Why the change to "from bzrlib import lazy_import", rather than "from bzrlib.lazy_import import lazy_import". We overwhelmingly prefer the latter elsewhere in our code base.

316 + if trans_id in tt._removed_contents:
317 + orphans = []
318 + empty = True
319 + # Find the potential orphans, stop if one item should be kept
320 + for c in tt.by_parent()[trans_id]:
321 + if tt.final_file_id(c) is None:
322 + orphans.append(c)
323 + else:
324 + empty = False
325 + break
326 + if empty:

The 'empty' variable seems redundant. You could just as easily test 'if orphans' (or 'if len(orphans) == 0' if you prefer).

That's all the comments I have so far. I don't have a verdict yet, as I have few too many questions, so I'll mark my review Needs Information for now.