[{"id":27486,"web_url":"https://patchwork.libcamera.org/comment/27486/","msgid":"<168856242834.4136131.8436031126245611937@Monstersaurus>","date":"2023-07-05T13:07:08","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2023-06-12 23:47:51)\n> The libcamera git history contains numerous examples of incorrect commit\n> message trailers due to invalid trailer types (e.g. Change-Id), typos\n> and other small issues. Those went unnoticed through reviews, which\n> shows that an automated checker is required.\n> \n> Add a trailers checker to checkstyle.py to catch invalid or malformed\n> trailers, with a set of supported trailers that match libcamera's commit\n> message practices. New trailer keys can easily be added later as new\n> needs arise.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> This reports a total of 42 issues through the project's history. 37 of\n> them should not be controversial:\n> \n> - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> and 'Reported-on')\n> \n> - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n> \n> - Typos in e-mail address (missing display name, missing space before\n> '<' and missing trailing '>')\n> \n> - Link in 'Fixes' trailer (should be a commit)\n> \n> - Too short commit ID in 'Fixes' trailer\n> \n> - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> space or extra ':' after commit ID, missing '\"' around commit subject)\n> \n> The five remaining issues may benefit from discussions:\n> \n> - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> trailer key commonly used in the kernel, but git..b both recommend\n> Co-authored-by. I'm sure which option would be best, so I haven't\n> included either for now.\n\nI think those can be figured out when we next need them then.\n\nI don't mind either, and the checkstyle won't prevent us using them -\njust highlights them for discussion anyway (which is good).\n\n\n> \n> - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> instance). 'Reported-by' usually has an e-mail address value, but we\n> have commonly used 'Coverity CID=' for issues reported by\n> Coverity. I've tentatively added support for this (feedback is\n> welcome), and one commit still got flagged as its 'Reported-by'\n> trailer has a space instead of an '=' after 'CID'.\n\nThis is fine with me - if it's defined in the checker we just use that\ngoing forwards.\n\nCoverity CID= looks good to me.\n\n\n> \n> - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> is to be able to identify users by name and e-mail address for\n> 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> trailers too. If someone *really* doesn't want their name included in\n> the git log when reporting an issue, we can simply omit the\n> 'Reported-by' trailer.\n\nAgreed,\n\n> \n> - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> instances). This should use a 'Link' trailer instead.\n\nLink sounds fine if it's a full link. Otherwise - we'll end up with\nBuildbot: Jenkins: Lava: ... \n\n\nThis looks good to me anyway, lets see how it runs in production!\n\n\nReviewed-by: Kieran Bingham \n--\nKieran\n\n\n> ---\n> utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 78 insertions(+), 2 deletions(-)\n> \n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index e68c874609bc..3558740d389d 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -210,13 +210,23 @@ class Commit:\n> \n> def _parse(self):\n> # Get the commit title and list of files.\n> - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> self.commit],\n> stdout=subprocess.PIPE).stdout.decode('utf-8')\n> lines = ret.splitlines()\n> - self._files = [CommitFile(f) for f in lines[1:] if f]\n> +\n> self._title = lines[0]\n> \n> + self._trailers = []\n> + for index in range(1, len(lines)):\n> + line = lines[index]\n> + if not line:\n> + break\n> +\n> + self._trailers.append(line)\n> +\n> + self._files = [CommitFile(f) for f in lines[index:] if f]\n> +\n> def files(self, filter='AMR'):\n> return [f.filename for f in self._files if f.status in filter]\n> \n> @@ -224,6 +234,10 @@ class Commit:\n> def title(self):\n> return self._title\n> \n> + @property\n> + def trailers(self):\n> + return self._trailers\n> +\n> def get_diff(self, top_level, filename):\n> diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> '--', '%s/%s' % (top_level, filename)],\n> @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> 'possible candidates are ' + candidates)]\n> \n> \n> +class TrailersChecker(CommitChecker):\n> + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> +\n> + coverity_regex = re.compile(r'Coverity CID=.*')\n> +\n> + # Simple e-mail address validator regex, with an additional trailing\n> + # comment. The complexity of a full RFC6531 validator isn't worth the\n> + # additional invalid addresses it would reject.\n> + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> +\n> + link_regex = re.compile(r'https?://.*')\n> +\n> + @staticmethod\n> + def validate_reported_by(value):\n> + if TrailersChecker.email_regex.fullmatch(value):\n> + return True\n> + if TrailersChecker.coverity_regex.fullmatch(value):\n> + return True\n> + return False\n> +\n> + known_trailers = {\n> + 'Acked-by': email_regex,\n> + 'Bug': link_regex,\n> + 'Fixes': commit_regex,\n> + 'Link': link_regex,\n> + 'Reported-by': validate_reported_by,\n> + 'Reviewed-by': email_regex,\n> + 'Signed-off-by': email_regex,\n> + 'Suggested-by': email_regex,\n> + 'Tested-by': email_regex,\n> + }\n> +\n> + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> +\n> + @classmethod\n> + def check(cls, commit, top_level):\n> + issues = []\n> +\n> + for trailer in commit.trailers:\n> + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> + if not match:\n> + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> +\n> + key, value = match.groups()\n> +\n> + validator = TrailersChecker.known_trailers.get(key)\n> + if not validator:\n> + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> + continue\n> +\n> + if isinstance(validator, re.Pattern):\n> + valid = bool(validator.fullmatch(value))\n> + else:\n> + valid = validator(value)\n> +\n> + if not valid:\n> + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> + continue\n> +\n> + return issues\n> +\n> +\n> # ------------------------------------------------------------------------------\n> # Style Checkers\n> #\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C173CBE175\n\tfor ;\n\tWed, 5 Jul 2023 13:07:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 323DC628BC;\n\tWed, 5 Jul 2023 15:07:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87ED761E37\n\tfor ;\n\tWed, 5 Jul 2023 15:07:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99164905;\n\tWed, 5 Jul 2023 15:06:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688562433;\n\tbh=Yffcr08x7SnWjU88w8oFrxJg4ekK0ZalloZw1FJCpR0=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=GFsSt7eA6H1xHt5JbdLpyQsDbKrMkJlYADCEUFmRtWn1g4VrgavyQV6G7U0Jfchs3\n\tEkRtxgKqxt3nRDXAFzHkmlIeqXZd0vk6RfJW+OjnN3T1k7Vbrjh02ERQfFHDU26eHO\n\tAo8R9U6555TpO127DXMDDQWII7KKPgZSvztggvFOPmW3hG438PGiaYByjZbPwmjUWo\n\tRaEBDqfPlbJM/jXcMrzY7BNqCaUDt8e+BirL7Cow2vENMJVg1MpOvA32meloPVL0ag\n\txyXStmB01rRxopuR4YVXbrCpkU4tdim8ix5J4A5vAztlK/q1ItncZzIV7Fqx0/g0zA\n\tT06hlBfrF4SaQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1688562386;\n\tbh=Yffcr08x7SnWjU88w8oFrxJg4ekK0ZalloZw1FJCpR0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=eD+uiF/wus84QhpX+v7P7z1lX7B3cWDZ/qA+TTPLUY+VmXA5hNb8scFVH2dGu8REL\n\tnk9cijQKcbyOauSX45rw7zUxUwplt2Yv47uPojq6IpBoqMm+Wu4dVMsWVKmafVV5Gt\n\t8n9/PG80bms/J6bY54PmK1v/lzgzOdq6OUoTDnfs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eD+uiF/w\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 05 Jul 2023 14:07:08 +0100","Message-ID":"<168856242834.4136131.8436031126245611937@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27515,"web_url":"https://patchwork.libcamera.org/comment/27515/","msgid":"","date":"2023-07-10T10:33:54","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nI've updated my tree and this change now seems to break my pre-commit hook:\n\nnaushir@work:~/libcamera/ $ git commit -a -s\n---------------\n Staged changes\n---------------\nTraceback (most recent call last):\n File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n1052, in \n sys.exit(main(sys.argv))\n File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n1042, in main\n issues += check_style(top_level, commit, args.checkers)\n File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n935, in check_style\n for issue in checker.check(commit, top_level):\n File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n479, in check\n for trailer in commit.trailers:\n File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n239, in trailers\n return self._trailers\nAttributeError: 'StagedChanges' object has no attribute '_trailers'.\nDid you mean: 'trailers'?\n\nI'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly\ntried debugging, but quickly found that I'm in way over my depth with the\ncheckstyle.py script :-)\n\nRegards,\nNaush\n\nOn Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel\n wrote:\n>\n> The libcamera git history contains numerous examples of incorrect commit\n> message trailers due to invalid trailer types (e.g. Change-Id), typos\n> and other small issues. Those went unnoticed through reviews, which\n> shows that an automated checker is required.\n>\n> Add a trailers checker to checkstyle.py to catch invalid or malformed\n> trailers, with a set of supported trailers that match libcamera's commit\n> message practices. New trailer keys can easily be added later as new\n> needs arise.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> This reports a total of 42 issues through the project's history. 37 of\n> them should not be controversial:\n>\n> - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> and 'Reported-on')\n>\n> - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n>\n> - Typos in e-mail address (missing display name, missing space before\n> '<' and missing trailing '>')\n>\n> - Link in 'Fixes' trailer (should be a commit)\n>\n> - Too short commit ID in 'Fixes' trailer\n>\n> - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> space or extra ':' after commit ID, missing '\"' around commit subject)\n>\n> The five remaining issues may benefit from discussions:\n>\n> - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> trailer key commonly used in the kernel, but git..b both recommend\n> Co-authored-by. I'm sure which option would be best, so I haven't\n> included either for now.\n>\n> - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> instance). 'Reported-by' usually has an e-mail address value, but we\n> have commonly used 'Coverity CID=' for issues reported by\n> Coverity. I've tentatively added support for this (feedback is\n> welcome), and one commit still got flagged as its 'Reported-by'\n> trailer has a space instead of an '=' after 'CID'.\n>\n> - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> is to be able to identify users by name and e-mail address for\n> 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> trailers too. If someone *really* doesn't want their name included in\n> the git log when reporting an issue, we can simply omit the\n> 'Reported-by' trailer.\n>\n> - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> instances). This should use a 'Link' trailer instead.\n> ---\n> utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 78 insertions(+), 2 deletions(-)\n>\n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index e68c874609bc..3558740d389d 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -210,13 +210,23 @@ class Commit:\n>\n> def _parse(self):\n> # Get the commit title and list of files.\n> - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> self.commit],\n> stdout=subprocess.PIPE).stdout.decode('utf-8')\n> lines = ret.splitlines()\n> - self._files = [CommitFile(f) for f in lines[1:] if f]\n> +\n> self._title = lines[0]\n>\n> + self._trailers = []\n> + for index in range(1, len(lines)):\n> + line = lines[index]\n> + if not line:\n> + break\n> +\n> + self._trailers.append(line)\n> +\n> + self._files = [CommitFile(f) for f in lines[index:] if f]\n> +\n> def files(self, filter='AMR'):\n> return [f.filename for f in self._files if f.status in filter]\n>\n> @@ -224,6 +234,10 @@ class Commit:\n> def title(self):\n> return self._title\n>\n> + @property\n> + def trailers(self):\n> + return self._trailers\n> +\n> def get_diff(self, top_level, filename):\n> diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> '--', '%s/%s' % (top_level, filename)],\n> @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> 'possible candidates are ' + candidates)]\n>\n>\n> +class TrailersChecker(CommitChecker):\n> + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> +\n> + coverity_regex = re.compile(r'Coverity CID=.*')\n> +\n> + # Simple e-mail address validator regex, with an additional trailing\n> + # comment. The complexity of a full RFC6531 validator isn't worth the\n> + # additional invalid addresses it would reject.\n> + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> +\n> + link_regex = re.compile(r'https?://.*')\n> +\n> + @staticmethod\n> + def validate_reported_by(value):\n> + if TrailersChecker.email_regex.fullmatch(value):\n> + return True\n> + if TrailersChecker.coverity_regex.fullmatch(value):\n> + return True\n> + return False\n> +\n> + known_trailers = {\n> + 'Acked-by': email_regex,\n> + 'Bug': link_regex,\n> + 'Fixes': commit_regex,\n> + 'Link': link_regex,\n> + 'Reported-by': validate_reported_by,\n> + 'Reviewed-by': email_regex,\n> + 'Signed-off-by': email_regex,\n> + 'Suggested-by': email_regex,\n> + 'Tested-by': email_regex,\n> + }\n> +\n> + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> +\n> + @classmethod\n> + def check(cls, commit, top_level):\n> + issues = []\n> +\n> + for trailer in commit.trailers:\n> + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> + if not match:\n> + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> +\n> + key, value = match.groups()\n> +\n> + validator = TrailersChecker.known_trailers.get(key)\n> + if not validator:\n> + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> + continue\n> +\n> + if isinstance(validator, re.Pattern):\n> + valid = bool(validator.fullmatch(value))\n> + else:\n> + valid = validator(value)\n> +\n> + if not valid:\n> + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> + continue\n> +\n> + return issues\n> +\n> +\n> # ------------------------------------------------------------------------------\n> # Style Checkers\n> #\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92242BDC71\n\tfor ;\n\tMon, 10 Jul 2023 10:34:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E1EF8628C1;\n\tMon, 10 Jul 2023 12:34:11 +0200 (CEST)","from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com\n\t[IPv6:2607:f8b0:4864:20::b32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB26661E31\n\tfor ;\n\tMon, 10 Jul 2023 12:34:10 +0200 (CEST)","by mail-yb1-xb32.google.com with SMTP id\n\t3f1490d57ef6-c5ce57836b8so5087527276.1\n\tfor ;\n\tMon, 10 Jul 2023 03:34:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1688985251;\n\tbh=SPd5t7lYZyVGcQxKs+XdRj9gXfz6ybv6EH4MEFJ161s=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zAHZXcgtSGDO0st3vQFiKOpd3sOr7wdl8paJPZ7vqGowkQPdY+53+5A4u7mM1ZLuk\n\t2t2ToAwxIaFEftkq2psCdZj5tHLYtnl6dgjNARs+g7xB10H1CeyE3GHtqDwUs1jHgZ\n\t8fpCclmc58HKzAlJXGevRYwdEylQYmTupOoVVbU38jRlrWNNit1F3q7I/N7yQS1GGU\n\tEogFIgNBCnEazQsS1fhwsAQMDp9uTx8qytKmGUnU+R+I19+DkIk7Q8BDslvty2Qpzy\n\tNEmghYSzzSvNCH3IbwvI+JvVvE7MXwWW0gNpJH1tbUi7Qly8LiX1rvgCObC45+50uC\n\tvOb4Kow6Hl6xQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1688985249; x=1691577249;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=NglnQYDl6T3GeYY11j531KGCaxUL/CS3XYsOEkNu4eA=;\n\tb=i+2LFo2PCUbytFahO+sYRNKhSHtkOcYoUQzWhxKS5T52mDlrUMkcts0RYqta3s1nBq\n\tt2jtlrE8GIjz1QYaglOEssqwOD6iThx2eIuNg5/pCI5ctgBZDlrwhWFhaUXco5nHRznI\n\tE1jJJCnS3Yv2p6LVFb7S/D28wbFsbv5GffWAuqSSExfRKgWkmDTSCeCHwveAFNiZiNKH\n\tRaIL/jw2gxWyOcrMctnTaFLwS16JVpSPQ/qKtQu0KZDqmOjJavmr86oOiOlKypNSKSdz\n\ttWlwV6z0HgbEysEl/2DvYmkCWODzOJ2ryywJI0HDesnmRlD4pzVGSO5m1V9HkZiDYoLs\n\tbRIw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"i+2LFo2P\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1688985249; x=1691577249;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=NglnQYDl6T3GeYY11j531KGCaxUL/CS3XYsOEkNu4eA=;\n\tb=hgm2oWg0T1VkZN+kwUU8ZqOwhtQcGSrOx8o0sR0k0fpnWtKPJrlYB0mH1alD+GBFIy\n\thRTWjeZAhUhvM8pS7sGqHVHIbRD+nLTDtPxENtgIDOS2Z9HYsZRCbx44v4bTPAz4YjxI\n\t6zo+3a//Qdtv9pdYfB8N+kN8WIVAaAi6B+3ehhjY/Qq1i5xG5sAj3pDanBejCi3WNmqY\n\tsveedTY9Z8CO6ssAB/kf7luyJFWEPWzhQDSArRO7KV9CXrADjtB7rijL00kMkmPSqwEY\n\t24ccAiUF/oUi+7B8X+oD5xIqoF019cFQSslVNhWE2XGREyH2+U/sWNkZFaXbNlYz2maa\n\tUqwg==","X-Gm-Message-State":"ABy/qLZ2cm+u8sgM51DUWHTeZcVRyJA414p30ChX/uc/rRik2PMT+QWL\n\tpKb6ZGDZokEenKL2Z6IePEGzn23CMHYXBgVaKjuYKj/alsyU1EcuvoqjUQ==","X-Google-Smtp-Source":"APBJJlEcp5dR5xqOOLydVIR9qX7uCzKXIbXnW5NJyPkpmujesJ7aR+IVYhEwGzHjkkl8cXiFriwWxa9BI1lJFXUUlGI=","X-Received":"by 2002:a0d:e8d3:0:b0:570:8856:5dbc with SMTP id\n\tr202-20020a0de8d3000000b0057088565dbcmr12453533ywe.41.1688985249376;\n\tMon, 10 Jul 2023 03:34:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>","Date":"Mon, 10 Jul 2023 11:33:54 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27523,"web_url":"https://patchwork.libcamera.org/comment/27523/","msgid":"<168900428498.3585053.8193941688746059923@Monstersaurus>","date":"2023-07-10T15:51:24","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-07-10 11:33:54)\n> Hi Laurent,\n> \n> I've updated my tree and this change now seems to break my pre-commit hook:\n\nYikes - You've gone pre-commit ;-)\n\nI usually use post-commit - as pre-commit makes it harder to actually\nwork in my experience as it prevents the commits in the first place.\n\nI think post-commit is a much better use of the hooks, as it tells you\nof issues while not getting in the way.\n\nStill - pre-commit is supposed to be supported - so I'll swap over and\nsee if I can debug this.\n\n--\nKieran\n\n> \n> naushir@work:~/libcamera/ $ git commit -a -s\n> ---------------\n> Staged changes\n> ---------------\n> Traceback (most recent call last):\n> File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> 1052, in \n> sys.exit(main(sys.argv))\n> File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> 1042, in main\n> issues += check_style(top_level, commit, args.checkers)\n> File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> 935, in check_style\n> for issue in checker.check(commit, top_level):\n> File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> 479, in check\n> for trailer in commit.trailers:\n> File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> 239, in trailers\n> return self._trailers\n> AttributeError: 'StagedChanges' object has no attribute '_trailers'.\n> Did you mean: 'trailers'?\n> \n> I'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly\n> tried debugging, but quickly found that I'm in way over my depth with the\n> checkstyle.py script :-)\n> \n> Regards,\n> Naush\n> \n> On Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel\n> wrote:\n> >\n> > The libcamera git history contains numerous examples of incorrect commit\n> > message trailers due to invalid trailer types (e.g. Change-Id), typos\n> > and other small issues. Those went unnoticed through reviews, which\n> > shows that an automated checker is required.\n> >\n> > Add a trailers checker to checkstyle.py to catch invalid or malformed\n> > trailers, with a set of supported trailers that match libcamera's commit\n> > message practices. New trailer keys can easily be added later as new\n> > needs arise.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > This reports a total of 42 issues through the project's history. 37 of\n> > them should not be controversial:\n> >\n> > - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> > and 'Reported-on')\n> >\n> > - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> > of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n> >\n> > - Typos in e-mail address (missing display name, missing space before\n> > '<' and missing trailing '>')\n> >\n> > - Link in 'Fixes' trailer (should be a commit)\n> >\n> > - Too short commit ID in 'Fixes' trailer\n> >\n> > - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> > space or extra ':' after commit ID, missing '\"' around commit subject)\n> >\n> > The five remaining issues may benefit from discussions:\n> >\n> > - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> > trailer key commonly used in the kernel, but git..b both recommend\n> > Co-authored-by. I'm sure which option would be best, so I haven't\n> > included either for now.\n> >\n> > - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> > instance). 'Reported-by' usually has an e-mail address value, but we\n> > have commonly used 'Coverity CID=' for issues reported by\n> > Coverity. I've tentatively added support for this (feedback is\n> > welcome), and one commit still got flagged as its 'Reported-by'\n> > trailer has a space instead of an '=' after 'CID'.\n> >\n> > - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> > is to be able to identify users by name and e-mail address for\n> > 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> > trailers too. If someone *really* doesn't want their name included in\n> > the git log when reporting an issue, we can simply omit the\n> > 'Reported-by' trailer.\n> >\n> > - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> > instances). This should use a 'Link' trailer instead.\n> > ---\n> > utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> > 1 file changed, 78 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > index e68c874609bc..3558740d389d 100755\n> > --- a/utils/checkstyle.py\n> > +++ b/utils/checkstyle.py\n> > @@ -210,13 +210,23 @@ class Commit:\n> >\n> > def _parse(self):\n> > # Get the commit title and list of files.\n> > - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> > + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> > self.commit],\n> > stdout=subprocess.PIPE).stdout.decode('utf-8')\n> > lines = ret.splitlines()\n> > - self._files = [CommitFile(f) for f in lines[1:] if f]\n> > +\n> > self._title = lines[0]\n> >\n> > + self._trailers = []\n> > + for index in range(1, len(lines)):\n> > + line = lines[index]\n> > + if not line:\n> > + break\n> > +\n> > + self._trailers.append(line)\n> > +\n> > + self._files = [CommitFile(f) for f in lines[index:] if f]\n> > +\n> > def files(self, filter='AMR'):\n> > return [f.filename for f in self._files if f.status in filter]\n> >\n> > @@ -224,6 +234,10 @@ class Commit:\n> > def title(self):\n> > return self._title\n> >\n> > + @property\n> > + def trailers(self):\n> > + return self._trailers\n> > +\n> > def get_diff(self, top_level, filename):\n> > diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> > '--', '%s/%s' % (top_level, filename)],\n> > @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> > 'possible candidates are ' + candidates)]\n> >\n> >\n> > +class TrailersChecker(CommitChecker):\n> > + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> > +\n> > + coverity_regex = re.compile(r'Coverity CID=.*')\n> > +\n> > + # Simple e-mail address validator regex, with an additional trailing\n> > + # comment. The complexity of a full RFC6531 validator isn't worth the\n> > + # additional invalid addresses it would reject.\n> > + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> > +\n> > + link_regex = re.compile(r'https?://.*')\n> > +\n> > + @staticmethod\n> > + def validate_reported_by(value):\n> > + if TrailersChecker.email_regex.fullmatch(value):\n> > + return True\n> > + if TrailersChecker.coverity_regex.fullmatch(value):\n> > + return True\n> > + return False\n> > +\n> > + known_trailers = {\n> > + 'Acked-by': email_regex,\n> > + 'Bug': link_regex,\n> > + 'Fixes': commit_regex,\n> > + 'Link': link_regex,\n> > + 'Reported-by': validate_reported_by,\n> > + 'Reviewed-by': email_regex,\n> > + 'Signed-off-by': email_regex,\n> > + 'Suggested-by': email_regex,\n> > + 'Tested-by': email_regex,\n> > + }\n> > +\n> > + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> > +\n> > + @classmethod\n> > + def check(cls, commit, top_level):\n> > + issues = []\n> > +\n> > + for trailer in commit.trailers:\n> > + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> > + if not match:\n> > + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> > +\n> > + key, value = match.groups()\n> > +\n> > + validator = TrailersChecker.known_trailers.get(key)\n> > + if not validator:\n> > + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> > + continue\n> > +\n> > + if isinstance(validator, re.Pattern):\n> > + valid = bool(validator.fullmatch(value))\n> > + else:\n> > + valid = validator(value)\n> > +\n> > + if not valid:\n> > + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> > + continue\n> > +\n> > + return issues\n> > +\n> > +\n> > # ------------------------------------------------------------------------------\n> > # Style Checkers\n> > #\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7352AC323E\n\tfor ;\n\tMon, 10 Jul 2023 15:51:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D43A5628BF;\n\tMon, 10 Jul 2023 17:51:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A70860384\n\tfor ;\n\tMon, 10 Jul 2023 17:51:28 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D40EEBEB;\n\tMon, 10 Jul 2023 17:50:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689004289;\n\tbh=8WRmzU5sR/5sMEKUNIU7+Rj40B9j/0lxKjojWOFbgDs=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Jxiif9a7jrlEl3Y6xDllpx77YO90Q/r86DH88VX/HhJT9SAduQDQ8lHodd++J4xGe\n\toxZD/YXaMZTqU5oAKtntuK39MZQAlOeFlEYiBa3DFnw4YxyykjDo61gAfH2avm+NLY\n\t5AEg0bFQby9zeHUxeQjSukjAfgoRkAW7/5+MRDkOwzbDiZwXVfb/vAGnQsSTIC0vpj\n\tT9zOS+cwOjKU5iilddtTC37gLirPXXY0tvzRt8LnkiSTUrhYITrVT9myXTErmQjwci\n\tDjcARpfM6wser8bae5llG7A/kEr67b7bzCN1fYSCH86G4uz6xkS8GAdSuR/wKddw91\n\tTz37CGWwNbvfg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689004239;\n\tbh=8WRmzU5sR/5sMEKUNIU7+Rj40B9j/0lxKjojWOFbgDs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=opzux7r7LpGzxwMb5/0SgXK1+EIAMdl0pTiEUcnVizZvGvqbriWt4mz7IH3ZI4bHX\n\tc6/4WENIyn6I4rOmAvI47zWqkw0lFFnr+FGAXxNPw+yLElzCy7H9fgoQ0a3bvzoEXV\n\tUErD7tryXMrKL2SoRcniX4u1dioZJzvlkg6ImH9M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"opzux7r7\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>\n\t","To":"Laurent Pinchart ,\n\tNaushir Patuck ,\n\tNaushir Patuck via libcamera-devel ","Date":"Mon, 10 Jul 2023 16:51:24 +0100","Message-ID":"<168900428498.3585053.8193941688746059923@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27525,"web_url":"https://patchwork.libcamera.org/comment/27525/","msgid":"","date":"2023-07-11T07:50:13","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Mon, 10 Jul 2023 at 16:51, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck via libcamera-devel (2023-07-10 11:33:54)\n> > Hi Laurent,\n> >\n> > I've updated my tree and this change now seems to break my pre-commit hook:\n>\n> Yikes - You've gone pre-commit ;-)\n>\n> I usually use post-commit - as pre-commit makes it harder to actually\n> work in my experience as it prevents the commits in the first place.\n>\n> I think post-commit is a much better use of the hooks, as it tells you\n> of issues while not getting in the way.\n\nOk, that's no problem, I can switch to post-commit hooks.\nPerhaps it's worth deprecating utils/hooks/pre-commit?\n\n>\n> Still - pre-commit is supposed to be supported - so I'll swap over and\n> see if I can debug this.\n\nI see you have a DNI fix for this, but maybe I'll just switch to post-commit\nhooks if that's what everyone else is using.\n\nNaush\n\n>\n> --\n> Kieran\n>\n> >\n> > naushir@work:~/libcamera/ $ git commit -a -s\n> > ---------------\n> > Staged changes\n> > ---------------\n> > Traceback (most recent call last):\n> > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > 1052, in \n> > sys.exit(main(sys.argv))\n> > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > 1042, in main\n> > issues += check_style(top_level, commit, args.checkers)\n> > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > 935, in check_style\n> > for issue in checker.check(commit, top_level):\n> > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > 479, in check\n> > for trailer in commit.trailers:\n> > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > 239, in trailers\n> > return self._trailers\n> > AttributeError: 'StagedChanges' object has no attribute '_trailers'.\n> > Did you mean: 'trailers'?\n> >\n> > I'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly\n> > tried debugging, but quickly found that I'm in way over my depth with the\n> > checkstyle.py script :-)\n> >\n> > Regards,\n> > Naush\n> >\n> > On Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel\n> > wrote:\n> > >\n> > > The libcamera git history contains numerous examples of incorrect commit\n> > > message trailers due to invalid trailer types (e.g. Change-Id), typos\n> > > and other small issues. Those went unnoticed through reviews, which\n> > > shows that an automated checker is required.\n> > >\n> > > Add a trailers checker to checkstyle.py to catch invalid or malformed\n> > > trailers, with a set of supported trailers that match libcamera's commit\n> > > message practices. New trailer keys can easily be added later as new\n> > > needs arise.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > This reports a total of 42 issues through the project's history. 37 of\n> > > them should not be controversial:\n> > >\n> > > - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> > > and 'Reported-on')\n> > >\n> > > - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> > > of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n> > >\n> > > - Typos in e-mail address (missing display name, missing space before\n> > > '<' and missing trailing '>')\n> > >\n> > > - Link in 'Fixes' trailer (should be a commit)\n> > >\n> > > - Too short commit ID in 'Fixes' trailer\n> > >\n> > > - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> > > space or extra ':' after commit ID, missing '\"' around commit subject)\n> > >\n> > > The five remaining issues may benefit from discussions:\n> > >\n> > > - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> > > trailer key commonly used in the kernel, but git..b both recommend\n> > > Co-authored-by. I'm sure which option would be best, so I haven't\n> > > included either for now.\n> > >\n> > > - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> > > instance). 'Reported-by' usually has an e-mail address value, but we\n> > > have commonly used 'Coverity CID=' for issues reported by\n> > > Coverity. I've tentatively added support for this (feedback is\n> > > welcome), and one commit still got flagged as its 'Reported-by'\n> > > trailer has a space instead of an '=' after 'CID'.\n> > >\n> > > - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> > > is to be able to identify users by name and e-mail address for\n> > > 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> > > trailers too. If someone *really* doesn't want their name included in\n> > > the git log when reporting an issue, we can simply omit the\n> > > 'Reported-by' trailer.\n> > >\n> > > - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> > > instances). This should use a 'Link' trailer instead.\n> > > ---\n> > > utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> > > 1 file changed, 78 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > index e68c874609bc..3558740d389d 100755\n> > > --- a/utils/checkstyle.py\n> > > +++ b/utils/checkstyle.py\n> > > @@ -210,13 +210,23 @@ class Commit:\n> > >\n> > > def _parse(self):\n> > > # Get the commit title and list of files.\n> > > - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> > > + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> > > self.commit],\n> > > stdout=subprocess.PIPE).stdout.decode('utf-8')\n> > > lines = ret.splitlines()\n> > > - self._files = [CommitFile(f) for f in lines[1:] if f]\n> > > +\n> > > self._title = lines[0]\n> > >\n> > > + self._trailers = []\n> > > + for index in range(1, len(lines)):\n> > > + line = lines[index]\n> > > + if not line:\n> > > + break\n> > > +\n> > > + self._trailers.append(line)\n> > > +\n> > > + self._files = [CommitFile(f) for f in lines[index:] if f]\n> > > +\n> > > def files(self, filter='AMR'):\n> > > return [f.filename for f in self._files if f.status in filter]\n> > >\n> > > @@ -224,6 +234,10 @@ class Commit:\n> > > def title(self):\n> > > return self._title\n> > >\n> > > + @property\n> > > + def trailers(self):\n> > > + return self._trailers\n> > > +\n> > > def get_diff(self, top_level, filename):\n> > > diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> > > '--', '%s/%s' % (top_level, filename)],\n> > > @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> > > 'possible candidates are ' + candidates)]\n> > >\n> > >\n> > > +class TrailersChecker(CommitChecker):\n> > > + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> > > +\n> > > + coverity_regex = re.compile(r'Coverity CID=.*')\n> > > +\n> > > + # Simple e-mail address validator regex, with an additional trailing\n> > > + # comment. The complexity of a full RFC6531 validator isn't worth the\n> > > + # additional invalid addresses it would reject.\n> > > + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> > > +\n> > > + link_regex = re.compile(r'https?://.*')\n> > > +\n> > > + @staticmethod\n> > > + def validate_reported_by(value):\n> > > + if TrailersChecker.email_regex.fullmatch(value):\n> > > + return True\n> > > + if TrailersChecker.coverity_regex.fullmatch(value):\n> > > + return True\n> > > + return False\n> > > +\n> > > + known_trailers = {\n> > > + 'Acked-by': email_regex,\n> > > + 'Bug': link_regex,\n> > > + 'Fixes': commit_regex,\n> > > + 'Link': link_regex,\n> > > + 'Reported-by': validate_reported_by,\n> > > + 'Reviewed-by': email_regex,\n> > > + 'Signed-off-by': email_regex,\n> > > + 'Suggested-by': email_regex,\n> > > + 'Tested-by': email_regex,\n> > > + }\n> > > +\n> > > + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> > > +\n> > > + @classmethod\n> > > + def check(cls, commit, top_level):\n> > > + issues = []\n> > > +\n> > > + for trailer in commit.trailers:\n> > > + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> > > + if not match:\n> > > + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> > > +\n> > > + key, value = match.groups()\n> > > +\n> > > + validator = TrailersChecker.known_trailers.get(key)\n> > > + if not validator:\n> > > + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> > > + continue\n> > > +\n> > > + if isinstance(validator, re.Pattern):\n> > > + valid = bool(validator.fullmatch(value))\n> > > + else:\n> > > + valid = validator(value)\n> > > +\n> > > + if not valid:\n> > > + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> > > + continue\n> > > +\n> > > + return issues\n> > > +\n> > > +\n> > > # ------------------------------------------------------------------------------\n> > > # Style Checkers\n> > > #\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3BB90BEFBE\n\tfor ;\n\tTue, 11 Jul 2023 07:50:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3D99628BC;\n\tTue, 11 Jul 2023 09:50:31 +0200 (CEST)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EA7B61E37\n\tfor ;\n\tTue, 11 Jul 2023 09:50:30 +0200 (CEST)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-57a6df91b1eso36198627b3.1\n\tfor ;\n\tTue, 11 Jul 2023 00:50:30 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689061831;\n\tbh=zYOZP245VhjL5xRT5Cq6/H4m3LPkT/DzlXUVINHCPKE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=g7w8+Tl3q48BVAzYvyT0fqcyaHXHAXL2nLxFm1Oyjeiw2Mxwjj/Dm7Yw5LRGUXeS/\n\tBs7e34rlqs439Xtk/t7TPsl/Qci7A7rdu7EI9hnEXNNUIO0txSXS+CDZpk4TRpBLMS\n\tBa9WDifULa9It5MPnl19ZxDShjUhrjG0qk0C7Eu3af5OCEJ9Drj+ky6WVep9dw9kNl\n\tIjs/yYJ54VTwcC0E9pqOks8wLX/aaxrQQohRB79JzxT87fFALNeMtRo6xr5eBMYRgJ\n\tmOLz8sLPjWS4Vt6hs5ycgo/3/9QtKkiSJ6eDGj+fe8+P+MQm+qZTtwIaBKnHjjdvsu\n\tFdaYQgfx0pyiA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689061829; x=1691653829;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=TBiWfYzMa8N8YKyr0rv2Sq2rYvv4lOriG6W1lrDmpFE=;\n\tb=KePXZoHRoM/8MEKa+SRwW2YUz5GYkhU5lnb2R9vuk4spo+qrDo1vgP4N5LDZ/FcL3U\n\tHbmi6oyXPRhWGrH2LJSW1rE2sIHEbI/mSJqt50XftJ91sSDldx081mV+k0z2asvw+gtC\n\tcH9UPiuXYegghYT/la++ZhNWw9EQZiJK/lNO8izg7tDUOusPKdT0DfaIMhQP73b69JmS\n\t835Rv554W/REDxrce9BeF01JCrurjrtWjTWdH5lmEf70q/1bEfEYAFGkDkfkpayi9Gad\n\tBor2hsQ7UXevE0PI/O14tJKCInLfp25RjZV4Gmm5yXU4pMbw9rfSLxoDSnauO8gSTcda\n\t1/Yw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"KePXZoHR\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689061829; x=1691653829;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=TBiWfYzMa8N8YKyr0rv2Sq2rYvv4lOriG6W1lrDmpFE=;\n\tb=hnEyUQt9YwgE6fJkE1EP11Mw0p68ciSn4aeXEKWSZMUq5+SgTZjKl37wLPR1Ab0qze\n\t6HKaDTI/nASCIVeJqqvWsI9c7RqIQJp+JWZ5CuXu5kr6y4IGaxvxBRkaYqGYR+KJTH6Z\n\tf/KvvQDOwvDWbDvlrGD3Rnusga5QkdV3rRWNJAeVt5i+aiI9R8vz12E2uwq7Flqinzx1\n\t++U3qqcRiRKPKDKMLfGaeGIgs9id4/5FNhFUnd725H5Nan0GghiAt8E2Vh4lpVOer3gZ\n\t1wRC2cTzY8mptGMxGMaOguufY87nNA4oRbOyBbX/EWn1fXlHBsnxeD3QaGCSedjr5woO\n\tr0Cg==","X-Gm-Message-State":"ABy/qLbUAEB+aitc/2VraPDhVRsMQBYf9nJUUVU23wj5ryfiHYUIJMO1\n\tIZWFSGfSfCj0ERXpgz5bxT99l9VmZJEJ6qNI2t9r9TQ4cL3M7IOIZ1SD7g==","X-Google-Smtp-Source":"APBJJlFRPEGKM9vH3rVNlKL9aPas4CVQAxFwyiWBW1IsD5og48B59ecbep939s94UoUdKDJPfIigcdLaQPggg9Xcj+c=","X-Received":"by 2002:a81:4e11:0:b0:577:1909:ee16 with SMTP id\n\tc17-20020a814e11000000b005771909ee16mr16181639ywb.30.1689061829212;\n\tTue, 11 Jul 2023 00:50:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<168900428498.3585053.8193941688746059923@Monstersaurus>","In-Reply-To":"<168900428498.3585053.8193941688746059923@Monstersaurus>","Date":"Tue, 11 Jul 2023 08:50:13 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Naushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27526,"web_url":"https://patchwork.libcamera.org/comment/27526/","msgid":"<168906248863.3585053.17970319067618110479@Monstersaurus>","date":"2023-07-11T08:01:28","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2023-07-11 08:50:13)\n> Hi Kieran,\n> \n> On Mon, 10 Jul 2023 at 16:51, Kieran Bingham\n> wrote:\n> >\n> > Quoting Naushir Patuck via libcamera-devel (2023-07-10 11:33:54)\n> > > Hi Laurent,\n> > >\n> > > I've updated my tree and this change now seems to break my pre-commit hook:\n> >\n> > Yikes - You've gone pre-commit ;-)\n> >\n> > I usually use post-commit - as pre-commit makes it harder to actually\n> > work in my experience as it prevents the commits in the first place.\n> >\n> > I think post-commit is a much better use of the hooks, as it tells you\n> > of issues while not getting in the way.\n> \n> Ok, that's no problem, I can switch to post-commit hooks.\n> Perhaps it's worth deprecating utils/hooks/pre-commit?\n> \n> >\n> > Still - pre-commit is supposed to be supported - so I'll swap over and\n> > see if I can debug this.\n> \n> I see you have a DNI fix for this, but maybe I'll just switch to post-commit\n> hooks if that's what everyone else is using.\n\nYes, the DNI quick fix is to just initialise the trailers in the common\nclass.\nTheres:\n\nCommit <- StagedCommit <- AmendedCommit\n\nAnd the trailers only get initialised if you create a Commit, and the\nderived classes don't call Commit::_parse() as they override it\nthemselves.\n\nI don't think AmendedCommit should derive from StagedCommit either ...\nand should also still check the trailers, while StagedCommit can't as\nthere is no commit message to check trailers in that instance.\n\nBut the issue goes away in a post-commit as there's no staged commit\nnor amended commit to check - as in 'post-commit' it's just a Commit\ninstance.\n\nI think post-commit gives a better developer experience anyway and would\nalways recommend that over pre-commit.\n\n--\nKieran\n\n> \n> Naush\n> \n> >\n> > --\n> > Kieran\n> >\n> > >\n> > > naushir@work:~/libcamera/ $ git commit -a -s\n> > > ---------------\n> > > Staged changes\n> > > ---------------\n> > > Traceback (most recent call last):\n> > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > 1052, in \n> > > sys.exit(main(sys.argv))\n> > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > 1042, in main\n> > > issues += check_style(top_level, commit, args.checkers)\n> > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > 935, in check_style\n> > > for issue in checker.check(commit, top_level):\n> > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > 479, in check\n> > > for trailer in commit.trailers:\n> > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > 239, in trailers\n> > > return self._trailers\n> > > AttributeError: 'StagedChanges' object has no attribute '_trailers'.\n> > > Did you mean: 'trailers'?\n> > >\n> > > I'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly\n> > > tried debugging, but quickly found that I'm in way over my depth with the\n> > > checkstyle.py script :-)\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > > On Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel\n> > > wrote:\n> > > >\n> > > > The libcamera git history contains numerous examples of incorrect commit\n> > > > message trailers due to invalid trailer types (e.g. Change-Id), typos\n> > > > and other small issues. Those went unnoticed through reviews, which\n> > > > shows that an automated checker is required.\n> > > >\n> > > > Add a trailers checker to checkstyle.py to catch invalid or malformed\n> > > > trailers, with a set of supported trailers that match libcamera's commit\n> > > > message practices. New trailer keys can easily be added later as new\n> > > > needs arise.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > This reports a total of 42 issues through the project's history. 37 of\n> > > > them should not be controversial:\n> > > >\n> > > > - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> > > > and 'Reported-on')\n> > > >\n> > > > - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> > > > of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n> > > >\n> > > > - Typos in e-mail address (missing display name, missing space before\n> > > > '<' and missing trailing '>')\n> > > >\n> > > > - Link in 'Fixes' trailer (should be a commit)\n> > > >\n> > > > - Too short commit ID in 'Fixes' trailer\n> > > >\n> > > > - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> > > > space or extra ':' after commit ID, missing '\"' around commit subject)\n> > > >\n> > > > The five remaining issues may benefit from discussions:\n> > > >\n> > > > - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> > > > trailer key commonly used in the kernel, but git..b both recommend\n> > > > Co-authored-by. I'm sure which option would be best, so I haven't\n> > > > included either for now.\n> > > >\n> > > > - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> > > > instance). 'Reported-by' usually has an e-mail address value, but we\n> > > > have commonly used 'Coverity CID=' for issues reported by\n> > > > Coverity. I've tentatively added support for this (feedback is\n> > > > welcome), and one commit still got flagged as its 'Reported-by'\n> > > > trailer has a space instead of an '=' after 'CID'.\n> > > >\n> > > > - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> > > > is to be able to identify users by name and e-mail address for\n> > > > 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> > > > trailers too. If someone *really* doesn't want their name included in\n> > > > the git log when reporting an issue, we can simply omit the\n> > > > 'Reported-by' trailer.\n> > > >\n> > > > - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> > > > instances). This should use a 'Link' trailer instead.\n> > > > ---\n> > > > utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> > > > 1 file changed, 78 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > > index e68c874609bc..3558740d389d 100755\n> > > > --- a/utils/checkstyle.py\n> > > > +++ b/utils/checkstyle.py\n> > > > @@ -210,13 +210,23 @@ class Commit:\n> > > >\n> > > > def _parse(self):\n> > > > # Get the commit title and list of files.\n> > > > - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> > > > + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> > > > self.commit],\n> > > > stdout=subprocess.PIPE).stdout.decode('utf-8')\n> > > > lines = ret.splitlines()\n> > > > - self._files = [CommitFile(f) for f in lines[1:] if f]\n> > > > +\n> > > > self._title = lines[0]\n> > > >\n> > > > + self._trailers = []\n> > > > + for index in range(1, len(lines)):\n> > > > + line = lines[index]\n> > > > + if not line:\n> > > > + break\n> > > > +\n> > > > + self._trailers.append(line)\n> > > > +\n> > > > + self._files = [CommitFile(f) for f in lines[index:] if f]\n> > > > +\n> > > > def files(self, filter='AMR'):\n> > > > return [f.filename for f in self._files if f.status in filter]\n> > > >\n> > > > @@ -224,6 +234,10 @@ class Commit:\n> > > > def title(self):\n> > > > return self._title\n> > > >\n> > > > + @property\n> > > > + def trailers(self):\n> > > > + return self._trailers\n> > > > +\n> > > > def get_diff(self, top_level, filename):\n> > > > diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> > > > '--', '%s/%s' % (top_level, filename)],\n> > > > @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> > > > 'possible candidates are ' + candidates)]\n> > > >\n> > > >\n> > > > +class TrailersChecker(CommitChecker):\n> > > > + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> > > > +\n> > > > + coverity_regex = re.compile(r'Coverity CID=.*')\n> > > > +\n> > > > + # Simple e-mail address validator regex, with an additional trailing\n> > > > + # comment. The complexity of a full RFC6531 validator isn't worth the\n> > > > + # additional invalid addresses it would reject.\n> > > > + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> > > > +\n> > > > + link_regex = re.compile(r'https?://.*')\n> > > > +\n> > > > + @staticmethod\n> > > > + def validate_reported_by(value):\n> > > > + if TrailersChecker.email_regex.fullmatch(value):\n> > > > + return True\n> > > > + if TrailersChecker.coverity_regex.fullmatch(value):\n> > > > + return True\n> > > > + return False\n> > > > +\n> > > > + known_trailers = {\n> > > > + 'Acked-by': email_regex,\n> > > > + 'Bug': link_regex,\n> > > > + 'Fixes': commit_regex,\n> > > > + 'Link': link_regex,\n> > > > + 'Reported-by': validate_reported_by,\n> > > > + 'Reviewed-by': email_regex,\n> > > > + 'Signed-off-by': email_regex,\n> > > > + 'Suggested-by': email_regex,\n> > > > + 'Tested-by': email_regex,\n> > > > + }\n> > > > +\n> > > > + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> > > > +\n> > > > + @classmethod\n> > > > + def check(cls, commit, top_level):\n> > > > + issues = []\n> > > > +\n> > > > + for trailer in commit.trailers:\n> > > > + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> > > > + if not match:\n> > > > + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> > > > +\n> > > > + key, value = match.groups()\n> > > > +\n> > > > + validator = TrailersChecker.known_trailers.get(key)\n> > > > + if not validator:\n> > > > + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> > > > + continue\n> > > > +\n> > > > + if isinstance(validator, re.Pattern):\n> > > > + valid = bool(validator.fullmatch(value))\n> > > > + else:\n> > > > + valid = validator(value)\n> > > > +\n> > > > + if not valid:\n> > > > + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> > > > + continue\n> > > > +\n> > > > + return issues\n> > > > +\n> > > > +\n> > > > # ------------------------------------------------------------------------------\n> > > > # Style Checkers\n> > > > #\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 73413BDC71\n\tfor ;\n\tTue, 11 Jul 2023 08:01:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E129A628BC;\n\tTue, 11 Jul 2023 10:01:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2635961E37\n\tfor ;\n\tTue, 11 Jul 2023 10:01:32 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 320E8DFB;\n\tTue, 11 Jul 2023 10:00:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689062493;\n\tbh=GVl+NSaJEm/0EyFESS3xATNxDP/TTt4lzSIKVkE2jbI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ri6bUiSwxfoUicsK+1jo4U4x58JP/o7L+ib2uyWov2Pw34MdbjQ/9unVys/cK7XwL\n\tK36/rMfTVo6QK4l+Wvd5t3mSF5Rb30n6TiajYH8k5KyYLkb0MJ9aTaW35/j+eRGXB1\n\tq0c/vUpzh7ng/k8m4oU4RFbRRCH++A7n+bzQjH87U/XOEh3zDuHFcKindueOC1MzJm\n\t2pz1v4Fi0OjfCKifW92Ou9YLsrwyZvxN+cXyn01U+aWOZbCQkKGSqXw4LH+comvlv+\n\tDi5wS+QM9CK0iNI23NibWNDBCe9DEnN6ogesnhnQbie3vgjSPDufq9J6hgE69piMIV\n\toYeX6s319O5Og==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689062443;\n\tbh=GVl+NSaJEm/0EyFESS3xATNxDP/TTt4lzSIKVkE2jbI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VVVeJ1xckIMu+3u2ll1CM8GIhGil2sbg4EbdCmVrJIq2nutpb1yV7TcioXvE/FDYz\n\tqv4o7F6JAoJ/uSlrkwxiI3baDgUDn1vQUO0R7yZ1UxDRgNNlNqgxuPbODimonUtvzf\n\tjFAM7u7yFHjKdInoXP0HQxgyOFDH3n6iNlj9NXQs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VVVeJ1xc\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<168900428498.3585053.8193941688746059923@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Tue, 11 Jul 2023 09:01:28 +0100","Message-ID":"<168906248863.3585053.17970319067618110479@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Naushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27527,"web_url":"https://patchwork.libcamera.org/comment/27527/","msgid":"<168906631496.3585053.10467348720232015988@Monstersaurus>","date":"2023-07-11T09:05:14","subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2023-07-11 09:01:28)\n> Quoting Naushir Patuck (2023-07-11 08:50:13)\n> > Hi Kieran,\n> > \n> > On Mon, 10 Jul 2023 at 16:51, Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Naushir Patuck via libcamera-devel (2023-07-10 11:33:54)\n> > > > Hi Laurent,\n> > > >\n> > > > I've updated my tree and this change now seems to break my pre-commit hook:\n> > >\n> > > Yikes - You've gone pre-commit ;-)\n> > >\n> > > I usually use post-commit - as pre-commit makes it harder to actually\n> > > work in my experience as it prevents the commits in the first place.\n> > >\n> > > I think post-commit is a much better use of the hooks, as it tells you\n> > > of issues while not getting in the way.\n> > \n> > Ok, that's no problem, I can switch to post-commit hooks.\n> > Perhaps it's worth deprecating utils/hooks/pre-commit?\n> > \n> > >\n> > > Still - pre-commit is supposed to be supported - so I'll swap over and\n> > > see if I can debug this.\n> > \n> > I see you have a DNI fix for this, but maybe I'll just switch to post-commit\n> > hooks if that's what everyone else is using.\n> \n> Yes, the DNI quick fix is to just initialise the trailers in the common\n> class.\n> Theres:\n> \n> Commit <- StagedCommit <- AmendedCommit\n> \n> And the trailers only get initialised if you create a Commit, and the\n> derived classes don't call Commit::_parse() as they override it\n> themselves.\n> \n> I don't think AmendedCommit should derive from StagedCommit either ...\n> and should also still check the trailers, while StagedCommit can't as\n> there is no commit message to check trailers in that instance.\n> \n> But the issue goes away in a post-commit as there's no staged commit\n> nor amended commit to check - as in 'post-commit' it's just a Commit\n> instance.\n\nAnd I'm starting to think that it really isn't worth checking Trailers\nin pre-commit anyway, as it's /before/ the user has even had the\nopportunity to adjust them.\n\nThe only thing that would make sense is if the trailer reporting was\nadded as a comment to the commit message so that the warning was visible\nat the point that the commit message could be written ... but that\nseems a lot more effort to support something that doesn't necessarily\nhelp.\n\nI think for now I'm tempted to suggest we merge the default\ninitialisation as in the DNI patch I posted to prevent errors in the\ncheckstyle script, without supporting Trailer checks in the pre-commit\nhooks.\n\n--\nKieran\n\n\n> \n> I think post-commit gives a better developer experience anyway and would\n> always recommend that over pre-commit.\n> \n> --\n> Kieran\n> \n> > \n> > Naush\n> > \n> > >\n> > > --\n> > > Kieran\n> > >\n> > > >\n> > > > naushir@work:~/libcamera/ $ git commit -a -s\n> > > > ---------------\n> > > > Staged changes\n> > > > ---------------\n> > > > Traceback (most recent call last):\n> > > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > > 1052, in \n> > > > sys.exit(main(sys.argv))\n> > > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > > 1042, in main\n> > > > issues += check_style(top_level, commit, args.checkers)\n> > > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > > 935, in check_style\n> > > > for issue in checker.check(commit, top_level):\n> > > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > > 479, in check\n> > > > for trailer in commit.trailers:\n> > > > File \"/home/naushir/projects/libcamera/./utils/checkstyle.py\", line\n> > > > 239, in trailers\n> > > > return self._trailers\n> > > > AttributeError: 'StagedChanges' object has no attribute '_trailers'.\n> > > > Did you mean: 'trailers'?\n> > > >\n> > > > I'm using the same old hook found in ./utils/hooks/pre-commit. I've briefly\n> > > > tried debugging, but quickly found that I'm in way over my depth with the\n> > > > checkstyle.py script :-)\n> > > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > > On Mon, 12 Jun 2023 at 23:48, Laurent Pinchart via libcamera-devel\n> > > > wrote:\n> > > > >\n> > > > > The libcamera git history contains numerous examples of incorrect commit\n> > > > > message trailers due to invalid trailer types (e.g. Change-Id), typos\n> > > > > and other small issues. Those went unnoticed through reviews, which\n> > > > > shows that an automated checker is required.\n> > > > >\n> > > > > Add a trailers checker to checkstyle.py to catch invalid or malformed\n> > > > > trailers, with a set of supported trailers that match libcamera's commit\n> > > > > message practices. New trailer keys can easily be added later as new\n> > > > > needs arise.\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > This reports a total of 42 issues through the project's history. 37 of\n> > > > > them should not be controversial:\n> > > > >\n> > > > > - Trailer keys not valid for libcamera ('Cc', 'Change-Id', 'Inspired-by'\n> > > > > and 'Reported-on')\n> > > > >\n> > > > > - Typos in trailer keys ('Fixed' instead of 'Fixes', 'Reviewed' instead\n> > > > > of 'Reviewed-by' and 'Signed-off-By' instead of 'Signed-off-by')\n> > > > >\n> > > > > - Typos in e-mail address (missing display name, missing space before\n> > > > > '<' and missing trailing '>')\n> > > > >\n> > > > > - Link in 'Fixes' trailer (should be a commit)\n> > > > >\n> > > > > - Too short commit ID in 'Fixes' trailer\n> > > > >\n> > > > > - Typos in 'Fixes' trailer (extra 'commit' before commit ID, missing\n> > > > > space or extra ':' after commit ID, missing '\"' around commit subject)\n> > > > >\n> > > > > The five remaining issues may benefit from discussions:\n> > > > >\n> > > > > - Invalid trailer key 'Co-developed-by' (one instance). This is a\n> > > > > trailer key commonly used in the kernel, but git..b both recommend\n> > > > > Co-authored-by. I'm sure which option would be best, so I haven't\n> > > > > included either for now.\n> > > > >\n> > > > > - Typo in 'Reported-by' trailer for issues reported by Coverity (one\n> > > > > instance). 'Reported-by' usually has an e-mail address value, but we\n> > > > > have commonly used 'Coverity CID=' for issues reported by\n> > > > > Coverity. I've tentatively added support for this (feedback is\n> > > > > welcome), and one commit still got flagged as its 'Reported-by'\n> > > > > trailer has a space instead of an '=' after 'CID'.\n> > > > >\n> > > > > - Usage of a github user URL in 'Reported-by' (one instance). Our policy\n> > > > > is to be able to identify users by name and e-mail address for\n> > > > > 'Signed-off-by' trailers, and I would prefer covering 'Reported-by'\n> > > > > trailers too. If someone *really* doesn't want their name included in\n> > > > > the git log when reporting an issue, we can simply omit the\n> > > > > 'Reported-by' trailer.\n> > > > >\n> > > > > - Usage of URLs in 'Reported-by' to point to buildbot.libcamera.org (two\n> > > > > instances). This should use a 'Link' trailer instead.\n> > > > > ---\n> > > > > utils/checkstyle.py | 80 +++++++++++++++++++++++++++++++++++++++++++--\n> > > > > 1 file changed, 78 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> > > > > index e68c874609bc..3558740d389d 100755\n> > > > > --- a/utils/checkstyle.py\n> > > > > +++ b/utils/checkstyle.py\n> > > > > @@ -210,13 +210,23 @@ class Commit:\n> > > > >\n> > > > > def _parse(self):\n> > > > > # Get the commit title and list of files.\n> > > > > - ret = subprocess.run(['git', 'show', '--format=%s', '--name-status',\n> > > > > + ret = subprocess.run(['git', 'show', '--format=%s%n%(trailers:only,unfold)', '--name-status',\n> > > > > self.commit],\n> > > > > stdout=subprocess.PIPE).stdout.decode('utf-8')\n> > > > > lines = ret.splitlines()\n> > > > > - self._files = [CommitFile(f) for f in lines[1:] if f]\n> > > > > +\n> > > > > self._title = lines[0]\n> > > > >\n> > > > > + self._trailers = []\n> > > > > + for index in range(1, len(lines)):\n> > > > > + line = lines[index]\n> > > > > + if not line:\n> > > > > + break\n> > > > > +\n> > > > > + self._trailers.append(line)\n> > > > > +\n> > > > > + self._files = [CommitFile(f) for f in lines[index:] if f]\n> > > > > +\n> > > > > def files(self, filter='AMR'):\n> > > > > return [f.filename for f in self._files if f.status in filter]\n> > > > >\n> > > > > @@ -224,6 +234,10 @@ class Commit:\n> > > > > def title(self):\n> > > > > return self._title\n> > > > >\n> > > > > + @property\n> > > > > + def trailers(self):\n> > > > > + return self._trailers\n> > > > > +\n> > > > > def get_diff(self, top_level, filename):\n> > > > > diff = subprocess.run(['git', 'diff', '%s~..%s' % (self.commit, self.commit),\n> > > > > '--', '%s/%s' % (top_level, filename)],\n> > > > > @@ -424,6 +438,68 @@ class TitleChecker(CommitChecker):\n> > > > > 'possible candidates are ' + candidates)]\n> > > > >\n> > > > >\n> > > > > +class TrailersChecker(CommitChecker):\n> > > > > + commit_regex = re.compile(r'[0-9a-f]{12}[0-9a-f]* \\(\".*\"\\)')\n> > > > > +\n> > > > > + coverity_regex = re.compile(r'Coverity CID=.*')\n> > > > > +\n> > > > > + # Simple e-mail address validator regex, with an additional trailing\n> > > > > + # comment. The complexity of a full RFC6531 validator isn't worth the\n> > > > > + # additional invalid addresses it would reject.\n> > > > > + email_regex = re.compile(r'[^<]+ <[^@>]+@[^>]+>( # .*)?')\n> > > > > +\n> > > > > + link_regex = re.compile(r'https?://.*')\n> > > > > +\n> > > > > + @staticmethod\n> > > > > + def validate_reported_by(value):\n> > > > > + if TrailersChecker.email_regex.fullmatch(value):\n> > > > > + return True\n> > > > > + if TrailersChecker.coverity_regex.fullmatch(value):\n> > > > > + return True\n> > > > > + return False\n> > > > > +\n> > > > > + known_trailers = {\n> > > > > + 'Acked-by': email_regex,\n> > > > > + 'Bug': link_regex,\n> > > > > + 'Fixes': commit_regex,\n> > > > > + 'Link': link_regex,\n> > > > > + 'Reported-by': validate_reported_by,\n> > > > > + 'Reviewed-by': email_regex,\n> > > > > + 'Signed-off-by': email_regex,\n> > > > > + 'Suggested-by': email_regex,\n> > > > > + 'Tested-by': email_regex,\n> > > > > + }\n> > > > > +\n> > > > > + trailer_regex = re.compile(r'([A-Z][a-zA-Z-]*)\\s*:\\s*(.*)')\n> > > > > +\n> > > > > + @classmethod\n> > > > > + def check(cls, commit, top_level):\n> > > > > + issues = []\n> > > > > +\n> > > > > + for trailer in commit.trailers:\n> > > > > + match = TrailersChecker.trailer_regex.fullmatch(trailer)\n> > > > > + if not match:\n> > > > > + raise RuntimeError(f\"Malformed commit trailer '{trailer}'\")\n> > > > > +\n> > > > > + key, value = match.groups()\n> > > > > +\n> > > > > + validator = TrailersChecker.known_trailers.get(key)\n> > > > > + if not validator:\n> > > > > + issues.append(CommitIssue(f\"Invalid commit trailer key '{key}'\"))\n> > > > > + continue\n> > > > > +\n> > > > > + if isinstance(validator, re.Pattern):\n> > > > > + valid = bool(validator.fullmatch(value))\n> > > > > + else:\n> > > > > + valid = validator(value)\n> > > > > +\n> > > > > + if not valid:\n> > > > > + issues.append(CommitIssue(f\"Malformed value '{value}' for commit trailer '{key}'\"))\n> > > > > + continue\n> > > > > +\n> > > > > + return issues\n> > > > > +\n> > > > > +\n> > > > > # ------------------------------------------------------------------------------\n> > > > > # Style Checkers\n> > > > > #\n> > > > > --\n> > > > > Regards,\n> > > > >\n> > > > > Laurent Pinchart\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BFD04BEFBE\n\tfor ;\n\tTue, 11 Jul 2023 09:05:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F295B628BF;\n\tTue, 11 Jul 2023 11:05:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E690E61E37\n\tfor ;\n\tTue, 11 Jul 2023 11:05:17 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A81B8CC;\n\tTue, 11 Jul 2023 11:04:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689066319;\n\tbh=d9WfJkgpSldkhWzWiuTnaNQ3ZifEDqItbWSKmPrjPf8=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=BEkL8sT2rxgAv1joKFguLU8SaFLvE/kvGo0zn+PwOiRHa1vVApQNu9d9PcxCuELsm\n\tkVmdboZ9/InWbyXccWX8YooDNkF7YqVLkQb3q7n7x/JtT3EVzBxgeoFjtbLh2jEMwp\n\tQp0VeQgmej15rUb0MtE+pEL1DAyc40Rp1KqTpRvRNUeIYqXwpqE5Km6mkE1bHYcOxM\n\tMdS5CYO1gKraF4TA7gNDBLDF0UGVXUNTb5Ihsgkc3HjfaLC87oQ/P1h2lKp1M9+J7u\n\twWrlCNl4jJL4n1std0IcznfEXpRmBOg5spahu8haTsXj4eBYUS2MOWUksWw48CO4jO\n\tNLSF+63lygbZg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689066269;\n\tbh=d9WfJkgpSldkhWzWiuTnaNQ3ZifEDqItbWSKmPrjPf8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JezkuGBuBUvGjsTNrMIiO1s2Wj90mlwNlhKm0+KkVdEp572mIaL2sgzpCK4VzneD8\n\tx6vYXsyv4U+LeecbGHLqTFsFqeYQ3QQ3MIGHDSUAcgxSDi9FggJRz7V+m2/xpNdMix\n\tuoXSJ7hW/W0VNpUDwWFAGa6bhSK1t8U323P/1OrU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JezkuGBu\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<168906248863.3585053.17970319067618110479@Monstersaurus>","References":"<20230612224751.4437-1-laurent.pinchart@ideasonboard.com>\n\t<20230612224751.4437-5-laurent.pinchart@ideasonboard.com>\n\t\n\t<168900428498.3585053.8193941688746059923@Monstersaurus>\n\t\n\t<168906248863.3585053.17970319067618110479@Monstersaurus>","To":"Naushir Patuck ","Date":"Tue, 11 Jul 2023 10:05:14 +0100","Message-ID":"<168906631496.3585053.10467348720232015988@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] utils: checkstyle: Add\n\ttrailers checker","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Naushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]