presubmit: Call 'git diff' once per change instead of once per file

tests/presubmit_unittest.py

Index: tests/presubmit_unittest.py
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 314901012bf3744e983ca86b29600377dd85aa28..515815db5bf22048c84494b2ab7b044cf9bd5450 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -147,6 +147,7 @@ def GetPreferredTrySlaves(project):
     self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileRead')
     self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileWrite')
     self.mox.StubOutWithMock(presubmit.scm.SVN, 'GenerateDiff')
+    self.mox.StubOutWithMock(presubmit.scm.GIT, 'GenerateDiff')
 
 
 class PresubmitUnittest(PresubmitTestsBase):
@@ -385,6 +386,180 @@ class PresubmitUnittest(PresubmitTestsBase):
     self.assertEquals(rhs_lines[13][1], 49)
     self.assertEquals(rhs_lines[13][2], 'this is line number 48.1')
 
+  def testGitChange(self):
+    description_lines = ('Hello there',
+                         'this is a change',
+                         'BUG=123',
+                         ' STORY =http://foo/  \t',
+                         'and some more regular text  \t')
+    unified_diff = ['diff --git binary_a.png binary_a.png',
+                    'new file mode 100644',
+                    'index 0000000..6fbdd6d',
+                    'Binary files /dev/null and binary_a.png differ',
+                    'diff --git binary_d.png binary_d.png',
+                    'deleted file mode 100644',
+                    'index 6fbdd6d..0000000',
+                    'Binary files binary_d.png and /dev/null differ',
+                    'diff --git binary_m.png binary_m.png',
+                    'index 6fbdd6d..be3d5d8 100644',
+                    'Binary files binary_m.png and binary_m.png differ',

[iannucci, 2013/06/07 02:17:36]

May also want to test support the git diff --binary format

[ncarter (slow), 2013/06/07 19:51:15]

Done, added a new file with a binary diff.

+                    'diff --git boo/blat.cc boo/blat.cc',
+                    'new file mode 100644',
+                    'index 0000000..37d18ad',
+                    '--- boo/blat.cc',
+                    '+++ boo/blat.cc',
+                    '@@ -0,0 +1,5 @@',
+                    '+This is some text',
+                    '+which lacks a copyright warning',
+                    '+but it is nonetheless interesting',
+                    '+and worthy of your attention.',
+                    '+Its freshness factor is through the roof.',
+                    'diff --git floo/delburt.cc floo/delburt.cc',
+                    'deleted file mode 100644',
+                    'index e06377a..0000000',
+                    '--- floo/delburt.cc',
+                    '+++ /dev/null',
+                    '@@ -1,14 +0,0 @@',
+                    '-This text used to be here',
+                    '-but someone, probably you,',
+                    '-having consumed the text',
+                    '-  (absorbed its meaning)',
+                    '-decided that it should be made to not exist',
+                    '-that others would not read it.',
+                    '-  (What happened here?',
+                    '-was the author incompetent?',
+                    '-or is the world today so different from the world',
+                    '-   the author foresaw',
+                    '-and past imaginination',
+                    '-   amounts to rubble, insignificant,',
+                    '-something to be tripped over',
+                    '-and frustrated by)',
+                    'diff --git foo/TestExpectations foo/TestExpectations',
+                    'index c6e12ab..d1c5f23 100644',
+                    '--- foo/TestExpectations',
+                    '+++ foo/TestExpectations',
+                    '@@ -1,12 +1,24 @@',
+                    '-Stranger, behold:',
+                    '+Strange to behold:',
+                    '   This is a text',
+                    ' Its contents existed before.',
+                    '',
+                    '-It is written:',
+                    '+Weasel words suggest:',
+                    '   its contents shall exist after',
+                    '   and its contents',
+                    ' with the progress of time',
+                    ' will evolve,',
+                    '-   snaillike,',
+                    '+   erratically,',
+                    ' into still different texts',
+                    '-from this.',
+                    '\ No newline at end of file',
+                    '+from this.',
+                    '+',
+                    '+For the most part,',
+                    '+I really think unified diffs',
+                    '+are elegant: the way you can type',
+                    '+diff --git inside/a/text inside/a/text',
+                    '+or something silly like',
+                    '+@@ -278,6 +278,10 @@',
+                    '+and have this not be interpreted',
+                    '+as the start of a new file',
+                    '+or anything messed up like that,',
+                    '+because you parsed the header',
+                    '+correctly.',
+                    '\ No newline at end of file',
+                    '']
+    files = [('A      ', 'binary_a.png'),
+             ('D      ', 'binary_d.png'),
+             ('M      ', 'binary_m.png'),
+             ('A      ', 'boo/blat.cc'),
+             ('D      ', 'floo/delburt.cc'),
+             ('M      ', 'foo/TestExpectations')]
+
+    for op, path in files:
+      full_path = presubmit.os.path.join(self.fake_root_dir, *path.split('/'))
+      if op.startswith('D'):
+        os.path.exists(full_path).AndReturn(False)
+      else:
+        os.path.exists(full_path).AndReturn(False)
+        os.path.isfile(full_path).AndReturn(True)
+
+    presubmit.scm.GIT.GenerateDiff(self.fake_root_dir, files=[], full_move=True
+        ).AndReturn('\n'.join(unified_diff))
+
+    self.mox.ReplayAll()
+
+    change = presubmit.GitChange(
+        'mychange',
+        '\n'.join(description_lines),
+        self.fake_root_dir,
+        files,
+        0,
+        0,
+        None)
+    self.failUnless(change.Name() == 'mychange')
+    self.failUnless(change.DescriptionText() ==
+                    'Hello there\nthis is a change\nand some more regular text')
+    self.failUnless(change.FullDescriptionText() ==
+                    '\n'.join(description_lines))
+
+    self.failUnless(change.BUG == '123')
+    self.failUnless(change.STORY == 'http://foo/')
+    self.failUnless(change.BLEH == None)
+
+    self.failUnless(len(change.AffectedFiles()) == 6)
+    self.failUnless(len(change.AffectedFiles(include_dirs=True)) == 6)
+    self.failUnless(len(change.AffectedFiles(include_deletes=False)) == 4)
+    self.failUnless(len(change.AffectedFiles(include_dirs=True,
+                                             include_deletes=False)) == 4)
+
+    # Note that on git, there's no distinction between binary files and text
+    # files; everything that's not a delete is a text file.
+    affected_text_files = change.AffectedTextFiles()
+    self.failUnless(len(affected_text_files) == 4)
+
+    local_paths = change.LocalPaths()
+    expected_paths = [os.path.normpath(f) for op, f in files]
+    self.assertEqual(local_paths, expected_paths)
+
+    try:
+      _ = change.ServerPaths()
+      self.fail("ServerPaths implemented.")
+    except NotImplementedError:
+      pass
+
+    actual_rhs_lines = []
+    for f, linenum, line in change.RightHandSideLines():
+      actual_rhs_lines.append((f.LocalPath(), linenum, line))
+
+    f_blat = os.path.normpath('boo/blat.cc')
+    f_test_expectations = os.path.normpath('foo/TestExpectations')
+    expected_rhs_lines = [
+        (f_blat, 1, 'This is some text'),
+        (f_blat, 2, 'which lacks a copyright warning'),
+        (f_blat, 3, 'but it is nonetheless interesting'),
+        (f_blat, 4, 'and worthy of your attention.'),
+        (f_blat, 5, 'Its freshness factor is through the roof.'),
+        (f_test_expectations, 1, 'Strange to behold:'),
+        (f_test_expectations, 5, 'Weasel words suggest:'),
+        (f_test_expectations, 10, '   erratically,'),
+        (f_test_expectations, 13, 'from this.'),
+        (f_test_expectations, 14, ''),
+        (f_test_expectations, 15, 'For the most part,'),
+        (f_test_expectations, 16, 'I really think unified diffs'),
+        (f_test_expectations, 17, 'are elegant: the way you can type'),
+        (f_test_expectations, 18, 'diff --git inside/a/text inside/a/text'),
+        (f_test_expectations, 19, 'or something silly like'),
+        (f_test_expectations, 20, '@@ -278,6 +278,10 @@'),
+        (f_test_expectations, 21, 'and have this not be interpreted'),
+        (f_test_expectations, 22, 'as the start of a new file'),
+        (f_test_expectations, 23, 'or anything messed up like that,'),
+        (f_test_expectations, 24, 'because you parsed the header'),
+        (f_test_expectations, 25, 'correctly.')]

[iannucci, 2013/06/07 02:17:36]

:)

[ncarter (slow), 2013/06/07 19:51:15]

Done.

+
+    self.assertEquals(expected_rhs_lines, actual_rhs_lines)
+
   def testInvalidChange(self):
     try:
       presubmit.SvnChange(
@@ -1251,7 +1426,8 @@ class InputApiUnittest(PresubmitTestsBase):
     input_api.ReadFile(path, 'x')
 
   def testReadFileAffectedFileDenied(self):
-    fileobj = presubmit.AffectedFile('boo', 'M', 'Unrelated')
+    fileobj = presubmit.AffectedFile('boo', 'M', 'Unrelated',
+                                     diff_cache=mox.IsA(presubmit._DiffCache))
     self.mox.ReplayAll()
 
     change = presubmit.Change(
@@ -1262,7 +1438,8 @@ class InputApiUnittest(PresubmitTestsBase):
     self.assertRaises(IOError, input_api.ReadFile, fileobj, 'x')
 
   def testReadFileAffectedFileAccepted(self):
-    fileobj = presubmit.AffectedFile('AA/boo', 'M', self.fake_root_dir)
+    fileobj = presubmit.AffectedFile('AA/boo', 'M', self.fake_root_dir,
+                                     diff_cache=mox.IsA(presubmit._DiffCache))
     presubmit.gclient_utils.FileRead(fileobj.AbsoluteLocalPath(), 'x'
                                      ).AndReturn(None)
     self.mox.ReplayAll()
@@ -1358,19 +1535,22 @@ class OutputApiUnittest(PresubmitTestsBase):
     self.failIf(output.should_continue())
     self.failUnless(output.getvalue().count('???'))
 
+
 class AffectedFileUnittest(PresubmitTestsBase):
   def testMembersChanged(self):
     self.mox.ReplayAll()
     members = [
-      'AbsoluteLocalPath', 'Action', 'ChangedContents', 'GenerateScmDiff',
-      'IsDirectory', 'IsTextFile', 'LocalPath', 'NewContents', 'OldContents',
-      'OldFileTempPath', 'Property', 'ServerPath',
+      'AbsoluteLocalPath', 'Action', 'ChangedContents', 'DIFF_CACHE',
+      'GenerateScmDiff', 'IsDirectory', 'IsTextFile', 'LocalPath',
+      'NewContents', 'Property', 'ServerPath',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(
         presubmit.AffectedFile('a', 'b', self.fake_root_dir), members)
     self.compareMembers(
         presubmit.SvnAffectedFile('a', 'b', self.fake_root_dir), members)
+    self.compareMembers(
+        presubmit.GitAffectedFile('a', 'b', self.fake_root_dir), members)
 
   def testAffectedFile(self):
     path = presubmit.os.path.join('foo', 'blat.cc')