Modify gcl lint to pull the IGNORE_PATH from codereview.settings rather than...

Modify gcl lint to use regexes to figure out which files should be linted.

I've added two settings to codereview.settings - LINT_REGEX and 
LINT_IGNORE_REGEX - to specify which files should be linted. The default
is to lint anything that ends in .cpp, .cc, .inl, or .h, and to ignore 
nothing.

I have also modified GetCachedFile() to look for a locally modified version 
of codereview.settings before looking in the repository.

  R=maruel@chromium.org
  TEST=none
  BUG=none

[Dirk Pranke, 2009-10-05 22:09:04]

I'm finally getting around to fixing the issue raised in
http://codereview.chromium.org/192078/show - removing the hardcoded path in
gcl.py.

However, while looking at this, I'm not sure that the use_root parameter has any
effect in GetCachedFile() - it seems fetching repo_root + filename and url_path
+ filename end up being the same version of the file in practice (based on how
this routine is actually used in gcl.py), and if they weren't, then the cache
would be inconsisent (i.e., if we had  src/chrome/codereview.settings and
src/codereview.settings then depending on where you were in the tree you could
get different versions of the file put into the cache). Am I misunderstanding
how this code works, or is there a bunch of unused complexity here?

[jam, 2009-10-06 05:46:29]

I haven't touched this code in a while and it's gone through a bunch of changes,
so maruel is a better reviewer.

[M-A Ruel, 2009-10-06 15:50:08]

http://codereview.chromium.org/257054/diff/4/3001
File gcl.py (right):

http://codereview.chromium.org/257054/diff/4/3001#newcode1089
Line 1089: if len([file for suffix in CPP_EXTENSIONS if file.endswith(suffix)]):
I think you should use a regexp, the same way the presubmit scripts use both a
white and black regexp lists.

So CPP_EXTENSIONS becomes
WHITE_LIST = (r".*\.cpp", r".*\.cc", r".*\.h", r".*\.inl")

and IGNORE_PATH becomes BLACK_LIST.

Both could be in codereview.settings.

http://codereview.chromium.org/257054/diff/4/3001#newcode1090
Line 1090: if len([file for prefix in ignore_path if prefix and \
if len([True for prefix in ignore_path
        if prefix and file.startswith(prefix)]):

this alignment makes it more readable.

http://codereview.chromium.org/257054/diff/4/3001#newcode1092
Line 1092: print "Ignoring non-Google styled file %s" % file
Do we care? Don't know, I don't think I ever used this command in fact.

I still think this whole stuff should be a presubmit warning instead of bloating
gcl and having to duplicate the code in git-cl.

[Pam (message me for reviews), 2009-10-06 15:57:40]

I'm not terribly familiar with this section either, so just a few style nits.
LGTM when fixed.

http://codereview.chromium.org/257054/diff/4/3001
File gcl.py (right):

http://codereview.chromium.org/257054/diff/4/3001#newcode135
Line 135: use_root=True and use_root=False on the same file. Don't be stupid.
While you're here, can you clear up this comment ("same file" repeated)?

http://codereview.chromium.org/257054/diff/4/3001#newcode154
Line 154: filename
Style nit: Please use surrounding parentheses instead of the \ continuation
character. That is,

local_path = (url_path.replace(repo_root, GetRepositoryRoot()) +
    filename)

http://codereview.chromium.org/257054/diff/4/3001#newcode163
Line 163: # First, look for a locally modified version of the file
Nit: please end comments with a period (here and below).

http://codereview.chromium.org/257054/diff/4/3001#newcode1091
Line 1091: file.startswith(prefix)]):
Ditto parens instead of \

[M-A Ruel, 2009-10-28 15:45:23]

http://codereview.chromium.org/257054/diff/6001/6002
File gcl.py (right):

http://codereview.chromium.org/257054/diff/6001/6002#newcode157
Line 157: local_path = url_path.replace(repo_root + "/trunk/src",
Ish, I'd rather retrieve this information from svn and not hard code that.

http://codereview.chromium.org/257054/diff/6001/6002#newcode162
Line 162: 
I'm a freak and I love compact code.

http://codereview.chromium.org/257054/diff/6001/6002#newcode164
Line 164: content, rc = RunShellWithReturnCode(["svn", "status", filename])
Do you know from which directory this is executed?

http://codereview.chromium.org/257054/diff/6001/6002#newcode166
Line 166: content, rc = RunShellWithReturnCode(["cat", filename])
You should just read it instead :)
There's FileRead()

[Dirk Pranke, 2009-10-29 01:26:29]

http://codereview.chromium.org/257054/diff/6001/6002
File gcl.py (right):

http://codereview.chromium.org/257054/diff/6001/6002#newcode157
Line 157: local_path = url_path.replace(repo_root + "/trunk/src",
On 2009/10/28 15:45:23, Marc-Antoine Ruel wrote:
> Ish, I'd rather retrieve this information from svn and not hard code that.

Turns out I don't even need this variable. I'm not sure why I added it at some
point.

http://codereview.chromium.org/257054/diff/6001/6002#newcode162
Line 162: 
On 2009/10/28 15:45:23, Marc-Antoine Ruel wrote:
> I'm a freak and I love compact code.

? I'm not sure what you're saying here. Is there something to change?

http://codereview.chromium.org/257054/diff/6001/6002#newcode164
Line 164: content, rc = RunShellWithReturnCode(["svn", "status", filename])
On 2009/10/28 15:45:23, Marc-Antoine Ruel wrote:
> Do you know from which directory this is executed?

Not offhand. I'm not sure how to get that.

http://codereview.chromium.org/257054/diff/6001/6002#newcode166
Line 166: content, rc = RunShellWithReturnCode(["cat", filename])
On 2009/10/28 15:45:23, Marc-Antoine Ruel wrote:
> You should just read it instead :)
> There's FileRead()

Done.

[Dirk Pranke, 2009-10-29 02:27:51]

Whoops, I got my reviews crossed and already checked in a version of
this. I'll post an updated one shortly.

Note that since I committed this, the next set of rounds are in a new CL:

http://codereview.chromium.org/340030/show

-- Dirk

On Wed, Oct 28, 2009 at 6:36 PM,  <maruel@chromium.org> wrote:
> On 2009/10/29 01:26:29, dpranke wrote:
>>
>> On 2009/10/28 15:45:23, Marc-Antoine Ruel wrote:
>> > I'm a freak and I love compact code.
>
>> ? I'm not sure what you're saying here. Is there something to change?
>
> I was referring to the fact you added an empty line. :D Don't bother.
>

Gotcha. I think I actually did catch and delete the line.

>
> I'm fine with the regexp part but the cache part seems unrelated and flaky
> to
> me.
>

Without the caching change, it was impossible to test the regexp
change (since it only ever pulls from the repo). More to the point,
it just seems wrong to have no way to test against a local version of
the codereview.settings file, so I added that.

>
> http://codereview.chromium.org/257054/diff/6005/6006
> File gcl.py (right):
>
> http://codereview.chromium.org/257054/diff/6005/6006#newcode156
> Line 156: # First, look for a locally modified version of
> codereview.settings.
> Can you explain what you are trying to do then? Especially that you are
> not 100% sure from which directory you are executing this command.
>

Think I explained it above.

> http://codereview.chromium.org/257054/diff/6005/6006#newcode1084
> Line 1084: IGNORE_PATHS = (os.path.join("webkit","api"),)
> Please remove since it's not necessary to hack that anymore. (And I
> don't think it's referenced anymore)
>

Done.

> http://codereview.chromium.org/257054/diff/6005/6006#newcode1087
> Line 1087: LINT_REGEX = r"(.*\.cpp|.*\.cc|.*\.h)"
> nit: I'd prefer DEFAULT_LINT.* just to be clear it's the default values
> but the variable name starts to be long.
>

Done.

> http://codereview.chromium.org/257054
>