Code review comment for lp:~vila/bzr/config-lock-remote

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> Could you point me where in the *actual* code this idiom is used and
> such a reporting is taken into account ?
>
> I find it highly unproductive to be blocked from landing incremental
> patches when my intent is to ease review.

I should also mention that I'm not blocking. Specifically I said:

> John
> =:->
>
> review: approve
>
And then later I said:
> So please consider giving us a nicer error for discoverability purposes.
            ^^^^^^^^

Both are meant as, "I really think this would be better if it trapped
for None, but if you feel strongly you can land it".

Bugs happen, in places we don't want them to. Getting AttributeError is
a really confusing way of finding out that a branch disappeared when we
expected it to exist. If you used a normal reference the code would be
clearer and would *actually* never fail. I believe you are doing this
because of working recently with Martin gz and running into issues with
python cycles taking a while to be closed by the garbage collector.

ATM, I feel that is premature optimization (Branch has about 5 different
cycles, so unless we close them all, it isn't really worth making this
code harder to follow on the off chance that we clean other stuff up.)

I'm willing to live with it, provided we don't make things harder later
because of hard-to-track-down-reference-counting bugs and getting
AttributeError: None has no attribute unlock().

It is obvious to you *now*, will it be obvious in 2 years when we get a
bug report because we cleaned up something that used to have a cycle and
as a side effect kept the branch alive and people start expecting that
behavior.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eKTQACgkQJdeBCYSNAAPJbwCghXd6lUrK0JF5cVTkG9P76nmm
dg0An0KtgA3EcUla0BnkUI5deZjBlyp+
=FzKW
-----END PGP SIGNATURE-----