Modify presubmit checks to work on changed lines only.

presubmit_support.py

Index: presubmit_support.py
diff --git a/presubmit_support.py b/presubmit_support.py
index 049438d7500eef415ab988178adf54fe3e9ca22d..7a09e9d34b5b94ecdb2f3b545a49d0ab5d9d868d 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -86,11 +86,9 @@ def PromptYesNo(input_stream, output_stream, prompt):
 def _RightHandSideLinesImpl(affected_files):
   """Implements RightHandSideLines for InputApi and GclChange."""
   for af in affected_files:
-    lines = af.NewContents()
-    line_number = 0
+    lines = af.ChangedContents()
     for line in lines:
-      line_number += 1
-      yield (af, line_number, line)
+      yield (af, line[0], line[1])
 
 
 class OutputApi(object):
@@ -481,6 +479,27 @@ class AffectedFile(object):
     """
     raise NotImplementedError()  # Implement if/when needed.
 
+  def ChangedContents(self):
+    '''Returns a list of tuples (line number, line text) of all new lines.

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

style nit: for consistency, please use """. That's what all other functions use.

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

Done.

+
+     This relies on the scm diff output describing each changed code section
+     with a line of the form
+
+     ^@@ <old line num>,<old size> <new line num>,<new size> @@$
+    '''
+    new_lines = []
+    line_num = 0

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

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.

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

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

+    for line in self.GenerateScmDiff().splitlines():
+      m = re.match(r'^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@', line)
+      if m:
+        line_num = int(m.groups(1)[0])
+        continue
+      if line.startswith('+') and not line.startswith('++'):
+        new_lines.append((line_num, line[1:]))
+      if not line.startswith('-'):
+        line_num += 1

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

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.

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

ok

+    return new_lines
+
   def __str__(self):
     return self.LocalPath()
 
@@ -532,6 +551,8 @@ class SvnAffectedFile(AffectedFile):
         self._is_text_file = (not mime_type or mime_type.startswith('text/'))
     return self._is_text_file
 
+  def GenerateScmDiff(self):
+    return scm.SVN.GenerateDiff(self.AbsoluteLocalPath())
 
 class GitAffectedFile(AffectedFile):
   """Representation of a file in a change out of a git checkout."""
@@ -577,6 +598,10 @@ class GitAffectedFile(AffectedFile):
         self._is_text_file = os.path.isfile(self.AbsoluteLocalPath())
     return self._is_text_file
 
+  def GenerateScmDiff(self):
+    return scm.GIT.GenerateDiff(
+      os.path.dirname(self.AbsoluteLocalPath()),

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

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]

sure thing, good suggestion.

+      files=[os.path.basename(self.AbsoluteLocalPath()),])
 
 class Change(object):
   """Describe a change.