Add presubmit check for dcommitted / CQed CLs

Add presubmit check for dcommitted / CQed CLs

We use the fact the dcommit adds a "Committed:" link to the
description.  If we wanted to be more strict, we could make
git cl dcommit post a comment on the CL (which would be nice
since it would notify reviewers that the CL was committed).

Note that this is untested as written, because it doesn't appear
that we have either git-cl integration tests, nor a test for
git cl dcommit adding the word "Committed:" so as written, this
presubmit check is prone to failure.  Would like suggestions to
improve.

Also adding DoNotSubmit checks and a couple others to
PanPresubmitChecks.

R=maruel@chromium.org,dpranke@chromium.org
BUG=161702
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=171158

[M-A Ruel, 2012-11-22 15:47:06]

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:8: import re
why not input_api.re?

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:51: """Checks that the user didn't add 'DO NOT
SUBMIT' to the CL description.
Why? This would block the CQ from processing this file.

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:75: if re.match('\nCommitted: \S+$', description):
You could use ^ instead with re.search(.., re.MULTILINE)

Also, regexp string should be of r type.

[Isaac (away), 2012-11-22 18:50:51]

some responses to comments, no new patch yet.

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:8: import re
On 2012/11/22 15:47:06, Marc-Antoine Ruel wrote:
> why not input_api.re?

I can do that -- is there a reason we're avoiding the python re class here?

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:51: """Checks that the user didn't add 'DO NOT
SUBMIT' to the CL description.
On 2012/11/22 15:47:06, Marc-Antoine Ruel wrote:
> Why? This would block the CQ from processing this file.

I didn't realize that was the reason for splitting this up.  OK.  We can use
pythons string concatenation and change to 'DO NOT ''SUBMIT', which is preferred
python style.

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:75: if re.match('\nCommitted: \S+$', description):
On 2012/11/22 15:47:06, Marc-Antoine Ruel wrote:
> You could use ^ instead with re.search(.., re.MULTILINE)
> 
> Also, regexp string should be of r type.

I only want to match Committed on the last line, so multiline wouldn't work.  A
revert which quotes the original commit might have a "Committed" line from the
original commit, but not as the last line.

[Dirk Pranke, 2012-11-22 18:54:07]

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/11421050/diff/1/presubmit_canned_checks.py#ne...
presubmit_canned_checks.py:8: import re
On 2012/11/22 18:50:52, Isaac wrote:
> On 2012/11/22 15:47:06, Marc-Antoine Ruel wrote:
> > why not input_api.re?
> 
> I can do that -- is there a reason we're avoiding the python re class here?

input_api.re would just be a pointer to the python module. You'll notice we
don't normally import anything directly into this file (I'm actually surprised
that "import os" has crept in). This makes it easier to mock out objects for
testing, and I seem to recall there's an actual reason to limit imports as well
(although that reason might've been obsolete).

[Isaac (away), 2012-11-25 02:12:44]

ptal

https://chromiumcodereview.appspot.com/11421050/diff/1/presubmit_canned_check...
File presubmit_canned_checks.py (right):

https://chromiumcodereview.appspot.com/11421050/diff/1/presubmit_canned_check...
presubmit_canned_checks.py:8: import re
OK, fixed.

https://chromiumcodereview.appspot.com/11421050/diff/1/presubmit_canned_check...
presubmit_canned_checks.py:51: """Checks that the user didn't add 'DO NOT
SUBMIT' to the CL description.
On 2012/11/22 18:50:52, Isaac wrote:
> On 2012/11/22 15:47:06, Marc-Antoine Ruel wrote:
> > Why? This would block the CQ from processing this file.
> 
> I didn't realize that was the reason for splitting this up.  OK.  We can use
> pythons string concatenation and change to 'DO NOT ''SUBMIT', which is
preferred
> python style.

Fixed.

[Isaac (away), 2012-11-28 22:29:21]

Still waiting for review for this one too.  Sorry for the barrage of CLs this
week!

[Isaac (away), 2012-12-04 00:27:04]

ping?

[M-A Ruel, 2012-12-04 11:47:53]

Thanks, it looks good. Note that you may want to alert on chromium-dev@ because
there was a discussion about that a ~year ago. I agree with this change, since
it confuses build sheriffs and is a pain to manage from a review standpoint,
i.e. it's impossible to know which comment is about which patchset and which one
was in the first or second commit. Nobody like to hunt a CL that was committed 3
times and figure out which patchset was committed when.

Since it may generate some discussion that will point at this CL, the canonical
way to reapply a patch that was reverted is:

# Make sure your clone is up to date.
git fetch origin
git svn fetch

# Find the svn revision number of the revert and revert it, again.
git checkout -b reapply origin/master
# In the comment, note which revision you are reverting and
# which you are reapplying.
git revert $(git svn find-rev r1234)
# If necessary, resolve conflicts.
git mergetool

# Upload the revert-of-the-revert as-is so the reviewer can see what changes you
did since the last CL that was reverted. Do not use --send-mail here.
git cl upload -r joe@chromium.org

# *ONLY AT THAT POINT* Stack to hack
vim build/whitespace_file.txt
git commit -a --amend
# Title the patchset "fix" or "diff against the revert in patchset 1"
git cl upload --send-mail


lgtm, sorry. Feel free to ping me after a few hours, I don't mind pings.

[commit-bot: I haz the power, 2012-12-05 04:22:38]

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

[commit-bot: I haz the power, 2012-12-05 04:25:16]

Change committed as 171158