Code review comment for lp:~mwhudson/launchpad/puller-import-hack

Revision history for this message
Aaron Bentley (abentley) wrote :

What is the impact of removing the testrunner bzr_imports_root_url? Does this cause it to default to a more suitable value?

Instead of remote_bzr_dir.transport.clone('..'), you should be able to use remote_bzr_dir.root_transport.

Some of the test changes look like drive-bys, e.g. not wanting it to leave directories behind. Is that right?

Is there a reason to join config.launchpad.bzr_imports_root_url and the branch id with path.join instead of urlutils.join?

Did you consider using more specific name than _is_import? e.g. _vfs_initial_clone ? Or what about providing an initial_clone method on the BranchPolicy itself?

What about using sets to represent files_before and files_after? That's closer to the abstract representation, because order isn't important, and entries can only appear once. I don't really understand the purpose of this loop, either.

It might be nice to turn _runWithTransformFallbackLocationHookInstalled into a ContextManager, but that's not necessary for this branch.

review: Needs Information

« Back to merge proposal