[libcamera-devel,v2,5/6] checkstyle: Add support for checking style on amendments
diff mbox series

Message ID 20200117191733.198897-6-nicolas@ndufresne.ca
State Superseded
Headers show
Series
  • Add the ability to do pre-commit style check
Related show

Commit Message

Nicolas Dufresne Jan. 17, 2020, 7:17 p.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This introduce a new argument "--amend" and a new special type of
commit "Amendment". It will check the style of changes that are in
the index combined with the changes of the last commit. So this is
the changes that would be applied by "git commit --amend" hence the
name of the argument.

This is needed to implement pre-commit hook.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 utils/checkstyle.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 mode change 100644 => 100755 utils/checkstyle.py

Comments

Niklas Söderlund Jan. 17, 2020, 10:11 p.m. UTC | #1 
Hi Nicolas,

Thanks for your work.

On 2020-01-17 14:17:32 -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This introduce a new argument "--amend" and a new special type of
> commit "Amendment". It will check the style of changes that are in
> the index combined with the changes of the last commit. So this is
> the changes that would be applied by "git commit --amend" hence the
> name of the argument.
> 
> This is needed to implement pre-commit hook.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  utils/checkstyle.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  mode change 100644 => 100755 utils/checkstyle.py
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> old mode 100644
> new mode 100755
> index 8e456cd..7c2ce00
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -501,6 +501,26 @@ class Index(Commit):
>                                stdout=subprocess.PIPE).stdout.decode('utf-8')
>  
>  
> +class Amendment(Index):
> +    def __init__(self):
> +        Commit.__init__(self, None)
> +
> +    def det_info(self, top_level):
> +        # Create a title using HEAD commit
> +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', 'HEAD'],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        title = 'Amendment of: ' + ret.splitlines()[0]
> +        # Extract the list of modifier files
> +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', 'HEAD~'],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        return title, ret.splitlines()
> +
> +    def get_diff(self, top_level, filename):
> +        return subprocess.run(['git', 'diff', '--staged', 'HEAD~', '--',
> +                               '%s/%s' % (top_level, 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)
> @@ -633,6 +653,8 @@ def main(argv):
>                          help='Code formatter. Default to clang-format if not specified.')
>      parser.add_argument('--staged', '-s', action='store_true',
>                          help='Include the changes in the index. Defaults to False')
> +    parser.add_argument('--amend', '-a', action='store_true',
> +                        help='Includes changes in the index and the previous patch combined. 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:])
> @@ -671,6 +693,8 @@ def main(argv):
>      revlist = []
>      if args.staged:
>          revlist.append(Index())
> +    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:
> -- 
> 2.24.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 17, 2020, 10:25 p.m. UTC | #2 
Hi Nicolas,

Thank you for the patch.

On Fri, Jan 17, 2020 at 02:17:32PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This introduce a new argument "--amend" and a new special type of
> commit "Amendment". It will check the style of changes that are in
> the index combined with the changes of the last commit. So this is
> the changes that would be applied by "git commit --amend" hence the
> name of the argument.
> 
> This is needed to implement pre-commit hook.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  utils/checkstyle.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  mode change 100644 => 100755 utils/checkstyle.py
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> old mode 100644
> new mode 100755
> index 8e456cd..7c2ce00
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -501,6 +501,26 @@ class Index(Commit):
>                                stdout=subprocess.PIPE).stdout.decode('utf-8')
>  
>  
> +class Amendment(Index):
> +    def __init__(self):
> +        Commit.__init__(self, None)
> +
> +    def det_info(self, top_level):

s/det_info/get_info/

This code path has never been tested :-)

> +        # Create a title using HEAD commit
> +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', 'HEAD'],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        title = 'Amendment of: ' + ret.splitlines()[0]
> +        # Extract the list of modifier files
> +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', 'HEAD~'],
> +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> +        return title, ret.splitlines()
> +
> +    def get_diff(self, top_level, filename):
> +        return subprocess.run(['git', 'diff', '--staged', 'HEAD~', '--',
> +                               '%s/%s' % (top_level, 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)
> @@ -633,6 +653,8 @@ def main(argv):
>                          help='Code formatter. Default to clang-format if not specified.')
>      parser.add_argument('--staged', '-s', action='store_true',
>                          help='Include the changes in the index. Defaults to False')
> +    parser.add_argument('--amend', '-a', action='store_true',
> +                        help='Includes changes in the index and the previous patch combined. Defaults to False')

s/Includes/Include/

>      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:])
> @@ -671,6 +693,8 @@ def main(argv):
>      revlist = []
>      if args.staged:
>          revlist.append(Index())
> +    if args.amend:
> +        revlist.append(Amendment())

I think you'll find out that it won't work correctly if you specify both
--staged and --amend, as they duplicate each other to some extent.
Should we at the very least ignore --staged if --amend is set ? Or use a
single parameter that would take two different values so that both can't
be set together ?

Should we also ignore the list of commits of --staged or --amend is set
?

>      # If nothing of --staged or --amend was passed, defaults to HEAD
>      if len(revlist) == 0 and not args.revision_range:
Nicolas Dufresne Jan. 17, 2020, 10:58 p.m. UTC | #3 
Le samedi 18 janvier 2020 à 00:25 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Fri, Jan 17, 2020 at 02:17:32PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > This introduce a new argument "--amend" and a new special type of
> > commit "Amendment". It will check the style of changes that are in
> > the index combined with the changes of the last commit. So this is
> > the changes that would be applied by "git commit --amend" hence the
> > name of the argument.
> > 
> > This is needed to implement pre-commit hook.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  utils/checkstyle.py | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >  mode change 100644 => 100755 utils/checkstyle.py
> > 
> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > old mode 100644
> > new mode 100755
> > index 8e456cd..7c2ce00
> > --- a/utils/checkstyle.py
> > +++ b/utils/checkstyle.py
> > @@ -501,6 +501,26 @@ class Index(Commit):
> >                                stdout=subprocess.PIPE).stdout.decode('utf-8')
> >  
> >  
> > +class Amendment(Index):
> > +    def __init__(self):
> > +        Commit.__init__(self, None)
> > +
> > +    def det_info(self, top_level):
> 
> s/det_info/get_info/
> 
> This code path has never been tested :-)

Arg, uncommited, sorry.

> 
> > +        # Create a title using HEAD commit
> > +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', 'HEAD'],
> > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > +        title = 'Amendment of: ' + ret.splitlines()[0]
> > +        # Extract the list of modifier files
> > +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', 'HEAD~'],
> > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > +        return title, ret.splitlines()
> > +
> > +    def get_diff(self, top_level, filename):
> > +        return subprocess.run(['git', 'diff', '--staged', 'HEAD~', '--',
> > +                               '%s/%s' % (top_level, 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)
> > @@ -633,6 +653,8 @@ def main(argv):
> >                          help='Code formatter. Default to clang-format if not specified.')
> >      parser.add_argument('--staged', '-s', action='store_true',
> >                          help='Include the changes in the index. Defaults to False')
> > +    parser.add_argument('--amend', '-a', action='store_true',
> > +                        help='Includes changes in the index and the previous patch combined. Defaults to False')
> 
> s/Includes/Include/

Ack.

> 
> >      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:])
> > @@ -671,6 +693,8 @@ def main(argv):
> >      revlist = []
> >      if args.staged:
> >          revlist.append(Index())
> > +    if args.amend:
> > +        revlist.append(Amendment())
> 
> I think you'll find out that it won't work correctly if you specify both
> --staged and --amend, as they duplicate each other to some extent.
> Should we at the very least ignore --staged if --amend is set ? Or use a
> single parameter that would take two different values so that both can't
> be set together ?
> 
> Should we also ignore the list of commits of --staged or --amend is set
> ?

Just tested, and there is bugs in there. Of course it's redundant, but that's a
user problem.

./utils/checkstyle.py --staged --amend HEAD~
--------------
Staged changes
--------------
No style issue detected

--------------------------------------------------------------------------------
---------------
Amendment of: f4079ec6365ef6a5c23ff844e9a92aa93e986f01 checkstyle: Add a pre-
commit hook script
--------------------------------------------------------------------------------
---------------
No style issue detected

--------------------------------------------------------------------------------
-----------------
b240df0a1857a653d7417f1bf6035f552d3815e8 checkstyle: Add support for checking
style on amendments
--------------------------------------------------------------------------------
-----------------
No style issue detected

> 
> >      # If nothing of --staged or --amend was passed, defaults to HEAD
> >      if len(revlist) == 0 and not args.revision_range:
Nicolas Dufresne Jan. 17, 2020, 11:29 p.m. UTC | #4 
Le vendredi 17 janvier 2020 à 17:58 -0500, Nicolas Dufresne a écrit :
> Le samedi 18 janvier 2020 à 00:25 +0200, Laurent Pinchart a écrit :
> > Hi Nicolas,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jan 17, 2020 at 02:17:32PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > 
> > > This introduce a new argument "--amend" and a new special type of
> > > commit "Amendment". It will check the style of changes that are in
> > > the index combined with the changes of the last commit. So this is
> > > the changes that would be applied by "git commit --amend" hence the
> > > name of the argument.
> > > 
> > > This is needed to implement pre-commit hook.
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > ---
> > >  utils/checkstyle.py | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >  mode change 100644 => 100755 utils/checkstyle.py
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > old mode 100644
> > > new mode 100755
> > > index 8e456cd..7c2ce00
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -501,6 +501,26 @@ class Index(Commit):
> > >                                stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >  
> > >  
> > > +class Amendment(Index):
> > > +    def __init__(self):
> > > +        Commit.__init__(self, None)
> > > +
> > > +    def det_info(self, top_level):
> > 
> > s/det_info/get_info/
> > 
> > This code path has never been tested :-)
> 
> Arg, uncommited, sorry.
> 
> > > +        # Create a title using HEAD commit
> > > +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', 'HEAD'],
> > > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > > +        title = 'Amendment of: ' + ret.splitlines()[0]
> > > +        # Extract the list of modifier files
> > > +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', 'HEAD~'],
> > > +                             stdout=subprocess.PIPE).stdout.decode('utf-8')
> > > +        return title, ret.splitlines()
> > > +
> > > +    def get_diff(self, top_level, filename):
> > > +        return subprocess.run(['git', 'diff', '--staged', 'HEAD~', '--',
> > > +                               '%s/%s' % (top_level, 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)
> > > @@ -633,6 +653,8 @@ def main(argv):
> > >                          help='Code formatter. Default to clang-format if not specified.')
> > >      parser.add_argument('--staged', '-s', action='store_true',
> > >                          help='Include the changes in the index. Defaults to False')
> > > +    parser.add_argument('--amend', '-a', action='store_true',
> > > +                        help='Includes changes in the index and the previous patch combined. Defaults to False')
> > 
> > s/Includes/Include/
> 
> Ack.
> 
> > >      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:])
> > > @@ -671,6 +693,8 @@ def main(argv):
> > >      revlist = []
> > >      if args.staged:
> > >          revlist.append(Index())
> > > +    if args.amend:
> > > +        revlist.append(Amendment())
> > 
> > I think you'll find out that it won't work correctly if you specify both
> > --staged and --amend, as they duplicate each other to some extent.
> > Should we at the very least ignore --staged if --amend is set ? Or use a
> > single parameter that would take two different values so that both can't
> > be set together ?
> > 
> > Should we also ignore the list of commits of --staged or --amend is set
> > ?
> 
> Just tested, and there is bugs in there. Of course it's redundant, but that's a
> user problem.

Typo: there is *no* bugs.

> 
> ./utils/checkstyle.py --staged --amend HEAD~
> --------------
> Staged changes
> --------------
> No style issue detected
> 
> --------------------------------------------------------------------------------
> ---------------
> Amendment of: f4079ec6365ef6a5c23ff844e9a92aa93e986f01 checkstyle: Add a pre-
> commit hook script
> --------------------------------------------------------------------------------
> ---------------
> No style issue detected
> 
> --------------------------------------------------------------------------------
> -----------------
> b240df0a1857a653d7417f1bf6035f552d3815e8 checkstyle: Add support for checking
> style on amendments
> --------------------------------------------------------------------------------
> -----------------
> No style issue detected
> 
> > >      # If nothing of --staged or --amend was passed, defaults to HEAD
> > >      if len(revlist) == 0 and not args.revision_range:
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
old mode 100644
new mode 100755
index 8e456cd..7c2ce00
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -501,6 +501,26 @@  class Index(Commit):
                               stdout=subprocess.PIPE).stdout.decode('utf-8')
 
 
+class Amendment(Index):
+    def __init__(self):
+        Commit.__init__(self, None)
+
+    def det_info(self, top_level):
+        # Create a title using HEAD commit
+        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', 'HEAD'],
+                             stdout=subprocess.PIPE).stdout.decode('utf-8')
+        title = 'Amendment of: ' + ret.splitlines()[0]
+        # Extract the list of modifier files
+        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', 'HEAD~'],
+                             stdout=subprocess.PIPE).stdout.decode('utf-8')
+        return title, ret.splitlines()
+
+    def get_diff(self, top_level, filename):
+        return subprocess.run(['git', 'diff', '--staged', 'HEAD~', '--',
+                               '%s/%s' % (top_level, 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)
@@ -633,6 +653,8 @@  def main(argv):
                         help='Code formatter. Default to clang-format if not specified.')
     parser.add_argument('--staged', '-s', action='store_true',
                         help='Include the changes in the index. Defaults to False')
+    parser.add_argument('--amend', '-a', action='store_true',
+                        help='Includes changes in the index and the previous patch combined. 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:])
@@ -671,6 +693,8 @@  def main(argv):
     revlist = []
     if args.staged:
         revlist.append(Index())
+    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: