[PATCH 2/3] utils: checkstyle.py: Skip title and trailers checkers for pre-commit

[Laurent Pinchart]

On Mon, Aug 12, 2024 at 01:07:08PM +0100, Daniel Scally wrote:
> Hi Laurent, thanks for the patch
> 
> On 10/08/2024 01:58, Laurent Pinchart wrote:
> > When running checkstyle.py in a pre-commit hook, there is either no
> > commit message at all (when committing staged changes as a new commit),
> > or the commit message comes from a previous commit being amended. In
> > either case, there's no new commit message yet, and thus nothing to
> > validate for the title and trailers checkers. The trailers checker, in
> > particular, will always flag an error, making all commits fail.
> >
> > To fix that, just skip the two checkers during pre-commit.
> 
> I like that this is done in a way which facilitates the same thing being done more easily in the 
> future if needed.

When I copied the existing 'if isinstance(commit, StagedChanges):' line
from TitleChecker, I knew before hitting 'p' that I had to do better :-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
> 
> > ---
> >   utils/checkstyle.py | 22 ++++++++++++++--------
> >   1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index aa0731abdb5a..2b1e1f6c1b9e 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -344,11 +344,16 @@ class CommitChecker(metaclass=ClassRegistry):
> >       # Class methods
> >       #
> >       @classmethod
> > -    def checkers(cls, names):
> > +    def checkers(cls, commit, names):
> >           for checker in cls.subclasses:
> >               if names and checker.__name__ not in names:
> >                   continue
> > -            yield checker
> > +            if checker.supports(commit):
> > +                yield checker
> > +
> > +    @classmethod
> > +    def supports(cls, commit):
> > +        return type(commit) in cls.commit_types
> >   
> >   
> >   class CommitIssue(object):
> > @@ -357,6 +362,8 @@ class CommitIssue(object):
> >   
> >   
> >   class HeaderAddChecker(CommitChecker):
> > +    commit_types = (Commit, StagedChanges, Amendment)
> > +
> >       @classmethod
> >       def check(cls, commit, top_level):
> >           issues = []
> > @@ -401,6 +408,8 @@ class HeaderAddChecker(CommitChecker):
> >   
> >   
> >   class TitleChecker(CommitChecker):
> > +    commit_types = (Commit,)
> > +
> >       prefix_regex = re.compile(r'^([a-zA-Z0-9_.-]+: )+')
> >       release_regex = re.compile(r'libcamera v[0-9]+\.[0-9]+\.[0-9]+')
> >   
> > @@ -408,11 +417,6 @@ class TitleChecker(CommitChecker):
> >       def check(cls, commit, top_level):
> >           title = commit.title
> >   
> > -        # Skip the check when validating staged changes (as done through a
> > -        # pre-commit hook) as there is no title to check in that case.
> > -        if isinstance(commit, StagedChanges):
> > -            return []
> > -
> >           # Ignore release commits, they don't need a prefix.
> >           if TitleChecker.release_regex.fullmatch(title):
> >               return []
> > @@ -468,6 +472,8 @@ class TitleChecker(CommitChecker):
> >   
> >   
> >   class TrailersChecker(CommitChecker):
> > +    commit_types = (Commit,)
> > +
> >       commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \(".*"\)')
> >   
> >       coverity_regex = re.compile(r'Coverity CID=.*')
> > @@ -1020,7 +1026,7 @@ def check_style(top_level, commit, checkers):
> >       issues = 0
> >   
> >       # Apply the commit checkers first.
> > -    for checker in CommitChecker.checkers(checkers):
> > +    for checker in CommitChecker.checkers(commit, checkers):
> >           for issue in checker.check(commit, top_level):
> >               print('%s%s%s' % (Colours.fg(Colours.Yellow), issue.msg, Colours.reset()))
> >               issues += 1

-- 
Regards,

Laurent Pinchart