Code review comment for lp:~viktor-nagy/bzr-email/tip-hook

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

On 2/17/2011 4:01 PM, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~viktor-nagy/bzr-email/tip-hook into lp:bzr-email.
>
> Requested reviews:
> Bazaar Developers (bzr)
>
> For more details, see:
> https://code.launchpad.net/~viktor-nagy/bzr-email/tip-hook/+merge/50238

This is conflicting with trunk after Jelmer's earlier update.

The way it is written it will default to smtp_use_tls = False, I would
rather default to True, but a user can disable it with an explicit
setting. (because get() defaults to returning None which evaluates to
False.)

I think just adding

use_tls = config.get_user_option('smtp_use_tls')
if use_tls is None:
  use_tls = True

...

On the other hand, isn't the connection side of things handled by bzr
code now? At least, I thought bzr-email was using bzrlib.smtp_connection
now. So that code should be pulled into bzr core.

I also don't fully understand what this is for:
+def tip_change_hook(params):
+ '''
+ This is the post_change_branch_tip commit hook that runs on a smart
server after the branch's tip has changed.
+ '''
+ from bzrlib.branch import BzrBranch6
+ if not isinstance(params.branch, BzrBranch6):
+ raise errors.UpgradeRequired(params.branch.base)
+ _emailer.EmailSender(params.branch, params.new_revid,
params.branch.get_config(),
+ local_branch=None).send_maybe()
+

Why do you have to have BzrBranch6 or better? Certainly that sounds like
something the user wanted to enforce, but should not be something in
bzr-email itself.)

 review: needsfixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1dnoMACgkQJdeBCYSNAAOD7wCePJk/Qk40OwoaSMcmTgEN9Xdu
/GoAoKPWkL91NlhA+wXlETOi4aNcKdEi
=M3nm
-----END PGP SIGNATURE-----