From ac29b6c6fc93330f5de2f2fbea4f45265e70a16b Mon Sep 17 00:00:00 2001 From: Rekai Gonzalez Alberquilla Date: Fri, 25 Nov 2016 10:31:21 +0000 Subject: [PATCH] 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: Rebased ontop of latest gem5] --- util/git-pre-commit.py | 40 +++++++++++++++++++++++------ util/style/verifiers.py | 56 ++++++++++++++++++++++++++++------------- 2 files changed, 72 insertions(+), 24 deletions(-) diff --git a/util/git-pre-commit.py b/util/git-pre-commit.py index 480bba1c9..276e21fc5 100755 --- 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 6d7d581ea..1be453601 100644 --- 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