Patchwork [4,of,5,v2] check-commit: try to fix multiline handling

login
register
mail settings
Submitter timeless@mozdev.org
Date Jan. 12, 2016, 8:56 a.m.
Message ID <cc73958b28c58766d93a.1452588996@waste.org>
Download mbox | patch
Permalink /patch/12691/
State Accepted
Commit 7291c8165e338fa1db7d2eb5482ddf8047b8869f
Headers show

Comments

timeless@mozdev.org - Jan. 12, 2016, 8:56 a.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1452588615 0
#      Tue Jan 12 08:50:15 2016 +0000
# Node ID cc73958b28c58766d93a6b94d4af52bb2bf14aa9
# Parent  0c92ea1a5f27ee095e09e52321d3cef9269bd220
check-commit: try to fix multiline handling

The old code did not understand the difference between the first line of the summary,
and a random line in the summary that happened to include a #, or a
random line in the changes that happened to include it.

965788d9ae09 is an example where it fails
timeless - Jan. 12, 2016, 3:55 p.m.
these first two commits are unchanged from v1.
#3 of 5 is just a change to the commit message
#4 changes more of the lines because the (bc) thing tripped me up (as
it did Augie when he tried to queue it), and it's annoying and not
useful to do so.

Patch

diff --git a/contrib/check-commit b/contrib/check-commit
--- a/contrib/check-commit
+++ b/contrib/check-commit
@@ -17,39 +17,56 @@ 
 
 import re, sys, os
 
+commitheader = r"^(?:# [^\n]*\n)*"
+afterheader = commitheader + r"(?!#)"
+beforepatch = afterheader + r"(?!\n(?!@@))"
+
 errors = [
-    (r"[(]bc[)]", "(BC) needs to be uppercase"),
-    (r"[(]issue \d\d\d", "no space allowed between issue and number"),
-    (r"[(]bug(\d|\s)", "use (issueDDDD) instead of bug"),
-    (r"^# User [^@\n]+$", "username is not an email address"),
-    (r"^# .*\n(?!merge with )[^#]\S+[^:] ",
+    (beforepatch + r".*[(]bc[)]", "(BC) needs to be uppercase"),
+    (beforepatch + r".*[(]issue \d\d\d", "no space allowed between issue and number"),
+    (beforepatch + r".*[(]bug(\d|\s)", "use (issueDDDD) instead of bug"),
+    (commitheader + r"# User [^@\n]+\n", "username is not an email address"),
+    (commitheader + r"(?!merge with )[^#]\S+[^:] ",
      "summary line doesn't start with 'topic: '"),
-    (r"^# .*\n[A-Z][a-z]\S+", "don't capitalize summary lines"),
-    (r"^# .*\n[^\n]*: *[A-Z][a-z]\S+", "don't capitalize summary lines"),
-    (r"^# [^\n]*\n\S*[^A-Za-z0-9-]\S*: ",
+    (afterheader + r"[A-Z][a-z]\S+", "don't capitalize summary lines"),
+    (afterheader + r"[^\n]*: *[A-Z][a-z]\S+", "don't capitalize summary lines"),
+    (afterheader + r"\S*[^A-Za-z0-9-]\S*: ",
      "summary keyword should be most user-relevant one-word command or topic"),
-    (r"^# .*\n.*\.\s+$", "don't add trailing period on summary line"),
-    (r"^# .*\n[^#].{78,}", "summary line too long (limit is 78)"),
-    (r"^\+\n \n", "adds double empty line"),
-    (r"^ \n\+\n", "adds double empty line"),
-    (r"^\+[ \t]+def [a-z]+_[a-z]", "adds a function with foo_bar naming"),
+    (afterheader + r".*\.\s*\n", "don't add trailing period on summary line"),
+    (afterheader + r".{79,}", "summary line too long (limit is 78)"),
+    (r"\n\+\n \n", "adds double empty line"),
+    (r"\n \n\+\n", "adds double empty line"),
+    (r"\n\+[ \t]+def [a-z]+_[a-z]", "adds a function with foo_bar naming"),
 ]
 
+word = re.compile('\S')
+def nonempty(first, second):
+    if word.search(first):
+        return first
+    return second
+
 def checkcommit(commit, node = None):
     exitcode = 0
     printed = node is None
     for exp, msg in errors:
-        m = re.search(exp, commit, re.MULTILINE)
+        m = re.search(exp, commit)
         if m:
             pos = 0
+            end = m.end()
+            trailing = re.search(r'(\\n)+$', exp)
+            if trailing:
+                end -= len(trailing.group()) / 2
+            last = ''
             for n, l in enumerate(commit.splitlines(True)):
                 pos += len(l)
-                if pos >= m.end():
+                if pos < end:
+                    last = nonempty(l, last)
+                else:
                     if not printed:
                         printed = True
                         print "node: %s" % node
                     print "%d: %s" % (n, msg)
-                    print " %s" % l[:-1]
+                    print " %s" % nonempty(l, last)[:-1]
                     if "BYPASS" not in os.environ:
                         exitcode = 1
                     break
diff --git a/tests/test-contrib-check-commit.t b/tests/test-contrib-check-commit.t
--- a/tests/test-contrib-check-commit.t
+++ b/tests/test-contrib-check-commit.t
@@ -103,7 +103,7 @@ 
   7: don't add trailing period on summary line
    This has no topic and ends with a period.
   19: adds double empty line
-    
+   +
   15: adds double empty line
    +
   16: adds a function with foo_bar naming