[libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add trailers checker

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2023-06-12 23:47:51)
> The libcamera git history contains numerous examples of incorrect commit
> message trailers due to invalid trailer types (e.g. Change-Id), typos
> and other small issues. Those went unnoticed through reviews, which
> shows that an automated checker is required.
> 
> Add a trailers checker to checkstyle.py to catch invalid or malformed
> trailers, with a set of supported trailers that match libcamera's commit
> message practices. New trailer keys can easily be added later as new
> needs arise.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> This reports a total of 42 issues through the project's history. 37 of
> them should not be controversial:
> 
> - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'
>   and 'Reported-on')
> 
> - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead
>   of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')
> 
> - Typos in e-mail address (missing display name, missing space before
>   '<' and missing trailing '>')
> 
> - Link in 'Fixes' trailer (should be a commit)
> 
> - Too short commit ID in 'Fixes' trailer
> 
> - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing
>   space or extra ':' after commit ID, missing '"' around commit subject)
> 
> The five remaining issues may benefit from discussions:
> 
> - Invalid trailer key 'Co-developed-by' (one instance). This is a
>   trailer key commonly used in the kernel, but git..b both recommend
>   Co-authored-by. I'm sure which option would be best, so I haven't
>   included either for now.

I think those can be figured out when we next need them then.

I don't mind either, and the checkstyle won't prevent us using them -
just highlights them for discussion anyway (which is good).


> 
> - Typo in 'Reported-by' trailer for issues reported by Coverity (one
>   instance). 'Reported-by' usually has an e-mail address value, but we
>   have commonly used 'Coverity CID=<CID>' for issues reported by
>   Coverity. I've tentatively added support for this (feedback is
>   welcome), and one commit still got flagged as its 'Reported-by'
>   trailer has a space instead of an '=' after 'CID'.

This is fine with me - if it's defined in the checker we just use that
going forwards.

Coverity CID=<CID> looks good to me.


> 
> - Usage of a github user URL in 'Reported-by' (one instance). Our policy
>   is to be able to identify users by name and e-mail address for
>   'Signed-off-by' trailers, and I would prefer covering 'Reported-by'
>   trailers too. If someone *really* doesn't want their name included in
>   the git log when reporting an issue, we can simply omit the
>   'Reported-by' trailer.

Agreed,

> 
> - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two
>   instances). This should use a 'Link' trailer instead.

Link sounds fine if it's a full link. Otherwise - we'll end up with
Buildbot: Jenkins: Lava: ... 


This looks good to me anyway, lets see how it runs in production!


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
--
Kieran


> ---
>  utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index e68c874609bc..3558740d389d 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -210,13 +210,23 @@ class Commit:
>  
>      def _parse(self):
>          # Get the commit title and list of files.
> -        ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',
> +        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
>                                self.commit],
>                               stdout=subprocess.PIPE).stdout.decode('utf-8')
>          lines = ret.splitlines()
> -        self._files = [CommitFile(f) for f in lines[1:] if f]
> +
>          self._title = lines[0]
>  
> +        self._trailers = []
> +        for index in range(1, len(lines)):
> +            line = lines[index]
> +            if not line:
> +                break
> +
> +            self._trailers.append(line)
> +
> +        self._files = [CommitFile(f) for f in lines[index:] if f]
> +
>      def files(self, filter='AMR'):
>          return [f.filename for f in self._files if f.status in filter]
>  
> @@ -224,6 +234,10 @@ class Commit:
>      def title(self):
>          return self._title
>  
> +    @property
> +    def trailers(self):
> +        return self._trailers
> +
>      def get_diff(self, top_level, filename):
>          diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),
>                                 '--', '%s/%s' % (top_level, filename)],
> @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):
>                              'possible candidates are ' + candidates)]
>  
>  
> +class TrailersChecker(CommitChecker):
> +    commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \(".*"\)')
> +
> +    coverity_regex = re.compile(r'Coverity CID=.*')
> +
> +    # Simple e-mail address validator regex, with an additional trailing
> +    # comment. The complexity of a full RFC6531 validator isn't worth the
> +    # additional invalid addresses it would reject.
> +    email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')
> +
> +    link_regex = re.compile(r'https?://.*')
> +
> +    @staticmethod
> +    def validate_reported_by(value):
> +        if TrailersChecker.email_regex.fullmatch(value):
> +            return True
> +        if TrailersChecker.coverity_regex.fullmatch(value):
> +            return True
> +        return False
> +
> +    known_trailers = {
> +        'Acked-by': email_regex,
> +        'Bug': link_regex,
> +        'Fixes': commit_regex,
> +        'Link': link_regex,
> +        'Reported-by': validate_reported_by,
> +        'Reviewed-by': email_regex,
> +        'Signed-off-by': email_regex,
> +        'Suggested-by': email_regex,
> +        'Tested-by': email_regex,
> +    }
> +
> +    trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\s*:\s*(.*)')
> +
> +    @classmethod
> +    def check(cls, commit, top_level):
> +        issues = []
> +
> +        for trailer in commit.trailers:
> +            match = TrailersChecker.trailer_regex.fullmatch(trailer)
> +            if not match:
> +                raise RuntimeError(f"Malformed commit trailer '{trailer}'")
> +
> +            key, value = match.groups()
> +
> +            validator = TrailersChecker.known_trailers.get(key)
> +            if not validator:
> +                issues.append(CommitIssue(f"Invalid commit trailer key '{key}'"))
> +                continue
> +
> +            if isinstance(validator, re.Pattern):
> +                valid = bool(validator.fullmatch(value))
> +            else:
> +                valid = validator(value)
> +
> +            if not valid:
> +                issues.append(CommitIssue(f"Malformed value '{value}' for commit trailer '{key}'"))
> +                continue
> +
> +        return issues
> +
> +
>  # ------------------------------------------------------------------------------
>  # Style Checkers
>  #
> -- 
> Regards,
> 
> Laurent Pinchart
>