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
=== modified file '.bzrignore'
--- .bzrignore 2010-10-13 00:26:41 +0000
+++ .bzrignore 2011-11-25 11:12:27 +0000
@@ -69,3 +69,4 @@
69bzrlib/_*.pyd69bzrlib/_*.pyd
70./.ccache70./.ccache
71.testrepository71.testrepository
72*.komodo*
7273
=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py 2011-11-22 11:48:31 +0000
+++ bzrlib/branch.py 2011-11-25 11:12:27 +0000
@@ -1480,6 +1480,13 @@
1480 lightweight)1480 lightweight)
1481 finally:1481 finally:
1482 basis_tree.unlock()1482 basis_tree.unlock()
1483
1484 # Triggers post_checkout hooks
1485 params = CheckoutHookParams(self, tree.last_revision(),
1486 tree, to_location )
1487 for hook in Branch.hooks['post_checkout']:
1488 hook(params)
1489
1483 return tree1490 return tree
14841491
1485 @needs_write_lock1492 @needs_write_lock
@@ -1895,6 +1902,10 @@
1895 "Called after a checkout switches branch. "1902 "Called after a checkout switches branch. "
1896 "post_switch is called with a "1903 "post_switch is called with a "
1897 "bzrlib.branch.SwitchHookParams.", (2, 2))1904 "bzrlib.branch.SwitchHookParams.", (2, 2))
1905 self.add_hook('post_checkout',
1906 "Called after a workingtree initialisation completes. post_checkout "
1907 " is called with a bzrlib.branch.CheckoutHookParams object and only "
1908 " runs in the bzr client.", (2, 5))
18981909
18991910
19001911
@@ -2013,6 +2024,36 @@
2013 self.revision_id)2024 self.revision_id)
20142025
20152026
2027class CheckoutHookParams(object):
2028 """Object holding parameters passed to `post_checkout` hooks.
2029
2030 :param master_branch: branch that the checkout is linked to
2031 :param revision_id: revision ID of the tree
2032 :param to_location: path of the checkout
2033 """
2034 def __init__(self, master_branch, revision_id, workingtree, to_location):
2035 """
2036 :param master_branch: branch that the checkout is linked to
2037 :param revision_id: revision ID of the tree
2038 :param to_location: path of the checkout
2039 """
2040 self.master_branch = master_branch
2041 self.revision_id = revision_id
2042 self.workingtree = workingtree
2043 self.to_location = to_location
2044
2045 def __eq__(self, other):
2046 return self.__dict__ == other.__dict__
2047
2048 def __repr__(self):
2049 return "<%s for (%s, %s, %s)>" % (self.__class__.__name__,
2050 self.master_branch,
2051 self.revision_id,
2052 self.to_location
2053 )
2054
2055
2056
2016class BranchFormatMetadir(BranchFormat):2057class BranchFormatMetadir(BranchFormat):
2017 """Common logic for meta-dir based branch formats."""2058 """Common logic for meta-dir based branch formats."""
20182059
20192060
=== modified file 'bzrlib/tests/per_branch/test_create_checkout.py'
--- bzrlib/tests/per_branch/test_create_checkout.py 2011-09-09 12:20:33 +0000
+++ bzrlib/tests/per_branch/test_create_checkout.py 2011-11-25 11:12:27 +0000
@@ -17,7 +17,7 @@
17"""Tests for the Branch.create_checkout"""17"""Tests for the Branch.create_checkout"""
1818
19from bzrlib.tests import per_branch19from bzrlib.tests import per_branch
2020from bzrlib import branch
2121
22class TestCreateCheckout(per_branch.TestCaseWithBranch):22class TestCreateCheckout(per_branch.TestCaseWithBranch):
2323
@@ -68,3 +68,55 @@
68 tree1 = self.make_branch_and_tree('base')68 tree1 = self.make_branch_and_tree('base')
69 self.build_tree(['checkout/'])69 self.build_tree(['checkout/'])
70 tree2 = tree1.branch.create_checkout('checkout', lightweight=True)70 tree2 = tree1.branch.create_checkout('checkout', lightweight=True)
71
72
73class TestCheckoutHook(per_branch.TestCaseWithBranch):
74
75 def setUp(self):
76 self.hook_calls = []
77 super(TestCheckoutHook, self).setUp()
78
79 def capture_post_checkout_hook(self, result):
80 """Capture post checkout hook calls to self.hook_calls.
81
82 The call is logged.
83 """
84 self.hook_calls.append(
85 ('post_checkout',
86 result.master_branch,
87 result.revision_id,
88 result.workingtree,
89 result.to_location
90 ))
91
92 def test_post_checkout_lightweight(self):
93 branch.Branch.hooks.install_named_hook(
94 'post_checkout', self.capture_post_checkout_hook, None)
95 a_branch = self.make_branch('branch')
96 tree = a_branch.create_checkout('checkout', lightweight=True)
97 self.assertEqual([('post_checkout', a_branch, tree.last_revision(),
98 tree,'checkout')],
99 self.hook_calls)
100
101 def test_post_checkout_heavyweight(self):
102 branch.Branch.hooks.install_named_hook(
103 'post_checkout', self.capture_post_checkout_hook, None)
104 a_branch = self.make_branch('branch')
105 tree = a_branch.create_checkout('checkout', lightweight=False)
106 self.assertEqual([('post_checkout', a_branch, tree.last_revision(),
107 tree,'checkout')],
108 self.hook_calls)
109
110 def test_post_checkout_earlier_revision(self):
111 #Create a branch with two revisions
112 tree1 = self.make_branch_and_tree('base')
113 rev1 = tree1.commit('first', allow_pointless=True, rev_id='rev-1')
114 tree1.commit('second', allow_pointless=True, rev_id='rev-2')
115 branch.Branch.hooks.install_named_hook('post_checkout',
116 self.capture_post_checkout_hook, None)
117 # Checkout one revision
118 tree2 = tree1.branch.create_checkout('checkout', revision_id='rev-1')
119 self.assertEqual([('post_checkout', tree1.branch, rev1,
120 tree2,'checkout')],
121 self.hook_calls)
122
71123
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt 2011-11-25 10:52:42 +0000
+++ doc/en/release-notes/bzr-2.5.txt 2011-11-25 11:12:27 +0000
@@ -147,6 +147,9 @@
147 revisions. I.e.: ``bzr pull -v -Olog_format=short`` will use the ``short``147 revisions. I.e.: ``bzr pull -v -Olog_format=short`` will use the ``short``
148 format instead of the default ``long`` one. (Vincent Ladeuil, #861472)148 format instead of the default ``long`` one. (Vincent Ladeuil, #861472)
149149
150* New hook post_checkout in bzrlib.branch fired after any checkout of a
151 branch. (Romain Chalumeau)
152
150* The new config scheme allows an alternative syntax for the 'appenpath'153* The new config scheme allows an alternative syntax for the 'appenpath'
151 policy relying on option expansion and defining a new 'relpath' option154 policy relying on option expansion and defining a new 'relpath' option
152 local to a section. Instead of using '<option>:policy=appendpath', the155 local to a section. Instead of using '<option>:policy=appendpath', the