[PATCH 1/3] checkpatch: warn about novice phrases in commit messages

[PATCH 1/3] checkpatch: warn about novice phrases in commit messages

[Ignacio Peña]

Add detection for common phrases used by newcomers that make patches
look less professional, such as "please apply" or "please consider".

These phrases are unnecessary and can make the submission appear less
confident. The kernel development process expects direct, professional
communication without apologetic or pleading language.

This check helps newcomers learn the expected communication style,
reducing the chance of patches being dismissed due to poor presentation
rather than technical merit.

Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ignacio Pena <ignacio.pena87@gmail.com>
---
 scripts/checkpatch.pl | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa..7ccdc774a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3266,6 +3266,26 @@ sub process {
 			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
 		}
 
+
+# Check for novice phrases in commit message
+		if ($in_commit_log && !$non_utf8_charset) {
+			my @novice_phrases = (
+				qr/please\s+(apply|merge|consider|review)/i,
+				qr/hope\s+this\s+helps/i,
+				qr/my\s+first\s+(patch|contribution)/i,
+				qr/(newbie|beginner)\s+here/i,
+				qr/not\s+sure\s+if\s+(this\s+is\s+)?correct/i,
+				qr/sorry\s+(if|for)/i,
+			);
+
+			foreach my $phrase (@novice_phrases) {
+				if ($line =~ /$phrase/) {
+					WARN("COMMIT_MESSAGE_NOVICE",
+					     "Avoid apologetic or uncertain language - be direct and professional\n" . $herecurr);
+					last;
+				}
+			}
+		}
 # Check for Gerrit Change-Ids not in any patch context
 		if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) {
 			if (ERROR("GERRIT_CHANGE_ID",
-- 
2.50.1

[PATCH 2/3] checkpatch: enforce 12-char SHA for Fixes tags

[Ignacio Peña]

The kernel documentation recommends using 12 character SHA-1 hashes
in Fixes tags as the best practice. While 8 characters might be
sufficient for uniqueness in smaller projects, the kernel's large
history makes collisions more likely with shorter hashes.

Change the existing check from "at least 8" to "exactly 12" to
encourage consistency and future-proof practices.

Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ignacio Pena <ignacio.pena87@gmail.com>
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7ccdc774a..204800232 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3242,7 +3242,7 @@ sub process {
 			my $tag_case = not ($tag eq "Fixes:");
 			my $tag_space = not ($line =~ /^fixes:? [0-9a-f]{5,40} ($balanced_parens)/i);
 
-			my $id_length = not ($orig_commit =~ /^[0-9a-f]{12,40}$/i);
+			my $id_length = not ($orig_commit =~ /^[0-9a-f]{12}$/i);
 			my $id_case = not ($orig_commit !~ /[A-F]/);
 
 			my $id = "0123456789ab";
@@ -3252,7 +3252,7 @@ sub process {
 			if (defined($cid) && ($ctitle ne $title || $tag_case || $tag_space || $id_length || $id_case || !$title_has_quotes)) {
 				my $fixed = "Fixes: $cid (\"$ctitle\")";
 				if (WARN("BAD_FIXES_TAG",
-				     "Please use correct Fixes: style 'Fixes: <12+ chars of sha1> (\"<title line>\")' - ie: '$fixed'\n" . $herecurr) &&
+				     "Please use correct Fixes: style 'Fixes: <exactly 12 chars of sha1> (\"<title line>\")' - ie: '$fixed'\n" . $herecurr) &&
 				    $fix) {
 					$fixed[$fixlinenr] = $fixed;
 				}
-- 
2.50.1

Re: [PATCH 2/3] checkpatch: enforce 12-char SHA for Fixes tags

[Greg Kroah-Hartman]

On Mon, Jul 21, 2025 at 12:24:33PM -0400, Ignacio Peña wrote:
> The kernel documentation recommends using 12 character SHA-1 hashes
> in Fixes tags as the best practice. While 8 characters might be
> sufficient for uniqueness in smaller projects, the kernel's large
> history makes collisions more likely with shorter hashes.
> 
> Change the existing check from "at least 8" to "exactly 12" to
> encourage consistency and future-proof practices.

That's not what this patch does.  You are changing it from "12+" to
"exactly 12".  There's no 8 character check here that I can see.


> 
> Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ignacio Pena <ignacio.pena87@gmail.com>
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7ccdc774a..204800232 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3242,7 +3242,7 @@ sub process {
>  			my $tag_case = not ($tag eq "Fixes:");
>  			my $tag_space = not ($line =~ /^fixes:? [0-9a-f]{5,40} ($balanced_parens)/i);
>  
> -			my $id_length = not ($orig_commit =~ /^[0-9a-f]{12,40}$/i);

That is checking for 12-40 long, right?  Not 8 :)

thanks,

greg k-h

[PATCH 3/3] checkpatch: suggest including testing evidence

[Ignacio Peña]

For non-trivial changes, it's valuable to know how the change was
tested. Add a gentle suggestion when commit messages don't mention
any testing, verification, or validation.

This is a CHECK level notification, not a WARNING, as testing methods
vary greatly depending on the subsystem and type of change.

The check is skipped for very short commit messages (documentation
fixes, typos) where testing information would be excessive.

Link: https://docs.kernel.org/process/submitting-patches.html
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ignacio Pena <ignacio.pena87@gmail.com>
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 204800232..358310e6c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3319,6 +3319,16 @@ sub process {
 		      $commit_log_possible_stack_dump)) {
 			WARN("COMMIT_LOG_LONG_LINE",
 			     "Prefer a maximum 75 chars per line (possible unwrapped commit description?)\n" . $herecurr);
+
+# Suggest including test information for non-trivial changes
+		if ($in_commit_log && !$non_utf8_charset &&
+		    $line !~ /test|verify|tried|ran|checked|confirmed/i &&
+		    $commit_log_lines > 3) {
+			if ($commit_log_long_line eq "" && $commit_log_has_diff) {
+				CHECK("NO_TEST_INFO",
+				      "Consider mentioning how this change was tested\n" . $herecurr);
+			}
+		}
 			$commit_log_long_line = 1;
 		}
 
-- 
2.50.1

Re: [PATCH 3/3] checkpatch: suggest including testing evidence

[Greg Kroah-Hartman]

On Mon, Jul 21, 2025 at 12:24:34PM -0400, Ignacio Peña wrote:
> For non-trivial changes, it's valuable to know how the change was
> tested. Add a gentle suggestion when commit messages don't mention
> any testing, verification, or validation.
> 
> This is a CHECK level notification, not a WARNING, as testing methods
> vary greatly depending on the subsystem and type of change.
> 
> The check is skipped for very short commit messages (documentation
> fixes, typos) where testing information would be excessive.

This will cause people to write short commit messages in order to avoid
this type of warning.

I don't think this is a necessary check at all, I can't remember the
last time any patch I wrote would pass this check :)

thanks,

greg k-h

Re: [PATCH 1/3] checkpatch: warn about novice phrases in commit messages

[Joe Perches]

On Mon, 2025-07-21 at 12:24 -0400, Ignacio Peña wrote:
> Add detection for common phrases used by newcomers that make patches
> look less professional, such as "please apply" or "please consider".
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3266,6 +3266,26 @@ sub process {
>  			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
>  		}
>  
> +
> +# Check for novice phrases in commit message
> +		if ($in_commit_log && !$non_utf8_charset) {

why !$non_utf8_charset?

> +			my @novice_phrases = (
> +				qr/please\s+(apply|merge|consider|review)/i,
> +				qr/hope\s+this\s+helps/i,
> +				qr/my\s+first\s+(patch|contribution)/i,
> +				qr/(newbie|beginner)\s+here/i,
> +				qr/not\s+sure\s+if\s+(this\s+is\s+)?correct/i,
> +				qr/sorry\s+(if|for)/i,
> +			);

No capture groups - use (?:..)
				
> +			foreach my $phrase (@novice_phrases) {
> +				if ($line =~ /$phrase/) {
> +					WARN("COMMIT_MESSAGE_NOVICE",
> +					     "Avoid apologetic or uncertain language - be direct and professional\n" . $herecurr);
> +					last;
> +				}
> +			}
> +		}

And this could like use a single search rather than a list like:

			my $novice_phrases = qr{(?xi:
				please\s+(?:apply|merge|consider|review) |
				hope\s+this\s+helps |
				my\s+first\s+(?:patch|contribution) |
				(?:newbie|beginner)\s+here |
				not\s+sure\s+if\s+(?:this\s+is\s+)?correct |
				sorry\s+(?:if|for)
			)};

			if ($line =~ /\b$novice_phrase\b/) {
				WARN("COMMIT_MESSAGE_NOVICE",
				     "Avoid apologetic or uncertain language - be direct\n" . $herecurr);
			}

IMO professional is unnecessary.

[PATCH v2 1/2] checkpatch: warn about novice phrases in commit messages

[Ignacio Peña]

Add detection for common phrases that make patches appear less
confident. A single regex efficiently matches multiple patterns:
- 'please apply/merge/consider/review'
- 'hope this helps'
- 'my first patch/contribution'
- 'newbie/beginner here'
- 'not sure if (this is) correct'
- 'sorry if/for'

This check helps newcomers learn kernel communication style,
reducing patches dismissed for presentation rather than merit.

Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Ignacio Peña <ignacio.pena87@gmail.com>
---
 scripts/checkpatch.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa..6953ad515 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3266,6 +3266,15 @@ sub process {
 			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
 		}
 
+
+# Check for novice phrases in commit message
+		if ($in_commit_log && !$non_utf8_charset) {
+			# Single regex with all phrases, using non-capturing groups as Joe suggested
+			if ($line =~ /\b(?:please\s+(?:apply|merge|consider|review)|hope\s+this\s+helps|my\s+first\s+(?:patch|contribution)|(?:newbie|beginner)\s+here|not\s+sure\s+if\s+(?:this\s+is\s+)?correct|sorry\s+(?:if|for))\b/i) {
+				WARN("COMMIT_MESSAGE_NOVICE",
+				     "Avoid apologetic or uncertain language - be direct and confident\n" . $herecurr);
+			}
+		}
 # Check for Gerrit Change-Ids not in any patch context
 		if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) {
 			if (ERROR("GERRIT_CHANGE_ID",
-- 
2.50.1

[PATCH v2 2/2] checkpatch: enforce 12-char SHA for Fixes tags

[Ignacio Peña]

The kernel documentation recommends 12 character SHA-1 hashes
for Fixes tags. The current code allows 12-40 characters but
we should enforce exactly 12 for consistency.

The regex is changed from {12,40} to {12} and the warning
message updated accordingly.

Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ignacio Peña <ignacio.pena87@gmail.com>
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6953ad515..ea7b6c69e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3242,7 +3242,7 @@ sub process {
 			my $tag_case = not ($tag eq "Fixes:");
 			my $tag_space = not ($line =~ /^fixes:? [0-9a-f]{5,40} ($balanced_parens)/i);
 
-			my $id_length = not ($orig_commit =~ /^[0-9a-f]{12,40}$/i);
+			my $id_length = not ($orig_commit =~ /^[0-9a-f]{12}$/i);
 			my $id_case = not ($orig_commit !~ /[A-F]/);
 
 			my $id = "0123456789ab";
@@ -3252,7 +3252,7 @@ sub process {
 			if (defined($cid) && ($ctitle ne $title || $tag_case || $tag_space || $id_length || $id_case || !$title_has_quotes)) {
 				my $fixed = "Fixes: $cid (\"$ctitle\")";
 				if (WARN("BAD_FIXES_TAG",
-				     "Please use correct Fixes: style 'Fixes: <12+ chars of sha1> (\"<title line>\")' - ie: '$fixed'\n" . $herecurr) &&
+				     "Please use correct Fixes: style 'Fixes: <exactly 12 chars of sha1> (\"<title line>\")' - ie: '$fixed'\n" . $herecurr) &&
 				    $fix) {
 					$fixed[$fixlinenr] = $fixed;
 				}
-- 
2.50.1

Re: [PATCH v2 2/2] checkpatch: enforce 12-char SHA for Fixes tags

[Greg Kroah-Hartman]

On Tue, Jul 22, 2025 at 11:02:57PM -0400, Ignacio Peña wrote:
> The kernel documentation recommends 12 character SHA-1 hashes
> for Fixes tags. The current code allows 12-40 characters but
> we should enforce exactly 12 for consistency.
> 
> The regex is changed from {12,40} to {12} and the warning
> message updated accordingly.
> 
> Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ignacio Peña <ignacio.pena87@gmail.com>
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

No list of what changed from v1?

And also, why make this change at all?  12+ is fine, I haven't seen
people use huge values in the past, have you?

thanks,

greg k-h

Re: [PATCH v2 1/2] checkpatch: warn about novice phrases in commit messages

[Joe Perches]

On Tue, 2025-07-22 at 23:02 -0400, Ignacio Peña wrote:
> Add detection for common phrases that make patches appear less
> confident. A single regex efficiently matches multiple patterns:
> - 'please apply/merge/consider/review'
> - 'hope this helps'
> - 'my first patch/contribution'
> - 'newbie/beginner here'
> - 'not sure if (this is) correct'
> - 'sorry if/for'
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3266,6 +3266,15 @@ sub process {
>  			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
>  		}
>  
> +
> +# Check for novice phrases in commit message
> +		if ($in_commit_log && !$non_utf8_charset) {
> +			# Single regex with all phrases, using non-capturing groups as Joe suggested

The comment referencing a suggetion should be removed.

> +			if ($line =~ /\b(?:please\s+(?:apply|merge|consider|review)|hope\s+this\s+helps|my\s+first\s+(?:patch|contribution)|(?:newbie|beginner)\s+here|not\s+sure\s+if\s+(?:this\s+is\s+)?correct|sorry\s+(?:if|for))\b/i) {

I think this is unreadable.
The qr{(...)} was and might be a tad faster as it could be precompiled.

[PATCH v3] checkpatch: warn about novice phrases in commit messages

[Ignacio Peña]

Add detection for common phrases that make patches appear less
confident. These phrases are often used by newcomers and can make
their contributions seem less professional or uncertain.

The regex uses qr{} syntax as suggested for better readability and
potential pre-compilation benefits.

Examples of detected phrases:
- 'please apply/merge/consider/review'
- 'hope this helps'
- 'my first patch/contribution'
- 'newbie/beginner here'
- 'not sure if (this is) correct'
- 'sorry if/for'

This helps newcomers learn the expected communication style in
kernel development, where direct and confident communication is
preferred.

Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Ignacio Peña <ignacio.pena87@gmail.com>
---
Changes in v3:
- Use qr{} syntax instead of // for the regex (Joe Perches)
- Remove comment about the suggestion (Joe Perches)
- Drop the SHA enforcement patch based on maintainer feedback

Changes in v2:
- Combined multiple regexes into single expression with non-capturing groups
- Changed warning message from 'professional' to 'confident'
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa..ac270f35b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3266,6 +3266,14 @@ sub process {
 			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
 		}
 
+# Check for novice phrases in commit message
+		if ($in_commit_log && !$non_utf8_charset) {
+			if ($line =~ qr{\b(?:please\s+(?:apply|merge|consider|review)|hope\s+this\s+helps|my\s+first\s+(?:patch|contribution)|(?:newbie|beginner)\s+here|not\s+sure\s+if\s+(?:this\s+is\s+)?correct|sorry\s+(?:if|for))\b}i) {
+				WARN("COMMIT_MESSAGE_NOVICE",
+				     "Avoid apologetic or uncertain language - be direct and confident\n" . $herecurr);
+			}
+		}
+
 # Check for Gerrit Change-Ids not in any patch context
 		if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) {
 			if (ERROR("GERRIT_CHANGE_ID",
-- 
2.50.1

Re: [PATCH v3] checkpatch: warn about novice phrases in commit messages

[Joe Perches]

On Wed, 2025-07-23 at 23:28 -0400, Ignacio Peña wrote:
> Add detection for common phrases that make patches appear less
> confident. These phrases are often used by newcomers and can make
> their contributions seem less professional or uncertain.
> 
> The regex uses qr{} syntax as suggested for better readability and
> potential pre-compilation benefits.

Unneessary paragraph.

> 
> Examples of detected phrases:
> - 'please apply/merge/consider/review'
> - 'hope this helps'
> - 'my first patch/contribution'
> - 'newbie/beginner here'
> - 'not sure if (this is) correct'
> - 'sorry if/for'
> 
> This helps newcomers learn the expected communication style in
> kernel development, where direct and confident communication is
> preferred.
> 
> Link: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> Suggested-by: Joe Perches <joe@perches.com>

I did not suggest this.
I am merely trying to improve the patch and its readability.
I do not need to have any credit for this.

> Signed-off-by: Ignacio Peña <ignacio.pena87@gmail.com>
> ---
> Changes in v3:
> - Use qr{} syntax instead of // for the regex (Joe Perches)
> - Remove comment about the suggestion (Joe Perches)
> - Drop the SHA enforcement patch based on maintainer feedback

This last line should be part of a cover letter to the patch set
rather than added to this specific patch.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3266,6 +3266,14 @@ sub process {
>  			     "A patch subject line should describe the change not the tool that found it\n" . $herecurr);
>  		}
>  
> +# Check for novice phrases in commit message
> +		if ($in_commit_log && !$non_utf8_charset) {

You still haven't answered why $!non_utf8_charset is useful.

> +			if ($line =~ qr{\b(?:please\s+(?:apply|merge|consider|review)|hope\s+this\s+helps|my\s+first\s+(?:patch|contribution)|(?:newbie|beginner)\s+here|not\s+sure\s+if\s+(?:this\s+is\s+)?correct|sorry\s+(?:if|for))\b}i) {

And this is still not human readable.
I much prefer describing each phrase on a separate line like:

			my $novice_phrases = qr{(?xi:
				please\s+(?:apply|merge|consider|review) |
				hope\s+this\s+helps |
				my\s+first\s+(?:patch|contribution) |
				(?:newbie|beginner)\s+here |
				not\s+sure\s+if\s+(?:this\s+is\s+)?correct |
				sorry\s+(?:if|for)
			)};

			if ($line =~ /\b$novice_phrase\b/) {