[MERGE] Add mechanism for pre_change_branch_tip hook to cleanly reject a change.

[John Arbash Meinel]

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

Andrew Bennetts wrote:
| John Arbash Meinel wrote:
| [...]
|> @@ -1522,11 +1524,18 @@
|> ~         check_not_reserved_id = _mod_revision.check_not_reserved_id
|> ~         for rev_id in rev_history:
|> ~             check_not_reserved_id(rev_id)
|> +        old_revno, old_revid = self.last_revision_info()
|> +        if len(rev_history) == 0:
|> +            revid = _mod_revision.NULL_REVISION
|> +        else:
|> +            revid = rev_history[-1]
|> +        self._run_pre_change_branch_tip_hooks(len(rev_history), revid)
|>
|> ^- Why did you chose to move it into set_revision_history() ? Just in
|> case something else called it directly?
|
| Not just in case, but because something else already does call it
directly.  In
| addition to Branch.set_last_revision_info there's
| Branch.generate_revision_history.
|
|> Should we have a test for that?
|
| I think what's needed is to add test_hooks tests to verify that *all*
methods
| that can the tip run the relevant hooks.
|
| That still leaves a gap that if there's a Branch method we forget (or
add later)
| that does change the tip then our test suite probably won't notice.
I'm not
| sure that there's any practical solutions to that other than vigilance :(
|
| Anyway, I've added per-implementation tests that set_revision_history,
| set_last_revision_info, test_generate_revision_history, test_pull and
test_push
| will invoke the pre/post_change_branch_tip hooks.
|
|> +class HookFailed(BzrError):
| [...]
|> +
|> +
|> +class TipChangeRejected(BzrError):
| [...]
|> ^- A docstring for when these are used, and what they represent would be
|> nice.
|
| Fair enough.  Here are the docstrings:
|
| class HookFailed(BzrError):
|     """Raised when a pre_change_branch_tip hook function fails
anything other
|     than TipChangeRejected.
|     """
|
| class TipChangeRejected(BzrError):
|     """A pre_change_branch_tip hook function may raise this to cleanly and
|     explicitly abort a change to a branch tip.
|     """
|
| (Possibly we should make all hook runners raise HookFailed if a hook
function
| has an unexpected exception, but I'm not sure it matters too much.  For
| pre_change_branch_tip it's helpful because it's good to have a clear
distinction
| between an “acceptable” error and a real error.  I guess it would at
least help
| make it clear when a traceback is the fault of a hook rather than bzr
itself.)
|
|> +    def test_tip_change_rejected(self):
| [...]
|> ^- you should really be using try/finally or addCleanup rather than a
|> bare unlock.
|
| Ok.
|
|> Also, shouldn't you be asserting the last revision hasn't changed?
|
| Not here.  That's a matter for the Branch object on the server-side.
The tests
| in test_remote are unit tests about the behaviour of the client-side
objects,
| the objects that are in bzrlib/remote.py.  The relevant thing to
verify in this
| test method is that RemoteBranch will raise TipChangeRejected (with
the intended
| message) if it gets a certain network response after sending a request
to change
| the tip.  In these tests, there is no actual underlying branch because the
| Remote* objects are using a stub SmartMedium (really a “Test Spy”
rather than a
| “Test Stub” using the terminology of the xUnit Test Patterns book, I
think).
|
| I've added a docstring to the test method to clarify its purpose:
|
|     def test_tip_change_rejected(self):
|         """TipChangeRejected responses cause a TipChangeRejected
exception to
|         be raised.
|         """
|
| FWIW, there are already per-branch-implementation tests in
test_hooks.py that
| the last revision doesn't change when pre_change_branch_tip raises an
error.
|
|> You should also have direct tests in test_errors() for the formatting of
|> these exceptions.
|
| Done.
|
|> And I wonder if we would want to add:
|>
|> _fmt = 'Tip change rejected: %(msg)s'
|>
|> Have you tried it in 'reality' so that we know what it looks like when
|> it fails?
|
| I have.  I think that would be good addition; done.
|
|> I really like the functionality, but it seems to need a bit more polish.
|>
|> BB:resubmit.
|
| Ok.  Updated patch attached.
|
| -Andrew.
|

You sort of snuck in changes to bzrlib/errors.py to return Unicode
strings from __unicode__ without mentioning it, but that seems like a
fine change.

BB:approve

John
=:->

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

iEYEARECAAYFAkiIlCMACgkQJdeBCYSNAAPYOgCfQqKU3fU3Owbne6/6OsSUjKzx
yYIAoJIfjNFcNBQ/FSTnPHssE807UVNg
=JxuD
-----END PGP SIGNATURE-----