Add gerrit Change-Id to commit messages if they do not have one.

Add gerrit Change-Id to commit messages if they do not have one.


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

[Siva Chandra, 2012-10-19 23:19:12]

Verified. Existing tests fixed. Working on a test for the added functionality.

[Siva Chandra, 2012-10-19 23:33:34]

Test for the new functionality is also in.

[cmp, 2012-10-19 23:47:08]

https://codereview.chromium.org/11236013/diff/1003/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode991
git_cl.py:991: def AddChangeIdToCommitMessage(message):
insert an empty line before line 991

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode992
git_cl.py:992: git_command = ['commit', '-a', '--amend', '-m %s' % message]
I'm surprised this works.  I expect the list to need to look like:
['commit', '-a', '--amend', '-m', message]
That is, that the -m arg needs to be on its own.  If both work, let's use the
latter.

before line 992, add a comment to the effect """Re-commits using the current
message, assumes the commit hook is in place."""

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode994
git_cl.py:994: print "git-cl: Added Change-Id to commit message."
before line 994, can we add a line that gets the new log message and asserts
that now re.search(CHANGE_ID_STR, ...) is in the message?  if it's not there, we
should add an ERROR: message to stderr (but still continue).

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode996
git_cl.py:996: def GerritUpload(options, args, cl):
insert an empty line before line 996

[M-A Ruel, 2012-10-20 00:01:39]

https://codereview.chromium.org/11236013/diff/1003/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode992
git_cl.py:992: git_command = ['commit', '-a', '--amend', '-m %s' % message]
On 2012/10/19 23:47:08, cmp wrote:
> I'm surprised this works. I expect the list to need to look like:
> ['commit', '-a', '--amend', '-m', message]
> That is, that the -m arg needs to be on its own.  If both work, let's use the
> latter.

Chase is right. In addition, using the -a flag means whatever local changes not
in the index will be silently added to the commit. This is bad. Remove it.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode1006
git_cl.py:1006: if not re.search(CHANGE_ID_STR, log_desc):
you probably want:
if CHANGE_ID_STR not in log_desc:

[Siva Chandra, 2012-10-20 00:31:24]

PTAL

https://codereview.chromium.org/11236013/diff/1003/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode991
git_cl.py:991: def AddChangeIdToCommitMessage(message):
On 2012/10/19 23:47:08, cmp wrote:
> insert an empty line before line 991

Done.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode992
git_cl.py:992: git_command = ['commit', '-a', '--amend', '-m %s' % message]
On 2012/10/19 23:47:08, cmp wrote:
> I'm surprised this works.  I expect the list to need to look like:
> ['commit', '-a', '--amend', '-m', message]
> That is, that the -m arg needs to be on its own.  If both work, let's use the
> latter.
> 
> before line 992, add a comment to the effect """Re-commits using the current
> message, assumes the commit hook is in place."""

Done.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode992
git_cl.py:992: git_command = ['commit', '-a', '--amend', '-m %s' % message]
On 2012/10/20 00:01:39, Marc-Antoine Ruel wrote:
> On 2012/10/19 23:47:08, cmp wrote:
> > I'm surprised this works. I expect the list to need to look like:
> > ['commit', '-a', '--amend', '-m', message]
> > That is, that the -m arg needs to be on its own.  If both work, let's use
the
> > latter.
> 
> Chase is right. In addition, using the -a flag means whatever local changes
not
> in the index will be silently added to the commit. This is bad. Remove it.

Done.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode994
git_cl.py:994: print "git-cl: Added Change-Id to commit message."
On 2012/10/19 23:47:08, cmp wrote:
> before line 994, can we add a line that gets the new log message and asserts
> that now re.search(CHANGE_ID_STR, ...) is in the message?  if it's not there,
we
> should add an ERROR: message to stderr (but still continue).

Done.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode996
git_cl.py:996: def GerritUpload(options, args, cl):
On 2012/10/19 23:47:08, cmp wrote:
> insert an empty line before line 996

Done.

https://codereview.chromium.org/11236013/diff/1003/git_cl.py#newcode1006
git_cl.py:1006: if not re.search(CHANGE_ID_STR, log_desc):
On 2012/10/20 00:01:39, Marc-Antoine Ruel wrote:
> you probably want:
> if CHANGE_ID_STR not in log_desc:

Done.

[cmp, 2012-10-20 22:42:31]

https://codereview.chromium.org/11236013/diff/6001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode42
git_cl.py:42: CHANGE_ID_STR = 'Change-Id:'
drop _STR from variable name (here and below)

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode1001
git_cl.py:1001: print "git-cl: Added Change-Id to commit message."
use single quotes here rather than double quotes

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode1003
git_cl.py:1003: sys.stderr.write("ERROR: Gerrit commit-msg hook not
available.\n")
use:
  print >>sys.stderr, 'ERROR: Gerrit commit-msg hook not running.'

https://codereview.chromium.org/11236013/diff/6001/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/11236013/diff/6001/tests/git_cl_test.py#newco...
tests/git_cl_test.py:398: if git_cl.CHANGE_ID_STR not in description:
CHANGE_ID_STR -> CHANGE_ID here, too

[Siva Chandra, 2012-10-22 07:13:55]

Addressed cmp's comments in patch set 4.
PTAL

https://codereview.chromium.org/11236013/diff/6001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode42
git_cl.py:42: CHANGE_ID_STR = 'Change-Id:'
On 2012/10/20 22:42:31, cmp wrote:
> drop _STR from variable name (here and below)

Done.

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode1001
git_cl.py:1001: print "git-cl: Added Change-Id to commit message."
On 2012/10/20 22:42:31, cmp wrote:
> use single quotes here rather than double quotes

Done.

https://codereview.chromium.org/11236013/diff/6001/git_cl.py#newcode1003
git_cl.py:1003: sys.stderr.write("ERROR: Gerrit commit-msg hook not
available.\n")
On 2012/10/20 22:42:31, cmp wrote:
> use:
>   print >>sys.stderr, 'ERROR: Gerrit commit-msg hook not running.'

Done.

https://codereview.chromium.org/11236013/diff/6001/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/11236013/diff/6001/tests/git_cl_test.py#newco...
tests/git_cl_test.py:398: if git_cl.CHANGE_ID_STR not in description:
On 2012/10/20 22:42:31, cmp wrote:
> CHANGE_ID_STR -> CHANGE_ID here, too

Done.

[cmp_google, 2012-10-22 19:27:31]

lgtm, please land using commit checkbox on this CL

[commit-bot: I haz the power, 2012-10-22 20:25:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/11236013/5002

[commit-bot: I haz the power, 2012-10-22 20:28:49]

Presubmit check for 11236013-5002 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    depot_tools

Presubmit checks took 179.3s to calculate.

[M-A Ruel, 2012-10-22 20:31:18]

rubberstamp lgtm

[commit-bot: I haz the power, 2012-10-22 20:31:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/11236013/5002

[commit-bot: I haz the power, 2012-10-22 20:34:22]

Change committed as 163366