[libcamera-devel] [PATCH v2] utils: checkstyle.py: Add commit title checker

[Laurent Pinchart]

Hi Paul,

On Fri, Dec 23, 2022 at 05:56:03PM -0600, Paul Elder wrote:
> On Thu, Dec 22, 2022 at 12:12:37AM +0200, Laurent Pinchart via libcamera-devel wrote:
> > Add a commit checker to ensure that commit titles start with a prefix.
> > The commit issue message lists prefix candidates retrieved from the git
> > log.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Reduce indentation
> > ---
> >  utils/checkstyle.py | 62 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > index a11d95cc5808..364e13d6533d 100755
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -352,6 +352,68 @@ class HeaderAddChecker(CommitChecker):
> >          return issues
> >  
> >  
> > +class TitleChecker(CommitChecker):
> > +    prefix_regex = re.compile(r'[0-9a-f]+ (([a-zA-Z0-9_.-]+: )+)')
> 
> Ah, the first chunk is for the commit ID...

That's right.

> > +    release_regex = re.compile(r'libcamera v[0-9]+\.[0-9]+\.[0-9]+')
> > +
> > +    @classmethod
> > +    def check(cls, commit, top_level):
> > +        title = commit.title
> > +
> > +        # Ignore release commits, they don't need a prefix.
> > +        if TitleChecker.release_regex.fullmatch(title):
> > +            return []
> > +
> > +        prefix_pos = title.find(': ')
> > +        if prefix_pos != -1 and prefix_pos != len(title) - 2:
> > +            return []
> > +
> > +        # Find prefix candidates by searching the git history
> > +        msgs = subprocess.run(['git', 'log', '--no-decorate', '--oneline', '-n100', '--'] + commit.files(),
> > +                              stdout=subprocess.PIPE).stdout.decode('utf-8')
> > +        prefixes = {}
> > +        prefixes_count = 0
> > +        for msg in msgs.splitlines():
> > +            prefix = TitleChecker.prefix_regex.match(msg)
> > +            if not prefix:
> > +                continue
> > +
> > +            prefix = prefix.group(1)
> > +            if prefix in prefixes:
> > +                prefixes[prefix] += 1
> > +            else:
> > +                prefixes[prefix] = 1
> > +
> > +            prefixes_count += 1
> > +
> > +        if not prefixes:
> > +            return [CommitIssue('Commit title is missing prefix')]
> > +
> > +        # Sort the candidates by number of occurrences and pick the best ones.
> > +        # When multiple prefixes are possible without a clear winner, we want to
> > +        # display the most common options to the user, but without the most
> > +        # unlikely options to avoid too long messages. As a heuristic, select
> > +        # enough candidates to cover at least 2/3 of the possible prefixes, but
> > +        # never more than 4 candidates.
> > +        prefixes = list(prefixes.items())
> > +        prefixes.sort(key=lambda x: x[1], reverse=True)
> > +
> > +        candidates = []
> > +        candidates_count = 0
> > +        for prefix in prefixes:
> > +            candidates.append(f"`{prefix[0]}'")
> > +            candidates_count += prefix[1]
> > +            if candidates_count >= prefixes_count * 2 / 3 or \
> > +               len(candidates) == 4:
> > +                break
> > +
> > +        candidates = candidates[:-2] + [' and '.join(candidates[-2:])]
> 
> imo, s/and/or/

They are all candidates, but I'm happy to use "or" if that's preferred.

> > +        candidates = ', '.join(candidates)
> 
> Also I feel like surrounding each candidate in quotes might be helpful.

Done already, see

            candidates.append(f"`{prefix[0]}'")

above.

> Otherwise,
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > +
> > +        return [CommitIssue('Commit title is missing prefix, '
> > +                            'possible candidates are ' + candidates)]
> > +
> > +
> >  # ------------------------------------------------------------------------------
> >  # Style Checkers
> >  #
> > 
> > base-commit: f66a5c447b65bce774a1bc2d01034f437bf764b5
> > prerequisite-patch-id: 55c457bf621237f99229898adb78e18b259ab74b

-- 
Regards,

Laurent Pinchart