Page MenuHomePhabricator

Perhaps remove the commit hook mode from linters
Closed, DuplicatePublic

Description

Possibly we should just remove the commit hook mode from linters and force all linters to operate on the file explicitly.

AFAIK, the commit hook mode is rarely used and is an artefact from Facebook days. One issue with passing data to a linter via stdin is that some linters behave differently when data is piped from stdin versus being passed a path explicitly.


Original Description
The results from arc lint and just running cpplint.py are different. I suspect the regular expression is broken.

$ arc lint
>>> Lint for foo.h:


   Warning  (legal/copyright) legal/copyright
    No copyright message found.  You should have a line: "Copyright [year]
    <Copyright Owner>"

    >>>        1 #ifndef __FOO_H__
               2 #define __FOO_H__
               3
               4 #endif
$ cpplint.py foo.h
foo.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
foo.h:1:  #ifndef header guard has wrong style, please use: FOO_H_  [build/header_guard] [5]
foo.h:4:  #endif line should be "#endif  // FOO_H_"  [build/header_guard] [5]
Done processing foo.h
Total errors found: 3

Event Timeline

paul.redmond raised the priority of this task from to Needs Triage.
paul.redmond updated the task description. (Show Details)
paul.redmond added a project: Arcanist.
paul.redmond added a subscriber: paul.redmond.

Does this still happen if you run arc lint --lintall, or arc lint foo.h?

both have the same results--just one warning.

The reason for this is that cpplint.py doesn't seem to report the second or third warnings when passed input as stdin.

> cpplint.py foo.h
foo.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
foo.h:1:  #ifndef header guard has wrong style, please use: FOO_H_  [build/header_guard] [5]
foo.h:3:  #endif line should be "#endif  // FOO_H_"  [build/header_guard] [5]
Done processing foo.h
Total errors found: 3

> cat foo.h | cpplint.py -
-:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Done processing -
Total errors found: 1

The ArcanistCpplintLinter lints files by parsing the file's contents to cpplint.py as stdin.

Yeah I know... it's something to do with this (taken from cpplint.py --help):

root=subdir
  The root directory used for deriving header guard CPP variable.
  By default, the header guard CPP variable is calculated as the relative
  path to the directory that contains .git, .hg, or .svn.  When this flag
  is specified, the relative path is calculated from the specified
  directory. If the specified directory does not exist, this flag is
  ignored.

  Examples:
    Assuing that src/.git exists, the header guard CPP variables for
    src/chrome/browser/ui/browser.h are:

    No flag => CHROME_BROWSER_UI_BROWSER_H_
    --root=chrome => BROWSER_UI_BROWSER_H_
    --root=chrome/browser => UI_BROWSER_H_

Ah, that sort of makes sense. We can just stop it from using stdin for the moment.

All of the stdin stuff is only really supporting running lint in commit hook modes, which I'm not sure if we're ever really going to pursue again. It was valuable at Facebook -- essentially, rejecting commits of syntax errors -- but requires a large amount of finesse because no VCS gives us a working copy and we have only a few seconds to reasonably choose whether to block the commit or not. Build/CI is an easier general solution to this class of problem. If we do pursue it, it would be reasonable to be surgical about it (e.g., specialize a small subset of the linters and make the mode more explicit).

(There's also some small performance benefit if, say, the working copy is on NFS and the machine is hot, but in general we're giving up very little by passing paths instead of data.)

joshuaspence updated the task description. (Show Details)Jul 13 2014, 7:11 PM
joshuaspence renamed this task from arc lint cpplint linter doesn't correctly match all output to Perhaps remove the commit hook mode from linters.Jul 13 2014, 7:15 PM
joshuaspence triaged this task as Normal priority.
joshuaspence updated the task description. (Show Details)

We removed this in T5228.

woah clipboards woah crazy new technology T7674