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

20870 diff mbox series

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/checkstyle.py | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> 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.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.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

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.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Reviewed-by: Daniel Scally <dan.scally@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

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@ideasonboard.com>
> 
> Reviewed-by: Daniel Scally <dan.scally@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