Switch ChangeDescription usage to be stricter.

Switch ChangeDescription usage to be stricter.

Include all the preparatory work to eventually update the R= line to match the
actual reviewers.

The goal here is that ChangeDescription becomes the official implementation for
handling and modifying commit messages accross git-cl, gcl and the commit queue.

This change does slightly tweak the spacing between the hot lines.
It is done on purpose to make them consistent.

R=dpranke@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=193514

[M-A Ruel, 2013-04-08 21:26:15]

(I had forgot to send the email :/)

[iannucci, 2013-04-08 22:49:40]

https://codereview.chromium.org/13741015/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode810
git_cl.py:810: def update_reviewers(self, reviewers):
Can we make reviewers a list of reviewers instead of expecting the
comma-delimited string? That will make it easier to manipulate and makes this
interface more logical.

[Dirk Pranke, 2013-04-09 00:07:52]

The change basically looks good to me, but a few minor comments.

https://codereview.chromium.org/13741015/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode810
git_cl.py:810: def update_reviewers(self, reviewers):
On 2013/04/08 22:49:40, iannucci wrote:
> Can we make reviewers a list of reviewers instead of expecting the
> comma-delimited string? That will make it easier to manipulate and makes this
> interface more logical.

I don't think we can easily do this without pushing logic into the client that
we don't want to be there.

An alternative might be to provide static methods that did the converstion
(splitting/joining).

If we did make reviewers a list internally, we'd also want to split it out as a
separate member variable distinct from the free-text part of the description.

There is a certain self-enforcing consistency with using self._description for
all of the fields, at least.

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode822
git_cl.py:822: self.description[matches[i].end()+1:])
switching back and forth between self._description and self.description is
confusing. You should use one or the other consistently.

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode856
git_cl.py:856: def append_line(self, line):
I would probably call this something like add_keyword, since (I think?) we don't
want this to be used to generically append lines, right?

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode1148
git_cl.py:1148: if CHANGE_ID not in change_desc.description:
I wonder if we should restructure this so that we add the change id during the
constructor (optionally?)

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode1468
git_cl.py:1468: # Keep a separate copy for the commit message.
Can you clarify *why* we need a separate copy here?

[M-A Ruel, 2013-04-11 00:01:38]

ptal, I rebased on HEAD so it now uses all .reviewers as list.

https://codereview.chromium.org/13741015/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode810
git_cl.py:810: def update_reviewers(self, reviewers):
So reviewers is now a list all the time.

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode822
git_cl.py:822: self.description[matches[i].end()+1:])
On 2013/04/09 00:07:52, Dirk Pranke wrote:
> switching back and forth between self._description and self.description is
> confusing. You should use one or the other consistently.

Done.

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode1148
git_cl.py:1148: if CHANGE_ID not in change_desc.description:
On 2013/04/09 00:07:52, Dirk Pranke wrote:
> I wonder if we should restructure this so that we add the change id during the
> constructor (optionally?)

I prefer to keep this out of this CL since it invokes testing with repo style
hooks and there is exactly 0 unit test covering this use case.

https://codereview.chromium.org/13741015/diff/1/git_cl.py#newcode1468
git_cl.py:1468: # Keep a separate copy for the commit message.
On 2013/04/09 00:07:52, Dirk Pranke wrote:
> Can you clarify *why* we need a separate copy here?

Done.

[Dirk Pranke, 2013-04-11 00:10:16]

lgtm.

https://codereview.chromium.org/13741015/diff/9001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/13741015/diff/9001/git_cl.py#newcode858
git_cl.py:858: # Adds a LF if the last line did not have 'FOO=BAR' or if the new
one isn't.
I still am not fond of the name for this method, nor the complexity of the
logic, but we can fix this later, I suppose ...

[M-A Ruel, 2013-04-11 00:12:09]

https://codereview.chromium.org/13741015/diff/9001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/13741015/diff/9001/git_cl.py#newcode858
git_cl.py:858: # Adds a LF if the last line did not have 'FOO=BAR' or if the new
one isn't.
On 2013/04/11 00:10:16, Dirk Pranke wrote:
> I still am not fond of the name for this method, nor the complexity of the
> logic, but we can fix this later, I suppose ...

I changed it to append_footer().

[commit-bot: I haz the power, 2013-04-11 00:15:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/13741015/10004

[commit-bot: I haz the power, 2013-04-11 00:17:52]

Change committed as 193514