Merge remote-tracking branch 'public/practracker_regen_overbroad_2'

7 files changed, 154 insertions, 6 deletions

diff --git a/Makefile.am b/Makefile.am
index 811694f0b6..ac61a990fc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -196,6 +196,8 @@ EXTRA_DIST+= \
 	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/ex1-regen-expected.txt	\
+	scripts/maint/practracker/testdata/ex1-regen-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/ticket32372 b/changes/ticket32372
new file mode 100644
index 0000000000..ed2d38b40e
--- /dev/null
+++ b/changes/ticket32372
@@ -0,0 +1,4 @@
+  o Minor features (best practices tracker):
+    - Practracker now supports a --regen-overbroad option to regenerate
+      the exceptions file, but only to revise exceptions to be _less_
+      tolerant of best-practices violations.  Closes ticket 32372.
diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py
index e806aef3b4..6149fb79cb 100755
--- a/scripts/maint/practracker/practracker.py
+++ b/scripts/maint/practracker/practracker.py
@@ -30,6 +30,7 @@ import metrics
 import util
 import problem
 import includes
+import shutil
 
 # The filename of the exceptions file (it should be placed in the practracker directory)
 EXCEPTIONS_FNAME = "./exceptions.txt"
@@ -185,6 +186,9 @@ def main(argv):
                         help="Regenerate the exceptions file")
     parser.add_argument("--list-overbroad", action="store_true",
                         help="List over-broad exceptions")
+    parser.add_argument("--regen-overbroad", action="store_true",
+                        help="Regenerate the exceptions file, "
+                             "removing over-broad exceptions.")
     parser.add_argument("--exceptions",
                         help="Override the location for the exceptions file")
     parser.add_argument("--strict", action="store_true",
@@ -227,8 +231,9 @@ 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",
+    if args.list_overbroad + args.regen + args.regen_overbroad > 1:
+        print("Cannot use more than one of --regen, --list-overbroad, and "
+              "--regen-overbroad.",
               file=sys.stderr)
         sys.exit(1)
 
@@ -247,13 +252,15 @@ 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.
+    if args.list_overbroad or args.regen_overbroad:
+        # If we're looking for overbroad exceptions, don't list problems
+        # immediately to the problem file.
         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):
+    if not (args.regen or args.list_overbroad or args.regen_overbroad or
+            args.strict):
         ProblemVault.set_tolerances(TOLERANCE_FNS)
 
     # 3) Go through all the files and report problems if they are not exceptions
@@ -269,7 +276,17 @@ def main(argv):
 
     if args.regen:
         tmpfile.close()
-        os.rename(tmpname, exceptions_file)
+        shutil.move(tmpname, exceptions_file)
+        sys.exit(0)
+
+    if args.regen_overbroad:
+        tmpname = exceptions_file + ".tmp"
+        tmpfile = open(tmpname, "w")
+        tmpfile.write(HEADER)
+        for item in ProblemVault.list_exceptions_without_overbroad():
+            print(item, file=tmpfile)
+        tmpfile.close()
+        shutil.move(tmpname, exceptions_file)
         sys.exit(0)
 
     # If new issues were found, try to give out some advice to the developer on how to resolve it.
@@ -296,6 +313,7 @@ variable.
             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 bee5eeb903..a3255dcc80 100644
--- a/scripts/maint/practracker/problem.py
+++ b/scripts/maint/practracker/problem.py
@@ -29,6 +29,8 @@ class ProblemVault(object):
     def __init__(self, exception_fname=None):
         # Exception dictionary: { problem.key() : Problem object }
         self.exceptions = {}
+        # Exception list: list of Problem objects, in the order added.
+        self.exception_list = []
         # Exception dictionary: maps key to the problem it was used to
         # suppress.
         self.used_exception_for = {}
@@ -63,6 +65,7 @@ class ProblemVault(object):
                 sys.exit(1)
 
             self.exceptions[problem.key()] = problem
+            self.exception_list.append(problem)
             #print "Registering exception: %s" % problem
 
     def register_problem(self, problem):
@@ -98,6 +101,24 @@ class ProblemVault(object):
             if p is None or e.is_worse_than(p):
                 yield (e, p)
 
+    def list_exceptions_without_overbroad(self):
+        """Return an iterator of new problems, such that overbroad
+           exceptions are replaced with minimally broad versions, or removed.
+        """
+        for e in self.exception_list:
+            p = self.used_exception_for.get(e.key())
+            if p is None:
+                # This exception wasn't needed at all.
+                continue
+            if e.is_worse_than(p):
+                # The exception is worse than the problem we found.
+                # Yield the problem as the new exception value.
+                yield p
+            else:
+                # The problem is as bad as the exception, or worse.
+                # Yield the exception.
+                yield e
+
     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
diff --git a/scripts/maint/practracker/test_practracker.sh b/scripts/maint/practracker/test_practracker.sh
index afc267a000..e29b9106de 100755
--- a/scripts/maint/practracker/test_practracker.sh
+++ b/scripts/maint/practracker/test_practracker.sh
@@ -82,3 +82,15 @@ run_practracker --exceptions "${DATA}/ex1.txt" --list-overbroad \
 
 compare "${TMPDIR}/ex1-overbroad-received.txt" \
         "${DATA}/ex1-overbroad-expected.txt"
+
+echo "ex1.regen:"
+
+cp "${DATA}/ex1.txt" "${TMPDIR}/ex1-copy.txt"
+run_practracker --exceptions "${TMPDIR}/ex1-copy.txt" --regen >/dev/null 2>&1
+compare "${TMPDIR}/ex1-copy.txt" "${DATA}/ex1-regen-expected.txt"
+
+echo "ex1.regen_overbroad:"
+
+cp "${DATA}/ex1.txt" "${TMPDIR}/ex1-copy.txt"
+run_practracker --exceptions "${TMPDIR}/ex1-copy.txt" --regen-overbroad >/dev/null 2>&1
+compare "${TMPDIR}/ex1-copy.txt" "${DATA}/ex1-regen-overbroad-expected.txt"
diff --git a/scripts/maint/practracker/testdata/ex1-regen-expected.txt b/scripts/maint/practracker/testdata/ex1-regen-expected.txt
new file mode 100644
index 0000000000..bdf3681edf
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex1-regen-expected.txt
@@ -0,0 +1,46 @@
+# Welcome to the exceptions file for Tor's best-practices tracker!
+#
+# Each line of this file represents a single violation of Tor's best
+# practices -- typically, a violation that we had before practracker.py
+# first existed.
+#
+# There are three kinds of problems that we recognize right now:
+#   function-size -- a function of more than 100 lines.
+#   file-size -- a .c file of more than 3000 lines, or a .h
+#      file with more than 500 lines.
+#   include-count -- a .c file with more than 50 #includes,
+#      or a .h file with more than 15 #includes.
+#   dependency-violation -- a file includes a header that it should
+#      not, according to an advisory .may_include file.
+#
+# Each line below represents a single exception that practracker should
+# _ignore_. Each line has four parts:
+#  1. The word "problem".
+#  2. The kind of problem.
+#  3. The location of the problem: either a filename, or a
+#     filename:functionname pair.
+#  4. The magnitude of the problem to ignore.
+#
+# So for example, consider this line:
+#    problem file-size /src/core/or/connection_or.c 3200
+#
+# It tells practracker to allow the mentioned file to be up to 3200 lines
+# long, even though ordinarily it would warn about any file with more than
+# 3000 lines.
+#
+# You can either edit this file by hand, or regenerate it completely by
+# running `make practracker-regen`.
+#
+# Remember: It is better to fix the problem than to add a new exception!
+
+problem file-size a.c 41
+problem include-count a.c 6
+problem function-size a.c:i_am_a_function() 9
+problem function-size a.c:another_function() 12
+problem dependency-violation a.c 4
+problem file-size b.c 15
+problem function-size b.c:foo() 4
+problem function-size b.c:bar() 5
+problem file-size header.h 8
+problem include-count header.h 4
+problem dependency-violation header.h 3
diff --git a/scripts/maint/practracker/testdata/ex1-regen-overbroad-expected.txt b/scripts/maint/practracker/testdata/ex1-regen-overbroad-expected.txt
new file mode 100644
index 0000000000..4521029b10
--- /dev/null
+++ b/scripts/maint/practracker/testdata/ex1-regen-overbroad-expected.txt
@@ -0,0 +1,45 @@
+# Welcome to the exceptions file for Tor's best-practices tracker!
+#
+# Each line of this file represents a single violation of Tor's best
+# practices -- typically, a violation that we had before practracker.py
+# first existed.
+#
+# There are three kinds of problems that we recognize right now:
+#   function-size -- a function of more than 100 lines.
+#   file-size -- a .c file of more than 3000 lines, or a .h
+#      file with more than 500 lines.
+#   include-count -- a .c file with more than 50 #includes,
+#      or a .h file with more than 15 #includes.
+#   dependency-violation -- a file includes a header that it should
+#      not, according to an advisory .may_include file.
+#
+# Each line below represents a single exception that practracker should
+# _ignore_. Each line has four parts:
+#  1. The word "problem".
+#  2. The kind of problem.
+#  3. The location of the problem: either a filename, or a
+#     filename:functionname pair.
+#  4. The magnitude of the problem to ignore.
+#
+# So for example, consider this line:
+#    problem file-size /src/core/or/connection_or.c 3200
+#
+# It tells practracker to allow the mentioned file to be up to 3200 lines
+# long, even though ordinarily it would warn about any file with more than
+# 3000 lines.
+#
+# You can either edit this file by hand, or regenerate it completely by
+# running `make practracker-regen`.
+#
+# Remember: It is better to fix the problem than to add a new exception!
+
+problem file-size a.c 41
+problem include-count a.c 6
+problem function-size a.c:i_am_a_function() 8
+problem function-size a.c:another_function() 11
+problem file-size b.c 15
+problem function-size b.c:bar() 5
+problem dependency-violation a.c 4
+problem dependency-violation header.h 3
+problem file-size header.h 8
+problem include-count header.h 4