Code review comment for lp:~jelmer/bzr/safe-open

Am 24/12/11 13:26, schrieb Martin Packman:
> Review: Needs Fixing
>
> Moving this up to the bzrlib level seems reasonable, but it really needs a better name. The problem with 'safe' is you need context to understand what it actually means and the term is too widely used for completely different things.
Hmm, interesting point. I took the name from launchpad, but I guess I've
just gotten used to it meaning this... What about "bzrlib.hookable_open"
or "bzrlib.policy_open" ?

> +def safe_open(allowed_scheme, url):
> + """Open the branch at `url`, only accessing URLs on `allowed_scheme`.
>
> Aha! A useful docstring on this function... which actually does something much more specific than the name or module content implies.
>
>
> So this is really about opening branches with certain URL based checks added. The naming should really reflect that limited scope.

I'm not sure I follow. Do you mean e.g. renaming it to e.g.
open_with_specific_scheme ?
>
> +class BlacklistPolicy(BranchOpenPolicy):
> + """Branch policy that forbids certain URLs."""
> +
> + def __init__(self, should_follow_references, unsafe_urls=None):
> + if unsafe_urls is None:
> + unsafe_urls = set()
> + self._unsafe_urls = unsafe_urls
>
> This is unsafe, bzrlib doesn't normalise URLs enough to ensure there's only one canonical form. Also, forgetting a param meaning "no restrictions" isn't a great interface. I'd just add an underscore to the class name for now to discourage use.
>
> There's some more to nitpick over with the code, but it doesn't seem that important.
This is only really used in the Launchpad testsuite. I guess I could
just leave it over in the Launchpad code.

Cheers,

Jelmer