Better linter filtering

.travis.yml

@@ -40,7 +40,7 @@ matrix:
       compiler: clang
       env: COMPILER=clang++
     - env: NAME="CPP-LINT"
-      script: scripts/run_lint.sh master HEAD || true
+      script: scripts/travis_lint.sh || true
 
 script:
   - if [ -L bin/gcc ] ; then export PATH=$PWD/bin:$PATH ; fi ;

scripts/filter_lint_by_diff.py

@@ -0,0 +1,33 @@
+#!/usr/bin/env python
+
+import sys
+import unidiff
+import os.path
+
+if len(sys.argv) != 3:
+  print >>sys.stderr, "Usage: filter_lint_by_diff.py diff.patch repository_root_directory < cpplint_warnings.txt"
+  sys.exit(1)
+
+added_lines = set()
+repository_root = sys.argv[2]
+
+with open(sys.argv[1], "r") as f:
+  diff = unidiff.PatchSet(f)
+  for diff_file in diff:
+    filename = diff_file.target_file
+    assert filename.startswith("b/")
+    filename = os.path.join(repository_root, filename[2:])
+    added_lines.add((filename, 0))
+    for diff_hunk in diff_file:
+      for diff_line in diff_hunk:
+        if diff_line.line_type == "+":
+          added_lines.add((filename, diff_line.target_line_no))
+
+for l in sys.stdin:
+  bits = l.split(":")
+  if len(bits) < 3:
+    continue
+  filename = os.path.join(repository_root, bits[0])
+  linenum = int(bits[1])
+  if (filename, linenum) in added_lines:
+    sys.stdout.write(l)

scripts/run_lint.sh

@@ -3,14 +3,13 @@
 set -e
 
 script_folder=`dirname $0`
+absolute_repository_root=`readlink -f $script_folder/..`
 
-if [[ "$#" -ne 2 ]]
+if [[ "$#" -gt 2 ]]
 then
-    echo "Script for running the CPP linter only on modified lines."
-    echo "Requires two arguments the start and the end"
-    echo "start - a git reference that marks the first commit whose changes to consider"
-    echo "end - a git reference that marks the last commit whose changes to consider"
-
+    echo "Script for running the CPP linter only on modified lines. Arguments:"
+    echo "target - a git reference to the branch we want to compare against (default: 'master')"
+    echo "tip - a git reference to the commit with changes (default: current working tree)"
     exit 1
 fi
 
@@ -21,75 +20,49 @@ then
   exit 1
 fi
 
-git_start=$1
-git_end=$2
+if [[ "$#" -gt 0 ]]
+then
+    git_start=$1
+else
+    git_start="master"
+fi
 
-# Get the list of files that have changed
-diff_files=`git diff --name-only $git_start $git_end`
+if [[ "$#" -gt 1 ]]
+then
+    git_end=$2
+    git_merge_base_end=$2
+else
+    git_end=""
+    git_merge_base_end="HEAD"
+fi
+
+git_start=`git merge-base $git_start $git_merge_base_end`
 
-# Build a filter that will filter the blame output
-# to only include lines that come from one of the relevant_commits
-# We do this by making the blame tool output the same hash for all
-# lines that are too old.
-blame_grep_filter=`git rev-parse "$git_start"`
+cleanup()
+{
+  rm -f $diff_file
+}
 
-# Build a regex for finding the line number of a given line inside blame
-# First matches the 40 digit hash of the commi
-# Then match an arbitary length number that represents the line in the original file
-# Finally matches (and groups) another arbitary length digit which is the
-# line in the final file
-regex="[0-9a-f]{40} [0-9]+ ([0-9]+)"
+trap cleanup EXIT
 
-# We only split on lines or otherwise the git  blame output is nonsense
-IFS=$'\n'
+diff_file=`mktemp`
 
-are_errors=0
+git diff $git_start $git_end > $diff_file
+
+# Get the list of files that have changed
+diff_files=`git diff --name-only $git_start $git_end`
 
 for file in $diff_files; do
+  file=$absolute_repository_root/$file
   # If the file has been deleted we don't want to run the linter on it
   if ! [[ -e $file ]]
   then
     continue
   fi
 
-  # We build another grep filter the output of the linting script
-  lint_grep_filter="^("
-
-  # Include line 0 errors (e.g. copyright)
-  lint_grep_filter+=$file
-  lint_grep_filter+=":0:"
-
-  # We first filter only the lines that start with a commit hash
-  # Then we filter out the ones that come from the start commit
-  modified_lines=`git blame $git_start..$git_end --line-porcelain $file | grep -E "^[0-9a-f]{40}" | { grep -v "$blame_grep_filter" || true; }`
-
-  # For each modified line we find the line number
-  for line in $modified_lines; do
-
-    # Use the above regex to match the line number
-    if [[ $line =~ $regex ]]
-    then
-      # Some bash magic to get the first group from the regex (the line number)
-      LINENUM="${BASH_REMATCH[1]}"
-
-      # The format from the linting script is filepath:linenum: [error type]
-      # So we build the first bit to filter out relevant lines
-      LINE_FILTER=$file:$LINENUM:
-
-      # Add the line filter on to the grep expression as we want
-      # lines that match any of the line filters
-      lint_grep_filter+="|"
-      lint_grep_filter+=$LINE_FILTER
-    fi
-  done
-
-  # Add the closing bracket
-  lint_grep_filter+=")"
-
-  # Run the linting script and filter by the filter we've build
-  # of all the modified lines
+  # Run the linting script and filter:
   # The errors from the linter go to STDERR so must be redirected to STDOUT
-  result=`$script_folder/cpplint.py $file 2>&1 | { grep -E "$lint_grep_filter" || true; }`
+  result=`$script_folder/cpplint.py $file 2>&1 | $script_folder/filter_lint_by_diff.py $diff_file $absolute_repository_root`
 
   # Providing some errors were relevant we print them out
   if [ "$result" ]
@@ -99,7 +72,5 @@ for file in $diff_files; do
   fi
 done
 
-unset IFS
-
 # Return an error code if errors are found
 exit $are_errors

scripts/travis_lint.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+
+set -e
+
+script_folder=`dirname $0`
+pip install --user unidiff
+
+if [ "$TRAVIS_PULL_REQUEST" == "false" ]; then
+  $script_folder/run_lint.sh HEAD~1 # Check for errors introduced in last commit
+else
+  $script_folder/run_lint.sh $TRAVIS_BRANCH # Check for errors compared to merge target
+fi
+