Make gcl use git_cl.py code for consistency in the CL description formatting.

Make gcl use git_cl.py code for consistency in the CL description formatting.

This way, git_cl.py's ChangeDescription becomes the canonical CL description
handler.

R=dpranke@chromium.org
BUG=

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

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

FTR, this CL depends on https://codereview.chromium.org/13741015.

[jam, 2013-04-08 22:40:14]

is this going to add R= to all commit messages?

[Dirk Pranke, 2013-04-08 22:46:32]

On 2013/04/08 22:40:14, jam wrote:
> is this going to add R= to all commit messages?

I haven't looked at the patch yet, but with regards to the larger issue, I don't
think we've decided yet what the policy will be. So far I think we've considered
three different options:

1) Alway add R= to every patch across all rietveld-using repos (including both
chromium and blink, obviously)

2) Make this configurable per-repo, so chromium might not do this, but blink
does

3) Only update the R= field if it is already in the description, so you could
avoid this by not having an R= in your changes.

I know you're not a fan of 1), but it's not clear to me if you're in the
minority here or not. I'm not sure which of any of these might get the most
votes yet, either, although I think everyone would agree that having an R= that
doesn't actually indicate who reviewed it should be fixed :).

[M-A Ruel, 2013-04-08 22:50:59]

On 2013/04/08 22:40:14, jam wrote:
> is this going to add R= to all commit messages?

No, please comment on the relevant CLs;
gcl/git-cl R= line: https://codereview.chromium.org/13800021
Corresponding CQ R= line: https://codereview.chromium.org/13483026

[jam, 2013-04-08 22:58:39]

There are 3 cls. can we discuss them in one place? It would be good to get
a description of what all these cl's are doing.

will https://codereview.chromium.org/13800021 update the commit message
with R=list of reviewers? I don't think we should do this. The example
given in the description can be done quickly by clicking on a link. It
seems redundant to copy information from the code review into the commit
message. i.e. why not also add the CC'd list, anyone else who commented,
etc.


On Mon, Apr 8, 2013 at 3:50 PM, <maruel@chromium.org> wrote:

> On 2013/04/08 22:40:14, jam wrote:
>
>> is this going to add R= to all commit messages?
>>
>
> No, please comment on the relevant CLs;
> gcl/git-cl R= line:
https://codereview.chromium.**org/13800021<https://codereview.chromium.org/13...
> Corresponding CQ R= line:
https://codereview.chromium.**org/13483026<https://codereview.chromium.org/13...
>
>
https://codereview.chromium.**org/13800018/<https://codereview.chromium.org/1...
>

[M-A Ruel, 2013-04-08 23:08:52]

On 2013/04/08 22:58:39, jam wrote:
> There are 3 cls. can we discuss them in one place?

Please discuss the R= line change at https://codereview.chromium.org/13800021.
It does what the CL description says.

This specific CL does exactly what the description says. This change has a small
effect in the whitespace formatting, as shown in the unit tests. It is mainly
caused by enforcing (finally!) consistent CL description formatting between the
tools. The whitespace formatting was slightly tweaked in
https://codereview.chromium.org/13741015. I changed the CL description to make
this clear. Feel free to comment about the whitespace formatting in
https://codereview.chromium.org/13741015.

[Dirk Pranke, 2013-04-08 23:36:37]

On 2013/04/08 23:08:52, Marc-Antoine Ruel wrote:
> On 2013/04/08 22:58:39, jam wrote:
> > There are 3 cls. can we discuss them in one place?
> 
> Please discuss the R= line change at https://codereview.chromium.org/13800021.
> It does what the CL description says.
> 
> This specific CL does exactly what the description says. This change has a
small
> effect in the whitespace formatting, as shown in the unit tests. It is mainly
> caused by enforcing (finally!) consistent CL description formatting between
the
> tools. The whitespace formatting was slightly tweaked in
> https://codereview.chromium.org/13741015. I changed the CL description to make
> this clear. Feel free to comment about the whitespace formatting in
> https://codereview.chromium.org/13741015.

Actually, I don't think trying to discuss the meta-level process questions is a
good thing to do in these CLs. Let's have that discussion on blink-dev and/or
chromium-dev so that more people can see it.

These patches concern themselves w/ the implementation details and we shouldn't
confuse the two (especially since we want to make most if not all of these
changes regardless of what we decide to do, processwise).

[Dirk Pranke, 2013-04-09 00:13:24]

the change basically looks fine; I expect this'll be a rubber stamp once we've
rebased after the git_cl change lands.

https://codereview.chromium.org/13800018/diff/1/gcl.py
File gcl.py (right):

https://codereview.chromium.org/13800018/diff/1/gcl.py#newcode275
gcl.py:275: # Act as an interface to git_cl.ChangeDescription.
I would probably rephrase this as "share the description processing with
git_cl".

https://codereview.chromium.org/13800018/diff/1/gcl.py#newcode1029
gcl.py:1029: change_info.append_line('Committed: ' + viewvc_url + revision)
I think in the previous CL I suggested renaming "append_line()" to "add_keyword"
or something. I see now that that wouldn't work for this, so maybe this should
be renamed to "add_footer" or something?

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

https://codereview.chromium.org/13800018/diff/1/gcl.py
File gcl.py (right):

https://codereview.chromium.org/13800018/diff/1/gcl.py#newcode275
gcl.py:275: # Act as an interface to git_cl.ChangeDescription.
On 2013/04/09 00:13:24, Dirk Pranke wrote:
> I would probably rephrase this as "share the description processing with
> git_cl".

Done.

https://codereview.chromium.org/13800018/diff/1/gcl.py#newcode1029
gcl.py:1029: change_info.append_line('Committed: ' + viewvc_url + revision)
On 2013/04/09 00:13:24, Dirk Pranke wrote:
> I think in the previous CL I suggested renaming "append_line()" to
"add_keyword"
> or something. I see now that that wouldn't work for this, so maybe this should
> be renamed to "add_footer" or something?

Done.

[Dirk Pranke, 2013-04-11 00:39:12]

lgtm

[commit-bot: I haz the power, 2013-04-11 00:46:27]

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

[commit-bot: I haz the power, 2013-04-11 00:48:46]

Change committed as 193525