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

[Nicolas Dufresne]

Le vendredi 17 janvier 2020 à 23:08 +0100, Niklas Söderlund a écrit :
> Ni Nicolas,
> 
> Thanks for your work.
> 
> On 2020-01-17 14:17:31 -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):
> 
> Nit and only for discussion but would it make sens to rename the class 
> Staged ?

Stage, Index or Cache is three synonyme in git. If you look at the documentation
text, it's all about the Index, while in the command line it's all about staged
or cached (no trace of indexed). So the answer will depend on what you read
first, I wrote this after reading "git add" doc as refered by Laurent. I don't
think it matters really.

> 
> > +    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:
> 
> The 'not' is quiet hard to spot and I had to read it twice to find out 
> what as going on, how about
> 
>     if args.staged:
>         revlist.append(Index())
> 
>     if args.revision_range:
>         revlist += extract_revlist(args.revision_range)
> 
>     if len(revlist) == 0:
>         args.revision_range = 'HEAD'

Make sense.

> 
> > +        args.revision_range = 'HEAD'
> > +
> > +    if args.revision_range:
> > +        revlist += extract_revlist(args.revision_range)
> >  
> >      issues = 0
> >      for commit in revlist:
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel