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]
This commit is contained in:
Rekai Gonzalez Alberquilla 2016-11-25 10:31:21 +00:00
parent b0856ab3b1
commit ac29b6c6fc
2 changed files with 72 additions and 24 deletions

View file

@ -37,7 +37,9 @@
# #
# Authors: Andreas Sandberg # Authors: Andreas Sandberg
from tempfile import TemporaryFile
import os import os
import subprocess
import sys import sys
from style.repo import GitRepo from style.repo import GitRepo
@ -62,6 +64,7 @@ ui = StdioUI()
os.chdir(repo_base) os.chdir(repo_base)
failing_files = set() failing_files = set()
staged_mismatch = set()
for status, fname in git.status(filter="MA", cached=True): for status, fname in git.status(filter="MA", cached=True):
if args.verbose: if args.verbose:
@ -73,18 +76,41 @@ for status, fname in git.status(filter="MA", cached=True):
else: else:
regions = all_regions 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 ] verifiers = [ v(ui, opts, base=repo_base) for v in all_verifiers ]
for v in 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) failing_files.add(fname)
if not v.check(fname, regions):
staged_mismatch.add(fname)
f.close()
if failing_files: if failing_files:
if len(failing_files) > len(staged_mismatch):
print >> sys.stderr print >> sys.stderr
print >> sys.stderr, "Style checker failed for the following files:" print >> sys.stderr, "Style checker failed for the following files:"
for f in failing_files: for f in failing_files:
if f not in staged_mismatch:
print >> sys.stderr, "\t%s" % f print >> sys.stderr, "\t%s" % f
print >> sys.stderr print >> sys.stderr
print >> sys.stderr, \ print >> sys.stderr, \
"Please run the style checker manually to fix the offending files.\n" \ "Please run the style checker manually to fix the offending files.\n" \
"To check your modifications, run: util/style.py -m" "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) sys.exit(1)

View file

@ -192,9 +192,20 @@ class Verifier(object):
return False return False
@abstractmethod @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'. """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 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 that returns True if the line is OK and False if it has an
error. Verifiers that need a multi-line view (like error. Verifiers that need a multi-line view (like
@ -216,24 +227,29 @@ class Verifier(object):
pass pass
class LineVerifier(Verifier): class LineVerifier(Verifier):
def check(self, filename, regions=all_regions): def check(self, filename, regions=all_regions, fobj=None, silent=False):
f = self.open(filename, 'r') close = False
if fobj is None:
fobj = self.open(filename, 'r')
close = True
lang = lang_type(filename) lang = lang_type(filename)
assert lang in self.languages assert lang in self.languages
errors = 0 errors = 0
for num,line in enumerate(f): for num,line in enumerate(fobj):
if num not in regions: if num not in regions:
continue continue
line = line.rstrip('\n') line = line.rstrip('\n')
if not self.check_line(line, language=lang): if not self.check_line(line, language=lang):
if not silent:
self.ui.write("invalid %s in %s:%d\n" % \ self.ui.write("invalid %s in %s:%d\n" % \
(self.test_name, filename, num + 1)) (self.test_name, filename, num + 1))
if self.ui.verbose: if self.ui.verbose:
self.ui.write(">>%s<<\n" % line[:-1]) self.ui.write(">>%s<<\n" % line[:-1])
errors += 1 errors += 1
f.close() if close:
fobj.close()
return errors return errors
@safefix @safefix
@ -329,12 +345,16 @@ class SortedIncludes(Verifier):
super(SortedIncludes, self).__init__(*args, **kwargs) super(SortedIncludes, self).__init__(*args, **kwargs)
self.sort_includes = sort_includes.SortIncludes() self.sort_includes = sort_includes.SortIncludes()
def check(self, filename, regions=all_regions): def check(self, filename, regions=all_regions, fobj=None, silent=False):
f = self.open(filename, 'r') close = False
if fobj is None:
fobj = self.open(filename, 'r')
close = True
norm_fname = self.normalize_filename(filename) norm_fname = self.normalize_filename(filename)
old = [ l.rstrip('\n') for l in f.xreadlines() ] old = [ l.rstrip('\n') for l in fobj.xreadlines() ]
f.close() if close:
fobj.close()
if len(old) == 0: if len(old) == 0:
return 0 return 0
@ -345,7 +365,9 @@ class SortedIncludes(Verifier):
modified = _modified_regions(old, new) & regions modified = _modified_regions(old, new) & regions
if modified: if modified:
self.ui.write("invalid sorting of includes in %s\n" % (filename)) if not silent:
self.ui.write("invalid sorting of includes in %s\n"
% (filename))
if self.ui.verbose: if self.ui.verbose:
for start, end in modified.regions: for start, end in modified.regions:
self.ui.write("bad region [%d, %d)\n" % (start, end)) self.ui.write("bad region [%d, %d)\n" % (start, end))