Bazaar

Merge lp:~rom1-chal/bzr/post_checkout_hook into lp:bzr

  1. post_checkout_hook
  2. Merge into bzr.dev
Proposed by Romain Chalumeau
Status: Work in progress
Proposed branch: lp:~rom1-chal/bzr/post_checkout_hook
Merge into: lp:bzr
Diff against target: 157 lines (+98/-1)
4 files modified
.bzrignore (+1/-0)
bzrlib/branch.py (+41/-0)
bzrlib/tests/per_branch/test_create_checkout.py (+53/-1)
doc/en/release-notes/bzr-2.5.txt (+3/-0)
To merge this branch: bzr merge lp:~rom1-chal/bzr/post_checkout_hook
Reviewer Review Type Date Requested Status
Romain Chalumeau (community) Approve
Jelmer Vernooij (community) Needs Information
Review via email: mp+81589@code.launchpad.net

Description of the change

Hi,

As mentioned in IRC chan, please find an implementation of a post_checkout hook.

* Code : It is called with bzrlib.branch.CheckoutHookParams class containing paramms of which are :
(checkouted branch, checkouted revision_id, created workingtree, checkout path). It fired at the end of the Branch.create_checkout method.

* Tests : per_branch tests take into account lightweight, heavyweight and earlier revision cehkout.
./bzr selftest -v TestCheckoutHook

* Doc : I haven't found the hooks document generator to test the 'bzr help hooks' output.
Yet, I have update the releasenotes.

My planned usage is the following :
- I have a CI server running merges from bzr-builder recipes,
- When a conflict is detected, it reports the conflicts and mails them to related users.
The goal now is to propose to the developer to simply checkout a branch to reproduce locally the conflicts from the recipe.
With the post_checkout hook, i trigger the merge directive right after the checkout.

Rgds/Romain

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Romain,

This looks good overall. My only concern is that we shouldn't be adding unnecessary hooks. Is there a particular reason you want additional merges to happen on checkout, rather than having users actually run the recipe locally?

Assuming we do indeed want to add this hook, I think this code looks really good. Some minor notes/things that need fixing:

The docs for "bzr help hooks" should be automatically updated, once new hooks are registered. Since you added this hook to BranchHooks, it should now show up in "bzr hooks".

Since checkouts can be heavyweight or lighweight, the term "branch" in the context of a checkout is ambiguous. I would suggest renaming the "branch" member of PostCheckoutHooks to "master_branch" to make it clearer which branch is being passed in.

19 + params = CheckoutHookParams(self, revision_id or self.last_revision(),
20 + tree, to_location )

There is a race condition here - the master branch can have changed since the working tree was created. It is probably better to use "tree.last_revision()" rather than "revision_id or self.last_revision()".

Also, you should take credit for this change in doc/en/release-notes/bzr-2.5.txt. :-)

Sorry it took so long to get back to you.

Reply
Revision history for this message
Jelmer Vernooij (jelmer) :
review: Needs Information
Reply
Revision history for this message
Romain Chalumeau (rom1-chal) wrote :

Hi Jelmer,

You're right on the recipe. In fact, I use a forked bzr-builder to run it through jenkins and which includes reporting and a way not to stop at the first conflict, but instead tries to merge the most of the recipe : the conflicts are reported (and reverted of course). So far, it is too specific to be proposed to merge as is.
But you gave me an other view on how to reproduce conflicts, thanks. So true, i can avoid using a post_checkout hook for that case.

Anyhow, it seems to me that a checkout hook helps in better handling some existing plugins without wrapping the builtins command. For instance, look at the externals plugin : it can be handled with a checkout hook rather than overload the checkout command.
I also see a usage as checkouting anywhere but using a configured and unique shared repository (something like bzrtools.cbranch, but with builtins commands).
Or a way to use the bzr-builder : when you branch or checkout a branch containing a recipe file, it launches the build from the checkouted file.
It can also be used to configure on the fly some development environments (automated configuration to test the code, automated downloads of components like in launchpad code, etc...).

Well, i see some usages to ease and automate the developpers work and which needs to deal with the working tree files.

You're right about the master_branch. I also agree on tree.last_revision. I will change those.

Rgds/Romain

Reply
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Cool - just ping us here, when you think this is ready for another review.

Reply
lp:~rom1-chal/bzr/post_checkout_hook updated
6246. By Romain Chalumeau

Fixes after code review (jelmer)

6247. By Romain Chalumeau

Merge trunk

6248. By Romain Chalumeau

Fixes CheckoutHookParams params comment for api doc

Revision history for this message
Romain Chalumeau (rom1-chal) wrote :

Hi,

I updated the code regarding the comments from jelmer.
I did not succeed in creating a unit test for the case where the branch is updated while checkouting it (comment about race condition). In other words, i didn't succeed in having the test failed with the race condition.
So i updated the code with tree.last_revision because I think this case could occur when the master branch is updated while creating the working tree. But no test with it...

Rgds/Romain

review: Needs Resubmitting
Reply
lp:~rom1-chal/bzr/post_checkout_hook updated
6249. By Romain Chalumeau

Fixes tests with tree.last_revision

Revision history for this message
Romain Chalumeau (rom1-chal) :
review: Approve
Reply

Unmerged revisions

6249. By Romain Chalumeau

Fixes tests with tree.last_revision

6248. By Romain Chalumeau

Fixes CheckoutHookParams params comment for api doc

6247. By Romain Chalumeau

Merge trunk

6246. By Romain Chalumeau

Fixes after code review (jelmer)

6245. By Romain Chalumeau

Add line for post_checkout hook in releasenotes

6244. By Romain Chalumeau

Add post_checkout hook in BranchHooks. Fired at the end of Branch.create_checkout
Tests : tests hook per branch with lightweight, heavyweight and earlier revision checkout

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2010-10-13 00:26:41 +0000
3+++ .bzrignore 2011-11-25 11:12:27 +0000
4@@ -69,3 +69,4 @@
5 bzrlib/_*.pyd
6 ./.ccache
7 .testrepository
8+*.komodo*
9
10=== modified file 'bzrlib/branch.py'
11--- bzrlib/branch.py 2011-11-22 11:48:31 +0000
12+++ bzrlib/branch.py 2011-11-25 11:12:27 +0000
13@@ -1480,6 +1480,13 @@
14 lightweight)
15 finally:
16 basis_tree.unlock()
17+
18+ # Triggers post_checkout hooks
19+ params = CheckoutHookParams(self, tree.last_revision(),
20+ tree, to_location )
21+ for hook in Branch.hooks['post_checkout']:
22+ hook(params)
23+
24 return tree
25
26 @needs_write_lock
27@@ -1895,6 +1902,10 @@
28 "Called after a checkout switches branch. "
29 "post_switch is called with a "
30 "bzrlib.branch.SwitchHookParams.", (2, 2))
31+ self.add_hook('post_checkout',
32+ "Called after a workingtree initialisation completes. post_checkout "
33+ " is called with a bzrlib.branch.CheckoutHookParams object and only "
34+ " runs in the bzr client.", (2, 5))
35
36
37
38@@ -2013,6 +2024,36 @@
39 self.revision_id)
40
41
42+class CheckoutHookParams(object):
43+ """Object holding parameters passed to `post_checkout` hooks.
44+
45+ :param master_branch: branch that the checkout is linked to
46+ :param revision_id: revision ID of the tree
47+ :param to_location: path of the checkout
48+ """
49+ def __init__(self, master_branch, revision_id, workingtree, to_location):
50+ """
51+ :param master_branch: branch that the checkout is linked to
52+ :param revision_id: revision ID of the tree
53+ :param to_location: path of the checkout
54+ """
55+ self.master_branch = master_branch
56+ self.revision_id = revision_id
57+ self.workingtree = workingtree
58+ self.to_location = to_location
59+
60+ def __eq__(self, other):
61+ return self.__dict__ == other.__dict__
62+
63+ def __repr__(self):
64+ return "<%s for (%s, %s, %s)>" % (self.__class__.__name__,
65+ self.master_branch,
66+ self.revision_id,
67+ self.to_location
68+ )
69+
70+
71+
72 class BranchFormatMetadir(BranchFormat):
73 """Common logic for meta-dir based branch formats."""
74
75
76=== modified file 'bzrlib/tests/per_branch/test_create_checkout.py'
77--- bzrlib/tests/per_branch/test_create_checkout.py 2011-09-09 12:20:33 +0000
78+++ bzrlib/tests/per_branch/test_create_checkout.py 2011-11-25 11:12:27 +0000
79@@ -17,7 +17,7 @@
80 """Tests for the Branch.create_checkout"""
81
82 from bzrlib.tests import per_branch
83-
84+from bzrlib import branch
85
86 class TestCreateCheckout(per_branch.TestCaseWithBranch):
87
88@@ -68,3 +68,55 @@
89 tree1 = self.make_branch_and_tree('base')
90 self.build_tree(['checkout/'])
91 tree2 = tree1.branch.create_checkout('checkout', lightweight=True)
92+
93+
94+class TestCheckoutHook(per_branch.TestCaseWithBranch):
95+
96+ def setUp(self):
97+ self.hook_calls = []
98+ super(TestCheckoutHook, self).setUp()
99+
100+ def capture_post_checkout_hook(self, result):
101+ """Capture post checkout hook calls to self.hook_calls.
102+
103+ The call is logged.
104+ """
105+ self.hook_calls.append(
106+ ('post_checkout',
107+ result.master_branch,
108+ result.revision_id,
109+ result.workingtree,
110+ result.to_location
111+ ))
112+
113+ def test_post_checkout_lightweight(self):
114+ branch.Branch.hooks.install_named_hook(
115+ 'post_checkout', self.capture_post_checkout_hook, None)
116+ a_branch = self.make_branch('branch')
117+ tree = a_branch.create_checkout('checkout', lightweight=True)
118+ self.assertEqual([('post_checkout', a_branch, tree.last_revision(),
119+ tree,'checkout')],
120+ self.hook_calls)
121+
122+ def test_post_checkout_heavyweight(self):
123+ branch.Branch.hooks.install_named_hook(
124+ 'post_checkout', self.capture_post_checkout_hook, None)
125+ a_branch = self.make_branch('branch')
126+ tree = a_branch.create_checkout('checkout', lightweight=False)
127+ self.assertEqual([('post_checkout', a_branch, tree.last_revision(),
128+ tree,'checkout')],
129+ self.hook_calls)
130+
131+ def test_post_checkout_earlier_revision(self):
132+ #Create a branch with two revisions
133+ tree1 = self.make_branch_and_tree('base')
134+ rev1 = tree1.commit('first', allow_pointless=True, rev_id='rev-1')
135+ tree1.commit('second', allow_pointless=True, rev_id='rev-2')
136+ branch.Branch.hooks.install_named_hook('post_checkout',
137+ self.capture_post_checkout_hook, None)
138+ # Checkout one revision
139+ tree2 = tree1.branch.create_checkout('checkout', revision_id='rev-1')
140+ self.assertEqual([('post_checkout', tree1.branch, rev1,
141+ tree2,'checkout')],
142+ self.hook_calls)
143+
144
145=== modified file 'doc/en/release-notes/bzr-2.5.txt'
146--- doc/en/release-notes/bzr-2.5.txt 2011-11-25 10:52:42 +0000
147+++ doc/en/release-notes/bzr-2.5.txt 2011-11-25 11:12:27 +0000
148@@ -147,6 +147,9 @@
149 revisions. I.e.: ``bzr pull -v -Olog_format=short`` will use the ``short``
150 format instead of the default ``long`` one. (Vincent Ladeuil, #861472)
151
152+* New hook post_checkout in bzrlib.branch fired after any checkout of a
153+ branch. (Romain Chalumeau)
154+
155 * The new config scheme allows an alternative syntax for the 'appenpath'
156 policy relying on option expansion and defining a new 'relpath' option
157 local to a section. Instead of using '<option>:policy=appendpath', the