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

presubmit_support: Call git once per change, to get a diff for all files. Parse this result to generate per-file diffs, which go into a cache that's shared between the AffectedFile instances.

Greatly improves presubmit performance on Blink where there may be rule violations (my test case went from 50s to 5s).

BUG=236206
TEST=new test in presubmit_unittest.py; manual performance test on Mac and Windows after touching hundreds of files in webkit; testing included add/move/edit/delete on both binary and text files.

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

[ncarter (slow), 2013-06-07 00:14:23]

Hi Marc-Antoine,

Please review.

[M-A Ruel, 2013-06-07 01:01:15]

Deferring to another depot_tools owner.

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

Hey Nick, thanks for looking into this :) Some comments below... in particular,
what do you thing about simply having the Git Scm implement the cache
internally?

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:492: class _DiffCache(object):
Why not build this into the scm interface and hide the logic in GenerateDiff?

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:495: def GetScmDiff(self, path, local_root):
Maybe 'GetDiff'?

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:503: _DiffCache.__init__(self)
super(_SvnDiffCache, self).__init__()

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:516: _DiffCache.__init__(self)
super(_GitDiffCache, self).__init__()

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:531: file_marker = re.compile('^diff --git (?P<filename>.*)
(?P=filename)$')
won't there be a/ and b/ here? Or does scm.GIT do magic?

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:533: for x in unified_diff.splitlines(True):
Since keepends is infrequently used, let's invoke this as
splitlines(keepends=True)

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest....
tests/presubmit_unittest.py:405: 'Binary files binary_m.png and binary_m.png
differ',
May also want to test support the git diff --binary format

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest....
tests/presubmit_unittest.py:559: (f_test_expectations, 25, 'correctly.')]
:)

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

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:492: class _DiffCache(object):
On 2013/06/07 02:17:36, iannucci wrote:
> Why not build this into the scm interface and hide the logic in GenerateDiff? 

It seemed risky, since there are many other callers to GenerateDiff() and I
didn't know what their needs were, or how to appropriately test them.

Here's what I didn't know: It wasn't clear to me whether any current or future
scripts might edit the files on disk and then try to recompute a diff. I've
heard rumors that whitespace errors are auto-corrected on commit or upload or
something

It also would mean that the cache would be global, and I'd likely have to
support the full interface of GenerateDiff (all its arguments would be a key
into the cache) as opposed to only needing to test and validate the subset of
GenerateDiff used here. With a global cache, would that cause any of the
existing unittests (which seem to create git repositories on the fly) fail
because of stale caching?

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:495: def GetScmDiff(self, path, local_root):
On 2013/06/07 02:17:36, iannucci wrote:
> Maybe 'GetDiff'?

Done.

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:503: _DiffCache.__init__(self)
On 2013/06/07 02:17:36, iannucci wrote:
> super(_SvnDiffCache, self).__init__()

D'oh, how rusty of me. I knew this looked funny but couldn't put my finger on
it.

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:516: _DiffCache.__init__(self)
On 2013/06/07 02:17:36, iannucci wrote:
> super(_GitDiffCache, self).__init__()

Done.

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:531: file_marker = re.compile('^diff --git (?P<filename>.*)
(?P=filename)$')
On 2013/06/07 02:17:36, iannucci wrote:
> won't there be a/ and b/ here? Or does scm.GIT do magic?

the a/ and b/ are omitted when the --no-prefix option to git diff is used.
GenerateDiff always uses the --no-prefix option.

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:533: for x in unified_diff.splitlines(True):
On 2013/06/07 02:17:36, iannucci wrote:
> Since keepends is infrequently used, let's invoke this as
> splitlines(keepends=True)

I agree, but actually splitlines is one of those python C functions whose
optional arguments actually can only be specified positionally: (
http://bugs.python.org/issue13012 ). Next best thing I could do was to create a
var called keep_line_endings = True, for the sake of readability. Done.

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest....
tests/presubmit_unittest.py:405: 'Binary files binary_m.png and binary_m.png
differ',
On 2013/06/07 02:17:36, iannucci wrote:
> May also want to test support the git diff --binary format

Done, added a new file with a binary diff.

https://codereview.chromium.org/15898005/diff/10001/tests/presubmit_unittest....
tests/presubmit_unittest.py:559: (f_test_expectations, 25, 'correctly.')]
On 2013/06/07 02:17:36, iannucci wrote:
> :)

Done.

[iannucci, 2013-06-07 21:30:16]

lgtm % nit

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:492: class _DiffCache(object):
On 2013/06/07 19:51:15, ncarter wrote:
> On 2013/06/07 02:17:36, iannucci wrote:
> > Why not build this into the scm interface and hide the logic in
GenerateDiff? 
> 
> It seemed risky, since there are many other callers to GenerateDiff() and I
> didn't know what their needs were, or how to appropriately test them.
> 
> Here's what I didn't know: It wasn't clear to me whether any current or future
> scripts might edit the files on disk and then try to recompute a diff. I've
> heard rumors that whitespace errors are auto-corrected on commit or upload or
> something
> 
> It also would mean that the cache would be global, and I'd likely have to
> support the full interface of GenerateDiff (all its arguments would be a key
> into the cache) as opposed to only needing to test and validate the subset of
> GenerateDiff used here. With a global cache, would that cause any of the
> existing unittests (which seem to create git repositories on the fly) fail
> because of stale caching?

Hmm.. fair point. This code is pretty labyrinthine :/

https://codereview.chromium.org/15898005/diff/10001/presubmit_support.py#newc...
presubmit_support.py:533: for x in unified_diff.splitlines(True):
On 2013/06/07 19:51:15, ncarter wrote:
> On 2013/06/07 02:17:36, iannucci wrote:
> > Since keepends is infrequently used, let's invoke this as
> > splitlines(keepends=True)
> 
> I agree, but actually splitlines is one of those python C functions whose
> optional arguments actually can only be specified positionally: (
> http://bugs.python.org/issue13012 ). Next best thing I could do was to create
a
> var called keep_line_endings = True, for the sake of readability. Done.

That's a bummer. New version lg though.

https://codereview.chromium.org/15898005/diff/22001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/15898005/diff/22001/git_cl.py#newcode699
git_cl.py:699: '    git branch --set-upstream-to=trunk\n'
This may be the old syntax because some users still have 'old' git versions.
Could you perhaps have both flavors in the message?

[ncarter (slow), 2013-06-10 19:28:46]

https://codereview.chromium.org/15898005/diff/22001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/15898005/diff/22001/git_cl.py#newcode699
git_cl.py:699: '    git branch --set-upstream-to=trunk\n'
On 2013/06/07 21:30:16, iannucci wrote:
> This may be the old syntax because some users still have 'old' git versions.
> Could you perhaps have both flavors in the message?

I think this is a case where adding more text only creates more confusion. Since
this feature isn't in git 1.7, I reverted to the old instructions.

[commit-bot: I haz the power, 2013-06-10 19:28:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/15898005/23006

[commit-bot: I haz the power, 2013-06-10 19:30:27]

Change committed as 205275