Code review comment for lp:~rom1-chal/bzr/post_checkout_hook

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.

« Back to merge proposal