[PATCH] checkpatch: enforce process for expected files

[PATCH] checkpatch: enforce process for expected files

[Michael S. Tsirkin]

If the process documented in tests/qtest/bios-tables-test.c
is followed, then same patch never touches both expected
files and code. Teach checkpatch to enforce this rule.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Peter, Igor what do you think?

 scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b27e4ff5e9..96583e3fff 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -35,6 +35,8 @@ my $summary_file = 0;
 my $root;
 my %debug;
 my $help = 0;
+my $testexpected;
+my $nontestexpected;
 
 sub help {
 	my ($exitcode) = @_;
@@ -1256,6 +1258,26 @@ sub WARN {
 	}
 }
 
+# According to tests/qtest/bios-tables-test.c: do not
+# change expected file in the same commit with adding test
+sub checkfilename {
+	my ($name) = @_;
+        if ($name =~ m#^tests/data/acpi/# and
+            # make exception for a shell script that rebuilds the files
+            not $name =~ m#^\.sh$# or
+            $name =~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
+            $testexpected = $name;
+        } else {
+            $nontestexpected = $name;
+        }
+        if (defined $testexpected and defined $nontestexpected) {
+            ERROR("Do not add expected files together with tests, " .
+                  "follow instructions in " .
+                  "tests/qtest/bios-tables-test.c: both " .
+                  $testexpected . " and " . $nontestexpected . " found\n");
+        }
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1431,9 +1453,11 @@ sub process {
 		if ($line =~ /^diff --git.*?(\S+)$/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
+                        checkfilename($realfile);
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
+                        checkfilename($realfile);
 
 			$p1_prefix = $1;
 			if (!$file && $tree && $p1_prefix ne '' &&
-- 
MST

Re: [PATCH] checkpatch: enforce process for expected files

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Sun, Mar 15, 2020 at 07:35:46AM -0400, Michael S. Tsirkin wrote:
> If the process documented in tests/qtest/bios-tables-test.c
> is followed, then same patch never touches both expected
> files and code. Teach checkpatch to enforce this rule.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Peter, Igor what do you think?

Minor comments below:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> 
>  scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b27e4ff5e9..96583e3fff 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -35,6 +35,8 @@ my $summary_file = 0;
>  my $root;
>  my %debug;
>  my $help = 0;
> +my $testexpected;
> +my $nontestexpected;

If you respin, please add acpi to these variable names since they are
specific to acpi.

>  
>  sub help {
>  	my ($exitcode) = @_;
> @@ -1256,6 +1258,26 @@ sub WARN {
>  	}
>  }
>  
> +# According to tests/qtest/bios-tables-test.c: do not
> +# change expected file in the same commit with adding test
> +sub checkfilename {
> +	my ($name) = @_;

There is a tab instead of spaces here that could be fixed if you decide
to respin.

> +        if ($name =~ m#^tests/data/acpi/# and
> +            # make exception for a shell script that rebuilds the files
> +            not $name =~ m#^\.sh$# or
> +            $name =~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
> +            $testexpected = $name;
> +        } else {
> +            $nontestexpected = $name;
> +        }
> +        if (defined $testexpected and defined $nontestexpected) {
> +            ERROR("Do not add expected files together with tests, " .
> +                  "follow instructions in " .
> +                  "tests/qtest/bios-tables-test.c: both " .
> +                  $testexpected . " and " . $nontestexpected . " found\n");
> +        }
> +}
> +
>  sub process {
>  	my $filename = shift;
>  
> @@ -1431,9 +1453,11 @@ sub process {
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +                        checkfilename($realfile);
>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +                        checkfilename($realfile);

The surrounding lines in this hunk use tab indentation, not spaces.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] checkpatch: enforce process for expected files

[Igor Mammedov]

On Sun, 15 Mar 2020 07:35:46 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> If the process documented in tests/qtest/bios-tables-test.c
> is followed, then same patch never touches both expected
> files and code. Teach checkpatch to enforce this rule.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Tested-by: Igor Mammedov <imammedo@redhat.com>

> ---
> 
> Peter, Igor what do you think?
> 
>  scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b27e4ff5e9..96583e3fff 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -35,6 +35,8 @@ my $summary_file = 0;
>  my $root;
>  my %debug;
>  my $help = 0;
> +my $testexpected;
> +my $nontestexpected;
>  
>  sub help {
>  	my ($exitcode) = @_;
> @@ -1256,6 +1258,26 @@ sub WARN {
>  	}
>  }
>  
> +# According to tests/qtest/bios-tables-test.c: do not
> +# change expected file in the same commit with adding test
> +sub checkfilename {
> +	my ($name) = @_;
> +        if ($name =~ m#^tests/data/acpi/# and
> +            # make exception for a shell script that rebuilds the files
> +            not $name =~ m#^\.sh$# or
> +            $name =~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
> +            $testexpected = $name;
> +        } else {
> +            $nontestexpected = $name;
> +        }
> +        if (defined $testexpected and defined $nontestexpected) {
> +            ERROR("Do not add expected files together with tests, " .
> +                  "follow instructions in " .
> +                  "tests/qtest/bios-tables-test.c: both " .
> +                  $testexpected . " and " . $nontestexpected . " found\n");
> +        }
> +}
> +
>  sub process {
>  	my $filename = shift;
>  
> @@ -1431,9 +1453,11 @@ sub process {
>  		if ($line =~ /^diff --git.*?(\S+)$/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +                        checkfilename($realfile);
>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +                        checkfilename($realfile);
>  
>  			$p1_prefix = $1;
>  			if (!$file && $tree && $p1_prefix ne '' &&