[{"id":3462,"web_url":"https://patchwork.libcamera.org/comment/3462/","msgid":"","date":"2020-01-16T22:25:31","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> From: Nicolas Dufresne \n> \n> This adds support for pre-commit hook workflow. In pre-commit hook we\n> check the style on the changes currently staged. Note that this patch\n> also includes a bit of sugar to support --amend.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> utils/hooks/pre-commit | 16 ++++++++++++\n> 2 files changed, 54 insertions(+), 17 deletions(-)\n> mode change 100644 => 100755 utils/checkstyle.py\n> create mode 100755 utils/hooks/pre-commit\n> \n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> old mode 100644\n> new mode 100755\n> index 7edea25..23eb3f6\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> # Style checking\n> #\n> \n> -def check_file(top_level, commit, filename):\n> +def check_file(top_level, commit, filename, staged):\n> # Extract the line numbers touched by the commit.\n> - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> - '%s/%s' % (top_level, filename)],\n> - stdout=subprocess.PIPE).stdout\n\nI wonder if we could/should automate this by detecting staged changes,\nand then setting staged based on that?\n\n\t\"git status --porcelain | grep ^M\"\n\nCould detect if there are staged changes.\n\nBut equally it might be better to allow the caller to be explicit.\n\n\n> + if staged:\n> + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> + '%s/%s' % (top_level, filename)],\n> + stdout=subprocess.PIPE).stdout\n> + else:\n> + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> + '%s/%s' % (top_level, filename)],\n> + stdout=subprocess.PIPE).stdout\n> diff = diff.decode('utf-8').splitlines(True)\n> commit_diff = parse_diff(diff)\n> \n> @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> \n> # Format the file after the commit with all formatters and compute the diff\n> # between the unformatted and formatted contents.\n> - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> - stdout=subprocess.PIPE).stdout\n> + if staged:\n> + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> + stdout=subprocess.PIPE).stdout\n> + else:\n> + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> + stdout=subprocess.PIPE).stdout\n> after = after.decode('utf-8')\n> \n> formatted = after\n> @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> return len(formatted_diff) + len(issues)\n> \n> \n> -def check_style(top_level, commit):\n> - # Get the commit title and list of files.\n> - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> - stdout=subprocess.PIPE)\n> - files = ret.stdout.decode('utf-8').splitlines()\n> - title = files[0]\n> - files = files[1:]\n> +def check_style(top_level, commit, staged):\n> + if staged:\n> + # Get list of staged changed files\n> + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> + stdout=subprocess.PIPE)\n> + files = ret.stdout.decode('utf-8').splitlines()\n> + title = \"Pre-commit\"\n\nThis is not necessarily a \"Pre-commit\" (hook). It's just staged.\nA user could run the tool directly to validate some staged changes\nbefore committing...\n\nSo I'd perhaps make this\n\ttitle = \"Staged\"\n\n\n> + else:\n> + # Get the commit title and list of files.\n> + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> + stdout=subprocess.PIPE)\n> + files = ret.stdout.decode('utf-8').splitlines()\n> + title = files[0]\n> + files = files[1:]\n> \n> separator = '-' * len(title)\n> print(separator)\n> @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> if len(files) == 0:\n> print(\"Commit doesn't touch source files, skipping\")\n> - return\n> + return 0\n> \n> issues = 0\n> for f in files:\n> - issues += check_file(top_level, commit, f)\n> + issues += check_file(top_level, commit, f, staged)\n> \n> if issues == 0:\n> print(\"No style issue detected\")\n> @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> print(\"%u potential style %s detected, please review\" % \\\n> (issues, 'issue' if issues == 1 else 'issues'))\n> \n> + return issues\n> +\n> \n> def extract_revlist(revs):\n> \"\"\"Extract a list of commits on which to operate from a revision or revision\n> @@ -595,6 +613,8 @@ def main(argv):\n> parser = argparse.ArgumentParser()\n> parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> help='Code formatter. Default to clang-format if not specified.')\n> + parser.add_argument('--staged', '-s', action='store_true',\n> + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> args = parser.parse_args(argv[1:])\n> @@ -632,11 +652,12 @@ def main(argv):\n> \n> revlist = extract_revlist(args.revision_range)\n> \n> + issues = 0\n> for commit in revlist:\n> - check_style(top_level, commit)\n> + issues += check_style(top_level, commit, args.staged)\n> print('')\n> \n> - return 0\n> + return issues\n\nI'd be tempted to split the\n \"checkstyle: Report issue count as a failure\"\nand\n \"checkstyle: Support validating staged git state\"\n\nto two patches, but perhaps that's not necessary unless you see benefit\nin splitting.\n\n\n> \n> \n> if __name__ == '__main__':\n> diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> new file mode 100755\n> index 0000000..57d23ef\n> --- /dev/null\n> +++ b/utils/hooks/pre-commit\n\nThough I really think this deserves it's own commit.\n\n \"utils/hooks: Provide a pre-commit checkstyle hook\"\n\n\n\n> @@ -0,0 +1,16 @@\n> +#!/bin/sh\n> +\n> +# Execute the checkstyle script before committing any code, failing the commit\n> +# if needed. With this workflow, false positive can be ignored using:\n> +# git commit -n\n> +#\n> +# To utilise this hook, install this file with:\n> +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n\nOne of the things I had toyed with is installing the hook by setting the\ngit hook directory to utils/hooks/ :\n\n git config core.hooksPath utils/hooks\n\nBut doing so now would invoke both the pre-commit and the post-commit hook.\n\nI think it's good to provide the option of how someone might install\ntheir hook\n\n> +\n> +commit=\n> +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> +then\n> + commit=\"HEAD~\"\n\nIs this really needed?\n\nEdit: Yes, I've just played around with it - and I see what's happening.\nGood catch to find that something extra was required here.\n\n\n> +fi\n> +\n> +./utils/checkstyle.py --staged $commit\n\n\nI wonder if the checkstyle.py could detect if we had staged changes if\nwe could fall back to needing only a single variant of the hook which\nthe user could choose to install as either a pre-commit or a post-commit\nhook.\n\nBut perhaps it's more straight-forward to have two hooks to keep it\neasier to support any future differences too.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F6B260705\n\tfor ;\n\tThu, 16 Jan 2020 23:25:34 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AAA3F2D2;\n\tThu, 16 Jan 2020 23:25:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579213533;\n\tbh=ZMYeGckuW//oUX4MfxxYcvkHY2GK+FpNy9xr3AI1zT8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=LlxX8O+LhKjk0VbIqa8qUTCK2Agasbg/NhYmsiEaNJ5aQPXf6fN/og+N6fd287t2x\n\tdSCuoAVw2E+l0CHDVJvTFienSs/8m6cxnZsBVgzAucIm2s2cjGo7GjfYqXewXk7uTe\n\tPKnij0CaHOPq0TnkthX08JNEeFOqkNoSdOhOfhCA=","Reply-To":"kieran.bingham@ideasonboard.com","To":"nicolas@ndufresne.ca, libcamera-devel@lists.libcamera.org","Cc":"Nicolas Dufresne ","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 16 Jan 2020 22:25:31 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200116182603.108966-3-nicolas@ndufresne.ca>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 16 Jan 2020 22:25:34 -0000"}},{"id":3463,"web_url":"https://patchwork.libcamera.org/comment/3463/","msgid":"<20200116223130.GA32545@pendragon.ideasonboard.com>","date":"2020-01-16T22:31:30","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> > From: Nicolas Dufresne \n> > \n> > This adds support for pre-commit hook workflow. In pre-commit hook we\n> > check the style on the changes currently staged. Note that this patch\n> > also includes a bit of sugar to support --amend.\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> > utils/hooks/pre-commit | 16 ++++++++++++\n> > 2 files changed, 54 insertions(+), 17 deletions(-)\n> > mode change 100644 => 100755 utils/checkstyle.py\n> > create mode 100755 utils/hooks/pre-commit\n> > \n> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > old mode 100644\n> > new mode 100755\n> > index 7edea25..23eb3f6\n> > --- a/utils/checkstyle.py\n> > +++ b/utils/checkstyle.py\n> > @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> > # Style checking\n> > #\n> > \n> > -def check_file(top_level, commit, filename):\n> > +def check_file(top_level, commit, filename, staged):\n> > # Extract the line numbers touched by the commit.\n> > - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > - '%s/%s' % (top_level, filename)],\n> > - stdout=subprocess.PIPE).stdout\n> \n> I wonder if we could/should automate this by detecting staged changes,\n> and then setting staged based on that?\n> \n> \t\"git status --porcelain | grep ^M\"\n> \n> Could detect if there are staged changes.\n> \n> But equally it might be better to allow the caller to be explicit.\n\nI've been wondering about this too, I have a slight preference for\nauto-detection, but that's not a hard requirement if too difficult to\nimplement or impractical to use.\n\n> > + if staged:\n> > + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> > + '%s/%s' % (top_level, filename)],\n> > + stdout=subprocess.PIPE).stdout\n> > + else:\n> > + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > + '%s/%s' % (top_level, filename)],\n> > + stdout=subprocess.PIPE).stdout\n> > diff = diff.decode('utf-8').splitlines(True)\n> > commit_diff = parse_diff(diff)\n> > \n> > @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> > \n> > # Format the file after the commit with all formatters and compute the diff\n> > # between the unformatted and formatted contents.\n> > - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > - stdout=subprocess.PIPE).stdout\n> > + if staged:\n> > + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> > + stdout=subprocess.PIPE).stdout\n> > + else:\n> > + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > + stdout=subprocess.PIPE).stdout\n> > after = after.decode('utf-8')\n> > \n> > formatted = after\n> > @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> > return len(formatted_diff) + len(issues)\n> > \n> > \n> > -def check_style(top_level, commit):\n> > - # Get the commit title and list of files.\n> > - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> > - stdout=subprocess.PIPE)\n> > - files = ret.stdout.decode('utf-8').splitlines()\n> > - title = files[0]\n> > - files = files[1:]\n> > +def check_style(top_level, commit, staged):\n> > + if staged:\n> > + # Get list of staged changed files\n> > + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> > + stdout=subprocess.PIPE)\n> > + files = ret.stdout.decode('utf-8').splitlines()\n> > + title = \"Pre-commit\"\n> \n> This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> A user could run the tool directly to validate some staged changes\n> before committing...\n> \n> So I'd perhaps make this\n> \ttitle = \"Staged\"\n\nOr maybe \"Staged changes\" to be slightly more explicit ?\n\n> > + else:\n> > + # Get the commit title and list of files.\n> > + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> > + stdout=subprocess.PIPE)\n> > + files = ret.stdout.decode('utf-8').splitlines()\n> > + title = files[0]\n> > + files = files[1:]\n> > \n> > separator = '-' * len(title)\n> > print(separator)\n> > @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> > files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> > if len(files) == 0:\n> > print(\"Commit doesn't touch source files, skipping\")\n> > - return\n> > + return 0\n> > \n> > issues = 0\n> > for f in files:\n> > - issues += check_file(top_level, commit, f)\n> > + issues += check_file(top_level, commit, f, staged)\n> > \n> > if issues == 0:\n> > print(\"No style issue detected\")\n> > @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> > print(\"%u potential style %s detected, please review\" % \\\n> > (issues, 'issue' if issues == 1 else 'issues'))\n> > \n> > + return issues\n> > +\n> > \n> > def extract_revlist(revs):\n> > \"\"\"Extract a list of commits on which to operate from a revision or revision\n> > @@ -595,6 +613,8 @@ def main(argv):\n> > parser = argparse.ArgumentParser()\n> > parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> > help='Code formatter. Default to clang-format if not specified.')\n> > + parser.add_argument('--staged', '-s', action='store_true',\n> > + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> > parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> > help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> > args = parser.parse_args(argv[1:])\n> > @@ -632,11 +652,12 @@ def main(argv):\n> > \n> > revlist = extract_revlist(args.revision_range)\n> > \n> > + issues = 0\n> > for commit in revlist:\n> > - check_style(top_level, commit)\n> > + issues += check_style(top_level, commit, args.staged)\n> > print('')\n> > \n> > - return 0\n> > + return issues\n> \n> I'd be tempted to split the\n> \"checkstyle: Report issue count as a failure\"\n> and\n> \"checkstyle: Support validating staged git state\"\n> \n> to two patches, but perhaps that's not necessary unless you see benefit\n> in splitting.\n\nI don't think there's a need to resubmit just for this, but if a v2 is\nneeded, it may be nice to split the changes indeed, to explain why\nchanging the return value is needed.\n\nAccording to the exit man page,\n\nRATIONALE\n As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n\n 126 A file to be executed was found, but it was not an executable utility.\n\n 127 A utility to be executed was not found.\n\n >128 A command was interrupted by a signal.\n\nAnd according to exit(),\n\nThe value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n\nIn the unfortunate event that a multiple of 256 issues would be found,\nthe caller would think everything went fine. Should we thus return 0 on\nsuccess and 1 if any issue was found ?\n\n> >\n> >\n> > if __name__ == '__main__':\n> > diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> > new file mode 100755\n> > index 0000000..57d23ef\n> > --- /dev/null\n> > +++ b/utils/hooks/pre-commit\n> \n> Though I really think this deserves it's own commit.\n> \n> \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> \n> > @@ -0,0 +1,16 @@\n> > +#!/bin/sh\n> > +\n> > +# Execute the checkstyle script before committing any code, failing the commit\n> > +# if needed. With this workflow, false positive can be ignored using:\n> > +# git commit -n\n> > +#\n> > +# To utilise this hook, install this file with:\n> > +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> \n> One of the things I had toyed with is installing the hook by setting the\n> git hook directory to utils/hooks/ :\n> \n> git config core.hooksPath utils/hooks\n> \n> But doing so now would invoke both the pre-commit and the post-commit hook.\n> \n> I think it's good to provide the option of how someone might install\n> their hook\n> \n> > +\n> > +commit=\n> > +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> > +then\n> > + commit=\"HEAD~\"\n> \n> Is this really needed?\n> \n> Edit: Yes, I've just played around with it - and I see what's happening.\n> Good catch to find that something extra was required here.\n> \n> > +fi\n> > +\n> > +./utils/checkstyle.py --staged $commit\n> \n> I wonder if the checkstyle.py could detect if we had staged changes if\n> we could fall back to needing only a single variant of the hook which\n> the user could choose to install as either a pre-commit or a post-commit\n> hook.\n> \n> But perhaps it's more straight-forward to have two hooks to keep it\n> easier to support any future differences too.\n\nI'm fine either way.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D0FE60705\n\tfor ;\n\tThu, 16 Jan 2020 23:31:45 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE3702D2;\n\tThu, 16 Jan 2020 23:31:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579213904;\n\tbh=JluQ8NHVPuRG4SuBPQQDz0OcKCIZvUtBD+sDxpwvSu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gLBOkiboMgh5IYHfGtflmC9A+PZVDB3eaa9LPbjQhp15KaoZypWiv4i2rB4s2x8Zd\n\t42Y4U/jsc2gWdqG1yTk94m72XOaEZd9+PV3dDvNv0x6mNPMyX2e29jtjUDYDkb5h6j\n\tzfu3FDFNNMbhYSzW1MeHbI9EqIKNDhbjObF7WXPg=","Date":"Fri, 17 Jan 2020 00:31:30 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"nicolas@ndufresne.ca, libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200116223130.GA32545@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 16 Jan 2020 22:31:45 -0000"}},{"id":3471,"web_url":"https://patchwork.libcamera.org/comment/3471/","msgid":"<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>","date":"2020-01-17T01:23:36","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le vendredi 17 janvier 2020 à 00:31 +0200, Laurent Pinchart a écrit :\n> Hi Kieran,\n> \n> On Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> > On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> > > From: Nicolas Dufresne \n> > > \n> > > This adds support for pre-commit hook workflow. In pre-commit hook we\n> > > check the style on the changes currently staged. Note that this patch\n> > > also includes a bit of sugar to support --amend.\n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> > > utils/hooks/pre-commit | 16 ++++++++++++\n> > > 2 files changed, 54 insertions(+), 17 deletions(-)\n> > > mode change 100644 => 100755 utils/checkstyle.py\n> > > create mode 100755 utils/hooks/pre-commit\n> > > \n> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > old mode 100644\n> > > new mode 100755\n> > > index 7edea25..23eb3f6\n> > > --- a/utils/checkstyle.py\n> > > +++ b/utils/checkstyle.py\n> > > @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> > > # Style checking\n> > > #\n> > > \n> > > -def check_file(top_level, commit, filename):\n> > > +def check_file(top_level, commit, filename, staged):\n> > > # Extract the line numbers touched by the commit.\n> > > - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > - '%s/%s' % (top_level, filename)],\n> > > - stdout=subprocess.PIPE).stdout\n> > \n> > I wonder if we could/should automate this by detecting staged changes,\n> > and then setting staged based on that?\n> > \n> > \t\"git status --porcelain | grep ^M\"\n> > \n> > Could detect if there are staged changes.\n> > \n> > But equally it might be better to allow the caller to be explicit.\n> \n> I've been wondering about this too, I have a slight preference for\n> auto-detection, but that's not a hard requirement if too difficult to\n> implement or impractical to use.\n\nI don't think it hard, but I wasn't sure it was a good idea. One thing\nthat I've been a little slowpy is with the meaning of commit here (of\nthe rev list in fact). Before I added amend support, I was simply\nignoring that (no meaning). But then I recycled it to support the\ncommit --amend case, where you want to included the staged changes and\nthe previous commit.\n\nWhat I found worked to get that information was to only pass one hash,\nwhich is the commit before the last commit (HEAD~). Anyway, I think I\nshould fix this, and so '%s~' % commit instead if I want to maintained\nthe behaviour. But by doing that, there is no longer anything to\ndescribe that when I commit --amend, I want the combined report, not\njust what was staged. And that I also don't want the previous commit to\nbe checked independently from the staged, because that would produce\nthat warning I'm trying to fix.\n\nSo my conclusion, is that to disambiguate all this, I should in fact be\neven more explicit, and also introduce --amend. A better implementation\nwould be to find a way to integrated these special revisions into the\nrevlist, this way (not sure if there is a use case) but you could\nrequest a report for specific commits, the staging changes and the\namended changes in one call.\n\n> \n> > > + if staged:\n> > > + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> > > + '%s/%s' % (top_level, filename)],\n> > > + stdout=subprocess.PIPE).stdout\n\n> > > + else:\n> > > + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > + '%s/%s' % (top_level, filename)],\n> > > + stdout=subprocess.PIPE).stdout\n> > > diff = diff.decode('utf-8').splitlines(True)\n> > > commit_diff = parse_diff(diff)\n> > > \n> > > @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> > > \n> > > # Format the file after the commit with all formatters and compute the diff\n> > > # between the unformatted and formatted contents.\n> > > - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > - stdout=subprocess.PIPE).stdout\n> > > + if staged:\n> > > + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> > > + stdout=subprocess.PIPE).stdout\n> > > + else:\n> > > + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > + stdout=subprocess.PIPE).stdout\n> > > after = after.decode('utf-8')\n> > > \n> > > formatted = after\n> > > @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> > > return len(formatted_diff) + len(issues)\n> > > \n> > > \n> > > -def check_style(top_level, commit):\n> > > - # Get the commit title and list of files.\n> > > - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> > > - stdout=subprocess.PIPE)\n> > > - files = ret.stdout.decode('utf-8').splitlines()\n> > > - title = files[0]\n> > > - files = files[1:]\n> > > +def check_style(top_level, commit, staged):\n> > > + if staged:\n> > > + # Get list of staged changed files\n> > > + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> > > + stdout=subprocess.PIPE)\n> > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > + title = \"Pre-commit\"\n> > \n> > This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> > A user could run the tool directly to validate some staged changes\n> > before committing...\n> > \n> > So I'd perhaps make this\n> > \ttitle = \"Staged\"\n> \n> Or maybe \"Staged changes\" to be slightly more explicit ?\n\nIt started \"Staged\", then I splitted it between amend and staged, and\nfor some reason ended up like this. I think I'll split it again, and\nuse \"Staged changes\" or \"Amended changes\".\n\n> \n> > > + else:\n> > > + # Get the commit title and list of files.\n> > > + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> > > + stdout=subprocess.PIPE)\n> > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > + title = files[0]\n> > > + files = files[1:]\n> > > \n> > > separator = '-' * len(title)\n> > > print(separator)\n> > > @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> > > files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> > > if len(files) == 0:\n> > > print(\"Commit doesn't touch source files, skipping\")\n> > > - return\n> > > + return 0\n> > > \n> > > issues = 0\n> > > for f in files:\n> > > - issues += check_file(top_level, commit, f)\n> > > + issues += check_file(top_level, commit, f, staged)\n> > > \n> > > if issues == 0:\n> > > print(\"No style issue detected\")\n> > > @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> > > print(\"%u potential style %s detected, please review\" % \\\n> > > (issues, 'issue' if issues == 1 else 'issues'))\n> > > \n> > > + return issues\n> > > +\n> > > \n> > > def extract_revlist(revs):\n> > > \"\"\"Extract a list of commits on which to operate from a revision or revision\n> > > @@ -595,6 +613,8 @@ def main(argv):\n> > > parser = argparse.ArgumentParser()\n> > > parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> > > help='Code formatter. Default to clang-format if not specified.')\n> > > + parser.add_argument('--staged', '-s', action='store_true',\n> > > + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> > > parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> > > help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> > > args = parser.parse_args(argv[1:])\n> > > @@ -632,11 +652,12 @@ def main(argv):\n> > > \n> > > revlist = extract_revlist(args.revision_range)\n> > > \n> > > + issues = 0\n> > > for commit in revlist:\n> > > - check_style(top_level, commit)\n> > > + issues += check_style(top_level, commit, args.staged)\n> > > print('')\n> > > \n> > > - return 0\n> > > + return issues\n> > \n> > I'd be tempted to split the\n> > \"checkstyle: Report issue count as a failure\"\n> > and\n> > \"checkstyle: Support validating staged git state\"\n> > \n> > to two patches, but perhaps that's not necessary unless you see benefit\n> > in splitting.\n> \n> I don't think there's a need to resubmit just for this, but if a v2 is\n> needed, it may be nice to split the changes indeed, to explain why\n> changing the return value is needed.\n\nI think a v2 is needed, so I'll do.\n\n> \n> According to the exit man page,\n> \n> RATIONALE\n> As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n> \n> 126 A file to be executed was found, but it was not an executable utility.\n> \n> 127 A utility to be executed was not found.\n> \n> >128 A command was interrupted by a signal.\n> \n> And according to exit(),\n> \n> The value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n> \n> In the unfortunate event that a multiple of 256 issues would be found,\n> the caller would think everything went fine. Should we thus return 0 on\n> success and 1 if any issue was found ?\n\nAgreed, will turn the number of issues into 0 and 1.\n\n> \n> > > \n> > > if __name__ == '__main__':\n> > > diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> > > new file mode 100755\n> > > index 0000000..57d23ef\n> > > --- /dev/null\n> > > +++ b/utils/hooks/pre-commit\n> > \n> > Though I really think this deserves it's own commit.\n> > \n> > \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> > \n> > > @@ -0,0 +1,16 @@\n> > > +#!/bin/sh\n> > > +\n> > > +# Execute the checkstyle script before committing any code, failing the commit\n> > > +# if needed. With this workflow, false positive can be ignored using:\n> > > +# git commit -n\n> > > +#\n> > > +# To utilise this hook, install this file with:\n> > > +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> > \n> > One of the things I had toyed with is installing the hook by setting the\n> > git hook directory to utils/hooks/ :\n> > \n> > git config core.hooksPath utils/hooks\n> > \n> > But doing so now would invoke both the pre-commit and the post-commit hook.\n> > \n> > I think it's good to provide the option of how someone might install\n> > their hook\n> > \n> > > +\n> > > +commit=\n> > > +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> > > +then\n> > > + commit=\"HEAD~\"\n> > \n> > Is this really needed?\n> > \n> > Edit: Yes, I've just played around with it - and I see what's happening.\n> > Good catch to find that something extra was required here.\n> > \n> > > +fi\n> > > +\n> > > +./utils/checkstyle.py --staged $commit\n> > \n> > I wonder if the checkstyle.py could detect if we had staged changes if\n> > we could fall back to needing only a single variant of the hook which\n> > the user could choose to install as either a pre-commit or a post-commit\n> > hook.\n> > \n> > But perhaps it's more straight-forward to have two hooks to keep it\n> > easier to support any future differences too.\n> \n> I'm fine either way.\n\nI think having two files in hook/ directory will be less confusing \neven if they are identical in the end. But I don't think the auto\ndetection is a good idea, it's makes the CLI of the tool ambiguous at\nleast.","headers":{"Return-Path":"","Received":["from mail-qt1-x841.google.com (mail-qt1-x841.google.com\n\t[IPv6:2607:f8b0:4864:20::841])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC2A86075C\n\tfor ;\n\tFri, 17 Jan 2020 02:23:39 +0100 (CET)","by mail-qt1-x841.google.com with SMTP id 5so20605663qtz.1\n\tfor ;\n\tThu, 16 Jan 2020 17:23:39 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\t24sm11143103qka.32.2020.01.16.17.23.36\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 16 Jan 2020 17:23:37 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=kKhJXkZpUjXqMi9BDVnr84arcylex5FJYHH4km+x5Ng=;\n\tb=NGb2XSUCOg+sFgz9h3GfG6nlP/aDCAVQR7ecbGCjKoY3edjDeQLkvbe80xX9Wmwu17\n\tF7jwDuen3dJlsgIw43Rt0shQLiDO6cXo0x/hDpPYqrrQ4AaxfWL01gq4cnrZMRkYeJfE\n\t0mMMiAnblTB7Lmkz8/VtzPxI1yKRfiMdg3J17Zr/a2JdrhftWv/JGVSFSKtMuwhzjUM8\n\tWFQSLYJqMwMhAxuzzlHFGJTQ7ZlREUWIdv2iw6PKpeInEhUwhiRIF2aOT+a/lYklhGb8\n\toYVFz9yGXfAfCLRy1240dHcqhzxcKIt1XFHXiE1fjuAxR8Kym+s865S7gi1DLXhkgtJQ\n\tLTlA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=kKhJXkZpUjXqMi9BDVnr84arcylex5FJYHH4km+x5Ng=;\n\tb=pYn9eWvj/uOaT2couIKmVUKMGNeUCDEMNziJamiH6b7E8JOsYWoxQShxIB13UC34dA\n\tqwE3RmiSzLwcdAPOwsuih21Sc3GmjXlVnZFnYOAOrtdwOecNgDRN3zSHtqnbQ7rgGELV\n\tp9U8p8bV0Z9uyyog0KS15veEkWH/XhlTyWpg7CDYn93hdV/MdPfiiiLnry37V1Nld1da\n\tDVMqD4arinGWM06lkqENz2pw9nDMuI08eQKMozso59c0SPVLqVmKX6a8bpcd/aNqGnwN\n\t85qGfCh6jes5WTzAqgcI45pR3Vf+SohOTyy6HsnI6Ss7C2FavOp3U8JU9mTEaTu3NIrp\n\tUiKA==","X-Gm-Message-State":"APjAAAXNhXTvJhWGCNlft2ZNiL6sI/8RHGcE3iGFCVvxebKCeAQFkY1Z\n\tKEXKXtJ79dUB3IWoentCPLMZgQ==","X-Google-Smtp-Source":"APXvYqwI4TbYAGQmx+zbfmR6+JiMrwMDubhHCrD/T9pg28xyaRb3vmJEzOrzGVhkArXTbFYfoPL0LQ==","X-Received":"by 2002:ac8:5205:: with SMTP id r5mr5339875qtn.230.1579224218190;\n\tThu, 16 Jan 2020 17:23:38 -0800 (PST)","Message-ID":"<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Kieran Bingham\n\t","Cc":"libcamera-devel@lists.libcamera.org","Date":"Thu, 16 Jan 2020 20:23:36 -0500","In-Reply-To":"<20200116223130.GA32545@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t\n\t<20200116223130.GA32545@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.2 (3.34.2-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 Jan 2020 01:23:40 -0000"}},{"id":3472,"web_url":"https://patchwork.libcamera.org/comment/3472/","msgid":"<20200117074652.GA5711@pendragon.ideasonboard.com>","date":"2020-01-17T07:46:52","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Thu, Jan 16, 2020 at 08:23:36PM -0500, Nicolas Dufresne wrote:\n> Le vendredi 17 janvier 2020 à 00:31 +0200, Laurent Pinchart a écrit :\n> > On Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> >> On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> >>> From: Nicolas Dufresne \n> >>> \n> >>> This adds support for pre-commit hook workflow. In pre-commit hook we\n> >>> check the style on the changes currently staged. Note that this patch\n> >>> also includes a bit of sugar to support --amend.\n> >>> \n> >>> Signed-off-by: Nicolas Dufresne \n> >>> ---\n> >>> utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> >>> utils/hooks/pre-commit | 16 ++++++++++++\n> >>> 2 files changed, 54 insertions(+), 17 deletions(-)\n> >>> mode change 100644 => 100755 utils/checkstyle.py\n> >>> create mode 100755 utils/hooks/pre-commit\n> >>> \n> >>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> >>> old mode 100644\n> >>> new mode 100755\n> >>> index 7edea25..23eb3f6\n> >>> --- a/utils/checkstyle.py\n> >>> +++ b/utils/checkstyle.py\n> >>> @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> >>> # Style checking\n> >>> #\n> >>> \n> >>> -def check_file(top_level, commit, filename):\n> >>> +def check_file(top_level, commit, filename, staged):\n> >>> # Extract the line numbers touched by the commit.\n> >>> - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> >>> - '%s/%s' % (top_level, filename)],\n> >>> - stdout=subprocess.PIPE).stdout\n> >> \n> >> I wonder if we could/should automate this by detecting staged changes,\n> >> and then setting staged based on that?\n> >> \n> >> \t\"git status --porcelain | grep ^M\"\n> >> \n> >> Could detect if there are staged changes.\n> >> \n> >> But equally it might be better to allow the caller to be explicit.\n> > \n> > I've been wondering about this too, I have a slight preference for\n> > auto-detection, but that's not a hard requirement if too difficult to\n> > implement or impractical to use.\n> \n> I don't think it hard, but I wasn't sure it was a good idea. One thing\n> that I've been a little slowpy is with the meaning of commit here (of\n> the rev list in fact). Before I added amend support, I was simply\n> ignoring that (no meaning). But then I recycled it to support the\n> commit --amend case, where you want to included the staged changes and\n> the previous commit.\n> \n> What I found worked to get that information was to only pass one hash,\n> which is the commit before the last commit (HEAD~). Anyway, I think I\n> should fix this, and so '%s~' % commit instead if I want to maintained\n> the behaviour. But by doing that, there is no longer anything to\n> describe that when I commit --amend, I want the combined report, not\n> just what was staged. And that I also don't want the previous commit to\n> be checked independently from the staged, because that would produce\n> that warning I'm trying to fix.\n> \n> So my conclusion, is that to disambiguate all this, I should in fact be\n> even more explicit, and also introduce --amend. A better implementation\n> would be to find a way to integrated these special revisions into the\n> revlist, this way (not sure if there is a use case) but you could\n> request a report for specific commits, the staging changes and the\n> amended changes in one call.\n\nThat would be neat indeed. From a quick look at git-rev-parse there's no\nrevision syntax to express this that I could find. We could extend the\nrevision syntax, but that may be dangerous.\n\nIf we instead go for explicit arguments, --staged or --cached would be\nfine with me, but I'd rather use --worktree (or something similar) than\n--amend to reflect that the revision corresponds to the working tree.\n\n> >>> + if staged:\n> >>> + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> >>> + '%s/%s' % (top_level, filename)],\n> >>> + stdout=subprocess.PIPE).stdout\n> >>> + else:\n> >>> + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> >>> + '%s/%s' % (top_level, filename)],\n> >>> + stdout=subprocess.PIPE).stdout\n> >>> diff = diff.decode('utf-8').splitlines(True)\n> >>> commit_diff = parse_diff(diff)\n> >>> \n> >>> @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> >>> \n> >>> # Format the file after the commit with all formatters and compute the diff\n> >>> # between the unformatted and formatted contents.\n> >>> - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> >>> - stdout=subprocess.PIPE).stdout\n> >>> + if staged:\n> >>> + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> >>> + stdout=subprocess.PIPE).stdout\n> >>> + else:\n> >>> + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> >>> + stdout=subprocess.PIPE).stdout\n> >>> after = after.decode('utf-8')\n> >>> \n> >>> formatted = after\n> >>> @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> >>> return len(formatted_diff) + len(issues)\n> >>> \n> >>> \n> >>> -def check_style(top_level, commit):\n> >>> - # Get the commit title and list of files.\n> >>> - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> >>> - stdout=subprocess.PIPE)\n> >>> - files = ret.stdout.decode('utf-8').splitlines()\n> >>> - title = files[0]\n> >>> - files = files[1:]\n> >>> +def check_style(top_level, commit, staged):\n> >>> + if staged:\n> >>> + # Get list of staged changed files\n> >>> + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> >>> + stdout=subprocess.PIPE)\n> >>> + files = ret.stdout.decode('utf-8').splitlines()\n> >>> + title = \"Pre-commit\"\n> >> \n> >> This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> >> A user could run the tool directly to validate some staged changes\n> >> before committing...\n> >> \n> >> So I'd perhaps make this\n> >> \ttitle = \"Staged\"\n> > \n> > Or maybe \"Staged changes\" to be slightly more explicit ?\n> \n> It started \"Staged\", then I splitted it between amend and staged, and\n> for some reason ended up like this. I think I'll split it again, and\n> use \"Staged changes\" or \"Amended changes\".\n\nMaybe \"Work tree changes\" instead of \"Amended changes\" ?\n\n> >>> + else:\n> >>> + # Get the commit title and list of files.\n> >>> + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> >>> + stdout=subprocess.PIPE)\n> >>> + files = ret.stdout.decode('utf-8').splitlines()\n> >>> + title = files[0]\n> >>> + files = files[1:]\n> >>> \n> >>> separator = '-' * len(title)\n> >>> print(separator)\n> >>> @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> >>> files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> >>> if len(files) == 0:\n> >>> print(\"Commit doesn't touch source files, skipping\")\n> >>> - return\n> >>> + return 0\n> >>> \n> >>> issues = 0\n> >>> for f in files:\n> >>> - issues += check_file(top_level, commit, f)\n> >>> + issues += check_file(top_level, commit, f, staged)\n> >>> \n> >>> if issues == 0:\n> >>> print(\"No style issue detected\")\n> >>> @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> >>> print(\"%u potential style %s detected, please review\" % \\\n> >>> (issues, 'issue' if issues == 1 else 'issues'))\n> >>> \n> >>> + return issues\n> >>> +\n> >>> \n> >>> def extract_revlist(revs):\n> >>> \"\"\"Extract a list of commits on which to operate from a revision or revision\n> >>> @@ -595,6 +613,8 @@ def main(argv):\n> >>> parser = argparse.ArgumentParser()\n> >>> parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> >>> help='Code formatter. Default to clang-format if not specified.')\n> >>> + parser.add_argument('--staged', '-s', action='store_true',\n> >>> + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> >>> parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> >>> help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> >>> args = parser.parse_args(argv[1:])\n> >>> @@ -632,11 +652,12 @@ def main(argv):\n> >>> \n> >>> revlist = extract_revlist(args.revision_range)\n> >>> \n> >>> + issues = 0\n> >>> for commit in revlist:\n> >>> - check_style(top_level, commit)\n> >>> + issues += check_style(top_level, commit, args.staged)\n> >>> print('')\n> >>> \n> >>> - return 0\n> >>> + return issues\n> >> \n> >> I'd be tempted to split the\n> >> \"checkstyle: Report issue count as a failure\"\n> >> and\n> >> \"checkstyle: Support validating staged git state\"\n> >> \n> >> to two patches, but perhaps that's not necessary unless you see benefit\n> >> in splitting.\n> > \n> > I don't think there's a need to resubmit just for this, but if a v2 is\n> > needed, it may be nice to split the changes indeed, to explain why\n> > changing the return value is needed.\n> \n> I think a v2 is needed, so I'll do.\n> \n> > According to the exit man page,\n> > \n> > RATIONALE\n> > As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n> > \n> > 126 A file to be executed was found, but it was not an executable utility.\n> > \n> > 127 A utility to be executed was not found.\n> > \n> > >128 A command was interrupted by a signal.\n> > \n> > And according to exit(),\n> > \n> > The value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n> > \n> > In the unfortunate event that a multiple of 256 issues would be found,\n> > the caller would think everything went fine. Should we thus return 0 on\n> > success and 1 if any issue was found ?\n> \n> Agreed, will turn the number of issues into 0 and 1.\n> \n> >>> \n> >>> if __name__ == '__main__':\n> >>> diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> >>> new file mode 100755\n> >>> index 0000000..57d23ef\n> >>> --- /dev/null\n> >>> +++ b/utils/hooks/pre-commit\n> >> \n> >> Though I really think this deserves it's own commit.\n> >> \n> >> \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> >> \n> >>> @@ -0,0 +1,16 @@\n> >>> +#!/bin/sh\n> >>> +\n> >>> +# Execute the checkstyle script before committing any code, failing the commit\n> >>> +# if needed. With this workflow, false positive can be ignored using:\n> >>> +# git commit -n\n> >>> +#\n> >>> +# To utilise this hook, install this file with:\n> >>> +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> >> \n> >> One of the things I had toyed with is installing the hook by setting the\n> >> git hook directory to utils/hooks/ :\n> >> \n> >> git config core.hooksPath utils/hooks\n> >> \n> >> But doing so now would invoke both the pre-commit and the post-commit hook.\n> >> \n> >> I think it's good to provide the option of how someone might install\n> >> their hook\n> >> \n> >>> +\n> >>> +commit=\n> >>> +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> >>> +then\n> >>> + commit=\"HEAD~\"\n> >> \n> >> Is this really needed?\n> >> \n> >> Edit: Yes, I've just played around with it - and I see what's happening.\n> >> Good catch to find that something extra was required here.\n> >> \n> >>> +fi\n> >>> +\n> >>> +./utils/checkstyle.py --staged $commit\n> >> \n> >> I wonder if the checkstyle.py could detect if we had staged changes if\n> >> we could fall back to needing only a single variant of the hook which\n> >> the user could choose to install as either a pre-commit or a post-commit\n> >> hook.\n> >> \n> >> But perhaps it's more straight-forward to have two hooks to keep it\n> >> easier to support any future differences too.\n> > \n> > I'm fine either way.\n> \n> I think having two files in hook/ directory will be less confusing \n> even if they are identical in the end. But I don't think the auto\n> detection is a good idea, it's makes the CLI of the tool ambiguous at\n> least.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51C8C600F5\n\tfor ;\n\tFri, 17 Jan 2020 08:47:07 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0603504;\n\tFri, 17 Jan 2020 08:47:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579247226;\n\tbh=n5rdqFbz/fjEHd4/+EzZEQ9dfgyaBiv6ZVY+Go1aATA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BwVMEvrIee2b6wGgiQaVdRhUXan7WxDlh4yG6/SpIjWxtY3v5Qd2IGadYki5l8cl0\n\trvpZModXTWwPF1feWy0yZxH+iVfc+L6WoNfqijDXWJp5bjVyNuH9JGS6FmxMfWaeaE\n\tK+RZ5FLD8+HEeFp0Vj41qR9sZOesGr0dhEa3Ft48=","Date":"Fri, 17 Jan 2020 09:46:52 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200117074652.GA5711@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t\n\t<20200116223130.GA32545@pendragon.ideasonboard.com>\n\t<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 Jan 2020 07:47:07 -0000"}},{"id":3473,"web_url":"https://patchwork.libcamera.org/comment/3473/","msgid":"<387744e700af491f204a1b1e1501e933cdeb1f3c.camel@ndufresne.ca>","date":"2020-01-17T13:50:56","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le vendredi 17 janvier 2020 à 09:46 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Thu, Jan 16, 2020 at 08:23:36PM -0500, Nicolas Dufresne wrote:\n> > Le vendredi 17 janvier 2020 à 00:31 +0200, Laurent Pinchart a écrit :\n> > > On Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> > > > On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> > > > > From: Nicolas Dufresne \n> > > > > \n> > > > > This adds support for pre-commit hook workflow. In pre-commit hook we\n> > > > > check the style on the changes currently staged. Note that this patch\n> > > > > also includes a bit of sugar to support --amend.\n> > > > > \n> > > > > Signed-off-by: Nicolas Dufresne \n> > > > > ---\n> > > > > utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> > > > > utils/hooks/pre-commit | 16 ++++++++++++\n> > > > > 2 files changed, 54 insertions(+), 17 deletions(-)\n> > > > > mode change 100644 => 100755 utils/checkstyle.py\n> > > > > create mode 100755 utils/hooks/pre-commit\n> > > > > \n> > > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > > > old mode 100644\n> > > > > new mode 100755\n> > > > > index 7edea25..23eb3f6\n> > > > > --- a/utils/checkstyle.py\n> > > > > +++ b/utils/checkstyle.py\n> > > > > @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> > > > > # Style checking\n> > > > > #\n> > > > > \n> > > > > -def check_file(top_level, commit, filename):\n> > > > > +def check_file(top_level, commit, filename, staged):\n> > > > > # Extract the line numbers touched by the commit.\n> > > > > - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > > > - '%s/%s' % (top_level, filename)],\n> > > > > - stdout=subprocess.PIPE).stdout\n> > > > \n> > > > I wonder if we could/should automate this by detecting staged changes,\n> > > > and then setting staged based on that?\n> > > > \n> > > > \t\"git status --porcelain | grep ^M\"\n> > > > \n> > > > Could detect if there are staged changes.\n> > > > \n> > > > But equally it might be better to allow the caller to be explicit.\n> > > \n> > > I've been wondering about this too, I have a slight preference for\n> > > auto-detection, but that's not a hard requirement if too difficult to\n> > > implement or impractical to use.\n> > \n> > I don't think it hard, but I wasn't sure it was a good idea. One thing\n> > that I've been a little slowpy is with the meaning of commit here (of\n> > the rev list in fact). Before I added amend support, I was simply\n> > ignoring that (no meaning). But then I recycled it to support the\n> > commit --amend case, where you want to included the staged changes and\n> > the previous commit.\n> > \n> > What I found worked to get that information was to only pass one hash,\n> > which is the commit before the last commit (HEAD~). Anyway, I think I\n> > should fix this, and so '%s~' % commit instead if I want to maintained\n> > the behaviour. But by doing that, there is no longer anything to\n> > describe that when I commit --amend, I want the combined report, not\n> > just what was staged. And that I also don't want the previous commit to\n> > be checked independently from the staged, because that would produce\n> > that warning I'm trying to fix.\n> > \n> > So my conclusion, is that to disambiguate all this, I should in fact be\n> > even more explicit, and also introduce --amend. A better implementation\n> > would be to find a way to integrated these special revisions into the\n> > revlist, this way (not sure if there is a use case) but you could\n> > request a report for specific commits, the staging changes and the\n> > amended changes in one call.\n> \n> That would be neat indeed. From a quick look at git-rev-parse there's no\n> revision syntax to express this that I could find. We could extend the\n> revision syntax, but that may be dangerous.\n\nI was thinking to keep the --staged/amend options, but to use mixed\ntype in the python list, and do type checks when iterating. That would\navoid the binary parameter, which is just bad API really.\n\n> \n> If we instead go for explicit arguments, --staged or --cached would be\n> fine with me, but I'd rather use --worktree (or something similar) than\n> --amend to reflect that the revision corresponds to the working tree.\n\nI believe worktree is a much worst choice as it matches a completely\nunrelated concept in git (see git worktree --help). The concept of an\namendment isn't defined, but can logically match the combination of the\nprevious commit and the staged changes, since this is what git commit \n--amend will combine to replace the last commit.\n\n> \n> > > > > + if staged:\n> > > > > + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> > > > > + '%s/%s' % (top_level, filename)],\n> > > > > + stdout=subprocess.PIPE).stdout\n> > > > > + else:\n> > > > > + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > > > + '%s/%s' % (top_level, filename)],\n> > > > > + stdout=subprocess.PIPE).stdout\n> > > > > diff = diff.decode('utf-8').splitlines(True)\n> > > > > commit_diff = parse_diff(diff)\n> > > > > \n> > > > > @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> > > > > \n> > > > > # Format the file after the commit with all formatters and compute the diff\n> > > > > # between the unformatted and formatted contents.\n> > > > > - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > > > - stdout=subprocess.PIPE).stdout\n> > > > > + if staged:\n> > > > > + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> > > > > + stdout=subprocess.PIPE).stdout\n> > > > > + else:\n> > > > > + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > > > + stdout=subprocess.PIPE).stdout\n> > > > > after = after.decode('utf-8')\n> > > > > \n> > > > > formatted = after\n> > > > > @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> > > > > return len(formatted_diff) + len(issues)\n> > > > > \n> > > > > \n> > > > > -def check_style(top_level, commit):\n> > > > > - # Get the commit title and list of files.\n> > > > > - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> > > > > - stdout=subprocess.PIPE)\n> > > > > - files = ret.stdout.decode('utf-8').splitlines()\n> > > > > - title = files[0]\n> > > > > - files = files[1:]\n> > > > > +def check_style(top_level, commit, staged):\n> > > > > + if staged:\n> > > > > + # Get list of staged changed files\n> > > > > + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> > > > > + stdout=subprocess.PIPE)\n> > > > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > > > + title = \"Pre-commit\"\n> > > > \n> > > > This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> > > > A user could run the tool directly to validate some staged changes\n> > > > before committing...\n> > > > \n> > > > So I'd perhaps make this\n> > > > \ttitle = \"Staged\"\n> > > \n> > > Or maybe \"Staged changes\" to be slightly more explicit ?\n> > \n> > It started \"Staged\", then I splitted it between amend and staged, and\n> > for some reason ended up like this. I think I'll split it again, and\n> > use \"Staged changes\" or \"Amended changes\".\n> \n> Maybe \"Work tree changes\" instead of \"Amended changes\" ?\n\nSee my comment above on why Worktree isn't a great choice. What I maybe\nsuggest, is to title this one after the previous commit. That would\nlook like \"Amend: \". The truth though is that nobody\nusing pre-commit will ever even look at the subject, so we are putting\na little too much effort in naming here.\n\n> \n> > > > > + else:\n> > > > > + # Get the commit title and list of files.\n> > > > > + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> > > > > + stdout=subprocess.PIPE)\n> > > > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > > > + title = files[0]\n> > > > > + files = files[1:]\n> > > > > \n> > > > > separator = '-' * len(title)\n> > > > > print(separator)\n> > > > > @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> > > > > files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> > > > > if len(files) == 0:\n> > > > > print(\"Commit doesn't touch source files, skipping\")\n> > > > > - return\n> > > > > + return 0\n> > > > > \n> > > > > issues = 0\n> > > > > for f in files:\n> > > > > - issues += check_file(top_level, commit, f)\n> > > > > + issues += check_file(top_level, commit, f, staged)\n> > > > > \n> > > > > if issues == 0:\n> > > > > print(\"No style issue detected\")\n> > > > > @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> > > > > print(\"%u potential style %s detected, please review\" % \\\n> > > > > (issues, 'issue' if issues == 1 else 'issues'))\n> > > > > \n> > > > > + return issues\n> > > > > +\n> > > > > \n> > > > > def extract_revlist(revs):\n> > > > > \"\"\"Extract a list of commits on which to operate from a revision or revision\n> > > > > @@ -595,6 +613,8 @@ def main(argv):\n> > > > > parser = argparse.ArgumentParser()\n> > > > > parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> > > > > help='Code formatter. Default to clang-format if not specified.')\n> > > > > + parser.add_argument('--staged', '-s', action='store_true',\n> > > > > + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> > > > > parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> > > > > help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> > > > > args = parser.parse_args(argv[1:])\n> > > > > @@ -632,11 +652,12 @@ def main(argv):\n> > > > > \n> > > > > revlist = extract_revlist(args.revision_range)\n> > > > > \n> > > > > + issues = 0\n> > > > > for commit in revlist:\n> > > > > - check_style(top_level, commit)\n> > > > > + issues += check_style(top_level, commit, args.staged)\n> > > > > print('')\n> > > > > \n> > > > > - return 0\n> > > > > + return issues\n> > > > \n> > > > I'd be tempted to split the\n> > > > \"checkstyle: Report issue count as a failure\"\n> > > > and\n> > > > \"checkstyle: Support validating staged git state\"\n> > > > \n> > > > to two patches, but perhaps that's not necessary unless you see benefit\n> > > > in splitting.\n> > > \n> > > I don't think there's a need to resubmit just for this, but if a v2 is\n> > > needed, it may be nice to split the changes indeed, to explain why\n> > > changing the return value is needed.\n> > \n> > I think a v2 is needed, so I'll do.\n> > \n> > > According to the exit man page,\n> > > \n> > > RATIONALE\n> > > As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n> > > \n> > > 126 A file to be executed was found, but it was not an executable utility.\n> > > \n> > > 127 A utility to be executed was not found.\n> > > \n> > > >128 A command was interrupted by a signal.\n> > > \n> > > And according to exit(),\n> > > \n> > > The value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n> > > \n> > > In the unfortunate event that a multiple of 256 issues would be found,\n> > > the caller would think everything went fine. Should we thus return 0 on\n> > > success and 1 if any issue was found ?\n> > \n> > Agreed, will turn the number of issues into 0 and 1.\n> > \n> > > > > if __name__ == '__main__':\n> > > > > diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> > > > > new file mode 100755\n> > > > > index 0000000..57d23ef\n> > > > > --- /dev/null\n> > > > > +++ b/utils/hooks/pre-commit\n> > > > \n> > > > Though I really think this deserves it's own commit.\n> > > > \n> > > > \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> > > > \n> > > > > @@ -0,0 +1,16 @@\n> > > > > +#!/bin/sh\n> > > > > +\n> > > > > +# Execute the checkstyle script before committing any code, failing the commit\n> > > > > +# if needed. With this workflow, false positive can be ignored using:\n> > > > > +# git commit -n\n> > > > > +#\n> > > > > +# To utilise this hook, install this file with:\n> > > > > +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> > > > \n> > > > One of the things I had toyed with is installing the hook by setting the\n> > > > git hook directory to utils/hooks/ :\n> > > > \n> > > > git config core.hooksPath utils/hooks\n> > > > \n> > > > But doing so now would invoke both the pre-commit and the post-commit hook.\n> > > > \n> > > > I think it's good to provide the option of how someone might install\n> > > > their hook\n> > > > \n> > > > > +\n> > > > > +commit=\n> > > > > +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> > > > > +then\n> > > > > + commit=\"HEAD~\"\n> > > > \n> > > > Is this really needed?\n> > > > \n> > > > Edit: Yes, I've just played around with it - and I see what's happening.\n> > > > Good catch to find that something extra was required here.\n> > > > \n> > > > > +fi\n> > > > > +\n> > > > > +./utils/checkstyle.py --staged $commit\n> > > > \n> > > > I wonder if the checkstyle.py could detect if we had staged changes if\n> > > > we could fall back to needing only a single variant of the hook which\n> > > > the user could choose to install as either a pre-commit or a post-commit\n> > > > hook.\n> > > > \n> > > > But perhaps it's more straight-forward to have two hooks to keep it\n> > > > easier to support any future differences too.\n> > > \n> > > I'm fine either way.\n> > \n> > I think having two files in hook/ directory will be less confusing \n> > even if they are identical in the end. But I don't think the auto\n> > detection is a good idea, it's makes the CLI of the tool ambiguous at\n> > least.","headers":{"Return-Path":"","Received":["from mail-qk1-x744.google.com (mail-qk1-x744.google.com\n\t[IPv6:2607:f8b0:4864:20::744])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AD5C60456\n\tfor ;\n\tFri, 17 Jan 2020 14:51:00 +0100 (CET)","by mail-qk1-x744.google.com with SMTP id 21so22737944qky.4\n\tfor ;\n\tFri, 17 Jan 2020 05:51:00 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\tr44sm13285489qta.26.2020.01.17.05.50.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 17 Jan 2020 05:50:57 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=muJR4osvtOuoveAnGW0a1oY3dvlZX+uTfxhFhi4Y+ZY=;\n\tb=zWwpOqwcbW3L1RDjIWZ6oUK1+hAjBPPouMKfwOAZ1c4ileAv0MIgzVAZKjdY3kY0Tf\n\tewqLEMFNZeNy0ORh/ENdO5MgO3yaQsGSzRFYROivgLaMeftPTmya6aFNby89y9P7+57m\n\tmfy1RD6p/oztnbKq6twNFtTrXnEdYWyPOJZXGdw5EEq8AfozdoEt4OzFD3KvUl/ifbyv\n\tS3dZyztGoV/e34YSwzvUunyVleDjiR045w+CRKkiVcbVdacCU0lYiye15EZSod0AHAiF\n\tAkjN2+gmd6HcX6riA/XFzvy7O+kuaTmvfbAJ4zSHnYKY1WJEkJUrqMMhgY/JI4tup0I2\n\tbAsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=muJR4osvtOuoveAnGW0a1oY3dvlZX+uTfxhFhi4Y+ZY=;\n\tb=h4Fr74cbLIP1oBqgyReCsflUSr98R3k50R64MgEIUx+Ct9JG9TtSHo7qeH+mmXNbpg\n\ttxnVtNNQenk653tVedIallJIWraaIU5Ehv++OAzxLcVbT3KpWuJfW+06Tfvn3cdrs4UY\n\tYSG6mcPuzo0PWfzsMT+Ze+0WTsWlBPZPWoN6oKKFuWqJeahPzuiXEFXbzN5zRXFsoR5O\n\tHR3FJXI6qhovzTyEbLbLeeFYhLlejzJhxWQLdcMGAwF4iBig9abAhxwzO1mT9PlTNtOU\n\tE0JNpBhWumenpFDSGitf6EPxaHt8CqdTjB0Se24S4ER04fwuHuUAomI4/pyAIX5IrLEv\n\t0XUg==","X-Gm-Message-State":"APjAAAXc8GNMal8aSda6V0ARF8gY/koWVV7cxUjThEU94YSIzEQZALdu\n\tVDJ9l8+EOoToQ8FpNooHiR2Iag==","X-Google-Smtp-Source":"APXvYqwRPH51WfJCEdG82oRXidWkmZcHm3VsdvPmse9lOSyqV5gKAt/0UuMGKwSnIRNqpFPxyHz1ZQ==","X-Received":"by 2002:a37:bcc7:: with SMTP id\n\tm190mr33902207qkf.103.1579269058507; \n\tFri, 17 Jan 2020 05:50:58 -0800 (PST)","Message-ID":"<387744e700af491f204a1b1e1501e933cdeb1f3c.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 17 Jan 2020 08:50:56 -0500","In-Reply-To":"<20200117074652.GA5711@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t\n\t<20200116223130.GA32545@pendragon.ideasonboard.com>\n\t<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>\n\t<20200117074652.GA5711@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.2 (3.34.2-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 Jan 2020 13:51:00 -0000"}},{"id":3475,"web_url":"https://patchwork.libcamera.org/comment/3475/","msgid":"<20200117145551.GH5711@pendragon.ideasonboard.com>","date":"2020-01-17T14:55:51","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Fri, Jan 17, 2020 at 08:50:56AM -0500, Nicolas Dufresne wrote:\n> Le vendredi 17 janvier 2020 à 09:46 +0200, Laurent Pinchart a écrit :\n> > On Thu, Jan 16, 2020 at 08:23:36PM -0500, Nicolas Dufresne wrote:\n> >> Le vendredi 17 janvier 2020 à 00:31 +0200, Laurent Pinchart a écrit :\n> >>> On Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> >>>> On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> >>>>> From: Nicolas Dufresne \n> >>>>> \n> >>>>> This adds support for pre-commit hook workflow. In pre-commit hook we\n> >>>>> check the style on the changes currently staged. Note that this patch\n> >>>>> also includes a bit of sugar to support --amend.\n> >>>>> \n> >>>>> Signed-off-by: Nicolas Dufresne \n> >>>>> ---\n> >>>>> utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> >>>>> utils/hooks/pre-commit | 16 ++++++++++++\n> >>>>> 2 files changed, 54 insertions(+), 17 deletions(-)\n> >>>>> mode change 100644 => 100755 utils/checkstyle.py\n> >>>>> create mode 100755 utils/hooks/pre-commit\n> >>>>> \n> >>>>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> >>>>> old mode 100644\n> >>>>> new mode 100755\n> >>>>> index 7edea25..23eb3f6\n> >>>>> --- a/utils/checkstyle.py\n> >>>>> +++ b/utils/checkstyle.py\n> >>>>> @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> >>>>> # Style checking\n> >>>>> #\n> >>>>> \n> >>>>> -def check_file(top_level, commit, filename):\n> >>>>> +def check_file(top_level, commit, filename, staged):\n> >>>>> # Extract the line numbers touched by the commit.\n> >>>>> - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> >>>>> - '%s/%s' % (top_level, filename)],\n> >>>>> - stdout=subprocess.PIPE).stdout\n> >>>> \n> >>>> I wonder if we could/should automate this by detecting staged changes,\n> >>>> and then setting staged based on that?\n> >>>> \n> >>>> \t\"git status --porcelain | grep ^M\"\n> >>>> \n> >>>> Could detect if there are staged changes.\n> >>>> \n> >>>> But equally it might be better to allow the caller to be explicit.\n> >>> \n> >>> I've been wondering about this too, I have a slight preference for\n> >>> auto-detection, but that's not a hard requirement if too difficult to\n> >>> implement or impractical to use.\n> >> \n> >> I don't think it hard, but I wasn't sure it was a good idea. One thing\n> >> that I've been a little slowpy is with the meaning of commit here (of\n> >> the rev list in fact). Before I added amend support, I was simply\n> >> ignoring that (no meaning). But then I recycled it to support the\n> >> commit --amend case, where you want to included the staged changes and\n> >> the previous commit.\n> >> \n> >> What I found worked to get that information was to only pass one hash,\n> >> which is the commit before the last commit (HEAD~). Anyway, I think I\n> >> should fix this, and so '%s~' % commit instead if I want to maintained\n> >> the behaviour. But by doing that, there is no longer anything to\n> >> describe that when I commit --amend, I want the combined report, not\n> >> just what was staged. And that I also don't want the previous commit to\n> >> be checked independently from the staged, because that would produce\n> >> that warning I'm trying to fix.\n> >> \n> >> So my conclusion, is that to disambiguate all this, I should in fact be\n> >> even more explicit, and also introduce --amend. A better implementation\n> >> would be to find a way to integrated these special revisions into the\n> >> revlist, this way (not sure if there is a use case) but you could\n> >> request a report for specific commits, the staging changes and the\n> >> amended changes in one call.\n> > \n> > That would be neat indeed. From a quick look at git-rev-parse there's no\n> > revision syntax to express this that I could find. We could extend the\n> > revision syntax, but that may be dangerous.\n> \n> I was thinking to keep the --staged/amend options, but to use mixed\n> type in the python list, and do type checks when iterating. That would\n> avoid the binary parameter, which is just bad API really.\n> \n> > If we instead go for explicit arguments, --staged or --cached would be\n> > fine with me, but I'd rather use --worktree (or something similar) than\n> > --amend to reflect that the revision corresponds to the working tree.\n> \n> I believe worktree is a much worst choice as it matches a completely\n> unrelated concept in git (see git worktree --help). The concept of an\n> amendment isn't defined, but can logically match the combination of the\n> previous commit and the staged changes, since this is what git commit \n> --amend will combine to replace the last commit.\n\n\"Working tree\" had an established meaning before worktree was introduced\n:-) That's how git-diff is documented:\n\n\"Show changes between the working tree and the index or a tree, changes\nbetween the index and a tree, changes between two trees, changes between\ntwo blob objects, or changes between two files on disk.\"\n\n--working-tree could work too but is a bit long, although if it's only\nused by the pre-commit hook (and/or if we add a short -w option) it\nshould work.\n\nIt would be really nice if git had a reference specifier for the index\nin the working tree, we could then do\n\ncheckstyle.py HEAD..INDEX\ncheckstyle.py HEAD..WORK\n\n(or similar). Unless I'm mistaken, the pre-commit hook would then use\n\ncheckstyle.py HEAD..INDEX\n\nfor the normal git commit case, and\n\ncheckstyle.py HEAD~..INDEX\n\nfor the --amend case. This would have my preference. And now that I've\nwritten this, --worktree seems a bad name indeed if it's an alias for\nHEAD~ on the left-hand side. I proposed it thinking it would be an alias\nfor ..WORK, but that doesn't seem needed for the pre-commit\nhook.\n\n> >>>>> + if staged:\n> >>>>> + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> >>>>> + '%s/%s' % (top_level, filename)],\n> >>>>> + stdout=subprocess.PIPE).stdout\n> >>>>> + else:\n> >>>>> + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> >>>>> + '%s/%s' % (top_level, filename)],\n> >>>>> + stdout=subprocess.PIPE).stdout\n> >>>>> diff = diff.decode('utf-8').splitlines(True)\n> >>>>> commit_diff = parse_diff(diff)\n> >>>>> \n> >>>>> @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> >>>>> \n> >>>>> # Format the file after the commit with all formatters and compute the diff\n> >>>>> # between the unformatted and formatted contents.\n> >>>>> - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> >>>>> - stdout=subprocess.PIPE).stdout\n> >>>>> + if staged:\n> >>>>> + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> >>>>> + stdout=subprocess.PIPE).stdout\n> >>>>> + else:\n> >>>>> + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> >>>>> + stdout=subprocess.PIPE).stdout\n> >>>>> after = after.decode('utf-8')\n> >>>>> \n> >>>>> formatted = after\n> >>>>> @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> >>>>> return len(formatted_diff) + len(issues)\n> >>>>> \n> >>>>> \n> >>>>> -def check_style(top_level, commit):\n> >>>>> - # Get the commit title and list of files.\n> >>>>> - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> >>>>> - stdout=subprocess.PIPE)\n> >>>>> - files = ret.stdout.decode('utf-8').splitlines()\n> >>>>> - title = files[0]\n> >>>>> - files = files[1:]\n> >>>>> +def check_style(top_level, commit, staged):\n> >>>>> + if staged:\n> >>>>> + # Get list of staged changed files\n> >>>>> + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> >>>>> + stdout=subprocess.PIPE)\n> >>>>> + files = ret.stdout.decode('utf-8').splitlines()\n> >>>>> + title = \"Pre-commit\"\n> >>>> \n> >>>> This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> >>>> A user could run the tool directly to validate some staged changes\n> >>>> before committing...\n> >>>> \n> >>>> So I'd perhaps make this\n> >>>> \ttitle = \"Staged\"\n> >>> \n> >>> Or maybe \"Staged changes\" to be slightly more explicit ?\n> >> \n> >> It started \"Staged\", then I splitted it between amend and staged, and\n> >> for some reason ended up like this. I think I'll split it again, and\n> >> use \"Staged changes\" or \"Amended changes\".\n> > \n> > Maybe \"Work tree changes\" instead of \"Amended changes\" ?\n> \n> See my comment above on why Worktree isn't a great choice. What I maybe\n> suggest, is to title this one after the previous commit. That would\n> look like \"Amend: \". The truth though is that nobody\n> using pre-commit will ever even look at the subject, so we are putting\n> a little too much effort in naming here.\n> \n> >>>>> + else:\n> >>>>> + # Get the commit title and list of files.\n> >>>>> + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> >>>>> + stdout=subprocess.PIPE)\n> >>>>> + files = ret.stdout.decode('utf-8').splitlines()\n> >>>>> + title = files[0]\n> >>>>> + files = files[1:]\n> >>>>> \n> >>>>> separator = '-' * len(title)\n> >>>>> print(separator)\n> >>>>> @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> >>>>> files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> >>>>> if len(files) == 0:\n> >>>>> print(\"Commit doesn't touch source files, skipping\")\n> >>>>> - return\n> >>>>> + return 0\n> >>>>> \n> >>>>> issues = 0\n> >>>>> for f in files:\n> >>>>> - issues += check_file(top_level, commit, f)\n> >>>>> + issues += check_file(top_level, commit, f, staged)\n> >>>>> \n> >>>>> if issues == 0:\n> >>>>> print(\"No style issue detected\")\n> >>>>> @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> >>>>> print(\"%u potential style %s detected, please review\" % \\\n> >>>>> (issues, 'issue' if issues == 1 else 'issues'))\n> >>>>> \n> >>>>> + return issues\n> >>>>> +\n> >>>>> \n> >>>>> def extract_revlist(revs):\n> >>>>> \"\"\"Extract a list of commits on which to operate from a revision or revision\n> >>>>> @@ -595,6 +613,8 @@ def main(argv):\n> >>>>> parser = argparse.ArgumentParser()\n> >>>>> parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> >>>>> help='Code formatter. Default to clang-format if not specified.')\n> >>>>> + parser.add_argument('--staged', '-s', action='store_true',\n> >>>>> + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> >>>>> parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> >>>>> help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> >>>>> args = parser.parse_args(argv[1:])\n> >>>>> @@ -632,11 +652,12 @@ def main(argv):\n> >>>>> \n> >>>>> revlist = extract_revlist(args.revision_range)\n> >>>>> \n> >>>>> + issues = 0\n> >>>>> for commit in revlist:\n> >>>>> - check_style(top_level, commit)\n> >>>>> + issues += check_style(top_level, commit, args.staged)\n> >>>>> print('')\n> >>>>> \n> >>>>> - return 0\n> >>>>> + return issues\n> >>>> \n> >>>> I'd be tempted to split the\n> >>>> \"checkstyle: Report issue count as a failure\"\n> >>>> and\n> >>>> \"checkstyle: Support validating staged git state\"\n> >>>> \n> >>>> to two patches, but perhaps that's not necessary unless you see benefit\n> >>>> in splitting.\n> >>> \n> >>> I don't think there's a need to resubmit just for this, but if a v2 is\n> >>> needed, it may be nice to split the changes indeed, to explain why\n> >>> changing the return value is needed.\n> >> \n> >> I think a v2 is needed, so I'll do.\n> >> \n> >>> According to the exit man page,\n> >>> \n> >>> RATIONALE\n> >>> As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n> >>> \n> >>> 126 A file to be executed was found, but it was not an executable utility.\n> >>> \n> >>> 127 A utility to be executed was not found.\n> >>> \n> >>> >128 A command was interrupted by a signal.\n> >>> \n> >>> And according to exit(),\n> >>> \n> >>> The value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n> >>> \n> >>> In the unfortunate event that a multiple of 256 issues would be found,\n> >>> the caller would think everything went fine. Should we thus return 0 on\n> >>> success and 1 if any issue was found ?\n> >> \n> >> Agreed, will turn the number of issues into 0 and 1.\n> >> \n> >>>>> if __name__ == '__main__':\n> >>>>> diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> >>>>> new file mode 100755\n> >>>>> index 0000000..57d23ef\n> >>>>> --- /dev/null\n> >>>>> +++ b/utils/hooks/pre-commit\n> >>>> \n> >>>> Though I really think this deserves it's own commit.\n> >>>> \n> >>>> \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> >>>> \n> >>>>> @@ -0,0 +1,16 @@\n> >>>>> +#!/bin/sh\n> >>>>> +\n> >>>>> +# Execute the checkstyle script before committing any code, failing the commit\n> >>>>> +# if needed. With this workflow, false positive can be ignored using:\n> >>>>> +# git commit -n\n> >>>>> +#\n> >>>>> +# To utilise this hook, install this file with:\n> >>>>> +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> >>>> \n> >>>> One of the things I had toyed with is installing the hook by setting the\n> >>>> git hook directory to utils/hooks/ :\n> >>>> \n> >>>> git config core.hooksPath utils/hooks\n> >>>> \n> >>>> But doing so now would invoke both the pre-commit and the post-commit hook.\n> >>>> \n> >>>> I think it's good to provide the option of how someone might install\n> >>>> their hook\n> >>>> \n> >>>>> +\n> >>>>> +commit=\n> >>>>> +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> >>>>> +then\n> >>>>> + commit=\"HEAD~\"\n> >>>> \n> >>>> Is this really needed?\n> >>>> \n> >>>> Edit: Yes, I've just played around with it - and I see what's happening.\n> >>>> Good catch to find that something extra was required here.\n> >>>> \n> >>>>> +fi\n> >>>>> +\n> >>>>> +./utils/checkstyle.py --staged $commit\n> >>>> \n> >>>> I wonder if the checkstyle.py could detect if we had staged changes if\n> >>>> we could fall back to needing only a single variant of the hook which\n> >>>> the user could choose to install as either a pre-commit or a post-commit\n> >>>> hook.\n> >>>> \n> >>>> But perhaps it's more straight-forward to have two hooks to keep it\n> >>>> easier to support any future differences too.\n> >>> \n> >>> I'm fine either way.\n> >> \n> >> I think having two files in hook/ directory will be less confusing \n> >> even if they are identical in the end. But I don't think the auto\n> >> detection is a good idea, it's makes the CLI of the tool ambiguous at\n> >> least.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E927960456\n\tfor ;\n\tFri, 17 Jan 2020 15:56:06 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 556249A1;\n\tFri, 17 Jan 2020 15:56:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579272966;\n\tbh=W3vHIlV8RHVd3Xu0xt/I6P+ebbzrByymmuf8CupBrz0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=niBDPQdq+w4R/RAl7mIhPtDgfYEmvJrTUAj2pIwGKWBxRHVqCcab1qx/R0I+z0NbN\n\tnQ7TkwZ6Lw8PJ4xuscb1/ONZVGKgCN3v3FnJKzifNaERo3VD1OwhwyeTOfjS9wBzZ7\n\t4tbIP8LK4X3/aCl8QRvEFl18NVvwQNLvm7H86Cwo=","Date":"Fri, 17 Jan 2020 16:55:51 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200117145551.GH5711@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t\n\t<20200116223130.GA32545@pendragon.ideasonboard.com>\n\t<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>\n\t<20200117074652.GA5711@pendragon.ideasonboard.com>\n\t<387744e700af491f204a1b1e1501e933cdeb1f3c.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<387744e700af491f204a1b1e1501e933cdeb1f3c.camel@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 Jan 2020 14:56:07 -0000"}},{"id":3479,"web_url":"https://patchwork.libcamera.org/comment/3479/","msgid":"","date":"2020-01-17T15:55:38","subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le vendredi 17 janvier 2020 à 16:55 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Fri, Jan 17, 2020 at 08:50:56AM -0500, Nicolas Dufresne wrote:\n> > Le vendredi 17 janvier 2020 à 09:46 +0200, Laurent Pinchart a écrit :\n> > > On Thu, Jan 16, 2020 at 08:23:36PM -0500, Nicolas Dufresne wrote:\n> > > > Le vendredi 17 janvier 2020 à 00:31 +0200, Laurent Pinchart a écrit :\n> > > > > On Thu, Jan 16, 2020 at 10:25:31PM +0000, Kieran Bingham wrote:\n> > > > > > On 16/01/2020 18:26, nicolas@ndufresne.ca wrote:\n> > > > > > > From: Nicolas Dufresne \n> > > > > > > \n> > > > > > > This adds support for pre-commit hook workflow. In pre-commit hook we\n> > > > > > > check the style on the changes currently staged. Note that this patch\n> > > > > > > also includes a bit of sugar to support --amend.\n> > > > > > > \n> > > > > > > Signed-off-by: Nicolas Dufresne \n> > > > > > > ---\n> > > > > > > utils/checkstyle.py | 55 +++++++++++++++++++++++++++++-------------\n> > > > > > > utils/hooks/pre-commit | 16 ++++++++++++\n> > > > > > > 2 files changed, 54 insertions(+), 17 deletions(-)\n> > > > > > > mode change 100644 => 100755 utils/checkstyle.py\n> > > > > > > create mode 100755 utils/hooks/pre-commit\n> > > > > > > \n> > > > > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > > > > > old mode 100644\n> > > > > > > new mode 100755\n> > > > > > > index 7edea25..23eb3f6\n> > > > > > > --- a/utils/checkstyle.py\n> > > > > > > +++ b/utils/checkstyle.py\n> > > > > > > @@ -458,11 +458,16 @@ class StripTrailingSpaceFormatter(Formatter):\n> > > > > > > # Style checking\n> > > > > > > #\n> > > > > > > \n> > > > > > > -def check_file(top_level, commit, filename):\n> > > > > > > +def check_file(top_level, commit, filename, staged):\n> > > > > > > # Extract the line numbers touched by the commit.\n> > > > > > > - diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > > > > > - '%s/%s' % (top_level, filename)],\n> > > > > > > - stdout=subprocess.PIPE).stdout\n> > > > > > \n> > > > > > I wonder if we could/should automate this by detecting staged changes,\n> > > > > > and then setting staged based on that?\n> > > > > > \n> > > > > > \t\"git status --porcelain | grep ^M\"\n> > > > > > \n> > > > > > Could detect if there are staged changes.\n> > > > > > \n> > > > > > But equally it might be better to allow the caller to be explicit.\n> > > > > \n> > > > > I've been wondering about this too, I have a slight preference for\n> > > > > auto-detection, but that's not a hard requirement if too difficult to\n> > > > > implement or impractical to use.\n> > > > \n> > > > I don't think it hard, but I wasn't sure it was a good idea. One thing\n> > > > that I've been a little slowpy is with the meaning of commit here (of\n> > > > the rev list in fact). Before I added amend support, I was simply\n> > > > ignoring that (no meaning). But then I recycled it to support the\n> > > > commit --amend case, where you want to included the staged changes and\n> > > > the previous commit.\n> > > > \n> > > > What I found worked to get that information was to only pass one hash,\n> > > > which is the commit before the last commit (HEAD~). Anyway, I think I\n> > > > should fix this, and so '%s~' % commit instead if I want to maintained\n> > > > the behaviour. But by doing that, there is no longer anything to\n> > > > describe that when I commit --amend, I want the combined report, not\n> > > > just what was staged. And that I also don't want the previous commit to\n> > > > be checked independently from the staged, because that would produce\n> > > > that warning I'm trying to fix.\n> > > > \n> > > > So my conclusion, is that to disambiguate all this, I should in fact be\n> > > > even more explicit, and also introduce --amend. A better implementation\n> > > > would be to find a way to integrated these special revisions into the\n> > > > revlist, this way (not sure if there is a use case) but you could\n> > > > request a report for specific commits, the staging changes and the\n> > > > amended changes in one call.\n> > > \n> > > That would be neat indeed. From a quick look at git-rev-parse there's no\n> > > revision syntax to express this that I could find. We could extend the\n> > > revision syntax, but that may be dangerous.\n> > \n> > I was thinking to keep the --staged/amend options, but to use mixed\n> > type in the python list, and do type checks when iterating. That would\n> > avoid the binary parameter, which is just bad API really.\n> > \n> > > If we instead go for explicit arguments, --staged or --cached would be\n> > > fine with me, but I'd rather use --worktree (or something similar) than\n> > > --amend to reflect that the revision corresponds to the working tree.\n> > \n> > I believe worktree is a much worst choice as it matches a completely\n> > unrelated concept in git (see git worktree --help). The concept of an\n> > amendment isn't defined, but can logically match the combination of the\n> > previous commit and the staged changes, since this is what git commit \n> > --amend will combine to replace the last commit.\n> \n> \"Working tree\" had an established meaning before worktree was introduced\n> :-) That's how git-diff is documented:\n> \n> \"Show changes between the working tree and the index or a tree, changes\n> between the index and a tree, changes between two trees, changes between\n> two blob objects, or changes between two files on disk.\"\n> \n> --working-tree could work too but is a bit long, although if it's only\n> used by the pre-commit hook (and/or if we add a short -w option) it\n> should work.\n\nI believe you didn't interpret this correctly. The working tree is the\ncode as seen on your file system, the index is the changes that are to\nbe commited. What I check is the index (pre-commit changes), not he\nworking tree.\n\nRight now, I'm just paying for git's mistake. What I want to deal with\nis the index, which in git-diff is shown through --cached or --staged\ncommand line option for unknown reasons. Looking at the changes in the\nworking tree to totally irrelevant to a pre-commit hook, but could be\nan interesting feature to add if you feel it's useful to you.\n\nBut here, my goal is to add support for pre-commit hook, which is a\ncheck run before you fill in the message. So I need to pick what git is\nabout to commit. In the case of commit, that's just the index, and for\ncommit --amend, it's the combination of the last commit and the index.\n\n> \n> It would be really nice if git had a reference specifier for the index\n> in the working tree, we could then do\n> \n> checkstyle.py HEAD..INDEX\n> checkstyle.py HEAD..WORK\n> \n> (or similar). Unless I'm mistaken, the pre-commit hook would then use\n> \n> checkstyle.py HEAD..INDEX\n> \n> for the normal git commit case, and\n> \n> checkstyle.py HEAD~..INDEX\n> \n> for the --amend case. This would have my preference. And now that I've\n> written this, --worktree seems a bad name indeed if it's an alias for\n> HEAD~ on the left-hand side. I proposed it thinking it would be an alias\n> for ..WORK, but that doesn't seem needed for the pre-commit\n> hook.\n> \n> > > > > > > + if staged:\n> > > > > > > + diff = subprocess.run(['git', 'diff', '--staged', commit, '--',\n> > > > > > > + '%s/%s' % (top_level, filename)],\n> > > > > > > + stdout=subprocess.PIPE).stdout\n> > > > > > > + else:\n> > > > > > > + diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',\n> > > > > > > + '%s/%s' % (top_level, filename)],\n> > > > > > > + stdout=subprocess.PIPE).stdout\n> > > > > > > diff = diff.decode('utf-8').splitlines(True)\n> > > > > > > commit_diff = parse_diff(diff)\n> > > > > > > \n> > > > > > > @@ -476,8 +481,12 @@ def check_file(top_level, commit, filename):\n> > > > > > > \n> > > > > > > # Format the file after the commit with all formatters and compute the diff\n> > > > > > > # between the unformatted and formatted contents.\n> > > > > > > - after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > > > > > - stdout=subprocess.PIPE).stdout\n> > > > > > > + if staged:\n> > > > > > > + after = subprocess.run(['git', 'show', ':%s' % (filename)],\n> > > > > > > + stdout=subprocess.PIPE).stdout\n> > > > > > > + else:\n> > > > > > > + after = subprocess.run(['git', 'show', '%s:%s' % (commit, filename)],\n> > > > > > > + stdout=subprocess.PIPE).stdout\n> > > > > > > after = after.decode('utf-8')\n> > > > > > > \n> > > > > > > formatted = after\n> > > > > > > @@ -521,13 +530,20 @@ def check_file(top_level, commit, filename):\n> > > > > > > return len(formatted_diff) + len(issues)\n> > > > > > > \n> > > > > > > \n> > > > > > > -def check_style(top_level, commit):\n> > > > > > > - # Get the commit title and list of files.\n> > > > > > > - ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],\n> > > > > > > - stdout=subprocess.PIPE)\n> > > > > > > - files = ret.stdout.decode('utf-8').splitlines()\n> > > > > > > - title = files[0]\n> > > > > > > - files = files[1:]\n> > > > > > > +def check_style(top_level, commit, staged):\n> > > > > > > + if staged:\n> > > > > > > + # Get list of staged changed files\n> > > > > > > + ret = subprocess.run(['git', 'diff', '--staged', '--name-only', commit],\n> > > > > > > + stdout=subprocess.PIPE)\n> > > > > > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > > > > > + title = \"Pre-commit\"\n> > > > > > \n> > > > > > This is not necessarily a \"Pre-commit\" (hook). It's just staged.\n> > > > > > A user could run the tool directly to validate some staged changes\n> > > > > > before committing...\n> > > > > > \n> > > > > > So I'd perhaps make this\n> > > > > > \ttitle = \"Staged\"\n> > > > > \n> > > > > Or maybe \"Staged changes\" to be slightly more explicit ?\n> > > > \n> > > > It started \"Staged\", then I splitted it between amend and staged, and\n> > > > for some reason ended up like this. I think I'll split it again, and\n> > > > use \"Staged changes\" or \"Amended changes\".\n> > > \n> > > Maybe \"Work tree changes\" instead of \"Amended changes\" ?\n> > \n> > See my comment above on why Worktree isn't a great choice. What I maybe\n> > suggest, is to title this one after the previous commit. That would\n> > look like \"Amend: \". The truth though is that nobody\n> > using pre-commit will ever even look at the subject, so we are putting\n> > a little too much effort in naming here.\n> > \n> > > > > > > + else:\n> > > > > > > + # Get the commit title and list of files.\n> > > > > > > + ret = subprocess.run(['git', 'show', '--pretty=oneline', '--name-only', commit],\n> > > > > > > + stdout=subprocess.PIPE)\n> > > > > > > + files = ret.stdout.decode('utf-8').splitlines()\n> > > > > > > + title = files[0]\n> > > > > > > + files = files[1:]\n> > > > > > > \n> > > > > > > separator = '-' * len(title)\n> > > > > > > print(separator)\n> > > > > > > @@ -541,11 +557,11 @@ def check_style(top_level, commit):\n> > > > > > > files = [f for f in files if len([p for p in patterns if fnmatch.fnmatch(os.path.basename(f), p)])]\n> > > > > > > if len(files) == 0:\n> > > > > > > print(\"Commit doesn't touch source files, skipping\")\n> > > > > > > - return\n> > > > > > > + return 0\n> > > > > > > \n> > > > > > > issues = 0\n> > > > > > > for f in files:\n> > > > > > > - issues += check_file(top_level, commit, f)\n> > > > > > > + issues += check_file(top_level, commit, f, staged)\n> > > > > > > \n> > > > > > > if issues == 0:\n> > > > > > > print(\"No style issue detected\")\n> > > > > > > @@ -554,6 +570,8 @@ def check_style(top_level, commit):\n> > > > > > > print(\"%u potential style %s detected, please review\" % \\\n> > > > > > > (issues, 'issue' if issues == 1 else 'issues'))\n> > > > > > > \n> > > > > > > + return issues\n> > > > > > > +\n> > > > > > > \n> > > > > > > def extract_revlist(revs):\n> > > > > > > \"\"\"Extract a list of commits on which to operate from a revision or revision\n> > > > > > > @@ -595,6 +613,8 @@ def main(argv):\n> > > > > > > parser = argparse.ArgumentParser()\n> > > > > > > parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],\n> > > > > > > help='Code formatter. Default to clang-format if not specified.')\n> > > > > > > + parser.add_argument('--staged', '-s', action='store_true',\n> > > > > > > + help='Looks at the staged changes, used for pre-commit, defaults to False')\n> > > > > > > parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',\n> > > > > > > help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')\n> > > > > > > args = parser.parse_args(argv[1:])\n> > > > > > > @@ -632,11 +652,12 @@ def main(argv):\n> > > > > > > \n> > > > > > > revlist = extract_revlist(args.revision_range)\n> > > > > > > \n> > > > > > > + issues = 0\n> > > > > > > for commit in revlist:\n> > > > > > > - check_style(top_level, commit)\n> > > > > > > + issues += check_style(top_level, commit, args.staged)\n> > > > > > > print('')\n> > > > > > > \n> > > > > > > - return 0\n> > > > > > > + return issues\n> > > > > > \n> > > > > > I'd be tempted to split the\n> > > > > > \"checkstyle: Report issue count as a failure\"\n> > > > > > and\n> > > > > > \"checkstyle: Support validating staged git state\"\n> > > > > > \n> > > > > > to two patches, but perhaps that's not necessary unless you see benefit\n> > > > > > in splitting.\n> > > > > \n> > > > > I don't think there's a need to resubmit just for this, but if a v2 is\n> > > > > needed, it may be nice to split the changes indeed, to explain why\n> > > > > changing the return value is needed.\n> > > > \n> > > > I think a v2 is needed, so I'll do.\n> > > > \n> > > > > According to the exit man page,\n> > > > > \n> > > > > RATIONALE\n> > > > > As explained in other sections, certain exit status values have been reserved for special uses and should be used by applications only for those purposes:\n> > > > > \n> > > > > 126 A file to be executed was found, but it was not an executable utility.\n> > > > > \n> > > > > 127 A utility to be executed was not found.\n> > > > > \n> > > > > >128 A command was interrupted by a signal.\n> > > > > \n> > > > > And according to exit(),\n> > > > > \n> > > > > The value status & 0377 is returned to the parent process as the process's exit status, and can be collected using one of the wait(2) family of calls.\n> > > > > \n> > > > > In the unfortunate event that a multiple of 256 issues would be found,\n> > > > > the caller would think everything went fine. Should we thus return 0 on\n> > > > > success and 1 if any issue was found ?\n> > > > \n> > > > Agreed, will turn the number of issues into 0 and 1.\n> > > > \n> > > > > > > if __name__ == '__main__':\n> > > > > > > diff --git a/utils/hooks/pre-commit b/utils/hooks/pre-commit\n> > > > > > > new file mode 100755\n> > > > > > > index 0000000..57d23ef\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/utils/hooks/pre-commit\n> > > > > > \n> > > > > > Though I really think this deserves it's own commit.\n> > > > > > \n> > > > > > \"utils/hooks: Provide a pre-commit checkstyle hook\"\n> > > > > > \n> > > > > > > @@ -0,0 +1,16 @@\n> > > > > > > +#!/bin/sh\n> > > > > > > +\n> > > > > > > +# Execute the checkstyle script before committing any code, failing the commit\n> > > > > > > +# if needed. With this workflow, false positive can be ignored using:\n> > > > > > > +# git commit -n\n> > > > > > > +#\n> > > > > > > +# To utilise this hook, install this file with:\n> > > > > > > +# cp utils/hooks/pre-commit .git/hooks/pre-commit\n> > > > > > \n> > > > > > One of the things I had toyed with is installing the hook by setting the\n> > > > > > git hook directory to utils/hooks/ :\n> > > > > > \n> > > > > > git config core.hooksPath utils/hooks\n> > > > > > \n> > > > > > But doing so now would invoke both the pre-commit and the post-commit hook.\n> > > > > > \n> > > > > > I think it's good to provide the option of how someone might install\n> > > > > > their hook\n> > > > > > \n> > > > > > > +\n> > > > > > > +commit=\n> > > > > > > +if ps -ocommand= -p $PPID | grep -- \"--amend\"\n> > > > > > > +then\n> > > > > > > + commit=\"HEAD~\"\n> > > > > > \n> > > > > > Is this really needed?\n> > > > > > \n> > > > > > Edit: Yes, I've just played around with it - and I see what's happening.\n> > > > > > Good catch to find that something extra was required here.\n> > > > > > \n> > > > > > > +fi\n> > > > > > > +\n> > > > > > > +./utils/checkstyle.py --staged $commit\n> > > > > > \n> > > > > > I wonder if the checkstyle.py could detect if we had staged changes if\n> > > > > > we could fall back to needing only a single variant of the hook which\n> > > > > > the user could choose to install as either a pre-commit or a post-commit\n> > > > > > hook.\n> > > > > > \n> > > > > > But perhaps it's more straight-forward to have two hooks to keep it\n> > > > > > easier to support any future differences too.\n> > > > > \n> > > > > I'm fine either way.\n> > > > \n> > > > I think having two files in hook/ directory will be less confusing \n> > > > even if they are identical in the end. But I don't think the auto\n> > > > detection is a good idea, it's makes the CLI of the tool ambiguous at\n> > > > least.","headers":{"Return-Path":"","Received":["from mail-qk1-x743.google.com (mail-qk1-x743.google.com\n\t[IPv6:2607:f8b0:4864:20::743])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5DD460456\n\tfor ;\n\tFri, 17 Jan 2020 16:55:41 +0100 (CET)","by mail-qk1-x743.google.com with SMTP id 21so23149613qky.4\n\tfor ;\n\tFri, 17 Jan 2020 07:55:41 -0800 (PST)","from nicolas-tpx395.localdomain ([2610:98:8005::127])\n\tby smtp.gmail.com with ESMTPSA id\n\tv7sm13534149qtk.89.2020.01.17.07.55.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 17 Jan 2020 07:55:39 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=ldxIF1EPcBYapHvTkpI1tmsZ2YePSIGOwTC0raqD1XE=;\n\tb=cM8HBxAjF4WwHgj1BqnmP6tkjDhxt94P9tZURXFpiLOZ7D7qT6In7DQnf9ScZUIG+d\n\t3qx5s1SkOZdlTuPtur6uDtUb19iFkAXln7YtvqzrITzPSqUhjES5sDvTcbw+2UjdcSqd\n\t9BhgwEp66Xuf0TpwiW7rvnxM6PU+wMheAZ39lRviyP0hz5u/6lPVcOu76CijyhgL/baL\n\tFJx5fA0miYObSho8dYKk1qo7eBZ/259rDUCiFzU058U3tDjOU8AFCCk2hv5dhP9LA9Qu\n\tyiwrKC6XFB/MAqMgYsNQHq45r6cnQ48XCYvpEdokVQqGYhZRET8oZ7aHZBkYqrecfwQA\n\toKuQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=ldxIF1EPcBYapHvTkpI1tmsZ2YePSIGOwTC0raqD1XE=;\n\tb=dzBDzjssGl/v5ipaLaw7+UXebpb5WkrWLni1EknodiLwskNbg1ZGyKTkgE5NVCvv+7\n\tMATF+aMUh/kP9wQU/x5POBVJeTMOztLv+3TzOwX5NC1dMCeMPrMdQAHu+mlHqyOvENmJ\n\t3QhYcRx1CZ434NsAvZJM080MT788nVVY8kWtj226K8XwifSs+OVibVhNCI5voEO8xtg1\n\tjzU3lOie03KnsSOijvwYZ4eEvmkce5TFJtupvu91yxL3UuISZ3FnGE1AfVWpvreBzG+c\n\tQauHbJk3n4agjCrNbkeU5m/3zODVY/bBW+ebVZLHLL6WPbxa4e9KwTD0CVLJPi2hEkjx\n\tNJ9w==","X-Gm-Message-State":"APjAAAWxr1McItx4NyZBYpXwgEbwWnS9wVetL7OczXiG3Jm2BlEqGHKD\n\tJBGdbu8HQ0/m+SXGGE6Fa6XjxterCtY=","X-Google-Smtp-Source":"APXvYqzT5R9VvN6kcGpytjr9BK3ZIMxzzWGELX8Nkb70DeKdAg7y2RMN80ACSzpgdc7H6tXhXNDCKw==","X-Received":"by 2002:a37:8ac4:: with SMTP id\n\tm187mr34242863qkd.277.1579276540242; \n\tFri, 17 Jan 2020 07:55:40 -0800 (PST)","Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 17 Jan 2020 10:55:38 -0500","In-Reply-To":"<20200117145551.GH5711@pendragon.ideasonboard.com>","References":"<20200116182603.108966-1-nicolas@ndufresne.ca>\n\t<20200116182603.108966-3-nicolas@ndufresne.ca>\n\t\n\t<20200116223130.GA32545@pendragon.ideasonboard.com>\n\t<13b6d74d24d022e06e266d1d5497853555ef9da4.camel@ndufresne.ca>\n\t<20200117074652.GA5711@pendragon.ideasonboard.com>\n\t<387744e700af491f204a1b1e1501e933cdeb1f3c.camel@ndufresne.ca>\n\t<20200117145551.GH5711@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] checkstyle: Add support for\n\tpre-commit hook","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 17 Jan 2020 15:55:41 -0000"}}]