[MERGE][Take Five] Show the diff in the commit messages

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Semi-approved
Comment:
I think changes can be done by the person who does the merge, but there 
are a
couple:

+def make_commit_message_template_encoded(working_tree, specific_files,
+                                 diff=None,
+                                 output_encoding=None):

^- I would have expected parameters to either be only indented a little, 
or
indented to line up with the the opening parenthesis. I'm not sure why 
you are
choosing to indent them to "almost lined up".

+    """Prepare a template file for a commit into a branch.
+
+    Returns an encoded string.
+    """
+    # TODO: Should probably be given the WorkingTree not the branch
+    #
+    # TODO: make provision for this to be overridden or modified by a 
hook
+    #
+    # TODO: Rather than running the status command, should prepare a 
draft of
+    # the revision to be committed, then pause and ask the user to
+    # confirm/write a message.
+    from StringIO import StringIO       # must be unicode-safe
+    from bzrlib.diff import show_diff_trees
+

^- I don't think we need to use StringIO.StringIO anymore, because
show_diff_trees is the only place that would be using it, and *it* 
should only
be using 8-bit characters.

+    template = make_commit_message_template(working_tree, 
specific_files).encode(output_encoding, "replace")
+

^- You allow the parameter to be None (output_encoding=None), but then 
you use
it unconditionally on the output of 'template'. Which means that you can 
get:

foo.encode(None, 'replace')

Which will raise an exception.

I think we can just add a:

if output_encoding is None:
   output_encoding = 'utf-8'

Or raise an error, so that callers know they cannot pass None.


+    if diff:
+        stream = StringIO()
+        template += "\n"
+        show_diff_trees(working_tree.basis_tree(),
+                working_tree, stream, specific_files,
+                path_encoding=output_encoding)
+        template = template + stream.getvalue()

^- In place of:

   template += "\n"

   I would just do:

   stream.write('\n')

   or

   template = template + '\n' + stream.getvalue()

   at the end. Otherwise we need to rebuild the string an extra time.


+    if not start_message is None:
+        start_message = start_message.encode(bzrlib.user_encoding)
+    return 
edit_commit_message_encoded(infotext.encode(bzrlib.user_encoding),
+                               ignoreline,
+                               start_message)
+
+def edit_commit_message_encoded(infotext, 
ignoreline=DEFAULT_IGNORE_LINE,

^- There should be two blank spaces between top-level functions.


The NEWS file is formated in reStructuredText so lines like:
+    * Add the option user_encoding to the function
+      - show_diff_trees
+      in order to move the user encoding at the UI level. (Goffredo 
Baroncelli)
+

Should have ``show_diff_trees``.


Rather than wait for him to return, I'll be happy to clean this up when
merging, if it gets approved by someone else.




For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C200707270045.12688.kreijack%40tiscalinet.it%3E