[libcamera-devel] [PATCH v2 4/6] checkstyle: Add support for checking style on indexed changes

[Nicolas Dufresne]

Le vendredi 17 janvier 2020 à 17:47 -0500, Nicolas Dufresne a écrit :
> Le samedi 18 janvier 2020 à 00:33 +0200, Laurent Pinchart a écrit :
> > Hi Nicolas,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jan 17, 2020 at 02:17:31PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > 
> > > This introduce a new command line "--staged" and a new special type of
> > > commit "Index". It will check the style of changes that are in the
> > > index, so the changes that would be committed by "git commit".
> > > 
> > > "--staged" was chosen to match with "git diff --staged" command line.
> > > Other valid name could have been "--index" or "--cached". This was
> > > my personal preference, alias can be added later. Note that we must
> > > not confuse this with working tree changes, as these changes are not
> > > picked by "git commit".
> > > 
> > > This feature is needed to implement pre-commit hook.
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > ---
> > >  utils/checkstyle.py | 34 ++++++++++++++++++++++++++++++++--
> > >  1 file changed, 32 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > index fb865c8..8e456cd 100644
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -482,6 +482,25 @@ class Commit:
> > >                                stdout=subprocess.PIPE).stdout.decode('utf-
> > > 8')
> > >  
> > >  
> > > +class Index(Commit):
> > 
> > Index is a bit generic as a class name to represent a commit that takes
> > the index into account. Maybe StagedChanges ? StagedCommit ?
> 
> I can suffix with the base class name if you prefer, but again, Index, Cache or
> Stage is all the same, git fault here. So we can spend a year flipping over, it
> will always be right. Remember that the Index word came from your review.

Changed to StagedChanges() as per our IRC discussion.

> 
> > > +    def __init__(self):
> > > +        Commit.__init__(self, None)
> > > +
> > > +    def get_info(self, top_level):
> > > +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only'],
> > > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > > +        return "Staged changes", ret.splitlines()
> > > +
> > > +    def get_diff(self, top_level, filename):
> > > +        return subprocess.run(['git', 'diff', '--staged', '--',
> > > +                               '%s/%s' % (top_level, filename)],
> > > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > > 8')
> > > +
> > > +    def get_file(self, filename):
> > > +        return subprocess.run(['git', 'show', ':%s' % (filename)],
> > > +                              stdout=subprocess.PIPE).stdout.decode('utf-
> > > 8')
> > > +
> > > +
> > >  def check_file(top_level, commit, filename):
> > >      # Extract the line numbers touched by the commit.
> > >      diff = commit.get_diff(top_level, filename)
> > > @@ -612,7 +631,9 @@ def main(argv):
> > >      parser = argparse.ArgumentParser()
> > >      parser.add_argument('--formatter', '-f', type=str, choices=['astyle',
> > > 'clang-format'],
> > >                          help='Code formatter. Default to clang-format if
> > > not specified.')
> > > -    parser.add_argument('revision_range', type=str, default='HEAD',
> > > nargs='?',
> > > +    parser.add_argument('--staged', '-s', action='store_true',
> > > +                        help='Include the changes in the index. Defaults to
> > > False')
> > > +    parser.add_argument('revision_range', type=str, default=None,
> > > nargs='?',
> > >                          help='Revision range (as defined by git rev-parse). 
> > > Defaults to HEAD if not specified.')
> > >      args = parser.parse_args(argv[1:])
> > >  
> > > @@ -647,7 +668,16 @@ def main(argv):
> > >      if top_level is None:
> > >              return 1
> > >  
> > > -    revlist = extract_revlist(args.revision_range)
> > > +    revlist = []
> > > +    if args.staged:
> > > +        revlist.append(Index())
> > > +
> > > +    # If nothing of --staged or --amend was passed, defaults to HEAD
> > > +    if len(revlist) == 0 and not args.revision_range:
> > > +        args.revision_range = 'HEAD'
> > > +
> > > +    if args.revision_range:
> > > +        revlist += extract_revlist(args.revision_range)
> > 
> > As mentioned in the review of another patch, I'm wondering if we should
> > ignore the revision range when --staged is set, it doesn't make much
> > sense to check both a list of commits and staged changes. It makes it
> > very tempting to replaced --staged and --amend with @STAGED@ and @AMEND@
> > :-) I know I said it may generate conflicts with future version of git,
> > but I think the risk is very small with this syntax, and we can also fix
> > the tool if issues arise.
> 
> Well, doing:
> 
>   ./checkstyle.py --stage HEAD~ HEAD~~
> 
> Works perfectly and is unambiguous. It will check three set of changes as
> expected. I'm don't want to dictate what useful or not, so I'm tempted to just
> keep it like this (with the suggested fix for readability of course).
> 
> > >  
> > >      issues = 0
> > >      for commit in revlist:
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel