Very basic starter presubmit script for Chromium, as requested by maruel....

Very basic starter presubmit script for Chromium, as requested by maruel.

At present this checks the following:
- No tabs in source files
- The words "DO-NOT-SUBMIT" (without the dashes) do not appear in the CL
  description or in files in the change
- No CRLF in source files (.cc, .h, .mm, .py)

Limitations:
- The general limitations of the current presubmit API implementation
  apply.  One of these is that gcl only walks up to the checkout root
  searching for presubmit files, so if you've mapped stuff into your
  client using DEPS, it will only walk up to where the root of that
  mapping.

Checked in as revision 10015.

[M-A Ruel, 2009-02-19 00:52:49]

lgtm with nits. As you prefer, they are optional.

http://codereview.chromium.org/20474/diff/1/3
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/1/3#newcode51
Line 51: crlf_files = {}
nit: Would a set() or a list [] be more appropriate? In fact, I don't even see
the need to use a set.

http://codereview.chromium.org/20474/diff/1/3#newcode62
Line 62: if contents.count('\r\n'):
nit: I thought you'd use
if '\r' in contents:

http://codereview.chromium.org/20474/diff/1/2
File PRESUBMIT_unittest.py (right):

http://codereview.chromium.org/20474/diff/1/2#newcode58
Line 58: self.failUnless(
nit: Why not self.failUnlessEqual() ?

[Jói Sigurðsson, 2009-02-19 01:43:54]

New version uploaded to address one of your nits :)

http://codereview.chromium.org/20474/diff/1/3
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/1/3#newcode51
Line 51: crlf_files = {}
On 2009/02/19 00:52:49, M-A wrote:
> nit: Would a set() or a list [] be more appropriate? In fact, I don't even see
> the need to use a set.

A set() would be more of a straight fit but it requires an extra import, and
using the built-in dictionaries this way is fairly common.

A list would be fine since the numbers are generally small but the dict should
be faster.

http://codereview.chromium.org/20474/diff/1/3#newcode62
Line 62: if contents.count('\r\n'):
On 2009/02/19 00:52:49, M-A wrote:
> nit: I thought you'd use
> if '\r' in contents:
> 
> 

Done; using \r now, updated function name, added unit test, updated the message
printed.

http://codereview.chromium.org/20474/diff/1/2
File PRESUBMIT_unittest.py (right):

http://codereview.chromium.org/20474/diff/1/2#newcode58
Line 58: self.failUnless(
On 2009/02/19 00:52:49, M-A wrote:
> nit: Why not self.failUnlessEqual() ?
> 

I find it less readable :)

[Pam (message me for reviews), 2009-02-19 01:47:49]

Drive-by...

http://codereview.chromium.org/20474/diff/5/1007
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/5/1007#newcode17
Line 17: SOURCE_FILE_EXTENSIONS = ['.cc', '.h', '.mm', '.py']
Please add .cpp, .c, and .m to this list. All are used in WebKit.

[Jói Sigurðsson, 2009-02-19 01:54:55]

Thanks Pam.

http://codereview.chromium.org/20474/diff/5/1007
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/5/1007#newcode17
Line 17: SOURCE_FILE_EXTENSIONS = ['.cc', '.h', '.mm', '.py']
On 2009/02/19 01:47:49, Pam wrote:
> Please add .cpp, .c, and .m to this list. All are used in WebKit.

Added, and new patch uploaded.

Note though, if WebKit is being brought in via DEPS, the way the presubmit API
is currently implemented, we would need a PRESUBMIT.py in the WebKit directory
as well.  This is because it currently searches for PRESUBMIT.py files only up
to the client root of the current SVN tree, and when something is mapped into
the client via DEPS the client root terminates at the point where it's mapped
in.

This current limitation of the presubmit API is something that can definitely be
fixed, but I'm not sure what the best way is, we can discuss.  If there is a way
to find the gclient root (rather than the svn root) then I could start building
absolute paths from the client-root-relative paths and walking up those to find
presubmit files until the gclient root is hit.

[M-A Ruel, 2009-02-19 12:29:19]

lgtm

[Mark Mentovai, 2009-02-19 14:28:31]

http://codereview.chromium.org/20474/diff/8/10
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/8/10#newcode62
Line 62: if contents.count('\r'):
You don't actually need a count, you can stop when you see the first '\r'.  How
about |if '\r' in contents|?  Same for '\t'.

http://codereview.chromium.org/20474/diff/8/10#newcode63
Line 63: cr_files[path] = 1
Earlier, you said that a dict is faster than a list, but I don't see why that
would be.  You never actually access this dict or the tab_files one by key, you
only check the dict for emptiness and get a list of its keys.  In fact, you've
got a comment saying "values ignored."  Why not make this a list (cr_files = []
above) and then use cr_files.append(path) here?

[Jói Sigurðsson, 2009-02-19 15:40:01]

Thanks Mark, I've addressed your comments and uploaded a new version.

http://codereview.chromium.org/20474/diff/8/10
File PRESUBMIT.py (right):

http://codereview.chromium.org/20474/diff/8/10#newcode62
Line 62: if contents.count('\r'):
On 2009/02/19 14:28:31, Mark Mentovai wrote:
> You don't actually need a count, you can stop when you see the first '\r'. 
How
> about |if '\r' in contents|?  Same for '\t'.

You're right, and this will be faster for the error case (but just as slow I
think for the normal case where there are no bad characters in the content). 
It's become a habit for me to check for existence using count, your approach
seems better.

Fixed.

http://codereview.chromium.org/20474/diff/8/10#newcode63
Line 63: cr_files[path] = 1
On 2009/02/19 14:28:31, Mark Mentovai wrote:
> Earlier, you said that a dict is faster than a list, but I don't see why that
> would be.  You never actually access this dict or the tab_files one by key,
you
> only check the dict for emptiness and get a list of its keys.  In fact, you've
> got a comment saying "values ignored."  Why not make this a list (cr_files =
[]
> above) and then use cr_files.append(path) here?

Good point.  When I wrote the code I was planning to use
input_api.RightHandSideLines() to scan all lines of all files, so the thought
was I would address the dict multiple times (and maybe keep a count or a list of
bad lines) but I ended up doing it differently because RightHandSideLines()
normalizes line endings for you.

I don't think it makes much difference in terms of performance, but I switched
to a list as you suggested, to make the code more readable.

[Mark Mentovai, 2009-02-19 15:51:35]

Excellent, lgtm too.

http://codereview.chromium.org/20474/diff/14/1011
File PRESUBMIT_unittest.py (right):

http://codereview.chromium.org/20474/diff/14/1011#newcode2
Line 2: # Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?