[MERGE/RFC] Merge directive support

[John Arbash Meinel]

John Arbash Meinel has voted +1 (conditional).
Status is now: Conditionally approved
Comment:
I'm guessing we want to change the format from 'experimental-1' once it 
is merged into bzr.dev.

You don't have a doc string on the MergeDirective class, or on any of 
its functions.

It seems like the patch_type handler should be using a dictionary rather 
than if/elif statements. But at the very least, you need an 'else:' in 
there to handle unknown patch formats.


'import smtplib' should really be done at the top of 
test_merge_directive, rather than in the middle of a function.

It would be easier to follow if you broke the test up into multiple 
tests.

It seems that the default diff you generate is using the 'a/' and 'b/' 
prefixes. I believe this is simply because the default for the internal 
diff function is to use prefixes.

You don't have to separately decode the 'sign' character. You can just 
do:

'%+04d' % (offset,)

That will put +3000 if the number is positive, and -3000 as appropriate.
(I'm happy to see those functions moved into a separate helper, though I 
wonder if we need to import them into the old location for 
compatibility)

We should probably have a specific test on the serialized output (the 
text form), since this is a transmitted text, we need to make sure it 
doesn't change between releases. It seems like you have a test at the 
EMAIL layer, though, so maybe that is enough.

The indenting on cmd_merge_directive.takes_options is a little hard to 
read. I would prefer if the nested scope had extra indentation.

We should promote "public_branch" to being an api call, just like we do 
for 'get_submit_branch' and others. Since we have used it enough times 
that we know it is something we want to have in core. I won't block this 
on doing it, but we should keep it in mind.

One problem with gpg signing a patch, is that it makes the '-' lines a 
little ugly. Specifically:
- -def format_highres_date(t, offset=0):
- -    """Format a date, such that it includes higher precision in the
- -    seconds field.

This doesn't have a whole lot to do with your patch, but it is probably 
something that we want to teach BundleBuggy about.


Is there a reason you used this indentation:
+    @staticmethod
+    def from_kwargs(name_, help=None, title=None, value_switches=False,
+        enum_switch=True, **kwargs):
+        """Convenience method to generate string-map registry options

Instead of:

+    @staticmethod
+    def from_kwargs(name_, help=None, title=None, value_switches=False,
+                    enum_switch=True, **kwargs):
+        """Convenience method to generate string-map registry options

I generally prefer the latter, but if there is a reason to use the 
former...

You need docstrings for the new functions in rio.py
Also calling them "to_patch_lines" and "read_patch_stanza" doesn't quite 
fit. These really aren't "patch lines". Maybe "to_width_limited" or 
something like that.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C45EDAB88.6010305%40utoronto.ca%3E