Merge lp:~shawn111/bzr/lp_propose_message into lp:bzr

[Shawn Wang]

Use initial_comment as commit_message for lp_propose.(Shawn Wang)

use initial_comment as commit_message for lp_propose

[Richard Wilbur (richard-wilbur)]

I was interested to see your branch but when I visit the merge request, even though you filed it 9 hours ago, it still says "Updating diff... An updated diff will be available in a few minutes. Reload to see the changes." I reload with no change to what is displayed.

I've seen this problem occur in the past when someone created a stacked branch of lp:bzr.

Here's the recipe I use:

bzr branch lp:bzr [local_branch_name_if_not_bzr]
cd {bzr|local_branch_name_if_not_bzr}
[edit the file(s) with your favorite editor]
./bzr selftest # To make sure you didn't break anything.
bzr commit -m 'Describe your changes.'
bzr push lp:~shawn111/bzr/lp_propose_message

Then visit here:

https://code.launchpad.net/~shawn111/bzr/lp_propose_message/+register-merge

Fill out the form and submit.

review: Needs Information (no changes visible.)

[Richard Wilbur (richard-wilbur)]

Shawn,

I did some more investigation and your branch landed fine in Launchpad but the branch summary page and merge review page don't show your revisions. Then I ran into the same problem on one of my own branches. I just filed a question on Launchpad regarding this problem.[0] I expect to hear back from them soon and get this merge on it's way.

Thanks for the contribution. If I understand it correctly, you are proposing to make the last commit message on a branch be the default merge comment for the merge proposal. This sounds like a decent default.

Sincerely,
Richard

Reference:
[0] https://answers.launchpad.net/launchpad/+question/267119

[Richard Wilbur (richard-wilbur)]

Launchpad admins poked it and after reattempting the branch scan, everything seems to be in order on the branch and review pages.

Colin Watson mentioned in his answer to my question that the way Launchpad stores Bazaar branches turns out to make merging new branches on a project with deep history take so much time that the operation often times out. He mentioned that William Grant knows more about this and that it would most likely get some attention after the Launchpad repository (deep history with many branches) is converted over to git (which will presumably lower the processing pressure on other projects).

[Richard Wilbur (richard-wilbur)]

Now that I have looked at the diff and the surrounding code in bzrlib/plugins/launchpad/lp_propose.py, I have a comment and a question:

Comment -- Thanks for finally making use of the commit message passed as a parameter into the Proposer class constructor.

Question -- Why lock the source and target branches and look up all the info to create a default message if we are going to ignore it and use the message passed to the Proposer constructor? I'd recommend flipping this routine to first try using self.commit_message, then only if it is None create the other message.

review: Needs Fixing

[Shawn Wang (shawn111)]

Hi Richard,

Thanks for your review.
I updated the code.

Please help me review again.

[Richard Wilbur (richard-wilbur)]

Nicely done. Now the only thing left to replicate the same treatment as 'initial_comment' (since I don't know the progeny of the string passed in to the Proposer constructor), is to apply strip() and encode('utf-8') to the string in the return statement. Example below:

return self.commit_message.strip().encode('utf-8')

review: Needs Fixing

[Shawn Wang (shawn111)]

@Richard,

I thought it is not necessary to have self.commit_message.strip().encode('utf-8').
You can refer below two pieces of code.
in bzrlib/plugins/launchpad/cmds.py, the option 'message' is type=unicode.
in bzrlib/plugins/launchpad/lp_propose.py, self.call_webservice already use commit_message=self.commit_message.

bzrlib/plugins/launchpad/cmds.py

class cmd_lp_propose_merge(Command):
...
    takes_options = [Option('staging',
                            help='Propose the merge on staging.'),
                     Option('message', short_name='m', type=unicode,
                            help='Commit message.'),
                     Option('approve',
                            help=('Mark the proposal as approved immediately, '
                                  'setting the approved revision to tip.')),
                     Option('fixes', 'The bug this proposal fixes.', str),
                     ListOption('review', short_name='R', type=unicode,
                            help='Requested reviewer and optional type.')]

bzrlib/plugins/launchpad/lp_propose.py
    def create_proposal(self):
        """Perform the submission."""
...
        initial_comment = self.get_comment(prerequisite_branch)
        mp = self.call_webservice(
            self.source_branch.lp.createMergeProposal,
            target_branch=self.target_branch.lp,
            prerequisite_branch=prereq,
            initial_comment=initial_comment,
            commit_message=self.commit_message, reviewers=reviewers,
            review_types=review_types)

[Richard Wilbur (richard-wilbur)]

Turns out I needed to learn more about Unicode so I did a bit of reading online. I found some useful descriptions of Unicode[0] and the UTF-8 encoding[1] on Wikipedia. Then I looked up specific information concerning Python support of Unicode and found a page documenting Python 2.7 usage.[2] An interesting excerpt from the Python documentation:
-------------------------------------------------
Tips for Writing Unicode-aware Programs

This section provides some suggestions on writing software that deals with Unicode.

The most important tip is:
    Software should only work with Unicode strings internally, converting to a particular encoding on output.
-------------------------------------------------
I appreciate your observation that the message string is a unicode object. That means we support multi-lingual input. It looks to me like in order to return the same type of string from Proposer.get_comment as before, we still want to remove leading and trailing whitespace (strip), and, from my reading, we still need to convert it to 'utf-8' for output (encode 'utf-8'), as that is the expected encoding for HTML.

References:
[0] https://en.wikipedia.org/wiki/Unicode
[1] https://en.wikipedia.org/wiki/UTF-8
[2] https://docs.python.org/2/howto/unicode.html

[Shawn Wang (shawn111)]

> Turns out I needed to learn more about Unicode so I did a bit of reading
> online. I found some useful descriptions of Unicode[0] and the UTF-8
> encoding[1] on Wikipedia. Then I looked up specific information concerning
> Python support of Unicode and found a page documenting Python 2.7 usage.[2]
> An interesting excerpt from the Python documentation:
> -------------------------------------------------
> Tips for Writing Unicode-aware Programs
>
> This section provides some suggestions on writing software that deals with
> Unicode.
>
> The most important tip is:
> Software should only work with Unicode strings internally, converting to a
> particular encoding on output.
> -------------------------------------------------
> I appreciate your observation that the message string is a unicode object.
> That means we support multi-lingual input. It looks to me like in order to
> return the same type of string from Proposer.get_comment as before, we still
> want to remove leading and trailing whitespace (strip), and, from my reading,
> we still need to convert it to 'utf-8' for output (encode 'utf-8'), as that is
> the expected encoding for HTML.
>
> References:
> [0] https://en.wikipedia.org/wiki/Unicode
> [1] https://en.wikipedia.org/wiki/UTF-8
> [2] https://docs.python.org/2/howto/unicode.html

Actually, I often confusing python2 and python3 unicode or utf-8 related functions.
Thanks, I updated the code, again.

[Richard Wilbur (richard-wilbur)]

Thanks Shawn for all the work. Looks good to me.
+1

review: Approve

[Shawn Wang (shawn111)]

@Richard,

Cool, thank you very much.

[Richard Wilbur (richard-wilbur)]

I guess Vincent is pretty busy. I'll go ahead and mark this request approved so we can merge it.

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

=== modified file 'bzrlib/plugins/launchpad/lp_propose.py'
--- bzrlib/plugins/launchpad/lp_propose.py	2012-10-22 16:25:40 +0000
+++ bzrlib/plugins/launchpad/lp_propose.py	2015-05-25 03:42:56 +0000
@@ -94,6 +94,8 @@
 
     def get_comment(self, prerequisite_branch):
         """Determine the initial comment for the merge proposal."""
+        if self.commit_message is not None:
+            return self.commit_message.strip().encode('utf-8')
         info = ["Source: %s\n" % self.source_branch.lp.bzr_identity]
         info.append("Target: %s\n" % self.target_branch.lp.bzr_identity)
         if prerequisite_branch is not None: