[libcamera-devel] [PATCH 2/2] checkstyle: Add support for pre-commit hook

[Kieran Bingham]

Hi Nicolas,

On 16/01/2020 18:26, nicolas at ndufresne.ca wrote:
> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
> This adds support for pre-commit hook workflow. In pre-commit hook we
> check the style on the changes currently staged. Note that this patch
> also includes a bit of sugar to support --amend.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
>  utils/checkstyle.py    | 55 +++++++++++++++++++++++++++++-------------
>  utils/hooks/pre-commit | 16 ++++++++++++
>  2 files changed, 54 insertions(+), 17 deletions(-)
>  mode change 100644 => 100755 utils/checkstyle.py
>  create mode 100755 utils/hooks/pre-commit
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> old mode 100644
> new mode 100755
> index 7edea25..23eb3f6
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):
>  # Style checking
>  #
>  
> -def check_file(top_level, commit, filename):
> +def check_file(top_level, commit, filename, staged):
>      # Extract the line numbers touched by the commit.
> -    diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> -                           '%s/%s' % (top_level, filename)],
> -                          stdout=subprocess.PIPE).stdout

I wonder if we could/should automate this by detecting staged changes,
and then setting staged based on that?

	"git status --porcelain | grep ^M"

Could detect if there are staged changes.

But equally it might be better to allow the caller to be explicit.


> +    if staged:
> +        diff = subprocess.run(['git', 'diff', '--staged', commit, '--',
> +                               '%s/%s' % (top_level, filename)],
> +                              stdout=subprocess.PIPE).stdout
> +    else:
> +        diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> +                               '%s/%s' % (top_level, filename)],
> +                              stdout=subprocess.PIPE).stdout
>      diff = diff.decode('utf-8').splitlines(True)
>      commit_diff = parse_diff(diff)
>  
> @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):
>  
>      # Format the file after the commit with all formatters and compute the diff
>      # between the unformatted and formatted contents.
> -    after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> -                           stdout=subprocess.PIPE).stdout
> +    if staged:
> +        after = subprocess.run(['git', 'show', ':%s' % (filename)],
> +                               stdout=subprocess.PIPE).stdout
> +    else:
> +        after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],
> +                               stdout=subprocess.PIPE).stdout
>      after = after.decode('utf-8')
>  
>      formatted = after
> @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):
>      return len(formatted_diff) + len(issues)
>  
>  
> -def check_style(top_level, commit):
> -    # Get the commit title and list of files.
> -    ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
> -                         stdout=subprocess.PIPE)
> -    files = ret.stdout.decode('utf-8').splitlines()
> -    title = files[0]
> -    files = files[1:]
> +def check_style(top_level, commit, staged):
> +    if staged:
> +        # Get list of staged changed files
> +        ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],
> +                             stdout=subprocess.PIPE)
> +        files = ret.stdout.decode('utf-8').splitlines()
> +        title = "Pre-commit"

This is not necessarily a "Pre-commit" (hook). It's just staged.
A user could run the tool directly to validate some staged changes
before committing...

So I'd perhaps make this
	title = "Staged"


> +    else:
> +        # Get the commit title and list of files.
> +        ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],
> +                             stdout=subprocess.PIPE)
> +        files = ret.stdout.decode('utf-8').splitlines()
> +        title = files[0]
> +        files = files[1:]
>  
>      separator = '-' * len(title)
>      print(separator)
> @@ -541,11 +557,11 @@ def check_style(top_level, commit):
>      files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]
>      if len(files) == 0:
>          print("Commit doesn't touch source files, skipping")
> -        return
> +        return 0
>  
>      issues = 0
>      for f in files:
> -        issues += check_file(top_level, commit, f)
> +        issues += check_file(top_level, commit, f, staged)
>  
>      if issues == 0:
>          print("No style issue detected")
> @@ -554,6 +570,8 @@ def check_style(top_level, commit):
>          print("%u potential style %s detected, please review" % \
>                  (issues, 'issue' if issues == 1 else 'issues'))
>  
> +    return issues
> +
>  
>  def extract_revlist(revs):
>      """Extract a list of commits on which to operate from a revision or revision
> @@ -595,6 +613,8 @@ 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('--staged', '-s', action='store_true',
> +                        help='Looks at the staged changes, used for pre-commit, defaults to False')
>      parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',
>                          help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
>      args = parser.parse_args(argv[1:])
> @@ -632,11 +652,12 @@ def main(argv):
>  
>      revlist = extract_revlist(args.revision_range)
>  
> +    issues = 0
>      for commit in revlist:
> -        check_style(top_level, commit)
> +        issues += check_style(top_level, commit, args.staged)
>          print('')
>  
> -    return 0
> +    return issues

I'd be tempted to split the
  "checkstyle: Report issue count as a failure"
and
  "checkstyle: Support validating staged git state"

to two patches, but perhaps that's not necessary unless you see benefit
in splitting.


>  
>  
>  if __name__ == '__main__':
> diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit
> new file mode 100755
> index 0000000..57d23ef
> --- /dev/null
> +++ b/utils/hooks/pre-commit

Though I really think this deserves it's own commit.

 "utils/hooks: Provide a pre-commit checkstyle hook"



> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +# Execute the checkstyle script before committing any code, failing the commit
> +# if needed. With this workflow, false positive can be ignored using:
> +#   git commit -n
> +#
> +# To utilise this hook, install this file with:
> +#   cp utils/hooks/pre-commit .git/hooks/pre-commit

One of the things I had toyed with is installing the hook by setting the
git hook directory to utils/hooks/ :

 git config core.hooksPath utils/hooks

But doing so now would invoke both the pre-commit and the post-commit hook.

I think it's good to provide the option of how someone might install
their hook

> +
> +commit=
> +if ps -ocommand= -p $PPID | grep -- "--amend"
> +then
> +   commit="HEAD~"

Is this really needed?

Edit: Yes, I've just played around with it - and I see what's happening.
Good catch to find that something extra was required here.


> +fi
> +
> +./utils/checkstyle.py --staged $commit


I wonder if the checkstyle.py could detect if we had staged changes if
we could fall back to needing only a single variant of the hook which
the user could choose to install as either a pre-commit or a post-commit
hook.

But perhaps it's more straight-forward to have two hooks to keep it
easier to support any future differences too.


-- 
Regards
--
Kieran