[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 ?
> 
> > +    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'

Sorry, that was not logically equivalent. I've done that instead to clarify,
hopefully it covers your concern.

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index 17d7c82..9e1f16a 100755
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -695,9 +695,11 @@ def main(argv):
     if args.amend:
         revlist.append(Amendment())
 
-    # 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 none of --staged or --amend was passed
+    if len(revlist) == 0:
+        # And no revisions was passed, then default to HEAD
+        if not args.revision_range:
+            args.revision_range = 'HEAD'
 
     if args.revision_range:
         revlist += extract_revlist(args.revision_range)

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