scripts/checkpatch: refactor & fix SPDX-License-Identifier detection

[PATCH v2 3/7] scripts/checkpatch: introduce tracking of file start/end

Posted by Daniel P. Berrangé 6 months, 1 week ago

From: Daniel P. Berrangé <berrange@redhat.com>

Some checks want to be performed either at the start of a new file
within a patch, or at the end. This is complicated by the fact that
the information relevant to the check may be spread across multiple
lines. It is further complicated by a need to support both git and
non-git diffs, and special handling for renames where there might
not be any patch hunks.

To handle this more sanely, introduce explicit tracking of file
start/end, taking account of git metadata, and calling a hook
function at each transition.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 109 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7675418b0b..b74391e63a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1417,6 +1417,38 @@ sub checkspdx {
     }
 }
 
+# All three of the methods below take a 'file info' record
+# which is a hash ref containing
+#
+#  'isgit': is this from an enhanced git diff or plain diff
+#  'linestart': line number of start of file diff
+#  'lineend': line number of end of file diff
+#  'filenew': the new filename
+#  'fileold': the old filename (same as 'new filename' except
+#             for renames in git diffs)
+#  'action': one of 'modified' (always) or 'new' or 'deleted' or
+#            'renamed' (git diffs only)
+#  'mode': file mode for new/deleted files (git diffs only)
+#  'similarity': file similarity when renamed (git diffs only)
+#  'facts': hash ref for storing any metadata related to checks
+#
+
+# Called at the end of each patch, with the list of
+# real filenames that were seen in the patch
+sub process_file_list {
+	my @fileinfos = @_;
+}
+
+# Called at the start of processing a diff hunk for a file
+sub process_start_of_file {
+	my $fileinfo = shift;
+}
+
+# Called at the end of processing a diff hunk for a file
+sub process_end_of_file {
+	my $fileinfo = shift;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1453,7 +1485,10 @@ sub process {
 	my $realfile = '';
 	my $realline = 0;
 	my $realcnt = 0;
+	my $fileinfo;
+	my @fileinfolist;
 	my $here = '';
+	my $oldhere = '';
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
@@ -1591,17 +1626,56 @@ sub process {
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
 
+		$oldhere = $here;
 		$here = "#$linenr: " if (!$file);
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
-			$realfile =~ s@^([^/]*)/@@ if (!$file);
+		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
+			my $fileold = $1;
+			my $filenew = $2;
+
+			if (defined $fileinfo) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo)
+		        }
+			$fileold =~ s@^([^/]*)/@@ if (!$file);
+			$filenew =~ s@^([^/]*)/@@ if (!$file);
+			$realfile = $filenew;
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
+
+			$fileinfo = {
+				"isgit" => 1,
+				"githeader" => 1,
+				"linestart" => $here,
+				"lineend" => 0,
+				"fileold" => $fileold,
+				"filenew" => $filenew,
+				"action" => "modified",
+				"mode" => 0,
+				"similarity" => 0,
+				"facts" => {},
+		        };
+			push @fileinfolist, $fileinfo;
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^(new|deleted) (?:file )?mode\s+([0-7]+)$/) {
+			$fileinfo->{action} = $1;
+			$fileinfo->{mode} = oct($2);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^similarity index (\d+)%/) {
+			$fileinfo->{similarity} = int($1);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^rename (from|to) [\w\/\.\-]+\s*$/) {
+			$fileinfo->{action} = "renamed";
+			# For a no-change rename, we'll never have any "+++..."
+			# lines, so trigger actions now
+			if ($1 eq "to" && $fileinfo->{similarity} == 100)  {
+				process_start_of_file($fileinfo);
+			}
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
+
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
 
 			$p1_prefix = $1;
@@ -1610,6 +1684,30 @@ sub process {
 				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
 			}
 
+			if (defined $fileinfo && !$fileinfo->{isgit}) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo);
+			}
+
+			if (!defined $fileinfo || !$fileinfo->{isgit}) {
+				$fileinfo = {
+					"isgit" => 0,
+					"githeader" => 0,
+					"linestart" => $here,
+					"lineend" => 0,
+					"fileold" => $realfile,
+					"filenew" => $realfile,
+					"action" => "modified",
+					"mode" => 0,
+					"similarity" => 0,
+					"facts" => {},
+			        };
+				push @fileinfolist, $fileinfo;
+			} else {
+				$fileinfo->{githeader} = 0;
+			}
+			process_start_of_file($fileinfo);
+
 			next;
 		}
 
@@ -3213,6 +3311,11 @@ sub process {
 		}
 	}
 
+	if (defined $fileinfo) {
+		process_end_of_file($fileinfo);
+	}
+	process_file_list(@fileinfolist);
+
 	if ($is_patch && $chk_signoff && $signoff == 0) {
 		ERROR("Missing Signed-off-by: line(s)\n");
 	}
-- 
2.49.0

Re: [PATCH v2 3/7] scripts/checkpatch: introduce tracking of file start/end

Posted by Cédric Le Goater 6 months ago

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> Some checks want to be performed either at the start of a new file
> within a patch, or at the end. This is complicated by the fact that
> the information relevant to the check may be spread across multiple
> lines. It is further complicated by a need to support both git and
> non-git diffs, and special handling for renames where there might
> not be any patch hunks.
> 
> To handle this more sanely, introduce explicit tracking of file
> start/end, taking account of git metadata, and calling a hook
> function at each transition.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   scripts/checkpatch.pl | 109 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7675418b0b..b74391e63a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1417,6 +1417,38 @@ sub checkspdx {
>       }
>   }
>   
> +# All three of the methods below take a 'file info' record
> +# which is a hash ref containing
> +#
> +#  'isgit': is this from an enhanced git diff or plain diff
> +#  'linestart': line number of start of file diff
> +#  'lineend': line number of end of file diff
> +#  'filenew': the new filename
> +#  'fileold': the old filename (same as 'new filename' except
> +#             for renames in git diffs)
> +#  'action': one of 'modified' (always) or 'new' or 'deleted' or
> +#            'renamed' (git diffs only)

It would be nice to have some support for 'quilt' patches too. This would
mean being able to match '(---|+++) /dev/null' for new and deleted files.
Anyhow, that's for another series.

> +#  'mode': file mode for new/deleted files (git diffs only)
> +#  'similarity': file similarity when renamed (git diffs only)
> +#  'facts': hash ref for storing any metadata related to checks
> +#
> +
> +# Called at the end of each patch, with the list of
> +# real filenames that were seen in the patch
> +sub process_file_list {
> +	my @fileinfos = @_;
> +}
> +
> +# Called at the start of processing a diff hunk for a file
> +sub process_start_of_file {
> +	my $fileinfo = shift;
> +}
> +
> +# Called at the end of processing a diff hunk for a file
> +sub process_end_of_file {
> +	my $fileinfo = shift;
> +}
> +
>   sub process {
>   	my $filename = shift;
>   
> @@ -1453,7 +1485,10 @@ sub process {
>   	my $realfile = '';
>   	my $realline = 0;
>   	my $realcnt = 0;
> +	my $fileinfo;
> +	my @fileinfolist;
>   	my $here = '';
> +	my $oldhere = '';
>   	my $in_comment = 0;
>   	my $comment_edge = 0;
>   	my $first_line = 0;
> @@ -1591,17 +1626,56 @@ sub process {
>   		$prefix = "$filename:$realline: " if ($emacs && $file);
>   		$prefix = "$filename:$linenr: " if ($emacs && !$file);
>   
> +		$oldhere = $here;
>   		$here = "#$linenr: " if (!$file);
>   		$here = "#$realline: " if ($file);
>   
>   		# extract the filename as it passes
> -		if ($line =~ /^diff --git.*?(\S+)$/) {
> -			$realfile = $1;
> -			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
> +			my $fileold = $1;
> +			my $filenew = $2;
> +
> +			if (defined $fileinfo) {
> +				$fileinfo->{lineend} = $oldhere;
> +				process_end_of_file($fileinfo)
> +		        }
> +			$fileold =~ s@^([^/]*)/@@ if (!$file);
> +			$filenew =~ s@^([^/]*)/@@ if (!$file);
> +			$realfile = $filenew;
>   	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
> +
> +			$fileinfo = {
> +				"isgit" => 1,
> +				"githeader" => 1,
> +				"linestart" => $here,
> +				"lineend" => 0,
> +				"fileold" => $fileold,
> +				"filenew" => $filenew,
> +				"action" => "modified",
> +				"mode" => 0,
> +				"similarity" => 0,
> +				"facts" => {},
> +		        };
> +			push @fileinfolist, $fileinfo;
> +		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
> +			 $line =~ /^(new|deleted) (?:file )?mode\s+([0-7]+)$/) {
> +			$fileinfo->{action} = $1;
> +			$fileinfo->{mode} = oct($2);
> +		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
> +			 $line =~ /^similarity index (\d+)%/) {
> +			$fileinfo->{similarity} = int($1);
> +		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
> +			 $line =~ /^rename (from|to) [\w\/\.\-]+\s*$/) {
> +			$fileinfo->{action} = "renamed";
> +			# For a no-change rename, we'll never have any "+++..."
> +			# lines, so trigger actions now
> +			if ($1 eq "to" && $fileinfo->{similarity} == 100)  {
> +				process_start_of_file($fileinfo);
> +			}
>   		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>   			$realfile = $1;
>   			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +
>   	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
>   
>   			$p1_prefix = $1;
> @@ -1610,6 +1684,30 @@ sub process {
>   				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
>   			}
>   
> +			if (defined $fileinfo && !$fileinfo->{isgit}) {
> +				$fileinfo->{lineend} = $oldhere;
> +				process_end_of_file($fileinfo);
> +			}
> +
> +			if (!defined $fileinfo || !$fileinfo->{isgit}) {
> +				$fileinfo = {
> +					"isgit" => 0,
> +					"githeader" => 0,
> +					"linestart" => $here,
> +					"lineend" => 0,
> +					"fileold" => $realfile,
> +					"filenew" => $realfile,
> +					"action" => "modified",
> +					"mode" => 0,
> +					"similarity" => 0,
> +					"facts" => {},
> +			        };
> +				push @fileinfolist, $fileinfo;
> +			} else {
> +				$fileinfo->{githeader} = 0;
> +			}
> +			process_start_of_file($fileinfo);
> +
>   			next;
>   		}
>   
> @@ -3213,6 +3311,11 @@ sub process {
>   		}
>   	}
>   
> +	if (defined $fileinfo) {
> +		process_end_of_file($fileinfo);
> +	}
> +	process_file_list(@fileinfolist);
> +
>   	if ($is_patch && $chk_signoff && $signoff == 0) {
>   		ERROR("Missing Signed-off-by: line(s)\n");
>   	}

[PATCH v2 1/7] Revert "scripts: mandate that new files have SPDX-License-Identifier"
[PATCH v2 2/7] scripts/checkpatch.pl: fix various indentation mistakes
[PATCH v2 3/7] scripts/checkpatch: introduce tracking of file start/end
[PATCH v2 4/7] scripts/checkpatch: use new hook for ACPI test data check
[PATCH v2 5/7] scripts/checkpatch: use new hook for file permissions check
[PATCH v2 6/7] scripts/checkpatch: use new hook for MAINTAINERS update check
[PATCH v2 7/7] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier