Code review comment for lp:~matsubara/tarmac/bundle-lander-fixes

Cool, thank you.

In the abstract, I feel like we ought to keep the try: except handling in commands.py (cmd_bundle_merge). If a tarmac_bundle_merge hook raises an error, we should still inform the merge proposal about it. Our hook no longer raises the usual errors--but if it raises an unexpected error, isn't that even more important to report? And keeping the logic that reverts the branch in commands.py better matches the basic code structure. Moreover, I think we could change the exception we raise in the hook to have all the collected information that we want in the email, so that we could follow the usual tarmac pattern, letting cmd_bundle_merge handle the reversion, and still get the emails we want.

I'd like that change, but given your schedule and our schedule, that change requires time we don't have. We can put it in the category of "things to revisit when proposing a merge to trunk" or maybe "things for Gary or Benji or Ursula to look at next week." Maybe just an XXX with a quick explanation of the issue? A bug would be extra credit. :-)

Quick tweaks:

=== modified file 'tarmac/bin/commands.py'
    # proposal) tuple twice, once for each pre and pos commit hook.
pos -> post

=== modified file 'tarmac/plugins/bundlecommand.py'
    # Uncommit and revert changes up to the point where target
    # was before the merge.
Per my comments above, please add an additional comment explaining that we normally expect the tarmac command to do this, We are doing it here because of the emails, but we hope to change it in the future.

tarmac/tests/test_commands.py : maybe comment out the test, rather than removing it, per my comments above?

I'll approve this, so you can commit with just these small changes, but if you disagree with my comments about the larger future changes, please raise your concerns, here or on another channel (in which case you or I can summarize here afterwards).

Thank you