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

[Nicolas Dufresne]

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.

> 
> > +    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: