From 4c095329964c47c7a37c812fdaf58b9238cc9ff5 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 14:00:28 +0200
Subject: [PATCH 01/18] Fix practracker_tests.py and practracker line counting.

The practracker_tests.py unit test file called a function by its old
name.

Also, practracker counted functions as starting one line after the
function name, and ending with the closing brace.  Now they start
with the open brace and end with the closing brace.
---
 scripts/maint/practracker/metrics.py           | 8 ++++++--
 scripts/maint/practracker/practracker_tests.py | 8 +++++---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/scripts/maint/practracker/metrics.py b/scripts/maint/practracker/metrics.py
index 5fa305a868..b35029b51d 100644
--- a/scripts/maint/practracker/metrics.py
+++ b/scripts/maint/practracker/metrics.py
@@ -31,6 +31,7 @@ def get_function_lines(f):
                             "DISABLE_GCC_WARNING", "DISABLE_GCC_WARNINGS"}
 
     in_function = False
+    found_openbrace = False
     for lineno, line in enumerate(f):
         if not in_function:
             # find the start of a function
@@ -41,10 +42,13 @@ def get_function_lines(f):
                     continue
                 func_start = lineno
                 in_function = True
-
+        elif not found_openbrace and line.startswith("{"):
+            found_openbrace = True
+            func_start = lineno
         else:
             # Find the end of a function
             if line.startswith("}"):
-                n_lines = lineno - func_start
+                n_lines = lineno - func_start + 1
                 in_function = False
+                found_openbrace = False
                 yield (func_name, n_lines)
diff --git a/scripts/maint/practracker/practracker_tests.py b/scripts/maint/practracker/practracker_tests.py
index cdbab2908e..f11c55a3bd 100755
--- a/scripts/maint/practracker/practracker_tests.py
+++ b/scripts/maint/practracker/practracker_tests.py
@@ -1,3 +1,5 @@
+#!/usr/bin/python
+
 """Some simple tests for practracker metrics"""
 
 import unittest
@@ -38,13 +40,13 @@ class TestFunctionLength(unittest.TestCase):
     def test_function_length(self):
         funcs = StringIO.StringIO(function_file)
         # All functions should have length 2
-        for name, lines in metrics.function_lines(funcs):
+        for name, lines in metrics.get_function_lines(funcs):
             self.assertEqual(name, "fun")
 
         funcs.seek(0)
 
-        for name, lines in metrics.function_lines(funcs):
-            self.assertEqual(lines, 2)
+        for name, lines in metrics.get_function_lines(funcs):
+            self.assertEqual(lines, 4)
 
 if __name__ == '__main__':
     unittest.main()

From 86d3d310f529bc1e13ce1ca5013c674837c9856b Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 14:02:17 +0200
Subject: [PATCH 02/18] Practracker: allow tabs in include lines

This isn't actually something that Tor does, but it's cleaner to do
it this way.  Part of 29746.
---
 scripts/maint/practracker/metrics.py           |  2 +-
 scripts/maint/practracker/practracker_tests.py | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/scripts/maint/practracker/metrics.py b/scripts/maint/practracker/metrics.py
index b35029b51d..82f1cd64e9 100644
--- a/scripts/maint/practracker/metrics.py
+++ b/scripts/maint/practracker/metrics.py
@@ -16,7 +16,7 @@ def get_include_count(f):
     """Get number of #include statements in the file"""
     include_count = 0
     for line in f:
-        if re.match(r' *# *include', line):
+        if re.match(r'\s*#\s*include', line):
             include_count += 1
     return include_count
 
diff --git a/scripts/maint/practracker/practracker_tests.py b/scripts/maint/practracker/practracker_tests.py
index f11c55a3bd..865f68d186 100755
--- a/scripts/maint/practracker/practracker_tests.py
+++ b/scripts/maint/practracker/practracker_tests.py
@@ -48,5 +48,15 @@ class TestFunctionLength(unittest.TestCase):
         for name, lines in metrics.get_function_lines(funcs):
             self.assertEqual(lines, 4)
 
+class TestIncludeCount(unittest.TestCase):
+    def test_include_count(self):
+        f = StringIO.StringIO("""
+  #   include <abc.h>
+  #   include "def.h"
+#include "ghi.h"
+\t#\t include "jkl.h"
+""")
+        self.assertEqual(metrics.get_include_count(f),4)
+
 if __name__ == '__main__':
     unittest.main()

From f93057fc0ae7308c85a888599643d2dc369444de Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 14:09:47 +0200
Subject: [PATCH 03/18] Pracktracker: give the number of new errors found.

Part of 29746.
---
 scripts/maint/practracker/practracker.py | 41 +++++++++++++-----------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index febb14639d..0dc63f765c 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -54,25 +54,27 @@ else:
         return open(fname, 'r', encoding='utf-8')
 
 def consider_file_size(fname, f):
-    """Consider file size issues for 'f' and return True if a new issue was found"""
+    """Consider file size issues for 'f' and return the number of new issues was found"""
     file_size = metrics.get_file_len(f)
     if file_size > MAX_FILE_SIZE:
         p = problem.FileSizeProblem(fname, file_size)
-        return ProblemVault.register_problem(p)
-    return False
+        if ProblemVault.register_problem(p):
+            return 1
+    return 0
 
 def consider_includes(fname, f):
-    """Consider #include issues for 'f' and return True if a new issue was found"""
+    """Consider #include issues for 'f' and return the number of new issues found"""
     include_count = metrics.get_include_count(f)
 
     if include_count > MAX_INCLUDE_COUNT:
         p = problem.IncludeCountProblem(fname, include_count)
-        return ProblemVault.register_problem(p)
-    return False
+        if ProblemVault.register_problem(p):
+            return 1
+    return 0
 
 def consider_function_size(fname, f):
-    """Consider the function sizes for 'f' and return True if a new issue was found"""
-    found_new_issues = False
+    """Consider the function sizes for 'f' and return the number of new issues found."""
+    found_new_issues = 0
 
     for name, lines in metrics.get_function_lines(f):
         # Don't worry about functions within our limits
@@ -82,41 +84,42 @@ def consider_function_size(fname, f):
         # That's a big function! Issue a problem!
         canonical_function_name = "%s:%s()" % (fname, name)
         p = problem.FunctionSizeProblem(canonical_function_name, lines)
-        found_new_issues |= ProblemVault.register_problem(p)
+        if ProblemVault.register_problem(p):
+            found_new_issues += 1
 
     return found_new_issues
 
 #######################################################
 
 def consider_all_metrics(files_list):
-    """Consider metrics for all files, and return True if new issues were found"""
-    found_new_issues = False
+    """Consider metrics for all files, and return the number of new issues found."""
+    found_new_issues = 0
     for fname in files_list:
         with open_file(fname) as f:
-            found_new_issues |= consider_metrics_for_file(fname, f)
+            found_new_issues += consider_metrics_for_file(fname, f)
     return found_new_issues
 
 def consider_metrics_for_file(fname, f):
     """
     Consider the various metrics for file with filename 'fname' and file descriptor 'f'.
-    Return True if we found new issues.
+    Return the number of new issues found.
     """
     # Strip the useless part of the path
     if fname.startswith(TOR_TOPDIR):
         fname = fname[len(TOR_TOPDIR):]
 
-    found_new_issues = False
+    found_new_issues = 0
 
     # Get file length
-    found_new_issues |= consider_file_size(fname, f)
+    found_new_issues += consider_file_size(fname, f)
 
     # Consider number of #includes
     f.seek(0)
-    found_new_issues |= consider_includes(fname, f)
+    found_new_issues += consider_includes(fname, f)
 
     # Get function length
     f.seek(0)
-    found_new_issues |= consider_function_size(fname, f)
+    found_new_issues += consider_function_size(fname, f)
 
     return found_new_issues
 
@@ -201,13 +204,13 @@ def main(argv):
     # If new issues were found, try to give out some advice to the developer on how to resolve it.
     if found_new_issues and not args.regen:
         new_issues_str = """\
-FAILURE: practracker found new problems in the code: see warnings above.
+FAILURE: practracker found {} new problem(s) in the code: see warnings above.
 
 Please fix the problems if you can, and update the exceptions file
 ({}) if you can't.
 
 See doc/HACKING/HelpfulTools.md for more information on using practracker.\
-""".format(exceptions_file)
+""".format(found_new_issues, exceptions_file)
         print(new_issues_str)
 
     sys.exit(found_new_issues)

From 43f163de80864a4918b1566c3fbc0b73aac6a327 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 14:30:12 +0200
Subject: [PATCH 04/18] Practracker: improve exclude-directory logic

Instead of excluding directories at the last minute if they happen
to appear in our filenames, we exclude them early, before recursing
into all their subdirectories.

Part of 29746.
---
 scripts/maint/practracker/util.py | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/scripts/maint/practracker/util.py b/scripts/maint/practracker/util.py
index b0ca73b997..5a8876a0f6 100644
--- a/scripts/maint/practracker/util.py
+++ b/scripts/maint/practracker/util.py
@@ -2,15 +2,24 @@ import os
 
 # We don't want to run metrics for unittests, automatically-generated C files,
 # external libraries or git leftovers.
-EXCLUDE_SOURCE_DIRS = {"/src/test/", "/src/trunnel/", "/src/ext/", "/.git/"}
+EXCLUDE_SOURCE_DIRS = {"src/test/", "src/trunnel/", "src/rust/",
+                       "src/ext/", ".git/"}
+
+def _norm(p):
+    return os.path.normcase(os.path.normpath(p))
 
 def get_tor_c_files(tor_topdir):
     """
     Return a list with the .c filenames we want to get metrics of.
     """
     files_list = []
+    exclude_dirs = { _norm(os.path.join(tor_topdir, p)) for p in EXCLUDE_SOURCE_DIRS }
+
 
     for root, directories, filenames in os.walk(tor_topdir):
+        # Remove all the directories that are excluded.
+        directories[:] = [ d for d in directories
+                           if _norm(os.path.join(root,d)) not in exclude_dirs ]
         directories.sort()
         filenames.sort()
         for filename in filenames:
@@ -18,10 +27,7 @@ def get_tor_c_files(tor_topdir):
             if not filename.endswith(".c"):
                 continue
 
-            # Exclude the excluded paths
             full_path = os.path.join(root,filename)
-            if any(os.path.normcase(exclude_dir) in full_path for exclude_dir in EXCLUDE_SOURCE_DIRS):
-                continue
 
             files_list.append(full_path)
 

From 78768aafe1068bd76419944bea1ed3453d85edfe Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 18 Jul 2019 09:27:49 -0400
Subject: [PATCH 05/18] Changes file for 29746.

---
 changes/ticket29746 | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changes/ticket29746

diff --git a/changes/ticket29746 b/changes/ticket29746
new file mode 100644
index 0000000000..63b9edb391
--- /dev/null
+++ b/changes/ticket29746
@@ -0,0 +1,4 @@
+  o Minor bugfixes (best practices tracker):
+    - Fix a few issues in the best-practices script, including tests, tab
+      tolerance, error reporting, and directory-exclusion logic. Fixes bug
+      29746; bugfix on 0.4.1.1-alpha.

From a5e1fa3a036b0200e049625c427080881b276730 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 15:06:34 +0200
Subject: [PATCH 06/18] Practracker: add a --list-overstrict option

This option lists every exception that is stricter than it needs to
be.

Part of 30752
---
 scripts/maint/practracker/practracker.py | 12 ++++++++++++
 scripts/maint/practracker/problem.py     | 17 +++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 0dc63f765c..1b6502fe5a 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -56,6 +56,7 @@ else:
 def consider_file_size(fname, f):
     """Consider file size issues for 'f' and return the number of new issues was found"""
     file_size = metrics.get_file_len(f)
+
     if file_size > MAX_FILE_SIZE:
         p = problem.FileSizeProblem(fname, file_size)
         if ProblemVault.register_problem(p):
@@ -164,6 +165,8 @@ def main(argv):
     parser = argparse.ArgumentParser(prog=progname)
     parser.add_argument("--regen", action="store_true",
                         help="Regenerate the exceptions file")
+    parser.add_argument("--list-overstrict", action="store_true",
+                        help="List over-strict exceptions")
     parser.add_argument("--exceptions",
                         help="Override the location for the exceptions file")
     parser.add_argument("topdir", default=".", nargs="?",
@@ -213,6 +216,15 @@ See doc/HACKING/HelpfulTools.md for more information on using practracker.\
 """.format(found_new_issues, exceptions_file)
         print(new_issues_str)
 
+    if args.list_overstrict:
+        def k_fn(tup):
+            return tup[0].key()
+        for (ex,p) in sorted(ProblemVault.list_overstrict_exceptions(), key=k_fn):
+            if p is None:
+                print(ex, "->", 0)
+            else:
+                print(ex, "->", p.metric_value)
+
     sys.exit(found_new_issues)
 
 if __name__ == '__main__':
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index c82c5db572..751ceb9105 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -22,6 +22,9 @@ class ProblemVault(object):
     def __init__(self, exception_fname=None):
         # Exception dictionary: { problem.key() : Problem object }
         self.exceptions = {}
+        # Exception dictionary: maps key to the problem it was used to
+        # suppress.
+        self.used_exception_for = {}
 
         if exception_fname == None:
             return
@@ -71,9 +74,23 @@ class ProblemVault(object):
         if problem.is_worse_than(self.exceptions[problem.key()]):
             print(problem)
             return True
+        else:
+            self.used_exception_for[problem.key()] = problem
 
         return False
 
+    def list_overstrict_exceptions(self):
+        """Return an iterator of tuples containing (ex,prob) where ex is an
+           exceptions in this vault that are stricter than it needs to be, and
+           prob is the worst problem (if any) that it covered.
+        """
+        for k in self.exceptions:
+            e = self.exceptions[k]
+            p = self.used_exception_for.get(k)
+            if p is None or e.is_worse_than(p):
+                yield (e, p)
+
+
 class Problem(object):
     """
     A generic problem in our source code. See the subclasses below for the

From 6303c9aa2647365fa66de2b6ce1af109417b6603 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 15:20:58 +0200
Subject: [PATCH 07/18] Practracker: add tolerances for exceptions

When an exception is present, we can now violate the limit by a little
bit and only produce a warning.  The strict flag overrides this
behavior.

I've given file sizes a 2% tolerances and function sizes/include
counts a 10% tolerance.

Part of 30752
---
 scripts/maint/practracker/practracker.py | 14 ++++++++++++++
 scripts/maint/practracker/problem.py     | 16 ++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 1b6502fe5a..75cd44d228 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -36,6 +36,13 @@ MAX_FUNCTION_SIZE = 100 # lines
 # Recommended number of #includes
 MAX_INCLUDE_COUNT = 50
 
+# Map from problem type to functions that adjust for tolerance
+TOLERANCE_FNS = {
+    'include-count': lambda n: int(n*1.1),
+    'function-size': lambda n: int(n*1.1),
+    'file-size': lambda n: int(n*1.02)
+}
+
 #######################################################
 
 # ProblemVault singleton
@@ -169,6 +176,8 @@ def main(argv):
                         help="List over-strict exceptions")
     parser.add_argument("--exceptions",
                         help="Override the location for the exceptions file")
+    parser.add_argument("--strict", action="store_true",
+                        help="Make all warnings into errors")
     parser.add_argument("topdir", default=".", nargs="?",
                         help="Top-level directory for the tor source")
     args = parser.parse_args(argv[1:])
@@ -196,6 +205,11 @@ def main(argv):
     else:
         ProblemVault = problem.ProblemVault(exceptions_file)
 
+    # 2.1) Adjust the exceptions so that we warn only about small problems,
+    # and produce errors on big ones.
+    if not (args.regen or args.list_overstrict or args.strict):
+        ProblemVault.set_tolerances(TOLERANCE_FNS)
+
     # 3) Go through all the files and report problems if they are not exceptions
     found_new_issues = consider_all_metrics(files_list)
 
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index 751ceb9105..89a8f12346 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -90,6 +90,15 @@ class ProblemVault(object):
             if p is None or e.is_worse_than(p):
                 yield (e, p)
 
+    def set_tolerances(self, fns):
+        """Adjust the tolerances for the exceptions in this vault.  Takes
+           a map of problem type to a function that adjusts the permitted
+           function to its new maximum value."""
+        for k in self.exceptions:
+            ex = self.exceptions[k]
+            fn = fns.get(ex.problem_type)
+            if fn is not None:
+                ex.metric_value = fn(ex.metric_value)
 
 class Problem(object):
     """
@@ -99,14 +108,21 @@ class Problem(object):
     def __init__(self, problem_type, problem_location, metric_value):
         self.problem_location = problem_location
         self.metric_value = int(metric_value)
+        self.warning_threshold = self.metric_value
         self.problem_type = problem_type
 
     def is_worse_than(self, other_problem):
         """Return True if this is a worse problem than other_problem"""
         if self.metric_value > other_problem.metric_value:
             return True
+        elif self.metric_value > other_problem.warning_threshold:
+            self.warn()
         return False
 
+    def warn(self):
+        """Warn about this problem on stderr only."""
+        print("(warning) {}".format(self), file=sys.stderr)
+
     def key(self):
         """Generate a unique key that describes this problem that can be used as a dictionary key"""
         # Problem location is a filesystem path, so we need to normalize this

From d6a3636cdcb2556e99744dc3685db2010e151291 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 17 Jul 2019 15:28:48 +0200
Subject: [PATCH 08/18] Add a TOR_DISABLE_PRACTRACKER envvar for use by folks
 who don't care

Fixes part of bug 30752
---
 Makefile.am                              | 4 +++-
 scripts/maint/practracker/practracker.py | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index e823f9e103..d6ea72c17a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -369,7 +369,9 @@ endif
 
 check-best-practices:
 if USEPYTHON
-	$(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir)
+	@if test "$$TOR_DISABLE_PRACTRACKER" = ""; then \
+		$(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir); \
+	fi
 endif
 
 practracker-regen:
diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 75cd44d228..70035a0ab6 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -227,6 +227,9 @@ Please fix the problems if you can, and update the exceptions file
 ({}) if you can't.
 
 See doc/HACKING/HelpfulTools.md for more information on using practracker.\
+
+You can disable this message by setting the TOR_DISABLE_PRACTRACKER environment
+variable.
 """.format(found_new_issues, exceptions_file)
         print(new_issues_str)
 

From 3efe5cc57af040f8adb9a9e24a6ad1e77998a7a1 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 18 Jul 2019 09:27:58 -0400
Subject: [PATCH 09/18] changes file for 30752

---
 changes/ticket30752 | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 changes/ticket30752

diff --git a/changes/ticket30752 b/changes/ticket30752
new file mode 100644
index 0000000000..754a9003d1
--- /dev/null
+++ b/changes/ticket30752
@@ -0,0 +1,6 @@
+  o Minor features (best practices tracker):
+    - Give a warning rather than an error when a practracker exception is
+      violated by a small amount; add a --list-overstrict option to
+      practracker that lists exceptions that are stricter than they need to
+      be, and provide an environment variable for disabling
+      practracker. Closes ticekt 30752.

From ec13a727b0120485d5a90dcd053b3503b3080b58 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 09:03:55 -0400
Subject: [PATCH 10/18] practracker: Rename "Problem" to "Item".

I'm about to refactor the code into a set of iterators that yield
*all* the metrics for the code, and then add a filter on top of that
to return the problems.
---
 scripts/maint/practracker/practracker.py |  6 +++---
 scripts/maint/practracker/problem.py     | 26 ++++++++++++------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 70035a0ab6..bcced93c5f 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -65,7 +65,7 @@ def consider_file_size(fname, f):
     file_size = metrics.get_file_len(f)
 
     if file_size > MAX_FILE_SIZE:
-        p = problem.FileSizeProblem(fname, file_size)
+        p = problem.FileSizeItem(fname, file_size)
         if ProblemVault.register_problem(p):
             return 1
     return 0
@@ -75,7 +75,7 @@ def consider_includes(fname, f):
     include_count = metrics.get_include_count(f)
 
     if include_count > MAX_INCLUDE_COUNT:
-        p = problem.IncludeCountProblem(fname, include_count)
+        p = problem.IncludeCountItem(fname, include_count)
         if ProblemVault.register_problem(p):
             return 1
     return 0
@@ -91,7 +91,7 @@ def consider_function_size(fname, f):
 
         # That's a big function! Issue a problem!
         canonical_function_name = "%s:%s()" % (fname, name)
-        p = problem.FunctionSizeProblem(canonical_function_name, lines)
+        p = problem.FunctionSizeItem(canonical_function_name, lines)
         if ProblemVault.register_problem(p):
             found_new_issues += 1
 
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index 89a8f12346..317e2a4a50 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -100,10 +100,10 @@ class ProblemVault(object):
             if fn is not None:
                 ex.metric_value = fn(ex.metric_value)
 
-class Problem(object):
+class Item(object):
     """
-    A generic problem in our source code. See the subclasses below for the
-    specific problems we are trying to tackle.
+    A generic measurement about some aspect of our source code. See
+    the subclasses below for the specific problems we are trying to tackle.
     """
     def __init__(self, problem_type, problem_location, metric_value):
         self.problem_location = problem_location
@@ -125,7 +125,7 @@ class Problem(object):
 
     def key(self):
         """Generate a unique key that describes this problem that can be used as a dictionary key"""
-        # Problem location is a filesystem path, so we need to normalize this
+        # Item location is a filesystem path, so we need to normalize this
         # across platforms otherwise same paths are not gonna match.
         canonical_location = os.path.normcase(self.problem_location)
         return "%s:%s" % (canonical_location, self.problem_type)
@@ -133,7 +133,7 @@ class Problem(object):
     def __str__(self):
         return "problem %s %s %s" % (self.problem_type, self.problem_location, self.metric_value)
 
-class FileSizeProblem(Problem):
+class FileSizeItem(Item):
     """
     Denotes a problem with the size of a .c file.
 
@@ -141,9 +141,9 @@ class FileSizeProblem(Problem):
     'metric_value' is the number of lines in the .c file.
     """
     def __init__(self, problem_location, metric_value):
-        super(FileSizeProblem, self).__init__("file-size", problem_location, metric_value)
+        super(FileSizeItem, self).__init__("file-size", problem_location, metric_value)
 
-class IncludeCountProblem(Problem):
+class IncludeCountItem(Item):
     """
     Denotes a problem with the number of #includes in a .c file.
 
@@ -151,9 +151,9 @@ class IncludeCountProblem(Problem):
     'metric_value' is the number of #includes in the .c file.
     """
     def __init__(self, problem_location, metric_value):
-        super(IncludeCountProblem, self).__init__("include-count", problem_location, metric_value)
+        super(IncludeCountItem, self).__init__("include-count", problem_location, metric_value)
 
-class FunctionSizeProblem(Problem):
+class FunctionSizeItem(Item):
     """
     Denotes a problem with a size of a function in a .c file.
 
@@ -164,7 +164,7 @@ class FunctionSizeProblem(Problem):
     The 'metric_value' is the size of the offending function in lines.
     """
     def __init__(self, problem_location, metric_value):
-        super(FunctionSizeProblem, self).__init__("function-size", problem_location, metric_value)
+        super(FunctionSizeItem, self).__init__("function-size", problem_location, metric_value)
 
 comment_re = re.compile(r'#.*$')
 
@@ -182,10 +182,10 @@ def get_old_problem_from_exception_str(exception_str):
         raise ValueError("Misformatted line {!r}".format(orig_str))
 
     if problem_type == "file-size":
-        return FileSizeProblem(problem_location, metric_value)
+        return FileSizeItem(problem_location, metric_value)
     elif problem_type == "include-count":
-        return IncludeCountProblem(problem_location, metric_value)
+        return IncludeCountItem(problem_location, metric_value)
     elif problem_type == "function-size":
-        return FunctionSizeProblem(problem_location, metric_value)
+        return FunctionSizeItem(problem_location, metric_value)
     else:
         raise ValueError("Unknown exception type {!r}".format(orig_str))

From bcef6a580254234c7396761f300cca79c10d21e5 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 09:20:08 -0400
Subject: [PATCH 11/18] practracker: Refactor flow to use generators

Instead of having "consider" functions that have to call a global
ProblemVault, we can now generate all the metrics for the code
separately from the decision about what to do for them.
---
 scripts/maint/practracker/practracker.py | 72 +++++++++++-------------
 scripts/maint/practracker/problem.py     | 21 +++++++
 2 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index bcced93c5f..92de021e00 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -36,6 +36,7 @@ MAX_FUNCTION_SIZE = 100 # lines
 # Recommended number of #includes
 MAX_INCLUDE_COUNT = 50
 
+
 # Map from problem type to functions that adjust for tolerance
 TOLERANCE_FNS = {
     'include-count': lambda n: int(n*1.1),
@@ -51,6 +52,12 @@ ProblemVault = None
 # The Tor source code topdir
 TOR_TOPDIR = None
 
+# ProblemFilter singleton.
+FILTER = problem.ProblemFilter()
+FILTER.addThreshold(problem.FileSizeItem("*", MAX_FILE_SIZE))
+FILTER.addThreshold(problem.IncludeCountItem("*", MAX_INCLUDE_COUNT))
+FILTER.addThreshold(problem.FunctionSizeItem("*", MAX_FUNCTION_SIZE))
+
 #######################################################
 
 if sys.version_info[0] <= 2:
@@ -61,75 +68,59 @@ else:
         return open(fname, 'r', encoding='utf-8')
 
 def consider_file_size(fname, f):
-    """Consider file size issues for 'f' and return the number of new issues was found"""
+    """Consider the size of 'f' and yield an FileSizeItem for it.
+    """
     file_size = metrics.get_file_len(f)
-
-    if file_size > MAX_FILE_SIZE:
-        p = problem.FileSizeItem(fname, file_size)
-        if ProblemVault.register_problem(p):
-            return 1
-    return 0
+    yield problem.FileSizeItem(fname, file_size)
 
 def consider_includes(fname, f):
-    """Consider #include issues for 'f' and return the number of new issues found"""
+    """Consider the #include count in for 'f' and yield an IncludeCountItem
+        for it.
+    """
     include_count = metrics.get_include_count(f)
 
-    if include_count > MAX_INCLUDE_COUNT:
-        p = problem.IncludeCountItem(fname, include_count)
-        if ProblemVault.register_problem(p):
-            return 1
-    return 0
+    yield problem.IncludeCountItem(fname, include_count)
 
 def consider_function_size(fname, f):
-    """Consider the function sizes for 'f' and return the number of new issues found."""
-    found_new_issues = 0
+    """yield a FunctionSizeItem for every function in f.
+    """
 
     for name, lines in metrics.get_function_lines(f):
-        # Don't worry about functions within our limits
-        if lines <= MAX_FUNCTION_SIZE:
-            continue
-
-        # That's a big function! Issue a problem!
         canonical_function_name = "%s:%s()" % (fname, name)
-        p = problem.FunctionSizeItem(canonical_function_name, lines)
-        if ProblemVault.register_problem(p):
-            found_new_issues += 1
-
-    return found_new_issues
+        yield problem.FunctionSizeItem(canonical_function_name, lines)
 
 #######################################################
 
 def consider_all_metrics(files_list):
-    """Consider metrics for all files, and return the number of new issues found."""
-    found_new_issues = 0
+    """Consider metrics for all files, and yield a sequence of problem.Item
+       object for those issues."""
     for fname in files_list:
         with open_file(fname) as f:
-            found_new_issues += consider_metrics_for_file(fname, f)
-    return found_new_issues
+            for item in consider_metrics_for_file(fname, f):
+                yield item
 
 def consider_metrics_for_file(fname, f):
     """
-    Consider the various metrics for file with filename 'fname' and file descriptor 'f'.
-    Return the number of new issues found.
+       Yield a sequence of problem.Item objects for all of the metrics in
+       'f'.
     """
     # Strip the useless part of the path
     if fname.startswith(TOR_TOPDIR):
         fname = fname[len(TOR_TOPDIR):]
 
-    found_new_issues = 0
-
     # Get file length
-    found_new_issues += consider_file_size(fname, f)
+    for item in consider_file_size(fname, f):
+        yield item
 
     # Consider number of #includes
     f.seek(0)
-    found_new_issues += consider_includes(fname, f)
+    for item in consider_includes(fname, f):
+        yield item
 
     # Get function length
     f.seek(0)
-    found_new_issues += consider_function_size(fname, f)
-
-    return found_new_issues
+    for item in consider_function_size(fname, f):
+        yield item
 
 HEADER="""\
 # Welcome to the exceptions file for Tor's best-practices tracker!
@@ -211,7 +202,10 @@ def main(argv):
         ProblemVault.set_tolerances(TOLERANCE_FNS)
 
     # 3) Go through all the files and report problems if they are not exceptions
-    found_new_issues = consider_all_metrics(files_list)
+    found_new_issues = 0
+    for item in FILTER.filter(consider_all_metrics(files_list)):
+        if ProblemVault.register_problem(item):
+            found_new_issues += 1
 
     if args.regen:
         tmpfile.close()
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index 317e2a4a50..cf804ad431 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -100,6 +100,24 @@ class ProblemVault(object):
             if fn is not None:
                 ex.metric_value = fn(ex.metric_value)
 
+class ProblemFilter(object):
+    def __init__(self):
+        self.thresholds = dict()
+
+    def addThreshold(self, item):
+        self.thresholds[item.get_type()] = item
+
+    def matches(self, item):
+        filt = self.thresholds.get(item.get_type(), None)
+        if filt is None:
+            return False
+        return item.is_worse_than(filt)
+
+    def filter(self, sequence):
+        for item in iter(sequence):
+            if self.matches(item):
+                yield item
+
 class Item(object):
     """
     A generic measurement about some aspect of our source code. See
@@ -133,6 +151,9 @@ class Item(object):
     def __str__(self):
         return "problem %s %s %s" % (self.problem_type, self.problem_location, self.metric_value)
 
+    def get_type(self):
+        return self.problem_type
+
 class FileSizeItem(Item):
     """
     Denotes a problem with the size of a .c file.

From 65cb4fead50903e7df40f593f12e56902b63458f Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 10:15:11 -0400
Subject: [PATCH 12/18] practracker: Move the warning/error distinction to a
 higher level.

Previously warnings were generated by magic inside ProblemVault; now
they're printed on demand.
---
 scripts/maint/practracker/practracker.py |  6 ++++-
 scripts/maint/practracker/problem.py     | 34 +++++++++++++++---------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 92de021e00..0e6490aab4 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -204,8 +204,12 @@ def main(argv):
     # 3) Go through all the files and report problems if they are not exceptions
     found_new_issues = 0
     for item in FILTER.filter(consider_all_metrics(files_list)):
-        if ProblemVault.register_problem(item):
+        status = ProblemVault.register_problem(item)
+        if status == problem.STATUS_ERR:
+            print(item)
             found_new_issues += 1
+        elif status == problem.STATUS_WARN:
+            item.warn()
 
     if args.regen:
         tmpfile.close()
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index cf804ad431..d162e19ef9 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -13,6 +13,10 @@ import os.path
 import re
 import sys
 
+STATUS_ERR = 2
+STATUS_WARN = 1
+STATUS_OK = 0
+
 class ProblemVault(object):
     """
     Singleton where we store the various new problems we
@@ -60,24 +64,23 @@ class ProblemVault(object):
 
     def register_problem(self, problem):
         """
-        Register this problem to the problem value. Return True if it was a new
-        problem or it worsens an already existing problem.
+        Register this problem to the problem value. Return true if it was a new
+        problem or it worsens an already existing problem.  A true
+        value may be STATUS_ERR to indicate a hard violation, or STATUS_WARN
+        to indicate a warning.
         """
         # This is a new problem, print it
         if problem.key() not in self.exceptions:
-            print(problem)
-            return True
+            return STATUS_ERR
 
         # If it's an old problem, we don't warn if the situation got better
         # (e.g. we went from 4k LoC to 3k LoC), but we do warn if the
         # situation worsened (e.g. we went from 60 includes to 80).
-        if problem.is_worse_than(self.exceptions[problem.key()]):
-            print(problem)
-            return True
-        else:
+        status = problem.is_worse_than(self.exceptions[problem.key()])
+        if status == STATUS_OK:
             self.used_exception_for[problem.key()] = problem
 
-        return False
+        return status
 
     def list_overstrict_exceptions(self):
         """Return an iterator of tuples containing (ex,prob) where ex is an
@@ -130,12 +133,17 @@ class Item(object):
         self.problem_type = problem_type
 
     def is_worse_than(self, other_problem):
-        """Return True if this is a worse problem than other_problem"""
+        """Return STATUS_ERR if this is a worse problem than other_problem.
+           Return STATUS_WARN if it is a little worse, but falls within the
+           warning threshold.  Return STATUS_OK if this problem is not
+           at all worse than other_problem.
+        """
         if self.metric_value > other_problem.metric_value:
-            return True
+            return STATUS_ERR
         elif self.metric_value > other_problem.warning_threshold:
-            self.warn()
-        return False
+            return STATUS_WARN
+        else:
+            return STATUS_OK
 
     def warn(self):
         """Warn about this problem on stderr only."""

From 31a0b818542ea459db996daa0dd364f32b310474 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 10:17:19 -0400
Subject: [PATCH 13/18] practracker: Remove problemvault global.

---
 scripts/maint/practracker/practracker.py | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 0e6490aab4..3bf16dcdec 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -46,9 +46,6 @@ TOLERANCE_FNS = {
 
 #######################################################
 
-# ProblemVault singleton
-ProblemVault = None
-
 # The Tor source code topdir
 TOR_TOPDIR = None
 
@@ -185,8 +182,6 @@ def main(argv):
 
     # 2) Initialize problem vault and load an optional exceptions file so that
     # we don't warn about the past
-    global ProblemVault
-
     if args.regen:
         tmpname = exceptions_file + ".tmp"
         tmpfile = open(tmpname, "w")

From 3f303c102a3adea24d8a3f513c8528eb417d5283 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 11:49:50 -0400
Subject: [PATCH 14/18] Practracker: new flags to control output.

These flags let you suppress the message about the number of
problems and warnings, and let you control the thresholds above
which something counts as a problem.

I need this for testing.
---
 scripts/maint/practracker/practracker.py | 25 +++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 3bf16dcdec..fb6649bdcb 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -36,7 +36,6 @@ MAX_FUNCTION_SIZE = 100 # lines
 # Recommended number of #includes
 MAX_INCLUDE_COUNT = 50
 
-
 # Map from problem type to functions that adjust for tolerance
 TOLERANCE_FNS = {
     'include-count': lambda n: int(n*1.1),
@@ -49,12 +48,6 @@ TOLERANCE_FNS = {
 # The Tor source code topdir
 TOR_TOPDIR = None
 
-# ProblemFilter singleton.
-FILTER = problem.ProblemFilter()
-FILTER.addThreshold(problem.FileSizeItem("*", MAX_FILE_SIZE))
-FILTER.addThreshold(problem.IncludeCountItem("*", MAX_INCLUDE_COUNT))
-FILTER.addThreshold(problem.FunctionSizeItem("*", MAX_FUNCTION_SIZE))
-
 #######################################################
 
 if sys.version_info[0] <= 2:
@@ -166,6 +159,14 @@ def main(argv):
                         help="Override the location for the exceptions file")
     parser.add_argument("--strict", action="store_true",
                         help="Make all warnings into errors")
+    parser.add_argument("--terse", action="store_true",
+                        help="Do not emit helpful instructions.")
+    parser.add_argument("--max-file-size", default=MAX_FILE_SIZE,
+                        help="Maximum lines per C file size")
+    parser.add_argument("--max-include-count", default=MAX_INCLUDE_COUNT,
+                        help="Maximum includes per C file")
+    parser.add_argument("--max-function-size", default=MAX_FUNCTION_SIZE,
+                        help="Maximum lines per function")
     parser.add_argument("topdir", default=".", nargs="?",
                         help="Top-level directory for the tor source")
     args = parser.parse_args(argv[1:])
@@ -177,6 +178,12 @@ def main(argv):
     else:
         exceptions_file = os.path.join(TOR_TOPDIR, "scripts/maint/practracker", EXCEPTIONS_FNAME)
 
+    # 0) Configure our thresholds of "what is a problem actually"
+    filt = problem.ProblemFilter()
+    filt.addThreshold(problem.FileSizeItem("*", int(args.max_file_size)))
+    filt.addThreshold(problem.IncludeCountItem("*", int(args.max_include_count)))
+    filt.addThreshold(problem.FunctionSizeItem("*", int(args.max_function_size)))
+
     # 1) Get all the .c files we care about
     files_list = util.get_tor_c_files(TOR_TOPDIR)
 
@@ -198,7 +205,7 @@ def main(argv):
 
     # 3) Go through all the files and report problems if they are not exceptions
     found_new_issues = 0
-    for item in FILTER.filter(consider_all_metrics(files_list)):
+    for item in filt.filter(consider_all_metrics(files_list)):
         status = ProblemVault.register_problem(item)
         if status == problem.STATUS_ERR:
             print(item)
@@ -212,7 +219,7 @@ def main(argv):
         sys.exit(0)
 
     # If new issues were found, try to give out some advice to the developer on how to resolve it.
-    if found_new_issues and not args.regen:
+    if found_new_issues and not args.regen and not args.terse:
         new_issues_str = """\
 FAILURE: practracker found {} new problem(s) in the code: see warnings above.
 

From a79e2c2975f478045c1a3a6fdd34d22d355cce8b Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 11:54:05 -0400
Subject: [PATCH 15/18] practracker: better warning/regen handling

Now that there is only one toplevel place where we print problems,
we can redirect just that one print to a file when we are
regenerating the exceptions.txt file.  Previously we redirected
sys.stdout, which is naughty, and forced us to send warnings (and
warnings alone) to stderr.
---
 scripts/maint/practracker/practracker.py | 9 +++++----
 scripts/maint/practracker/problem.py     | 4 ----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index fb6649bdcb..a60b0a8425 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -192,11 +192,11 @@ def main(argv):
     if args.regen:
         tmpname = exceptions_file + ".tmp"
         tmpfile = open(tmpname, "w")
-        sys.stdout = tmpfile
-        sys.stdout.write(HEADER)
+        problem_file = tmpfile
         ProblemVault = problem.ProblemVault()
     else:
         ProblemVault = problem.ProblemVault(exceptions_file)
+        problem_file = sys.stdout
 
     # 2.1) Adjust the exceptions so that we warn only about small problems,
     # and produce errors on big ones.
@@ -208,10 +208,11 @@ def main(argv):
     for item in filt.filter(consider_all_metrics(files_list)):
         status = ProblemVault.register_problem(item)
         if status == problem.STATUS_ERR:
-            print(item)
+            print(item, file=problem_file)
             found_new_issues += 1
         elif status == problem.STATUS_WARN:
-            item.warn()
+            # warnings always go to stdout.
+            print("(warning) {}".format(item))
 
     if args.regen:
         tmpfile.close()
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index d162e19ef9..d09e941518 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -145,10 +145,6 @@ class Item(object):
         else:
             return STATUS_OK
 
-    def warn(self):
-        """Warn about this problem on stderr only."""
-        print("(warning) {}".format(self), file=sys.stderr)
-
     def key(self):
         """Generate a unique key that describes this problem that can be used as a dictionary key"""
         # Item location is a filesystem path, so we need to normalize this

From 8d3f3e5d30fa97479bc218a2896cb538ea220514 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 30 Jul 2019 12:07:40 -0400
Subject: [PATCH 16/18] Practracker: add an integration test.

This test runs practracker with a set of 0 thresholds, to make sure
that it enumerates all its values right.  It tries running with an
empty exceptions file, and with an exceptions file that covers
_some_ of the data, and it makes sure that the outputs are as expected.
---
 scripts/maint/practracker/test_practracker.sh | 50 +++++++++++++++++++
 scripts/maint/practracker/testdata/a.c        | 38 ++++++++++++++
 scripts/maint/practracker/testdata/b.c        | 15 ++++++
 scripts/maint/practracker/testdata/ex.txt     |  0
 .../practracker/testdata/ex0-expected.txt     |  7 +++
 scripts/maint/practracker/testdata/ex0.txt    |  0
 .../practracker/testdata/ex1-expected.txt     |  3 ++
 scripts/maint/practracker/testdata/ex1.txt    | 11 ++++
 scripts/maint/practracker/testdata/not_c_file |  2 +
 9 files changed, 126 insertions(+)
 create mode 100755 scripts/maint/practracker/test_practracker.sh
 create mode 100644 scripts/maint/practracker/testdata/a.c
 create mode 100644 scripts/maint/practracker/testdata/b.c
 create mode 100644 scripts/maint/practracker/testdata/ex.txt
 create mode 100644 scripts/maint/practracker/testdata/ex0-expected.txt
 create mode 100644 scripts/maint/practracker/testdata/ex0.txt
 create mode 100644 scripts/maint/practracker/testdata/ex1-expected.txt
 create mode 100644 scripts/maint/practracker/testdata/ex1.txt
 create mode 100644 scripts/maint/practracker/testdata/not_c_file

diff --git a/scripts/maint/practracker/test_practracker.sh b/scripts/maint/practracker/test_practracker.sh
new file mode 100755
index 0000000000..590525660b
--- /dev/null
+++ b/scripts/maint/practracker/test_practracker.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+umask 077
+
+TMPDIR=""
+clean () {
+  if [ -n "$TMPDIR" ] && [ -d "$TMPDIR" ]; then
+    rm -rf "$TMPDIR"
+  fi
+}
+trap clean EXIT HUP INT TERM
+
+if test "${PRACTRACKER_DIR}" = "" ||
+        test ! -e "${PRACTRACKER_DIR}/practracker.py" ; then
+    PRACTRACKER_DIR=$(dirname "$0")
+fi
+
+TMPDIR="$(mktemp -d -t pracktracker.test.XXXXXX)"
+if test -z "${TMPDIR}" || test ! -d "${TMPDIR}" ; then
+    echo >&2 "mktemp failed."
+    exit 1;
+fi
+
+DATA="${PRACTRACKER_DIR}/testdata"
+
+run_practracker() {
+    "${PYTHON:-python}" "${PRACTRACKER_DIR}/practracker.py" \
+        --max-include-count=0 --max-file-size=0 --max-function-size=0 --terse \
+        "${DATA}/" "$@";
+}
+
+echo "ex0:"
+
+run_practracker --exceptions "${DATA}/ex0.txt" > "${TMPDIR}/ex0-received.txt"
+
+if cmp "${TMPDIR}/ex0-received.txt" "${DATA}/ex0-expected.txt" ; then
+    echo "  OK"
+else
+    exit 1
+fi
+
+echo "ex1:"
+
+run_practracker --exceptions "${DATA}/ex1.txt" > "${TMPDIR}/ex1-received.txt"
+
+if cmp "${TMPDIR}/ex1-received.txt" "${DATA}/ex1-expected.txt" ;then
+    echo "  OK"
+else
+    exit 1
+fi
diff --git a/scripts/maint/practracker/testdata/a.c b/scripts/maint/practracker/testdata/a.c
new file mode 100644
index 0000000000..b52a14f56a
--- /dev/null
+++ b/scripts/maint/practracker/testdata/a.c
@@ -0,0 +1,38 @@
+
+#include "one.h"
+#include "two.h"
+#incldue "three.h"
+
+# include "four.h"
+
+int
+i_am_a_function(void)
+{
+  call();
+  call();
+  /* comment
+
+     another */
+
+  return 3;
+}
+
+#	include  "five.h"
+
+long
+another_function(long x,
+                 long y)
+{
+  int abcd;
+
+  abcd = x+y;
+  abcd *= abcd;
+
+  /* comment here */
+
+  return abcd +
+    abcd +
+    abcd;
+}
+
+/* And a comment to grow! */
diff --git a/scripts/maint/practracker/testdata/b.c b/scripts/maint/practracker/testdata/b.c
new file mode 100644
index 0000000000..bef277aaae
--- /dev/null
+++ b/scripts/maint/practracker/testdata/b.c
@@ -0,0 +1,15 @@
+
+MOCK_IMPL(int,
+foo,(void))
+{
+  // blah1
+  return 0;
+}
+
+MOCK_IMPL(int,
+bar,( long z))
+{
+  // blah2
+
+  return (int)(z+2);
+}
diff --git a/scripts/maint/practracker/testdata/ex.txt b/scripts/maint/practracker/testdata/ex.txt
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/scripts/maint/practracker/testdata/ex0-expected.txt b/scripts/maint/practracker/testdata/ex0-expected.txt
new file mode 100644
index 0000000000..c021e6f710
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex0-expected.txt
@@ -0,0 +1,7 @@
+problem file-size a.c 38
+problem include-count a.c 4
+problem function-size a.c:i_am_a_function() 9
+problem function-size a.c:another_function() 12
+problem file-size b.c 15
+problem function-size b.c:foo() 4
+problem function-size b.c:bar() 5
diff --git a/scripts/maint/practracker/testdata/ex0.txt b/scripts/maint/practracker/testdata/ex0.txt
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/scripts/maint/practracker/testdata/ex1-expected.txt b/scripts/maint/practracker/testdata/ex1-expected.txt
new file mode 100644
index 0000000000..58140a4d9a
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex1-expected.txt
@@ -0,0 +1,3 @@
+problem function-size a.c:i_am_a_function() 9
+(warning) problem function-size a.c:another_function() 12
+problem function-size b.c:foo() 4
diff --git a/scripts/maint/practracker/testdata/ex1.txt b/scripts/maint/practracker/testdata/ex1.txt
new file mode 100644
index 0000000000..db42ae8450
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex1.txt
@@ -0,0 +1,11 @@
+
+problem file-size a.c 38
+problem include-count a.c 4
+# this problem will produce an error
+problem function-size a.c:i_am_a_function() 8
+# this problem will produce a warning
+problem function-size a.c:another_function() 11
+problem file-size b.c 15
+# This is removed, and so will produce an error.
+# problem function-size b.c:foo() 4
+problem function-size b.c:bar() 5
diff --git a/scripts/maint/practracker/testdata/not_c_file b/scripts/maint/practracker/testdata/not_c_file
new file mode 100644
index 0000000000..e150962c02
--- /dev/null
+++ b/scripts/maint/practracker/testdata/not_c_file
@@ -0,0 +1,2 @@
+
+This isn't a C file, so practracker shouldn't care about it.

From 3221dc1b32f651f45184efab6bc3a8fa4863aea4 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 1 Aug 2019 08:40:56 -0400
Subject: [PATCH 17/18] Lower check of TOR_DISABLE_PRACTRACKER

Since we sometimes call practracker directly, that's where we should
check the TOR_DISABLE_PRACTRACKER envvar.
---
 Makefile.am                              | 4 +---
 scripts/maint/practracker/practracker.py | 2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d6ea72c17a..b1e94ae0fc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -369,9 +369,7 @@ endif
 
 check-best-practices:
 if USEPYTHON
-	@if test "$$TOR_DISABLE_PRACTRACKER" = ""; then \
-		$(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir); \
-	fi
+	@$(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir)
 endif
 
 practracker-regen:
diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index a60b0a8425..245d01d36d 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -246,4 +246,6 @@ variable.
     sys.exit(found_new_issues)
 
 if __name__ == '__main__':
+    if os.environ.get("TOR_DISABLE_PRACTRACKER"):
+        sys.exit(0)
     main(sys.argv)

From 19536fd18d02a52031f7914d5930645bb53b9dba Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 1 Aug 2019 09:35:33 -0400
Subject: [PATCH 18/18] practracker: replaces "overstrict" with "overbroad"

I had the logic reversed here.
---
 changes/ticket30752                      | 2 +-
 scripts/maint/practracker/practracker.py | 8 ++++----
 scripts/maint/practracker/problem.py     | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/changes/ticket30752 b/changes/ticket30752
index 754a9003d1..044c7c7d93 100644
--- a/changes/ticket30752
+++ b/changes/ticket30752
@@ -1,6 +1,6 @@
   o Minor features (best practices tracker):
     - Give a warning rather than an error when a practracker exception is
-      violated by a small amount; add a --list-overstrict option to
+      violated by a small amount; add a --list-overbroad option to
       practracker that lists exceptions that are stricter than they need to
       be, and provide an environment variable for disabling
       practracker. Closes ticekt 30752.
diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index 245d01d36d..a4e951ac7c 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -153,7 +153,7 @@ def main(argv):
     parser = argparse.ArgumentParser(prog=progname)
     parser.add_argument("--regen", action="store_true",
                         help="Regenerate the exceptions file")
-    parser.add_argument("--list-overstrict", action="store_true",
+    parser.add_argument("--list-overbroad", action="store_true",
                         help="List over-strict exceptions")
     parser.add_argument("--exceptions",
                         help="Override the location for the exceptions file")
@@ -200,7 +200,7 @@ def main(argv):
 
     # 2.1) Adjust the exceptions so that we warn only about small problems,
     # and produce errors on big ones.
-    if not (args.regen or args.list_overstrict or args.strict):
+    if not (args.regen or args.list_overbroad or args.strict):
         ProblemVault.set_tolerances(TOLERANCE_FNS)
 
     # 3) Go through all the files and report problems if they are not exceptions
@@ -234,10 +234,10 @@ variable.
 """.format(found_new_issues, exceptions_file)
         print(new_issues_str)
 
-    if args.list_overstrict:
+    if args.list_overbroad:
         def k_fn(tup):
             return tup[0].key()
-        for (ex,p) in sorted(ProblemVault.list_overstrict_exceptions(), key=k_fn):
+        for (ex,p) in sorted(ProblemVault.list_overbroad_exceptions(), key=k_fn):
             if p is None:
                 print(ex, "->", 0)
             else:
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index d09e941518..73519d446f 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -82,7 +82,7 @@ class ProblemVault(object):
 
         return status
 
-    def list_overstrict_exceptions(self):
+    def list_overbroad_exceptions(self):
         """Return an iterator of tuples containing (ex,prob) where ex is an
            exceptions in this vault that are stricter than it needs to be, and
            prob is the worst problem (if any) that it covered.