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?

No :)

> Is this a list of hypothetical alternative
> orphan-handling policies, or scenarios alternative implementations of orphan
> handling ought to consider,

Yes.

> or something else?
>
> I guess basically I don't understand who the intended audience is for this
> comment.

People implementing new orphaning policies and wondering how to test them :)

Which I'm currently doing ;)

So the next submission, addressing the config
variable (https://code.edge.launchpad.net/~vila/bzr/323111-orphan-config-option/+merge/35690)
take care of these comments which were, obviously, addressed to
me as stealth way to avoid implementing it :-D

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

Except it has been said several times that we shouldn't import
symbols from modules but prefer importing modules and prefix
symbols and that we should migrate to this form progressively.

I've been doing many such cleanups, some addressing weird and
subtle aliasing bugs, including ones related to lazy
imports (lazy imports more or less requires using
<module>.<symbol> in some cases).

There are a few exceptions to this rule:
- symbol_versioning but even there I've commented that we may
  want to rework this module too,
- bzrlib.tests aliasing TestCase, etc symbols from
  bzrlib.tests.TestUtils (but isn't it a sign that they should be
  define into bzrlib.tests ?)

I also found that simplifying the import list at the beginning of
the module makes it clearer and avoid leaving cruft there.

It generally makes it easier to move code from one module to the
others since there are fewer dependencies to update.

It makes the code easier to read (you don't have to go back to
the beginning of the module to know where this apparently global
symbol is coming from).

Another reason from the leaking tests saga:

,----
| I did so by wrapping transport.get_transport() only to realise
| that far too many tests were not using it. I also uncovered a
| nasty reason why we should always use the 'import <module> ;
| <module>.<function>' idiom instead of 'from <module> import
| <function> ; <function>': the later doesn't allow
| wrappers... Obvious a posteriori, but I was quite puzzled before
| I realised that. Those were worth fixing even if we decide to replace
| the wrapping by a hook as mentioned below.
`----

Now, we can make lazy_import another exception... but why ?

And why do I keep bumping into people that don't want to use the
'import module ; module.symbol' given that we laready encounter
several related problems.

The only pro I can find for doing 'from module import symbol ;
symbol' is that it requires less typing. Given the code is
overwhelmingly more often read that written, the priority should
be to help the reader, not the writer.

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

I could have used 'orphans = None' instead to make it clearer that
the orphan research is meaningless as soon as one versioned file
is present. I've simplified it in the last submission.

Note that there is also lp:~vila/bzr/323111-orphans with the loom itself in case it helps
the review.