Add a canned clang-format presubmit check

Add a canned clang-format presubmit check

For simplicity, the canned presubmit check just calls git cl format
directly with a new option --dry-run which does not change the files on
disk.

Because some users may not have git or clang-format might somehow have
errors, this canned check is a warning.  Additionally, if git cl format
fails with an error then the presubmit check will silently report
success to avoid spamming misconfigured users during upload.

BUG=none

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

[enne (OOO), 2014-01-17 02:35:05]

PTAL

https://codereview.chromium.org/141493002/diff/1/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/141493002/diff/1/tests/presubmit_unittest.py#...
tests/presubmit_unittest.py:1755: 'CheckPatchFormatted',
As per discussion offline with iannucci who didn't think unit tests would be
useful here, I just silenced the checker.

[ncarter (slow), 2014-01-17 07:07:08]

https://codereview.chromium.org/141493002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/141493002/diff/1/git_cl.py#newcode90
git_cl.py:90: stderr = open("/dev/null", "w")
"/dev/null" -> os.devnull

This is very much TIL territory, but here's a little session on Windows for
reference:

D:\src\crgit2\src>python
Python 2.7.6
% f = open('/dev/null', 'w')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IOError: [Errno 2] No such file or directory: '/dev/null'
% f = open('nul', 'w')
% import os
% os.devnull
'nul'

https://codereview.chromium.org/141493002/diff/1/git_cl.py#newcode91
git_cl.py:91: else:
pylint usually will yell at you to use single-quotes instead of double quotes,
to be consistent with the rest of the file.

[enne (OOO), 2014-01-17 19:24:41]

https://codereview.chromium.org/141493002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/141493002/diff/1/git_cl.py#newcode90
git_cl.py:90: stderr = open("/dev/null", "w")
On 2014/01/17 07:07:08, ncarter wrote:
> "/dev/null" -> os.devnull
> 
> This is very much TIL territory, but here's a little session on Windows for
> reference:
> 
> D:\src\crgit2\src>python
> Python 2.7.6
> % f = open('/dev/null', 'w')
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> IOError: [Errno 2] No such file or directory: '/dev/null'
> % f = open('nul', 'w')
> % import os
> % os.devnull
> 'nul'

Thanks! Done.

https://codereview.chromium.org/141493002/diff/1/git_cl.py#newcode91
git_cl.py:91: else:
On 2014/01/17 07:07:08, ncarter wrote:
> pylint usually will yell at you to use single-quotes instead of double quotes,
> to be consistent with the rest of the file.

Oops, done.  C++ has infected by brain.  :(

[iannucci, 2014-01-17 20:28:50]

https://codereview.chromium.org/141493002/diff/90001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/141493002/diff/90001/git_cl.py#newcode96
git_cl.py:96: stderr=stderr)
I think that subprocess2.VOID is the correct way to do this in subprocess2...

https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks....
presubmit_canned_checks.py:1049: suppress_stderr=True)
kinda unfortunate that we throw away the results... how long does this take to
run?

[enne (OOO), 2014-01-17 21:26:12]

https://codereview.chromium.org/141493002/diff/90001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/141493002/diff/90001/git_cl.py#newcode96
git_cl.py:96: stderr=stderr)
On 2014/01/17 20:28:50, iannucci wrote:
> I think that subprocess2.VOID is the correct way to do this in subprocess2...

Done.

https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks....
presubmit_canned_checks.py:1049: suppress_stderr=True)
On 2014/01/17 20:28:50, iannucci wrote:
> kinda unfortunate that we throw away the results... how long does this take to
> run?

If you had results, what would you do with them? Do you want to stick the output
in /tmp somewhere and give a command that folks can apply rather than just
rerunning git cl format?


In terms of time, for this example patch:
https://codereview.chromium.org/139053002/

enne@tropy:/work/src$ time git cl format

real	0m1.040s
user	0m0.790s
sys	0m0.120s

This is about the same for git cl format --full that formats the whole file.

It definitely does take longer on patches that touch more files.  On a patch
that had about 1800 files changed, it was more in the ballpark of 15 seconds.

[iannucci, 2014-01-17 23:16:47]

On 2014/01/17 21:26:12, enne wrote:
> https://codereview.chromium.org/141493002/diff/90001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/141493002/diff/90001/git_cl.py#newcode96
> git_cl.py:96: stderr=stderr)
> On 2014/01/17 20:28:50, iannucci wrote:
> > I think that subprocess2.VOID is the correct way to do this in
subprocess2...
> 
> Done.
> 
>
https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
> 
>
https://codereview.chromium.org/141493002/diff/90001/presubmit_canned_checks....
> presubmit_canned_checks.py:1049: suppress_stderr=True)
> On 2014/01/17 20:28:50, iannucci wrote:
> > kinda unfortunate that we throw away the results... how long does this take
to
> > run?
> 
> If you had results, what would you do with them? Do you want to stick the
output
> in /tmp somewhere and give a command that folks can apply rather than just
> rerunning git cl format?
> 

Yeah, exactly. Instead of telling them to `git cl format`, it would maybe
suggest `patch -p1 < /tmp/garbage.patch`

> 
> In terms of time, for this example patch:
> https://codereview.chromium.org/139053002/
> 
> enne@tropy:/work/src$ time git cl format
> 
> real	0m1.040s
> user	0m0.790s
> sys	0m0.120s
> 
> This is about the same for git cl format --full that formats the whole file.
> 
> It definitely does take longer on patches that touch more files.  On a patch
> that had about 1800 files changed, it was more in the ballpark of 15 seconds.

Yeah that's what I was wondering. It would be nice to add this as an
optimization in a followup cl.

[iannucci, 2014-01-17 23:17:13]

(oh lgtm, too)

[ncarter (slow), 2014-01-18 00:56:41]

lgtm

[commit-bot: I haz the power, 2014-01-21 17:37:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/141493002/160001

[commit-bot: I haz the power, 2014-01-21 17:37:27]

Failed to apply patch for depot_tools/git_cl.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file depot_tools/git_cl.py
  Hunk #3 FAILED at 2366.
  Hunk #4 FAILED at 2378.
  2 out of 4 hunks FAILED -- saving rejects to file depot_tools/git_cl.py.rej

Patch:       depot_tools/git_cl.py
Index: git_cl.py
diff --git depot_tools/git_cl.py depot_tools/git_cl.py
index
452794451766fb9fd8691207e8e0ee8fd80d36bc..13c28805271e172e1f6c286023e4f999926a1ae3
100755
--- a/depot_tools/git_cl.py
+++ b/depot_tools/git_cl.py
@@ -80,15 +80,20 @@ def RunGit(args, **kwargs):
   return RunCommand(['git'] + args, **kwargs)
 
 
-def RunGitWithCode(args):
+def RunGitWithCode(args, suppress_stderr=False):
   """Returns return code and stdout."""
   try:
     env = os.environ.copy()
     # 'cat' is a magical git string that disables pagers on all platforms.
     env['GIT_PAGER'] = 'cat'
+    if suppress_stderr:
+      stderr = subprocess2.VOID
+    else:
+      stderr = sys.stderr
     out, code = subprocess2.communicate(['git'] + args,
                                         env=env,
-                                        stdout=subprocess2.PIPE)
+                                        stdout=subprocess2.PIPE,
+                                        stderr=stderr)
     return code, out[0]
   except ValueError:
     # When the subprocess fails, it returns None.  That triggers a ValueError
@@ -2301,7 +2306,10 @@ def CMDowners(parser, args):
 def CMDformat(parser, args):
   """Runs clang-format on the diff."""
   CLANG_EXTS = ['.cc', '.cpp', '.h']
-  parser.add_option('--full', action='store_true', default=False)
+  parser.add_option('--full', action='store_true',
+                    help='Reformat the full content of all touched files')
+  parser.add_option('--dry-run', action='store_true',
+                    help='Don\'t modify any file on disk.')
   opts, args = parser.parse_args(args)
   if args:
     parser.error('Unrecognized args: %s' % ' '.join(args))
@@ -2358,8 +2366,10 @@ def CMDformat(parser, args):
     if not files:
       print "Nothing to format."
       return 0
-    RunCommand([clang_format_tool, '-i', '-style', 'Chromium'] + files,
-               cwd=top_dir)
+    cmd = [clang_format_tool, '-style', 'Chromium']
+    if not opts.dry_run:
+      cmd.append('-i')
+    stdout = RunCommand(cmd + files, cwd=top_dir)
   else:
     env = os.environ.copy()
     env['PATH'] = os.path.dirname(clang_format_tool)
@@ -2370,9 +2380,13 @@ def CMDformat(parser, args):
     except clang_format.NotFoundError, e:
       DieWithError(e)
 
-    cmd = [sys.executable, script, '-p0', '-style', 'Chromium', '-i']
+    cmd = [sys.executable, script, '-p0', '-style', 'Chromium']
+    if not opts.dry_run:
+      cmd.append('-i')
 
-    RunCommand(cmd, stdin=diff_output, cwd=top_dir, env=env)
+    stdout = RunCommand(cmd, stdin=diff_output, cwd=top_dir, env=env)
+    if opts.dry_run and len(stdout) > 0:
+      return 2
 
   return 0

[commit-bot: I haz the power, 2014-01-21 17:43:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/141493002/220001

[commit-bot: I haz the power, 2014-01-21 17:44:48]

Change committed as 246066