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

[Andrew Bennetts]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clean-reject.patch
Type: text/x-diff
Size: 39268 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080724/9c6e53cf/attachment-0001.bin