Add a warning for fixes tags that does not follow community conventions.
Signed-off-by: Niklas Söderlund <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Reviewed-by: Louis Peens <[email protected]>
Reviewed-by: Philippe Schenker <[email protected]>
Acked-by: Dwaipayan Ray <[email protected]>
Reviewed-by: Lukas Bulwahn <[email protected]>
Acked-by: Lukas Bulwahn <[email protected]>
---
* Changes since v6
- Update first check to make sure that there is a likely SHA1 of some
minimum length after the fixes line.
- s/fall in line with community standard/follow community conventions/.
- Improve grammar, thanks Lukas.
* Changes since v5
- Add support for --fix option for checkpatch.pl.
* Changes since v4
- Extend test to cover lines with whitespace before the fixes: tag, e.g.
match check on /^\s*fixes:?/i.
* Changes since v3
- Add test that title in tag match title of commit referenced by sha1.
* Changes since v2
- Change the pattern to match on 'fixes:?' to catch more malformed
tags.
* Changes since v1
- Update the documentation wording and add mention one cause of the
message can be that email program splits the tag over multiple lines.
---
Documentation/dev-tools/checkpatch.rst | 7 ++++
scripts/checkpatch.pl | 44 ++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..c3389c6f3838 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -612,6 +612,13 @@ Commit message
See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+ **BAD_FIXES_TAG**
+ The Fixes: tag is malformed or does not follow the community conventions.
+ This can occur if the tag have been split into multiple lines (e.g., when
+ pasted in an email program with word wrapping enabled).
+
+ See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+
Comparison style
----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..ddc5c9d730c3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3140,6 +3140,50 @@ sub process {
}
}
+# Check Fixes: styles is correct
+ if (!$in_header_lines &&
+ $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
+ my $orig_commit = "";
+ my $id = "0123456789ab";
+ my $title = "commit title";
+ my $tag_case = 1;
+ my $tag_space = 1;
+ my $id_length = 1;
+ my $id_case = 1;
+ my $title_has_quotes = 0;
+
+ if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
+ my $tag = $1;
+ $orig_commit = $2;
+ $title = $3;
+
+ $tag_case = 0 if $tag eq "Fixes:";
+ $tag_space = 0 if ($line =~ /^fixes:? [0-9a-f]{5,} ($balanced_parens)/i);
+
+ $id_length = 0 if ($orig_commit =~ /^[0-9a-f]{12}$/i);
+ $id_case = 0 if ($orig_commit !~ /[A-F]/);
+
+ # Always strip leading/trailing parens then double quotes if existing
+ $title = substr($title, 1, -1);
+ if ($title =~ /^".*"$/) {
+ $title = substr($title, 1, -1);
+ $title_has_quotes = 1;
+ }
+ }
+
+ my ($cid, $ctitle) = git_commit_info($orig_commit, $id,
+ $title);
+
+ if ($ctitle ne $title || $tag_case || $tag_space ||
+ $id_length || $id_case || !$title_has_quotes) {
+ if (WARN("BAD_FIXES_TAG",
+ "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $cid (\"$ctitle\")'\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] = "Fixes: $cid (\"$ctitle\")";
+ }
+ }
+ }
+
# Check email subject for common tools that don't need to be mentioned
if ($in_header_lines &&
$line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
--
2.37.3
On Wed, 2022-09-14 at 12:02 +0200, Niklas S?derlund wrote:
> Add a warning for fixes tags that does not follow community conventions.
[]
> * Changes since v6
> - Update first check to make sure that there is a likely SHA1 of some
> minimum length after the fixes line.
https://lore.kernel.org/lkml/[email protected]/
The goal here should be to identify a line that looks like a commit
reference.
So find lines that starts with 'fixes' and have a SHA1 commit id as
broadly as reasonable.
Did you run the grep pattern and look at the results?
One grep pattern to verify the non canonical fixes format that
are mistakenly used is:
$ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
grep -P -i '^\s*fixes' | \
grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
[]
There are many different styles.
Parenthesea are sometimes not used.
> + if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
How about some pattern like
/fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i
or maybe even more broadly:
/fixes\b.*\b[0-9a-f]{5,}\b/i
Hi Joe,
On 2022-09-14 09:09:25 -0700, Joe Perches wrote:
> On Wed, 2022-09-14 at 12:02 +0200, Niklas S?derlund wrote:
> > Add a warning for fixes tags that does not follow community conventions.
> []
> > * Changes since v6
> > - Update first check to make sure that there is a likely SHA1 of some
> > minimum length after the fixes line.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> The goal here should be to identify a line that looks like a commit
> reference.
>
> So find lines that starts with 'fixes' and have a SHA1 commit id as
> broadly as reasonable.
>
> Did you run the grep pattern and look at the results?
>
> One grep pattern to verify the non canonical fixes format that
> are mistakenly used is:
>
> $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
> grep -P -i '^\s*fixes' | \
> grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
>
> []
>
> There are many different styles.
> Parenthesea are sometimes not used.
I understand this, and I did have a look at it.
>
> > + if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
>
> How about some pattern like
>
> /fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i
>
> or maybe even more broadly:
>
> /fixes\b.*\b[0-9a-f]{5,}\b/i
Maybe I misunderstand your comment, but this is what I do in this patch?
if (!$in_header_lines &&
$line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
...
if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
...
}
}
This will catch and warn about such tags but not attempt to break out
it's components in order to suggest a potentially more correct fix. Is
it this second filter you would like me to change?
--
Kind Regards,
Niklas S?derlund
Hello Joe,
Just wanted to check on my misunderstanding below before posting a v8.
On 2022-09-15 11:15:06 +0200, Niklas S?derlund wrote:
> Hi Joe,
>
> On 2022-09-14 09:09:25 -0700, Joe Perches wrote:
> > On Wed, 2022-09-14 at 12:02 +0200, Niklas S?derlund wrote:
> > > Add a warning for fixes tags that does not follow community conventions.
> > []
> > > * Changes since v6
> > > - Update first check to make sure that there is a likely SHA1 of some
> > > minimum length after the fixes line.
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > The goal here should be to identify a line that looks like a commit
> > reference.
> >
> > So find lines that starts with 'fixes' and have a SHA1 commit id as
> > broadly as reasonable.
> >
> > Did you run the grep pattern and look at the results?
> >
> > One grep pattern to verify the non canonical fixes format that
> > are mistakenly used is:
> >
> > $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
> > grep -P -i '^\s*fixes' | \
> > grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
> >
> > []
> >
> > There are many different styles.
> > Parenthesea are sometimes not used.
>
> I understand this, and I did have a look at it.
>
> >
> > > + if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> >
> > How about some pattern like
> >
> > /fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i
> >
> > or maybe even more broadly:
> >
> > /fixes\b.*\b[0-9a-f]{5,}\b/i
>
> Maybe I misunderstand your comment, but this is what I do in this patch?
>
> if (!$in_header_lines &&
> $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
>
> ...
>
> if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> ...
> }
> }
>
> This will catch and warn about such tags but not attempt to break out
> it's components in order to suggest a potentially more correct fix. Is
> it this second filter you would like me to change?
>
> --
> Kind Regards,
> Niklas S?derlund
--
Kind Regards,
Niklas S?derlund
On Wed, 2022-09-21 at 16:25 +0200, Niklas S?derlund wrote:
> Hello Joe,
Hello Niklas
> Just wanted to check on my misunderstanding below before posting a v8.
I think v7 is ok, unless you found something else better.
> > > One grep pattern to verify the non canonical fixes format that
> > > are mistakenly used is:
> > >
> > > $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
> > > grep -P -i '^\s*fixes' | \
> > > grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
> > > There are many different styles.
> > > Parentheses are sometimes not used.
> >
> > I understand this, and I did have a look at it.
Good enough for me. Thanks.
On Wed, 2022-09-14 at 12:02 +0200, Niklas S?derlund wrote:
> Add a warning for fixes tags that does not follow community conventions.
>
> Signed-off-by: Niklas S?derlund <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> Reviewed-by: Louis Peens <[email protected]>
> Reviewed-by: Philippe Schenker <[email protected]>
> Acked-by: Dwaipayan Ray <[email protected]>
> Reviewed-by: Lukas Bulwahn <[email protected]>
> Acked-by: Lukas Bulwahn <[email protected]>
Acked-by: Joe Perches <[email protected]>
> ---
> * Changes since v6
> - Update first check to make sure that there is a likely SHA1 of some
> minimum length after the fixes line.
> - s/fall in line with community standard/follow community conventions/.
> - Improve grammar, thanks Lukas.
>
> * Changes since v5
> - Add support for --fix option for checkpatch.pl.
>
> * Changes since v4
> - Extend test to cover lines with whitespace before the fixes: tag, e.g.
> match check on /^\s*fixes:?/i.
>
> * Changes since v3
> - Add test that title in tag match title of commit referenced by sha1.
>
> * Changes since v2
> - Change the pattern to match on 'fixes:?' to catch more malformed
> tags.
>
> * Changes since v1
> - Update the documentation wording and add mention one cause of the
> message can be that email program splits the tag over multiple lines.
> ---
> Documentation/dev-tools/checkpatch.rst | 7 ++++
> scripts/checkpatch.pl | 44 ++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index b52452bc2963..c3389c6f3838 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -612,6 +612,13 @@ Commit message
>
> See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
> + **BAD_FIXES_TAG**
> + The Fixes: tag is malformed or does not follow the community conventions.
> + This can occur if the tag have been split into multiple lines (e.g., when
> + pasted in an email program with word wrapping enabled).
> +
> + See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> +
>
> Comparison style
> ----------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 79e759aac543..ddc5c9d730c3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3140,6 +3140,50 @@ sub process {
> }
> }
>
> +# Check Fixes: styles is correct
> + if (!$in_header_lines &&
> + $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
> + my $orig_commit = "";
> + my $id = "0123456789ab";
> + my $title = "commit title";
> + my $tag_case = 1;
> + my $tag_space = 1;
> + my $id_length = 1;
> + my $id_case = 1;
> + my $title_has_quotes = 0;
> +
> + if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> + my $tag = $1;
> + $orig_commit = $2;
> + $title = $3;
> +
> + $tag_case = 0 if $tag eq "Fixes:";
> + $tag_space = 0 if ($line =~ /^fixes:? [0-9a-f]{5,} ($balanced_parens)/i);
> +
> + $id_length = 0 if ($orig_commit =~ /^[0-9a-f]{12}$/i);
> + $id_case = 0 if ($orig_commit !~ /[A-F]/);
> +
> + # Always strip leading/trailing parens then double quotes if existing
> + $title = substr($title, 1, -1);
> + if ($title =~ /^".*"$/) {
> + $title = substr($title, 1, -1);
> + $title_has_quotes = 1;
> + }
> + }
> +
> + my ($cid, $ctitle) = git_commit_info($orig_commit, $id,
> + $title);
> +
> + if ($ctitle ne $title || $tag_case || $tag_space ||
> + $id_length || $id_case || !$title_has_quotes) {
> + if (WARN("BAD_FIXES_TAG",
> + "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $cid (\"$ctitle\")'\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] = "Fixes: $cid (\"$ctitle\")";
> + }
> + }
> + }
> +
> # Check email subject for common tools that don't need to be mentioned
> if ($in_header_lines &&
> $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {