Merge branch 'tor-github/pr/1337'

8 files changed, 37 insertions, 3 deletions

diff --git a/Makefile.am b/Makefile.am
index 491b4c8f9f..845440b56f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -183,6 +183,7 @@ EXTRA_DIST+= \
 	scripts/maint/practracker/testdata/ex0.txt 	\
 	scripts/maint/practracker/testdata/ex1-expected.txt \
 	scripts/maint/practracker/testdata/ex1.txt 	\
+	scripts/maint/practracker/testdata/ex1-overbroad-expected.txt 	\
 	scripts/maint/practracker/testdata/ex.txt 	\
 	scripts/maint/practracker/testdata/header.h 	\
 	scripts/maint/practracker/testdata/not_c_file	\
diff --git a/changes/ticket31338 b/changes/ticket31338
new file mode 100644
index 0000000000..b76add635d
--- /dev/null
+++ b/changes/ticket31338
@@ -0,0 +1,4 @@
+  o Minor bugfixes (best practices tracker):
+    - When listing overbroad exceptions, do not also list problems,
+      and do not list insufficiently broad exceptions. Fixes bug 31338;
+      bugfix on 0.4.2.1-alpha.
diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index b5a68fd27c..f6aac9d15e 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -224,6 +224,11 @@ def main(argv):
     filt.addThreshold(problem.DependencyViolationItem("*.c", int(args.max_dependency_violations)))
     filt.addThreshold(problem.DependencyViolationItem("*.h", int(args.max_dependency_violations)))
 
+    if args.list_overbroad and args.regen:
+        print("Cannot use --regen with --list-overbroad",
+              file=sys.stderr)
+        sys.exit(1)
+
     # 1) Get all the .c files we care about
     files_list = util.get_tor_c_files(TOR_TOPDIR, args.include_dir)
 
@@ -239,6 +244,10 @@ def main(argv):
         ProblemVault = problem.ProblemVault(exceptions_file)
         problem_file = sys.stdout
 
+    if args.list_overbroad:
+        # If we're listing overbroad exceptions, don't list problems.
+        problem_file = util.NullFile()
+
     # 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_overbroad or args.strict):
diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py
index 88e1044fb7..d21840a213 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -77,8 +77,10 @@ class ProblemVault(object):
         # (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).
         status = problem.is_worse_than(self.exceptions[problem.key()])
-        if status == STATUS_OK:
-            self.used_exception_for[problem.key()] = problem
+
+        # Remember that we used this exception, so that we can later
+        # determine whether the exception was overbroad.
+        self.used_exception_for[problem.key()] = problem
 
         return status
 
diff --git a/scripts/maint/practracker/test_practracker.sh b/scripts/maint/practracker/test_practracker.sh
index bfbd0c6560..9b107e071d 100755
--- a/scripts/maint/practracker/test_practracker.sh
+++ b/scripts/maint/practracker/test_practracker.sh
@@ -61,3 +61,9 @@ echo "ex1:"
 run_practracker --exceptions "${DATA}/ex1.txt" > "${TMPDIR}/ex1-received.txt"
 
 compare "${TMPDIR}/ex1-received.txt" "${DATA}/ex1-expected.txt"
+
+echo "ex1.overbroad:"
+
+run_practracker --exceptions "${DATA}/ex1.txt" --list-overbroad > "${TMPDIR}/ex1-overbroad-received.txt"
+
+compare "${TMPDIR}/ex1-overbroad-received.txt" "${DATA}/ex1-overbroad-expected.txt"
diff --git a/scripts/maint/practracker/testdata/ex1-overbroad-expected.txt b/scripts/maint/practracker/testdata/ex1-overbroad-expected.txt
new file mode 100644
index 0000000000..f69c608f40
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex1-overbroad-expected.txt
@@ -0,0 +1,2 @@
+problem file-size a.c 40 -> 38
+problem file-size z.c 100 -> 0
diff --git a/scripts/maint/practracker/testdata/ex1.txt b/scripts/maint/practracker/testdata/ex1.txt
index f619e33b22..c698005d07 100644
--- a/scripts/maint/practracker/testdata/ex1.txt
+++ b/scripts/maint/practracker/testdata/ex1.txt
@@ -1,5 +1,5 @@
 
-problem file-size a.c 38
+problem file-size a.c 40
 problem include-count a.c 4
 # this problem will produce an error
 problem function-size a.c:i_am_a_function() 8
@@ -8,6 +8,9 @@ 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
+# This exception isn't used.
+problem file-size z.c 100
+
 problem function-size b.c:bar() 5
 problem dependency-violation a.c 3
 problem dependency-violation header.h 3
diff --git a/scripts/maint/practracker/util.py b/scripts/maint/practracker/util.py
index 4b42565528..df629110c2 100644
--- a/scripts/maint/practracker/util.py
+++ b/scripts/maint/practracker/util.py
@@ -41,3 +41,10 @@ def get_tor_c_files(tor_topdir, include_dirs=None):
                 files_list.append(full_path)
 
     return files_list
+
+class NullFile:
+    """A file-like object that we can us to suppress output."""
+    def __init__(self):
+        pass
+    def write(self, s):
+        pass