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

[Goffredo Baroncelli]

I updated the patch; below the details

Goffredo

On Friday 27 July 2007, John Arbash Meinel wrote:
> 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".

Sometime the fingers are faster than the brain :-)); however correct

> 
> +    """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.

StringIO is require by the function show_diff_tree
> 
> +    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'

Correct

> 
> +    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.

Updated
 
> 
> +    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.
Correct

> 
> 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.
> 
Correct

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



-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) 
<kreijack_AT_inwind.it>
Key fingerprint = CE3C 7E01 6782 30A3 5B87  87C0 BB86 505C 6B2A CFF9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-ci-diff.bundle.patch
Type: text/x-diff
Size: 32686 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070813/c6a40d99/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070813/c6a40d99/attachment-0001.pgp