[libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add trailers checker
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 11 11:05:14 CEST 2023
Quoting Kieran Bingham (2023-07-11 09:01:28)
> Quoting Naushir Patuck (2023-07-11 08:50:13)
> > Hi Kieran,
> >
> > On Mon, 10 Jul 2023 at 16:51, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > Quoting Naushir Patuck via libcamera-devel (2023-07-10 11:33:54)
> > > > Hi Laurent,
> > > >
> > > > I've updated my tree and this change now seems to break my pre-commit hook:
> > >
> > > Yikes - You've gone pre-commit ;-)
> > >
> > > I usually use post-commit - as pre-commit makes it harder to actually
> > > work in my experience as it prevents the commits in the first place.
> > >
> > > I think post-commit is a much better use of the hooks, as it tells you
> > > of issues while not getting in the way.
> >
> > Ok, that's no problem, I can switch to post-commit hooks.
> > Perhaps it's worth deprecating utils/hooks/pre-commit?
> >
> > >
> > > Still - pre-commit is supposed to be supported - so I'll swap over and
> > > see if I can debug this.
> >
> > I see you have a DNI fix for this, but maybe I'll just switch to post-commit
> > hooks if that's what everyone else is using.
>
> Yes, the DNI quick fix is to just initialise the trailers in the common
> class.
> Theres:
>
> Commit <- StagedCommit <- AmendedCommit
>
> And the trailers only get initialised if you create a Commit, and the
> derived classes don't call Commit::_parse() as they override it
> themselves.
>
> I don't think AmendedCommit should derive from StagedCommit either ...
> and should also still check the trailers, while StagedCommit can't as
> there is no commit message to check trailers in that instance.
>
> But the issue goes away in a post-commit as there's no staged commit
> nor amended commit to check - as in 'post-commit' it's just a Commit
> instance.
And I'm starting to think that it really isn't worth checking Trailers
in pre-commit anyway, as it's /before/ the user has even had the
opportunity to adjust them.
The only thing that would make sense is if the trailer reporting was
added as a comment to the commit message so that the warning was visible
at the point that the commit message could be written ... but that
seems a lot more effort to support something that doesn't necessarily
help.
I think for now I'm tempted to suggest we merge the default
initialisation as in the DNI patch I posted to prevent errors in the
checkstyle script, without supporting Trailer checks in the pre-commit
hooks.
--
Kieran
>
> I think post-commit gives a better developer experience anyway and would
> always recommend that over pre-commit.
>
> --
> Kieran
>
> >
> > Naush
> >
> > >
> > > --
> > > Kieran
> > >
> > > >
> > > > naushir at work:~/libcamera/ $ git commit -a -s
> > > > ---------------
> > > > Staged changes
> > > > ---------------
> > > > Traceback (most recent call last):
> > > > File "/home/naushir/projects/libcamera/./utils/checkstyle.py", line
> > > > 1052, in <module>
> > > > sys.exit(main(sys.argv))
> > > > File "/home/naushir/projects/libcamera/./utils/checkstyle.py", line
> > > > 1042, in main
> > > > issues += check_style(top_level, commit, args.checkers)
> > > > File "/home/naushir/projects/libcamera/./utils/checkstyle.py", line
> > > > 935, in check_style
> > > > for issue in checker.check(commit, top_level):
> > > > File "/home/naushir/projects/libcamera/./utils/checkstyle.py", line
> > > > 479, in check
> > > > for trailer in commit.trailers:
> > > > File "/home/naushir/projects/libcamera/./utils/checkstyle.py", line
> > > > 239, in trailers
> > > > return self._trailers
> > > > AttributeError: 'StagedChanges' object has no attribute '_trailers'.
> > > > Did you mean: 'trailers'?
> > > >
> > > > I'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly
> > > > tried debugging, but quickly found that I'm in way over my depth with the
> > > > checkstyle.py script :-)
> > > >
> > > > Regards,
> > > > Naush
> > > >
> > > > On Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel
> > > > <libcamera-devel at lists.libcamera.org> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > - 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'.
> > > > >
> > > > > - 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.
> > > > >
> > > > > - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two
> > > > > instances). This should use a 'Link' trailer instead.
> > > > > ---
> > > > > 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
> > > > >
More information about the libcamera-devel
mailing list