Merge lp:~viktor-nagy/bzr-email/tip-hook into lp:bzr-email

Proposed by Jelmer Vernooij on 2011-02-17
Status: Work in progress
Proposed branch: lp:~viktor-nagy/bzr-email/tip-hook
Merge into: lp:bzr-email
Diff against target: 75 lines (+23/-0) (has conflicts)
2 files modified
__init__.py (+15/-0)
smtp_connection.py (+8/-0)
Text conflict in __init__.py
Text conflict in smtp_connection.py
To merge this branch: bzr merge lp:~viktor-nagy/bzr-email/tip-hook
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing on 2011-02-17
Jelmer Vernooij (community) code Needs Fixing on 2011-02-17
Review via email: mp+50238@code.launchpad.net

This proposal supersedes a proposal from 2008-08-18.

To post a comment you must log in.
Jelmer Vernooij (jelmer) wrote :

Hi,

I'm sorry it's taken so long before we had a chance to look at this branch. Unfortunately it now has conflicts with trunk. If you still care about getting this functionality in, is there any chance you can resolve them and resubmit? Please don't hesitate to ask us if we don't follow up to a MP.

review: Needs Fixing (code)
John A Meinel (jameinel) wrote :

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

review: Needs Fixing

Unmerged revisions

38. By ViktorNagy on 2008-08-21

Change the name for the post change branch tip hook from tip_commit to
tip_change

37. By ViktorNagy on 2008-08-18

* added post_change_branch_tip hook
* adds a new config option smtp_use_ttls = True | False, and corrects
the statttls() negotiation

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '__init__.py'
2--- __init__.py 2010-10-09 23:15:45 +0000
3+++ __init__.py 2011-02-17 22:01:52 +0000
4@@ -79,6 +79,7 @@
5 # lazy_import emailer so that it doesn't get loaded if it isn't used
6 lazy_import(globals(), """\
7 from bzrlib.plugins.email import emailer as _emailer
8+from bzrlib.branch import BzrBranch6
9 """)
10
11
12@@ -94,6 +95,16 @@
13 _emailer.EmailSender(master, new_revid, master.get_config(),
14 local_branch=local).send_maybe()
15
16+def tip_change_hook(params):
17+ '''
18+ This is the post_change_branch_tip commit hook that runs on a smart server after the branch's tip has changed.
19+ '''
20+ from bzrlib.branch import BzrBranch6
21+ if not isinstance(params.branch, BzrBranch6):
22+ raise errors.UpgradeRequired(params.branch.base)
23+ _emailer.EmailSender(params.branch, params.new_revid, params.branch.get_config(),
24+ local_branch=None).send_maybe()
25+
26
27 def branch_post_change_hook(params):
28 """This is the post_change_branch_tip hook."""
29@@ -107,9 +118,13 @@
30 install_named_hook = getattr(Branch.hooks, 'install_named_hook', None)
31 if install_named_hook is not None:
32 install_named_hook('post_commit', branch_commit_hook, 'bzr-email')
33+<<<<<<< TREE
34 if 'post_change_branch_tip' in Branch.hooks:
35 install_named_hook('post_change_branch_tip',
36 branch_post_change_hook, 'bzr-email')
37+=======
38+ install_named_hook('post_change_branch_tip', tip_change_hook, 'bzr-email-on-tip')
39+>>>>>>> MERGE-SOURCE
40 else:
41 Branch.hooks.install_hook('post_commit', branch_commit_hook)
42 if getattr(Branch.hooks, 'name_hook', None) is not None:
43
44=== modified file 'smtp_connection.py'
45--- smtp_connection.py 2011-02-14 14:22:40 +0000
46+++ smtp_connection.py 2011-02-17 22:01:52 +0000
47@@ -55,6 +55,7 @@
48
49 self._smtp_username = config.get_user_option('smtp_username')
50 self._smtp_password = config.get_user_option('smtp_password')
51+ self._use_ttls = config.get_user_option('smtp_use_ttls')
52
53 self._connection = None
54
55@@ -85,6 +86,7 @@
56 # If this fails, it just returns an error, but it shouldn't raise an
57 # exception unless something goes really wrong (in which case we want
58 # to fail anyway).
59+<<<<<<< TREE
60 try:
61 self._connection.starttls()
62 except smtplib.SMTPException, e:
63@@ -95,6 +97,12 @@
64 pass
65 else:
66 raise
67+=======
68+ if self._use_ttls:
69+ self._connection.ehlo()
70+ self._connection.starttls()
71+ self._connection.ehlo()
72+>>>>>>> MERGE-SOURCE
73
74 def _authenticate(self):
75 """If necessary authenticate yourself to the server."""

Subscribers

People subscribed via source and target branches

to all changes: