[RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[Sergey Senozhatsky]

If a patch contains "commit hash (commit name)", in other words
if the patch fixes some particular commit, then require "Fixes:"
tag.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Change-Id: I066ae0e607a67ac83c6ba0ff8d883f7e7a4a73e0
---
 scripts/checkpatch.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index df8b23dc1eb0..4e97d0f6b995 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2493,6 +2493,9 @@ sub process {
 	my $commit_log_has_diff = 0;
 	my $reported_maintainer_file = 0;
 	my $non_utf8_charset = 0;
+	my $fixes_id = 0;
+	my $fixes_description = 0;
+	my $fixes_tag = 0;
 
 	my $last_blank_line = 0;
 	my $last_coalesced_string_linenr = -1;
@@ -3076,6 +3079,11 @@ sub process {
 			}
 		}
 
+# Check for Fixes tag
+		if ($in_commit_log && $line =~ /\b[Ff]ixes:\s*[0-9a-f]{12,40}/) {
+			$fixes_tag = 1;
+		}
+
 # Check for git id commit length and improperly formed commit descriptions
 		if ($in_commit_log && !$commit_log_possible_stack_dump &&
 		    $line !~ /^\s*(?:Link|Patchwork|http|https|BugLink|base-commit):/i &&
@@ -3128,6 +3136,11 @@ sub process {
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
+			if (defined($id)) {
+				$fixes_id = $id;
+				$fixes_description = $description;
+			}
+
 			if (defined($id) &&
 			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
 				ERROR("GIT_COMMIT_ID",
@@ -7326,6 +7339,12 @@ sub process {
 		}
 	}
 
+	if ($is_patch && $has_commit_log && $fixes_id && !$fixes_tag) {
+		ERROR("GIT_COMMIT_ID",
+		      "Please add 'Fixes: $fixes_id (\"$fixes_description\")'\n", );
+		$fixes_id = 0;
+	}
+
 	print report_dump();
 	if ($summary && !($clean == 1 && $quiet == 1)) {
 		print "$filename " if ($summary_file);
-- 
2.34.1.703.g22d0c6ccf7-goog

Re: [RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[Sergey Senozhatsky]

On (22/01/19 16:46), Sergey Senozhatsky wrote:
>
> Change-Id: I066ae0e607a67ac83c6ba0ff8d883f7e7a4a73e0
>

It was not supposed to be there, but it's there (spotted by Tomasz).
Let me know if I need to re-send.

Re: [RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[Joe Perches]

On Wed, 2022-01-19 at 16:46 +0900, Sergey Senozhatsky wrote:
> If a patch contains "commit hash (commit name)", in other words
> if the patch fixes some particular commit, then require "Fixes:"
> tag.

I do not like this patch as many commits merely reference a
previous patch and do not actually fix anything.

Re: [RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[Andy Whitcroft]

On Wed, Jan 19, 2022 at 9:42 AM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2022-01-19 at 16:46 +0900, Sergey Senozhatsky wrote:
> > If a patch contains "commit hash (commit name)", in other words
> > if the patch fixes some particular commit, then require "Fixes:"
> > tag.
>
> I do not like this patch as many commits merely reference a
> previous patch and do not actually fix anything.

Agree.  It would need to be a tighter form of language to be safe to
automatically suggest a Fixes tag.  The point of a Fixes tag is to be
a semantically safe indicator of this relationship not relying on the
vagaries of English for that connection.

You might be ok with something which is a tighter match on like
"fixes <hash> (<name>)" and only suggesting a Fixes.

-apw

Re: [RESEND][PATCH] checkpatch: make sure fix-up patches have Fixes tag

[Andrew Morton]

On Wed, 19 Jan 2022 09:48:32 +0000 Andy Whitcroft <apw@canonical.com> wrote:

> On Wed, Jan 19, 2022 at 9:42 AM Joe Perches <joe@perches.com> wrote:
> >
> > On Wed, 2022-01-19 at 16:46 +0900, Sergey Senozhatsky wrote:
> > > If a patch contains "commit hash (commit name)", in other words
> > > if the patch fixes some particular commit, then require "Fixes:"
> > > tag.
> >
> > I do not like this patch as many commits merely reference a
> > previous patch and do not actually fix anything.
> 
> Agree.  It would need to be a tighter form of language to be safe to
> automatically suggest a Fixes tag.  The point of a Fixes tag is to be
> a semantically safe indicator of this relationship not relying on the
> vagaries of English for that connection.
> 
> You might be ok with something which is a tighter match on like
> "fixes <hash> (<name>)" and only suggesting a Fixes.

Also.

stable tree maintainers appear to have the habit of taking anything
which has Fixes and cheerfully backporting it.  Sometimes undesirably. 
This patch will encourage people to worsen this problem.

I wish this would simply stop, kernel-wide.  Make developers and
tree-owners actually *think* about the backport desirability.

If that were the global approach then checkpatch could

a) ask developers if they should have added "Fixes:" (this patch) then

b) if it has "Fixes:", ask developers if they should have added cc:stable.