[PATCH 1/2] utils: checkstyle.py: Add author property to Commit class

[Kieran Bingham]

Quoting Laurent Pinchart (2024-08-06 10:02:01)
> On Tue, Aug 06, 2024 at 09:05:04AM +0200, Stefan Klug wrote:
> > Hi Laurent,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Aug 05, 2024 at 08:48:06PM +0300, Laurent Pinchart wrote:
> > > Extend the Commit class with an author property, retrieved from the
> > > commit. It will be used to extend checkers.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  utils/checkstyle.py | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > index 7d480bdf4a2f..1eeb3809f7fe 100755
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -216,7 +216,7 @@ class Commit:
> > >          self._parse()
> > >  
> > >      def _parse_trailers(self, lines):
> > > -        for index in range(1, len(lines)):
> > > +        for index in range(2, len(lines)):
> > >              line = lines[index]
> > >              if not line:
> > >                  break
> > > @@ -227,12 +227,13 @@ class Commit:
> > >  
> > >      def _parse(self):
> > >          # Get the commit title and list of files.
> > > -        ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',
> > > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%s%n%(trailers:only,unfold)', '--name-status',
> > 
> > Nit: This line starts to be quite long...
> 
> I'll wrap it.
> 
> > >                                self.commit],
> > >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >          lines = ret.splitlines()
> > >  
> > > -        self._title = lines[0]
> > > +        self._author = lines[0]
> > > +        self._title = lines[1]
> > >  
> > >          index = self._parse_trailers(lines)
> > >          self._files = [CommitFile(f) for f in lines[index:] if f]
> > > @@ -240,6 +241,10 @@ class Commit:
> > >      def files(self, filter='AMR'):
> > >          return [f.filename for f in self._files if f.status in filter]
> > >  
> > > +    @property
> > > +    def author(self):
> > > +        return self._author
> > > +
> > >      @property
> > >      def title(self):
> > >          return self._title
> > > @@ -282,12 +287,13 @@ class Amendment(Commit):
> > >  
> > >      def _parse(self):
> > >          # Create a title using HEAD commit and parse the trailers.
> > > -        ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unfold)',
> > > +        ret = subprocess.run(['git', 'show', '--format=%an <%ae>%n%H %s%n%(trailers:only,unfold)',
> > >                               '--no-patch'],
> > >                               stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >          lines = ret.splitlines()
> > >  
> > > -        self._title = 'Amendment of ' + lines[0].strip()
> > > +        self._author = lines[0]
> > > +        self._title = 'Amendment of ' + lines[1].strip()
> > 
> > This raises the question why the title needs stripping but not the
> > author.
> 
> I asked myself the same question and didn't orignally investigate. Now
> that I have, I think I can drop the strip. It was originally added in
> commit 17b3c794095e ("checkstyle: Add support for checking style on
> amendments"):
> 
>         ret = subprocess.run(['git', 'show', '--pretty=oneline', '--no-patch'],
>                              stdout=subprocess.PIPE).stdout.decode('utf-8')
>         title = 'Amendment of ' + ret.strip()
> 
> There the strip was needed to drop the newline character. Then commit
> 4694e441c313 ("utils: checkstyle.py: Extract title and trailers with one
> command") modified the code to
> 
>         ret = subprocess.run(['git', 'show', '--format=%H %s%n%(trailers:only,unf
> 
>                              '--no-patch'],
>                              stdout=subprocess.PIPE).stdout.decode('utf-8')
>         lines = ret.splitlines()
> 
>         self._title = 'Amendment of ' + lines[0].strip()
> 
> The splitlines() call removes the new line characters, but the strip()
> was kept. I'll drop it in v2.

I'm fine with all that.

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

> 
> > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 
> > 
> > >  
> > >          self._parse_trailers(lines)
> > >  
> > > 
> > > base-commit: 8af95d6854889dc66746429ccf8888e3a81f6baf
> 
> -- 
> Regards,
> 
> Laurent Pinchart