Modify presubmit checks to work on changed lines only.

Modify presubmit checks to work on changed lines only.

Unittest is being also updated. 

TEST=manual
. run ./presubmit_unittest.py, observe success
.create a CL with code style violations (long lines, traling spaces) and observe the violations reported by 'git cl presubmit'

Patch contributed by Vadim

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

[M-A Ruel, 2011-02-09 14:16:09]

Oh, interestingly enough your checkout wasn't automatically configured. Do *not*
run "git cl config", I'll take a look on your workstation to see what's
happening.

In the meantime you want to "git svn rebase" to get some fixes and you can
upload another change with the items below. The code looks great, thanks.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode484
presubmit_support.py:484: # file. This relies on the git diff output describing
each changed code
the scm diff output

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode492
presubmit_support.py:492: for line in diff_text.split('\n'):
for line in self.GenerateScmDiff().splitlines():

and remove line 491.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode493
presubmit_support.py:493: m = re.match('^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@',
line)
m = re.match(r'^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@', line)

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode497
presubmit_support.py:497: if re.search('^\+[^\+]', line):
if re.search(r'^\+[^\+]', line):

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode499
presubmit_support.py:499: if not re.search('^\-[^\-]', line):
if not re.search(r'^\-[^\-]', line):

[vb, 2011-02-09 18:24:41]

PTAL

Rebased to the latest svn revision, retested, all presubmit unittests pass.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode484
presubmit_support.py:484: # file. This relies on the git diff output describing
each changed code
On 2011/02/09 14:16:10, Marc-Antoine Ruel wrote:
> the scm diff output

Done.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode492
presubmit_support.py:492: for line in diff_text.split('\n'):
On 2011/02/09 14:16:10, Marc-Antoine Ruel wrote:
> for line in self.GenerateScmDiff().splitlines():
> 
> and remove line 491.

Done.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode493
presubmit_support.py:493: m = re.match('^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@',
line)
On 2011/02/09 14:16:10, Marc-Antoine Ruel wrote:
> m = re.match(r'^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@', line)

Done.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode497
presubmit_support.py:497: if re.search('^\+[^\+]', line):
On 2011/02/09 14:16:10, Marc-Antoine Ruel wrote:
> if re.search(r'^\+[^\+]', line):

Done.

http://codereview.chromium.org/6461011/diff/10/presubmit_support.py#newcode499
presubmit_support.py:499: if not re.search('^\-[^\-]', line):
On 2011/02/09 14:16:10, Marc-Antoine Ruel wrote:
> if not re.search(r'^\-[^\-]', line):

Done.

[M-A Ruel, 2011-02-09 18:30:26]

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode483
presubmit_support.py:483: # Return a list of tuples (line number, line text) of
all new lines in the
I think it should be a docstring.

"""Returns a list of ...

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode492
presubmit_support.py:492: for line in diff_text.splitlines():
You didn't merge lines 491 and 492. You prefer to keep that on two lines? I
don't mind much but I don't think this data needs a named variable.

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode497
presubmit_support.py:497: if re.search(r'^\+[^\+]', line):
Actually, this regexp is wrong:

what if the line was empty, e.g. there's no character on this line, I don't
think this regexp will trigger. Can you confirm?

http://codereview.chromium.org/6461011/diff/7001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6461011/diff/7001/tests/presubmit_unittest.py#...
tests/presubmit_unittest.py:75: +this is line number 32.1
Add a
+
on a single line to verify my concern about empty lines.

[vb, 2011-02-09 18:54:11]

The unittests still pass.

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode483
presubmit_support.py:483: # Return a list of tuples (line number, line text) of
all new lines in the
On 2011/02/09 18:30:26, Marc-Antoine Ruel wrote:
> I think it should be a docstring.
> 
> """Returns a list of ...

Done.

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode492
presubmit_support.py:492: for line in diff_text.splitlines():
On 2011/02/09 18:30:26, Marc-Antoine Ruel wrote:
> You didn't merge lines 491 and 492. You prefer to keep that on two lines? I
> don't mind much but I don't think this data needs a named variable.

good point, it was a leftover of previous code, now refactored.

http://codereview.chromium.org/6461011/diff/7001/presubmit_support.py#newcode497
presubmit_support.py:497: if re.search(r'^\+[^\+]', line):
On 2011/02/09 18:30:26, Marc-Antoine Ruel wrote:
> Actually, this regexp is wrong:
> 
> what if the line was empty, e.g. there's no character on this line, I don't
> think this regexp will trigger. Can you confirm?

Yes, a good catch. Did away with regexp here, I think it is even better readable
this way. And we don't care about the '^---' case , because it would be followed
by a @@ line and reset the line counter anyways.

http://codereview.chromium.org/6461011/diff/7001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6461011/diff/7001/tests/presubmit_unittest.py#...
tests/presubmit_unittest.py:75: +this is line number 32.1
On 2011/02/09 18:30:26, Marc-Antoine Ruel wrote:
> Add a
> +
> on a single line to verify my concern about empty lines.

Done.

[M-A Ruel, 2011-02-09 19:43:55]

Sorry for the back and forth, I just realized a few things.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode483
presubmit_support.py:483: '''Returns a list of tuples (line number, line text)
of all new lines.
style nit: for consistency, please use """. That's what all other functions use.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode491
presubmit_support.py:491: line_num = 0
Do you think you should add this?

if self.IsDirectory():
  return []

Maybe it will work as-is without this check. It's probably just a case that will
happen with svn anyway.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode500
presubmit_support.py:500: line_num += 1
I was wondering about svn props in a diff. Example:


Index: __init__.py
===================================================================
--- __init__.py (revision 660)
+++ __init__.py (working copy)
@@ -0,0 +1 @@
+foo
Property changes on: __init__.py
___________________________________________________________________
Modified: svn:eol-style
   - native
   + CR


Actually, I think it'll be fine since svn always puts leading space on the svn
property value. So it's just FYI.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode603
presubmit_support.py:603: os.path.dirname(self.AbsoluteLocalPath()),
I'd prefer if you used self._local_root and files=[self.LocalPath()].

The reason I'm saying that is that if the file and its containing directory are
deleted from a git repository, I'm just afraid this could cause an exception
because the base directory wouldn't exist while trying to shell out git. What do
you think?

[vb, 2011-02-09 22:30:43]

No problem with the 'back and forth' - the more thorough it is reviewed the less
exception emails you receive :)


I fixed the issues you mentioned, and then I noticed that it ignores Assembler
source files (.S), adding them turned out a bit tricky as it contradicted the
default black list filter, but I think I got it right.

All unit tests pass as well as 'git cl presubmit' on a real directory.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode483
presubmit_support.py:483: '''Returns a list of tuples (line number, line text)
of all new lines.
On 2011/02/09 19:43:55, Marc-Antoine Ruel wrote:
> style nit: for consistency, please use """. That's what all other functions
use.

Done.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode491
presubmit_support.py:491: line_num = 0
On 2011/02/09 19:43:55, Marc-Antoine Ruel wrote:
> Do you think you should add this?
> 
> if self.IsDirectory():
>   return []
> 
> Maybe it will work as-is without this check. It's probably just a case that
will
> happen with svn anyway.

I did not realize this could be invoked for a directory, but will add a check if
you say so.

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode500
presubmit_support.py:500: line_num += 1
On 2011/02/09 19:43:55, Marc-Antoine Ruel wrote:
> I was wondering about svn props in a diff. Example:
> 
> 
> Index: __init__.py
> ===================================================================
> --- __init__.py (revision 660)
> +++ __init__.py (working copy)
> @@ -0,0 +1 @@
> +foo
> Property changes on: __init__.py
> ___________________________________________________________________
> Modified: svn:eol-style
>    - native
>    + CR
> 
> 
> Actually, I think it'll be fine since svn always puts leading space on the svn
> property value. So it's just FYI.

ok

http://codereview.chromium.org/6461011/diff/2003/presubmit_support.py#newcode603
presubmit_support.py:603: os.path.dirname(self.AbsoluteLocalPath()),
On 2011/02/09 19:43:55, Marc-Antoine Ruel wrote:
> I'd prefer if you used self._local_root and files=[self.LocalPath()].
> 
> The reason I'm saying that is that if the file and its containing directory
are
> deleted from a git repository, I'm just afraid this could cause an exception
> because the base directory wouldn't exist while trying to shell out git. What
do
> you think?

sure thing, good suggestion.

[M-A Ruel, 2011-02-09 22:39:56]

lgtm, just check the commit box to have the commit queue commit it for you.