checkpatch: warn about hunks which only add blank lines

[PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Denis V. Lunev 1 month, 4 weeks ago

Patches sometimes carry a hunk whose entire content is one or two
added blank lines. It changes nothing, it makes the diff longer and
it survives review because nobody looks twice at a blank line. The
blank line itself is perfectly fine, the gratuitous hunk is not.

Tally the added blank lines and the other changes of every hunk in
the pre-scan loop which already walks the raw patch, and record the
hunks which only add blanks. Such a hunk is then reported at the
first blank line it adds, so the warning carries the usual file and
line context.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Chao Liu <chao.liu@processmission.com>
CC: "Daniel P. Berrange" <berrange@redhat.com>
CC: "Philippe Mathieu-Daude" <philmd@oss.qualcomm.com>
CC: Thomas Huth <thuth@redhat.com>
---
Note: this patch is sent on behalf of the yesterday finding inside IDE
      pull request. This should be found with a checkpatch.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 03f35e7501..dfa9878006 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1567,6 +1567,10 @@ sub process {
 	my $first_line = 0;
 	my $p1_prefix = '';
 
+	my %blank_only_hunk;
+	my $hunk_first_blank = 0;
+	my $hunk_has_change = 0;
+
 	my $prev_values = 'E';
 
 	# suppression flags
@@ -1583,6 +1587,10 @@ sub process {
 		$line = $rawline;
 
 		if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
+			$blank_only_hunk{$hunk_first_blank} = 1
+				if ($hunk_first_blank && !$hunk_has_change);
+			($hunk_first_blank, $hunk_has_change) = (0, 0);
+
 			$realline=$1-1;
 			if (defined $2) {
 				$realcnt=$3+1;
@@ -1633,6 +1641,13 @@ sub process {
 		push(@lines, $line);
 
 		if ($realcnt > 1) {
+			if ($rawline =~ /^\+\s*$/) {
+				$hunk_first_blank = $linenr
+					if (!$hunk_first_blank);
+			} elsif ($rawline =~ /^[-+]/) {
+				$hunk_has_change = 1;
+			}
+
 			$realcnt-- if ($line =~ /^(?:\+| |$)/);
 		} else {
 			$realcnt = 0;
@@ -1641,6 +1656,8 @@ sub process {
 		#print "==>$rawline\n";
 		#print "-->$line\n";
 	}
+	$blank_only_hunk{$hunk_first_blank} = 1
+		if ($hunk_first_blank && !$hunk_has_change);
 
 	$prefix = '';
 
@@ -1780,6 +1797,10 @@ sub process {
 
 		$cnt_lines++ if ($realcnt != 0);
 
+		if ($blank_only_hunk{$linenr}) {
+			WARN("this hunk only adds blank lines\n" . $herecurr);
+		}
+
 # Only allow Python 3 interpreter
 		if ($realline == 1 &&
 			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {

base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
-- 
2.53.0

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Denis V. Lunev 1 month, 1 week ago

On 7/31/26 15:40, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> Patches sometimes carry a hunk whose entire content is one or two
> added blank lines. It changes nothing, it makes the diff longer and
> it survives review because nobody looks twice at a blank line. The
> blank line itself is perfectly fine, the gratuitous hunk is not.
>
> Tally the added blank lines and the other changes of every hunk in
> the pre-scan loop which already walks the raw patch, and record the
> hunks which only add blanks. Such a hunk is then reported at the
> first blank line it adds, so the warning carries the usual file and
> line context.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Chao Liu <chao.liu@processmission.com>
> CC: "Daniel P. Berrange" <berrange@redhat.com>
> CC: "Philippe Mathieu-Daude" <philmd@oss.qualcomm.com>
> CC: Thomas Huth <thuth@redhat.com>
> ---
> Note: this patch is sent on behalf of the yesterday finding inside IDE
>       pull request. This should be found with a checkpatch.
>
>  scripts/checkpatch.pl | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 03f35e7501..dfa9878006 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1567,6 +1567,10 @@ sub process {
>  	my $first_line = 0;
>  	my $p1_prefix = '';
>  
> +	my %blank_only_hunk;
> +	my $hunk_first_blank = 0;
> +	my $hunk_has_change = 0;
> +
>  	my $prev_values = 'E';
>  
>  	# suppression flags
> @@ -1583,6 +1587,10 @@ sub process {
>  		$line = $rawline;
>  
>  		if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
> +			$blank_only_hunk{$hunk_first_blank} = 1
> +				if ($hunk_first_blank && !$hunk_has_change);
> +			($hunk_first_blank, $hunk_has_change) = (0, 0);
> +
>  			$realline=$1-1;
>  			if (defined $2) {
>  				$realcnt=$3+1;
> @@ -1633,6 +1641,13 @@ sub process {
>  		push(@lines, $line);
>  
>  		if ($realcnt > 1) {
> +			if ($rawline =~ /^\+\s*$/) {
> +				$hunk_first_blank = $linenr
> +					if (!$hunk_first_blank);
> +			} elsif ($rawline =~ /^[-+]/) {
> +				$hunk_has_change = 1;
> +			}
> +
>  			$realcnt-- if ($line =~ /^(?:\+| |$)/);
>  		} else {
>  			$realcnt = 0;
> @@ -1641,6 +1656,8 @@ sub process {
>  		#print "==>$rawline\n";
>  		#print "-->$line\n";
>  	}
> +	$blank_only_hunk{$hunk_first_blank} = 1
> +		if ($hunk_first_blank && !$hunk_has_change);
>  
>  	$prefix = '';
>  
> @@ -1780,6 +1797,10 @@ sub process {
>  
>  		$cnt_lines++ if ($realcnt != 0);
>  
> +		if ($blank_only_hunk{$linenr}) {
> +			WARN("this hunk only adds blank lines\n" . $herecurr);
> +		}
> +
>  # Only allow Python 3 interpreter
>  		if ($realline == 1 &&
>  			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {
>
> base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
Guys,

does it make sense to continue discussion or this patch
goes to /dev/null as nobody is interested in?

Den

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Thomas Huth 1 month, 1 week ago

On 18/08/2026 11.29, Denis V. Lunev wrote:
> On 7/31/26 15:40, Denis V. Lunev wrote:
>> This email originated from an IP that might not be authorized by the domain it was sent from.
>> Do not click links or open attachments unless it is an email you expected to receive.
>> Patches sometimes carry a hunk whose entire content is one or two
>> added blank lines. It changes nothing, it makes the diff longer and
>> it survives review because nobody looks twice at a blank line. The
>> blank line itself is perfectly fine, the gratuitous hunk is not.
>>
>> Tally the added blank lines and the other changes of every hunk in
>> the pre-scan loop which already walks the raw patch, and record the
>> hunks which only add blanks. Such a hunk is then reported at the
>> first blank line it adds, so the warning carries the usual file and
>> line context.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Chao Liu <chao.liu@processmission.com>
>> CC: "Daniel P. Berrange" <berrange@redhat.com>
>> CC: "Philippe Mathieu-Daude" <philmd@oss.qualcomm.com>
>> CC: Thomas Huth <thuth@redhat.com>
>> ---
>> Note: this patch is sent on behalf of the yesterday finding inside IDE
>>        pull request. This should be found with a checkpatch.
>>
>>   scripts/checkpatch.pl | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 03f35e7501..dfa9878006 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1567,6 +1567,10 @@ sub process {
>>   	my $first_line = 0;
>>   	my $p1_prefix = '';
>>   
>> +	my %blank_only_hunk;
>> +	my $hunk_first_blank = 0;
>> +	my $hunk_has_change = 0;
>> +
>>   	my $prev_values = 'E';
>>   
>>   	# suppression flags
>> @@ -1583,6 +1587,10 @@ sub process {
>>   		$line = $rawline;
>>   
>>   		if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
>> +			$blank_only_hunk{$hunk_first_blank} = 1
>> +				if ($hunk_first_blank && !$hunk_has_change);
>> +			($hunk_first_blank, $hunk_has_change) = (0, 0);
>> +
>>   			$realline=$1-1;
>>   			if (defined $2) {
>>   				$realcnt=$3+1;
>> @@ -1633,6 +1641,13 @@ sub process {
>>   		push(@lines, $line);
>>   
>>   		if ($realcnt > 1) {
>> +			if ($rawline =~ /^\+\s*$/) {
>> +				$hunk_first_blank = $linenr
>> +					if (!$hunk_first_blank);
>> +			} elsif ($rawline =~ /^[-+]/) {
>> +				$hunk_has_change = 1;
>> +			}
>> +
>>   			$realcnt-- if ($line =~ /^(?:\+| |$)/);
>>   		} else {
>>   			$realcnt = 0;
>> @@ -1641,6 +1656,8 @@ sub process {
>>   		#print "==>$rawline\n";
>>   		#print "-->$line\n";
>>   	}
>> +	$blank_only_hunk{$hunk_first_blank} = 1
>> +		if ($hunk_first_blank && !$hunk_has_change);
>>   
>>   	$prefix = '';
>>   
>> @@ -1780,6 +1797,10 @@ sub process {
>>   
>>   		$cnt_lines++ if ($realcnt != 0);
>>   
>> +		if ($blank_only_hunk{$linenr}) {
>> +			WARN("this hunk only adds blank lines\n" . $herecurr);
>> +		}
>> +
>>   # Only allow Python 3 interpreter
>>   		if ($realline == 1 &&
>>   			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {
>>
>> base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
> Guys,
> 
> does it make sense to continue discussion or this patch
> goes to /dev/null as nobody is interested in?
  Hi,

sorry, but I still don't have a proper clue whether this is really a good 
idea or not. There could also be valid cases where a hunk that adds a single 
blank line could be fine ... so I'd tend to rather not add this patch? What 
do others think?

  Thomas

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Philippe Mathieu-Daudé 1 month, 1 week ago

On 18/8/26 11:29, Denis V. Lunev wrote:
> On 7/31/26 15:40, Denis V. Lunev wrote:
>> This email originated from an IP that might not be authorized by the domain it was sent from.
>> Do not click links or open attachments unless it is an email you expected to receive.
>> Patches sometimes carry a hunk whose entire content is one or two
>> added blank lines. It changes nothing, it makes the diff longer and
>> it survives review because nobody looks twice at a blank line. The
>> blank line itself is perfectly fine, the gratuitous hunk is not.
>>
>> Tally the added blank lines and the other changes of every hunk in
>> the pre-scan loop which already walks the raw patch, and record the
>> hunks which only add blanks. Such a hunk is then reported at the
>> first blank line it adds, so the warning carries the usual file and
>> line context.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Chao Liu <chao.liu@processmission.com>
>> CC: "Daniel P. Berrange" <berrange@redhat.com>
>> CC: "Philippe Mathieu-Daude" <philmd@oss.qualcomm.com>
>> CC: Thomas Huth <thuth@redhat.com>
>> ---
>> Note: this patch is sent on behalf of the yesterday finding inside IDE
>>        pull request. This should be found with a checkpatch.
>>
>>   scripts/checkpatch.pl | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 03f35e7501..dfa9878006 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1567,6 +1567,10 @@ sub process {
>>   	my $first_line = 0;
>>   	my $p1_prefix = '';
>>   
>> +	my %blank_only_hunk;
>> +	my $hunk_first_blank = 0;
>> +	my $hunk_has_change = 0;
>> +
>>   	my $prev_values = 'E';
>>   
>>   	# suppression flags
>> @@ -1583,6 +1587,10 @@ sub process {
>>   		$line = $rawline;
>>   
>>   		if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
>> +			$blank_only_hunk{$hunk_first_blank} = 1
>> +				if ($hunk_first_blank && !$hunk_has_change);
>> +			($hunk_first_blank, $hunk_has_change) = (0, 0);
>> +
>>   			$realline=$1-1;
>>   			if (defined $2) {
>>   				$realcnt=$3+1;
>> @@ -1633,6 +1641,13 @@ sub process {
>>   		push(@lines, $line);
>>   
>>   		if ($realcnt > 1) {
>> +			if ($rawline =~ /^\+\s*$/) {
>> +				$hunk_first_blank = $linenr
>> +					if (!$hunk_first_blank);
>> +			} elsif ($rawline =~ /^[-+]/) {
>> +				$hunk_has_change = 1;
>> +			}
>> +
>>   			$realcnt-- if ($line =~ /^(?:\+| |$)/);
>>   		} else {
>>   			$realcnt = 0;
>> @@ -1641,6 +1656,8 @@ sub process {
>>   		#print "==>$rawline\n";
>>   		#print "-->$line\n";
>>   	}
>> +	$blank_only_hunk{$hunk_first_blank} = 1
>> +		if ($hunk_first_blank && !$hunk_has_change);
>>   
>>   	$prefix = '';
>>   
>> @@ -1780,6 +1797,10 @@ sub process {
>>   
>>   		$cnt_lines++ if ($realcnt != 0);
>>   
>> +		if ($blank_only_hunk{$linenr}) {
>> +			WARN("this hunk only adds blank lines\n" . $herecurr);
>> +		}
>> +
>>   # Only allow Python 3 interpreter
>>   		if ($realline == 1 &&
>>   			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {
>>
>> base-commit: e1705a25aff35635c360bbaba4c2731d019a422a
> Guys,
> 
> does it make sense to continue discussion or this patch
> goes to /dev/null as nobody is interested in?

I'm not sure why I am in the Cc list, but TBH I try to not look
at Perl changes for my own mental health, so I'll politely pass.

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Thomas Huth 1 month, 3 weeks ago

On 31/07/2026 15.40, Denis V. Lunev wrote:
> Patches sometimes carry a hunk whose entire content is one or two
> added blank lines. It changes nothing, it makes the diff longer and
> it survives review because nobody looks twice at a blank line. The
> blank line itself is perfectly fine, the gratuitous hunk is not.

Well, I wouldn't say that the blank line itself is fine. It depends.
In source code, there normally should not be more than one empty line 
between code blocks, that's why I complained to the patch in your pull request.

Anyway, thanks for tackling this! But looking at the checkpatch.pl script in 
the Linux kernel, they added a slightly different check in the course of time:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=365dd4eaafa22d2c79913d5f057d636e8842c470

Maybe we should adapt that one, to avoid that the scripts diverge even further?

  Thomas

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Markus Armbruster 1 month, 3 weeks ago

Thomas Huth <thuth@redhat.com> writes:

> On 31/07/2026 15.40, Denis V. Lunev wrote:
>> Patches sometimes carry a hunk whose entire content is one or two
>> added blank lines. It changes nothing, it makes the diff longer and
>> it survives review because nobody looks twice at a blank line. The
>> blank line itself is perfectly fine, the gratuitous hunk is not.
>
> Well, I wouldn't say that the blank line itself is fine. It depends.
> In source code, there normally should not be more than one empty line between code blocks, that's why I complained to the patch in your pull request.

For what it's worth, Python style *demands* two blank lines in places.
Here's what happens when I delete one of them:

    $ pycodestyle scripts/qapi/parser.py 
    scripts/qapi/parser.py:656:1: E302 expected 2 blank lines, found 1

Why?  PEP 8 "Style Guide for Python Code" section "Blank Lines":

--> Surround top-level function and class definitions with two blank
--> lines.

    Method definitions inside a class are surrounded by a single blank
    line.

    Extra blank lines may be used (sparingly) to separate groups of
    related functions. Blank lines may be omitted between a bunch of
    related one-liners (e.g. a set of dummy implementations).

    Use blank lines in functions, sparingly, to indicate logical
    sections.

    Python accepts the control-L (i.e. ^L) form feed character as
    whitespace; many tools treat these characters as page separators, so
    you may use them to separate pages of related sections of your
    file. Note, some editors and web-based code viewers may not
    recognize control-L as a form feed and will show another glyph in
    its place.

We run pycodestyle via flake8 from python/tests/linters.py.  Still not
part of "make check".

[...]

Re: [PATCH] checkpatch: warn about hunks which only add blank lines

Posted by Denis V. Lunev 1 month, 2 weeks ago

On 8/6/26 07:28, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> On 31/07/2026 15.40, Denis V. Lunev wrote:
>>> Patches sometimes carry a hunk whose entire content is one or two
>>> added blank lines. It changes nothing, it makes the diff longer and
>>> it survives review because nobody looks twice at a blank line. The
>>> blank line itself is perfectly fine, the gratuitous hunk is not.
>> Well, I wouldn't say that the blank line itself is fine. It depends.
>> In source code, there normally should not be more than one empty line between code blocks, that's why I complained to the patch in your pull request.
> For what it's worth, Python style *demands* two blank lines in places.
> Here's what happens when I delete one of them:
>
>     $ pycodestyle scripts/qapi/parser.py 
>     scripts/qapi/parser.py:656:1: E302 expected 2 blank lines, found 1
>
> Why?  PEP 8 "Style Guide for Python Code" section "Blank Lines":
>
> --> Surround top-level function and class definitions with two blank
> --> lines.
>
>     Method definitions inside a class are surrounded by a single blank
>     line.
>
>     Extra blank lines may be used (sparingly) to separate groups of
>     related functions. Blank lines may be omitted between a bunch of
>     related one-liners (e.g. a set of dummy implementations).
>
>     Use blank lines in functions, sparingly, to indicate logical
>     sections.
>
>     Python accepts the control-L (i.e. ^L) form feed character as
>     whitespace; many tools treat these characters as page separators, so
>     you may use them to separate pages of related sections of your
>     file. Note, some editors and web-based code viewers may not
>     recognize control-L as a form feed and will show another glyph in
>     its place.
>
> We run pycodestyle via flake8 from python/tests/linters.py.  Still not
> part of "make check".
>
> [...]
>
That is actually the difference in between what is done in
the kernel and here.

There are 3 main notes:
* kernel check is not running always, it is hidden under
  --strict-only
* kernel check is not executed over Python
* kernel check catches consecutive blank lines

Here my check is different - I am trying to catch junk
hunk, which was missed with manual eye review - just
blank line. Such hunks at my opinion are absolutely
useless and that is worth to flag.

Thus kernel style check would be complementary to my
one and they are orthogonal.

I propose to stay with this version. Anyway, if you
prefer, I could port Linux one. We could try to make
voting :-)

Thank you in advance,
    Den