[PATCH 0/4] checkpatch: improve exit criterias

[PATCH 0/4] checkpatch: improve exit criterias

[Bjorn Andersson]

When invoking checkpatch with the --mailback argument over a range of
patches the tool unexpectedly exits successfully on the first clean
patch. For example:

  ./scripts/checkpatch.pl --mailback --git v7.3-rc1..

Checks the single commit and then exits successfully. In contrast,
omitting --mailback causes checkpatch to check the thousands of commits
in the range.

The same behavior can be observed with --no-patch or when empty patches
are involved, which exits successfully after the first non-patch has
passed the checks, either skipping further checks or failing to return a
non-zero exit value even though errors has been reported.

Lastly, checkpatch fails with a non-zero status when invoked on a
missing patch file, but zero when invoked on a missing file.

---
Bjorn Andersson (4):
      checkpatch: don't stop at the first clean patch with --mailback
      checkpatch: don't stop at the first non-patch input with --no-patch
      checkpatch: don't stop at the first empty input
      checkpatch: fail on missing or unreadable files with --file

 scripts/checkpatch.pl | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
---
base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
change-id: 20261108-checkpatch-fixes-hash-0466f2c1d2e4

Best regards,
--  
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

[PATCH 1/4] checkpatch: don't stop at the first clean patch with --mailback

[Bjorn Andersson]

When checkpatch is given multiple patches, either as several files or
as a --git range, --mailback causes process() to call exit(0) as soon
as it encounters a clean patch. The remaining patches are never
checked, and the run is reported as successful even if later patches
contain errors or warnings.

E.g. with a clean patch followed by a patch with a bogus Fixes: tag:

  $ ./scripts/checkpatch.pl -q --terse --mailback clean.patch bad.patch
  $ echo $?
  0

Return from process() instead, so the clean patch is still silently
accepted while the remaining patches are checked.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 741b189d5e01..e84bb4c12eb4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7880,7 +7880,7 @@ sub process {
 	# In mailback mode only produce a report in the negative, for
 	# things that appear to be patches.
 	if ($mailback && ($clean == 1 || !$is_patch)) {
-		exit(0);
+		return 1;
 	}
 
 	# This is not a patch, and we are in 'no-patch' mode so

-- 
2.55.0

[PATCH 2/4] checkpatch: don't stop at the first non-patch input with --no-patch

[Bjorn Andersson]

When checkpatch is given multiple inputs with --no-patch, process()
calls exit(0) as soon as it encounters an input that isn't a patch.
The remaining inputs are never checked, and the run is reported as
successful even if later inputs contain errors or warnings.

E.g. with a non-patch file followed by a patch with a bogus Fixes: tag:

  $ ./scripts/checkpatch.pl -q --terse --no-patch notes.txt bad.patch
  $ echo $?
  0

Return from process() instead, so the non-patch input is still
silently skipped while the remaining inputs are checked.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e84bb4c12eb4..45279c17f70a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7886,7 +7886,7 @@ sub process {
 	# This is not a patch, and we are in 'no-patch' mode so
 	# just keep quiet.
 	if (!$chk_patch && !$is_patch) {
-		exit(0);
+		return 1;
 	}
 
 	if (!$is_patch && $filename !~ /cover-letter\.patch$/) {

-- 
2.55.0

[PATCH 3/4] checkpatch: don't stop at the first empty input

[Bjorn Andersson]

When checkpatch is given multiple inputs, process() calls exit(0) as
soon as it encounters an empty one. This discards the failure status
of inputs already checked, and the remaining inputs are never checked.

E.g. a patch with a bogus Fixes: tag followed by an empty file reports
the warning, but the run is still reported as successful:

  $ ./scripts/checkpatch.pl -q --terse bad.patch empty.patch
  bad.patch:9: WARNING: Unknown commit id '6393a36fe184', ...
  total: 0 errors, 1 warnings, 7 lines checked
  $ echo $?
  0

With the order reversed, bad.patch is never checked at all.

The same happens with --file when one of the files doesn't exist, as
the diff against /dev/null then produces no output. A mistyped file
name therefore causes all following files to be silently skipped:

  $ ./scripts/checkpatch.pl -q --terse -f missing.c existing.c
  diff: missing.c: No such file or directory
  $ echo $?
  0

Return from process() instead, so the empty input is still silently
skipped while the remaining inputs are checked and the exit status
reflects all of them.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45279c17f70a..a5e6f96477c3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7874,7 +7874,7 @@ sub process {
 	# If we have no input at all, then there is nothing to report on
 	# so just keep quiet.
 	if ($#rawlines == -1) {
-		exit(0);
+		return 1;
 	}
 
 	# In mailback mode only produce a report in the negative, for

-- 
2.55.0

[PATCH 4/4] checkpatch: fail on missing or unreadable files with --file

[Bjorn Andersson]

With --file, checkpatch reads each input through "diff -u /dev/null"
and doesn't check whether diff managed to read the file. A file that
doesn't exist or can't be read therefore produces no output and is
silently treated as an empty, and thereby clean, input. Only diff's
own error message hints at the problem, and the exit status reports
success:

  $ ./scripts/checkpatch.pl -q --terse -f missing.c
  diff: missing.c: No such file or directory
  $ echo $?
  0

This differs from patch mode, where an input that can't be opened is
treated as a fatal error.

Open the file before running diff, and fail in the same way as patch
mode does when that isn't possible:

  $ ./scripts/checkpatch.pl -q --terse -f missing.c
  ./scripts/checkpatch.pl: missing.c: open failed - No such file or directory

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a5e6f96477c3..7233a19329f8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1346,6 +1346,12 @@ for my $filename (@ARGV) {
 		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
 			die "$P: $filename: git format-patch failed - $!\n";
 	} elsif ($file) {
+		# diff failing to read the file would look like an empty file
+		if ($filename ne '-') {
+			open(my $fh, '<', $filename) ||
+				die "$P: $filename: open failed - $!\n";
+			close($fh);
+		}
 		open($FILE, '-|', "diff -u /dev/null $filename") ||
 			die "$P: $filename: diff failed - $!\n";
 	} elsif ($filename eq '-') {

-- 
2.55.0