util: git pre-commit hook to check staged files

This patch updates the git-pre-commit hook to check the files as they
will be after the commit, instead of as they are currently, this way we
prevent the undesired situation:
    - unstylish modification of a file
    - stage said file for commit
    - try to commit and fail due to style
    - fix style, forgetting staging changes
    - try to commit and fail, as although the changes staged are not
      styly, the current content of the file is.

Change-Id: I5cc3f783375d9e4162e310e176103ebbf0a59023
Reviewed-by: Andreas Sandberg <andreas.sandberg@arm.com>
[andreas.sandberg@arm.com: Rebased ontop of latest gem5]

diff --git a/util/git-pre-commit.py b/util/git-pre-commit.py
index 480bba1c929eb4ac6bbde35bce084ce6f5c1f0db..276e21fc5b64587f49e1433d94a51630ae41378f 100755 (executable)
--- a/util/git-pre-commit.py
+++ b/util/git-pre-commit.py
@@ -37,7 +37,9 @@
 #
 # Authors: Andreas Sandberg
 
+from tempfile import TemporaryFile
 import os
+import subprocess
 import sys
 
 from style.repo import GitRepo
@@ -62,6 +64,7 @@ ui = StdioUI()
 
 os.chdir(repo_base)
 failing_files = set()
+staged_mismatch = set()
 
 for status, fname in git.status(filter="MA", cached=True):
     if args.verbose:
@@ -73,18 +76,41 @@ for status, fname in git.status(filter="MA", cached=True):
     else:
         regions = all_regions
 
+    # Show they appropriate object and dump it to a file
+    status = git.file_from_index(fname)
+    f = TemporaryFile()
+    f.write(status)
+
     verifiers = [ v(ui, opts, base=repo_base) for v in all_verifiers ]
     for v in verifiers:
-        if not v.skip(fname) and v.check(fname, regions):
+        f.seek(0)
+        # It is prefered that the first check is silent as it is in the
+        # staged file. If the check fails, then we will do it non-silently
+        # on the current file, reporting meaningful shortcomings
+        if not v.skip(fname) and v.check(fname, regions, fobj=f, silent=True):
             failing_files.add(fname)
+            if not v.check(fname, regions):
+                staged_mismatch.add(fname)
+    f.close()
 
 if failing_files:
-    print >> sys.stderr
-    print >> sys.stderr, "Style checker failed for the following files:"
-    for f in failing_files:
-        print >> sys.stderr, "\t%s" % f
-    print >> sys.stderr
-    print >> sys.stderr, \
+    if len(failing_files) > len(staged_mismatch):
+        print >> sys.stderr
+        print >> sys.stderr, "Style checker failed for the following files:"
+        for f in failing_files:
+            if f not in staged_mismatch:
+                print >> sys.stderr, "\t%s" % f
+        print >> sys.stderr
+        print >> sys.stderr, \
         "Please run the style checker manually to fix the offending files.\n" \
         "To check your modifications, run: util/style.py -m"
+
+    print >> sys.stderr
+    if staged_mismatch:
+        print >> sys.stderr, \
+        "It looks like you have forgotten to stage your fixes for commit in\n"\
+        "the following files: "
+        for f in staged_mismatch:
+            print >> sys.stderr, "\t%s" % f
+        print >> sys.stderr, "Please `git --add' them"
     sys.exit(1)

diff --git a/util/style/verifiers.py b/util/style/verifiers.py
index 6d7d581ea9d086e191cc72acfb14f1159f0662ed..1be4536017f9ed355a563a1f4c5edd558d5684e4 100644 (file)
--- a/util/style/verifiers.py
+++ b/util/style/verifiers.py
@@ -192,9 +192,20 @@ class Verifier(object):
         return False
 
     @abstractmethod
-    def check(self, filename, regions=all_regions):
+    def check(self, filename, regions=all_regions, fobj=None, silent=False):
         """Check specified regions of file 'filename'.
 
+        Given that it is possible that the current contents of the file
+        differ from the file as 'staged to commit', for those cases, and
+        maybe others, the argument fobj should be a file object open and reset
+        with the contents matching what the file would look like after the
+        commit. This is needed keep the messages using 'filename' meaningful.
+
+        The argument silent is useful to prevent output when we run check in
+        the staged file vs the actual file to detect if the user forgot
+        staging fixes to the commit. This way, we prevent reporting errors
+        twice in stderr.
+
         Line-by-line checks can simply provide a check_line() method
         that returns True if the line is OK and False if it has an
         error.  Verifiers that need a multi-line view (like
@@ -216,24 +227,29 @@ class Verifier(object):
         pass
 
 class LineVerifier(Verifier):
-    def check(self, filename, regions=all_regions):
-        f = self.open(filename, 'r')
+    def check(self, filename, regions=all_regions, fobj=None, silent=False):
+        close = False
+        if fobj is None:
+            fobj = self.open(filename, 'r')
+            close = True
 
         lang = lang_type(filename)
         assert lang in self.languages
 
         errors = 0
-        for num,line in enumerate(f):
+        for num,line in enumerate(fobj):
             if num not in regions:
                 continue
             line = line.rstrip('\n')
             if not self.check_line(line, language=lang):
-                self.ui.write("invalid %s in %s:%d\n" % \
-                              (self.test_name, filename, num + 1))
-                if self.ui.verbose:
-                    self.ui.write(">>%s<<\n" % line[:-1])
+                if not silent:
+                    self.ui.write("invalid %s in %s:%d\n" % \
+                                  (self.test_name, filename, num + 1))
+                    if self.ui.verbose:
+                        self.ui.write(">>%s<<\n" % line[:-1])
                 errors += 1
-        f.close()
+        if close:
+            fobj.close()
         return errors
 
     @safefix
@@ -329,12 +345,16 @@ class SortedIncludes(Verifier):
         super(SortedIncludes, self).__init__(*args, **kwargs)
         self.sort_includes = sort_includes.SortIncludes()
 
-    def check(self, filename, regions=all_regions):
-        f = self.open(filename, 'r')
+    def check(self, filename, regions=all_regions, fobj=None, silent=False):
+        close = False
+        if fobj is None:
+            fobj = self.open(filename, 'r')
+            close = True
         norm_fname = self.normalize_filename(filename)
 
-        old = [ l.rstrip('\n') for l in f.xreadlines() ]
-        f.close()
+        old = [ l.rstrip('\n') for l in fobj.xreadlines() ]
+        if close:
+            fobj.close()
 
         if len(old) == 0:
             return 0
@@ -345,10 +365,12 @@ class SortedIncludes(Verifier):
         modified = _modified_regions(old, new) & regions
 
         if modified:
-            self.ui.write("invalid sorting of includes in %s\n" % (filename))
-            if self.ui.verbose:
-                for start, end in modified.regions:
-                    self.ui.write("bad region [%d, %d)\n" % (start, end))
+            if not silent:
+                self.ui.write("invalid sorting of includes in %s\n"
+                                % (filename))
+                if self.ui.verbose:
+                    for start, end in modified.regions:
+                        self.ui.write("bad region [%d, %d)\n" % (start, end))
             return 1
 
         return 0