[PATCH] checkpatch: warn if changed lines exceeds a maximum size

[PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Brown, Nicholas]

Changed lines is the total of inserted and deleted lines.
By default there is no limit, --max-changed-lines may be used to set a
value. Some users may wish to encourage that patches are split into
smaller parts using this.
See Documentation/process/submitting-patches.rst#split-changes

Signed-off-by: Nicholas Brown <nick.brown@att.com>
---
 scripts/checkpatch.pl | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 31031f10fe56..1217d782b6bb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -49,6 +49,7 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
+my $max_changed_lines; # undef = no max
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
 my $min_conf_desc_length = 4;
@@ -209,6 +210,7 @@ GetOptions(
        'show-types!'   => \$show_types,
        'list-types!'   => \$list_types,
        'max-line-length=i' => \$max_line_length,
+       'max-changed-lines=i' => \$max_changed_lines,
        'min-conf-desc-length=i' => \$min_conf_desc_length,
        'root=s'        => \$root,
        'summary!'      => \$summary,
@@ -2165,6 +2167,8 @@ sub process {
        my $filename = shift;
 
        my $linenr=0;
+       my $inserted_lines_total=0;
+       my $deleted_lines_total=0;
        my $prevline="";
        my $prevrawline="";
        my $stashline="";
@@ -2233,6 +2237,14 @@ sub process {
 
                push(@fixed, $rawline) if ($fix);
 
+               if ($rawline=~/^\+/) {
+                 $inserted_lines_total++
+               }
+
+               if ($rawline=~/^-/) {
+                 $deleted_lines_total++
+               }
+
                if ($rawline=~/^\+\+\+\s+(\S+)/) {
                        $setup_docs = 0;
                        if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
@@ -2306,6 +2318,11 @@ sub process {
 
        $prefix = '';
 
+       if (defined $max_changed_lines &&
+           ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
+         WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
+       }
+
        $realcnt = 0;
        $linenr = 0;
        $fixlinenr = -1;
-- 
2.14.1

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Joe Perches]

On Tue, 2018-01-30 at 15:57 +0000, Brown, Nicholas wrote:
> Changed lines is the total of inserted and deleted lines.
> By default there is no limit, --max-changed-lines may be used to set a
> value. Some users may wish to encourage that patches are split into
> smaller parts using this.
> See Documentation/process/submitting-patches.rst#split-changes

(This patch seems whitespace damaged)

I don't care for this much as is either.

This patch doesn't add help text and it
should probably add a check for
	"if (!$file"
so new files aren't size limited.

Also, it double counts lines that are
added and deleted so doing things like
refactoring a block of code into a new
separate function would potentially trip
this.

> Signed-off-by: Nicholas Brown <nick.brown@att.com>
> ---
>  scripts/checkpatch.pl | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 31031f10fe56..1217d782b6bb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -49,6 +49,7 @@ my @ignore = ();
>  my $help = 0;
>  my $configuration_file = ".checkpatch.conf";
>  my $max_line_length = 80;
> +my $max_changed_lines; # undef = no max
>  my $ignore_perl_version = 0;
>  my $minimum_perl_version = 5.10.0;
>  my $min_conf_desc_length = 4;
> @@ -209,6 +210,7 @@ GetOptions(
>         'show-types!'   => \$show_types,
>         'list-types!'   => \$list_types,
>         'max-line-length=i' => \$max_line_length,
> +       'max-changed-lines=i' => \$max_changed_lines,
>         'min-conf-desc-length=i' => \$min_conf_desc_length,
>         'root=s'        => \$root,
>         'summary!'      => \$summary,
> @@ -2165,6 +2167,8 @@ sub process {
>         my $filename = shift;
>  
>         my $linenr=0;
> +       my $inserted_lines_total=0;
> +       my $deleted_lines_total=0;
>         my $prevline="";
>         my $prevrawline="";
>         my $stashline="";
> @@ -2233,6 +2237,14 @@ sub process {
>  
>                 push(@fixed, $rawline) if ($fix);
>  
> +               if ($rawline=~/^\+/) {
> +                 $inserted_lines_total++
> +               }
> +
> +               if ($rawline=~/^-/) {
> +                 $deleted_lines_total++
> +               }
> +
>                 if ($rawline=~/^\+\+\+\s+(\S+)/) {
>                         $setup_docs = 0;
>                         if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
> @@ -2306,6 +2318,11 @@ sub process {
>  
>         $prefix = '';
>  
> +       if (defined $max_changed_lines &&
> +           ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
> +         WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
> +       }
> +
>         $realcnt = 0;
>         $linenr = 0;
>         $fixlinenr = -1;
> -- 
> 2.14.1

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Brown, Nicholas]

On Tue, 2018-01-30 at 08:10 -0800, Joe Perches wrote:
> On Tue, 2018-01-30 at 15:57 +0000, Brown, Nicholas wrote:
> > Changed lines is the total of inserted and deleted lines.
> > By default there is no limit, --max-changed-lines may be used to
> > set a
> > value. Some users may wish to encourage that patches are split into
> > smaller parts using this.
> > See Documentation/process/submitting-patches.rst#split-changes
> 
> (This patch seems whitespace damaged)
> 
> I don't care for this much as is either.
> 
> This patch doesn't add help text and it

I'll look to add some help text.

> should probably add a check for
> 	"if (!$file"
> so new files aren't size limited.
> 
> Also, it double counts lines that are
> added and deleted so doing things like
> refactoring a block of code into a new
> separate function would potentially trip
> this.

These were both intentional. Large new files are just as equal a
potential target for splitting as a large change to a file, and could
for example add basic infra/stubs in an initial patch, and then flesh
out implementation in follow up patches.
Similarly it counts both insertions and deletions, as it's the
cumulative total that's a better measure of change, as compared
(insertions - deletions) which could be for example 0 while cumulative
total is large.
By making the --max-changed-lines completely user defined, with no
default, it's left to individual developers/maintainers to determine
what they consider a meaningful value is to represent a "large change"
to warn on for splitting a patch.

Thanks,
Nick


> 
> > Signed-off-by: Nicholas Brown <nick.brown@att.com>
> > ---
> >  scripts/checkpatch.pl | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 31031f10fe56..1217d782b6bb 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -49,6 +49,7 @@ my @ignore = ();
> >  my $help = 0;
> >  my $configuration_file = ".checkpatch.conf";
> >  my $max_line_length = 80;
> > +my $max_changed_lines; # undef = no max
> >  my $ignore_perl_version = 0;
> >  my $minimum_perl_version = 5.10.0;
> >  my $min_conf_desc_length = 4;
> > @@ -209,6 +210,7 @@ GetOptions(
> >         'show-types!'   => \$show_types,
> >         'list-types!'   => \$list_types,
> >         'max-line-length=i' => \$max_line_length,
> > +       'max-changed-lines=i' => \$max_changed_lines,
> >         'min-conf-desc-length=i' => \$min_conf_desc_length,
> >         'root=s'        => \$root,
> >         'summary!'      => \$summary,
> > @@ -2165,6 +2167,8 @@ sub process {
> >         my $filename = shift;
> >  
> >         my $linenr=0;
> > +       my $inserted_lines_total=0;
> > +       my $deleted_lines_total=0;
> >         my $prevline="";
> >         my $prevrawline="";
> >         my $stashline="";
> > @@ -2233,6 +2237,14 @@ sub process {
> >  
> >                 push(@fixed, $rawline) if ($fix);
> >  
> > +               if ($rawline=~/^\+/) {
> > +                 $inserted_lines_total++
> > +               }
> > +
> > +               if ($rawline=~/^-/) {
> > +                 $deleted_lines_total++
> > +               }
> > +
> >                 if ($rawline=~/^\+\+\+\s+(\S+)/) {
> >                         $setup_docs = 0;
> >                         if ($1 =~ m@Documentation/admin-
> > guide/kernel-parameters.rst$@) {
> > @@ -2306,6 +2318,11 @@ sub process {
> >  
> >         $prefix = '';
> >  
> > +       if (defined $max_changed_lines &&
> > +           ($inserted_lines_total+$deleted_lines_total >
> > $max_changed_lines)) {
> > +         WARN("MAX_CHANGED_LINES", "please split the change into
> > smaller parts\n");
> > +       }
> > +
> >         $realcnt = 0;
> >         $linenr = 0;
> >         $fixlinenr = -1;
> > -- 
> > 2.14.1

[PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Nicholas Brown]

Changed lines is the total of inserted and deleted lines.
By default there is no limit, --max-changed-lines may be used to set a
value. Some users may wish to encourage that patches are split into
smaller parts using this.
See Documentation/process/submitting-patches.rst#split-changes

Signed-off-by: Nicholas Brown <nick.brown@att.com>
---
 scripts/checkpatch.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 31031f10fe56..2847109b4def 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -49,6 +49,7 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
+my $max_changed_lines; # undef = no max
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
 my $min_conf_desc_length = 4;
@@ -92,6 +93,8 @@ Options:
   --ignore TYPE(,TYPE2...)   ignore various comma separated message types
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
+  --max-changed-lines=n      set the maximum number of changed lines allowed,
+                             if exceeded, warn. (insertions + deletions)
   --min-conf-desc-length=n   set the min description length, if shorter, warn
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
@@ -209,6 +212,7 @@ GetOptions(
 	'show-types!'	=> \$show_types,
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
+	'max-changed-lines=i' => \$max_changed_lines,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
@@ -2165,6 +2169,8 @@ sub process {
 	my $filename = shift;
 
 	my $linenr=0;
+	my $inserted_lines_total=0;
+	my $deleted_lines_total=0;
 	my $prevline="";
 	my $prevrawline="";
 	my $stashline="";
@@ -2233,6 +2239,14 @@ sub process {
 
 		push(@fixed, $rawline) if ($fix);
 
+		if ($rawline=~/^\+/) {
+		  $inserted_lines_total++
+		}
+
+		if ($rawline=~/^-/) {
+		  $deleted_lines_total++
+		}
+
 		if ($rawline=~/^\+\+\+\s+(\S+)/) {
 			$setup_docs = 0;
 			if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
@@ -2306,6 +2320,11 @@ sub process {
 
 	$prefix = '';
 
+	if (defined $max_changed_lines &&
+	    ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
+	  WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
+	}
+
 	$realcnt = 0;
 	$linenr = 0;
 	$fixlinenr = -1;
-- 
2.14.1

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Joe Perches]

On Tue, 2018-01-30 at 19:01 +0000, Nicholas Brown wrote:
> Changed lines is the total of inserted and deleted lines.
> By default there is no limit, --max-changed-lines may be used to set a
> value. Some users may wish to encourage that patches are split into
> smaller parts using this.
> See Documentation/process/submitting-patches.rst#split-changes

Still whitespace damaged.
Indentations use tabs not 2 spaces.

and another issue below

> Signed-off-by: Nicholas Brown <nick.brown@att.com>
> ---
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 31031f10fe56..2847109b4def 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -49,6 +49,7 @@ my @ignore = ();
>  my $help = 0;
>  my $configuration_file = ".checkpatch.conf";
>  my $max_line_length = 80;
> +my $max_changed_lines; # undef = no max
>  my $ignore_perl_version = 0;
>  my $minimum_perl_version = 5.10.0;
>  my $min_conf_desc_length = 4;
> @@ -92,6 +93,8 @@ Options:
>    --ignore TYPE(,TYPE2...)   ignore various comma separated message types
>    --show-types               show the specific message type in the output
>    --max-line-length=n        set the maximum line length, if exceeded, warn
> +  --max-changed-lines=n      set the maximum number of changed lines allowed,
> +                             if exceeded, warn. (insertions + deletions)
>    --min-conf-desc-length=n   set the min description length, if shorter, warn
>    --root=PATH                PATH to the kernel tree root
>    --no-summary               suppress the per-file summary
> @@ -209,6 +212,7 @@ GetOptions(
>  	'show-types!'	=> \$show_types,
>  	'list-types!'	=> \$list_types,
>  	'max-line-length=i' => \$max_line_length,
> +	'max-changed-lines=i' => \$max_changed_lines,
>  	'min-conf-desc-length=i' => \$min_conf_desc_length,
>  	'root=s'	=> \$root,
>  	'summary!'	=> \$summary,
> @@ -2165,6 +2169,8 @@ sub process {
>  	my $filename = shift;
>  
>  	my $linenr=0;
> +	my $inserted_lines_total=0;
> +	my $deleted_lines_total=0;
>  	my $prevline="";
>  	my $prevrawline="";
>  	my $stashline="";
> @@ -2233,6 +2239,14 @@ sub process {
>  
>  		push(@fixed, $rawline) if ($fix);
>  
> +		if ($rawline=~/^\+/) {
> +		  $inserted_lines_total++

This counts patch header blocks with \+\+\+

> +		}
> +
> +		if ($rawline=~/^-/) {
> +		  $deleted_lines_total++
> +		}

and \-\-\-

> +
>  		if ($rawline=~/^\+\+\+\s+(\S+)/) {
>  			$setup_docs = 0;
>  			if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
> @@ -2306,6 +2320,11 @@ sub process {
>  
>  	$prefix = '';
>  
> +	if (defined $max_changed_lines &&
> +	    ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
> +	  WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
> +	}
> +
>  	$realcnt = 0;
>  	$linenr = 0;
>  	$fixlinenr = -1;

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Brown, Nicholas]

On Tue, 2018-01-30 at 11:09 -0800, Joe Perches wrote:
> On Tue, 2018-01-30 at 19:01 +0000, Nicholas Brown wrote:
> > Changed lines is the total of inserted and deleted lines.
> > By default there is no limit, --max-changed-lines may be used to
> > set a
> > value. Some users may wish to encourage that patches are split into
> > smaller parts using this.
> > See Documentation/process/submitting-patches.rst#split-changes
> 
> Still whitespace damaged.
> Indentations use tabs not 2 spaces.

Hopefully editor set correctly now, so follow-up should fix this.

> 
> and another issue below
> 
> > 
> > @@ -2233,6 +2239,14 @@ sub process {
> >  
> >  		push(@fixed, $rawline) if ($fix);
> >  
> > +		if ($rawline=~/^\+/) {
> > +		  $inserted_lines_total++
> 
> This counts patch header blocks with \+\+\+

Good catch. Will fix.

> 
> > +		}
> > +
> > +		if ($rawline=~/^-/) {
> > +		  $deleted_lines_total++
> > +		}
> 
> and \-\-\-

Ditto.

Thanks, Nick

[PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Nicholas Brown]

Changed lines is the total of inserted and deleted lines.
By default there is no limit, --max-changed-lines may be used to set a
value. Some users may wish to encourage that patches are split into
smaller parts using this.
See Documentation/process/submitting-patches.rst#split-changes

Signed-off-by: Nicholas Brown <nick.brown@att.com>
---
 scripts/checkpatch.pl | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 31031f10fe56..a71bc4f15ce7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -49,6 +49,7 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
+my $max_changed_lines; # undef = no max
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
 my $min_conf_desc_length = 4;
@@ -92,6 +93,8 @@ Options:
   --ignore TYPE(,TYPE2...)   ignore various comma separated message types
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
+  --max-changed-lines=n      set the maximum number of changed lines allowed,
+                             if exceeded, warn. (insertions + deletions)
   --min-conf-desc-length=n   set the min description length, if shorter, warn
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
@@ -209,6 +212,7 @@ GetOptions(
 	'show-types!'	=> \$show_types,
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
+	'max-changed-lines=i' => \$max_changed_lines,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
@@ -2165,6 +2169,8 @@ sub process {
 	my $filename = shift;
 
 	my $linenr=0;
+	my $inserted_lines_total=0;
+	my $deleted_lines_total=0;
 	my $prevline="";
 	my $prevrawline="";
 	my $stashline="";
@@ -2233,6 +2239,14 @@ sub process {
 
 		push(@fixed, $rawline) if ($fix);
 
+		if ($rawline=~/^\+/ && $rawline!~/^\+\+\+/) {
+			$inserted_lines_total++
+		}
+
+		if ($rawline=~/^-/ && $rawline!~/^---/) {
+			$deleted_lines_total++;
+		}
+
 		if ($rawline=~/^\+\+\+\s+(\S+)/) {
 			$setup_docs = 0;
 			if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
@@ -2306,6 +2320,13 @@ sub process {
 
 	$prefix = '';
 
+	#print "inserted: $inserted_lines_total\n";
+	#print "deleted: $deleted_lines_total\n";
+	if (defined $max_changed_lines &&
+	    ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
+		WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
+	}
+
 	$realcnt = 0;
 	$linenr = 0;
 	$fixlinenr = -1;
-- 
2.14.1

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Joe Perches]

On Thu, 2018-02-01 at 10:38 +0000, Brown, Nicholas wrote:
> Hi,
> 
> Would one of you be happy to pick this patch up for carrying into the next release?

Not me.

I think the metric is too simplistic and
not particularly useful.

If others want it, I think you should
try to build or find a bit more agreement
from the many other linux developers that
this change is actually desired by them.

> Thanks,
> Nick

cheers, Joe

> 
> On Tue, 2018-01-30 at 20:26 +0000, Nicholas Brown wrote:
> > Changed lines is the total of inserted and deleted lines.
> > By default there is no limit, --max-changed-lines may be used to set a
> > value. Some users may wish to encourage that patches are split into
> > smaller parts using this.
> > See Documentation/process/submitting-patches.rst#split-changes
> > 
> > Signed-off-by: Nicholas Brown <nick.brown@att.com>
> > ---
> >  scripts/checkpatch.pl | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 31031f10fe56..a71bc4f15ce7 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -49,6 +49,7 @@ my @ignore = ();
> >  my $help = 0;
> >  my $configuration_file = ".checkpatch.conf";
> >  my $max_line_length = 80;
> > +my $max_changed_lines; # undef = no max
> >  my $ignore_perl_version = 0;
> >  my $minimum_perl_version = 5.10.0;
> >  my $min_conf_desc_length = 4;
> > @@ -92,6 +93,8 @@ Options:
> >    --ignore TYPE(,TYPE2...)   ignore various comma separated message types
> >    --show-types               show the specific message type in the output
> >    --max-line-length=n        set the maximum line length, if exceeded, warn
> > +  --max-changed-lines=n      set the maximum number of changed lines allowed,
> > +                             if exceeded, warn. (insertions + deletions)
> >    --min-conf-desc-length=n   set the min description length, if shorter, warn
> >    --root=PATH                PATH to the kernel tree root
> >    --no-summary               suppress the per-file summary
> > @@ -209,6 +212,7 @@ GetOptions(
> >  	'show-types!'	=> \$show_types,
> >  	'list-types!'	=> \$list_types,
> >  	'max-line-length=i' => \$max_line_length,
> > +	'max-changed-lines=i' => \$max_changed_lines,
> >  	'min-conf-desc-length=i' => \$min_conf_desc_length,
> >  	'root=s'	=> \$root,
> >  	'summary!'	=> \$summary,
> > @@ -2165,6 +2169,8 @@ sub process {
> >  	my $filename = shift;
> >  
> >  	my $linenr=0;
> > +	my $inserted_lines_total=0;
> > +	my $deleted_lines_total=0;
> >  	my $prevline="";
> >  	my $prevrawline="";
> >  	my $stashline="";
> > @@ -2233,6 +2239,14 @@ sub process {
> >  
> >  		push(@fixed, $rawline) if ($fix);
> >  
> > +		if ($rawline=~/^\+/ && $rawline!~/^\+\+\+/) {
> > +			$inserted_lines_total++
> > +		}
> > +
> > +		if ($rawline=~/^-/ && $rawline!~/^---/) {
> > +			$deleted_lines_total++;
> > +		}
> > +
> >  		if ($rawline=~/^\+\+\+\s+(\S+)/) {
> >  			$setup_docs = 0;
> >  			if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
> > @@ -2306,6 +2320,13 @@ sub process {
> >  
> >  	$prefix = '';
> >  
> > +	#print "inserted: $inserted_lines_total\n";
> > +	#print "deleted: $deleted_lines_total\n";

Not much utility in the debugging code either.

> > +	if (defined $max_changed_lines &&
> > +	    ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
> > +		WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
> > +	}
> > +
> >  	$realcnt = 0;
> >  	$linenr = 0;
> >  	$fixlinenr = -1;

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Brown, Nicholas]

On Thu, 2018-02-01 at 04:54 -0800, Joe Perches wrote:
> On Thu, 2018-02-01 at 10:38 +0000, Brown, Nicholas wrote:
> > Hi,
> > 
> > Would one of you be happy to pick this patch up for carrying into
> > the next release?
> 
> Not me.
> 
> I think the metric is too simplistic and
> not particularly useful.

I'm not sure it's any more simplistic than than the character line
length limit, which is there to prompt thought on code nesting levels,
etc. And as it has to be explicitly configured it allows developers the
discretion to determine a code change size that meaningful in a given
situation.

> 
> If others want it, I think you should
> try to build or find a bit more agreement
> from the many other linux developers that
> this change is actually desired by them.

It's been used as a heuristic as part of code reviews within our
development team, with differing sizes depending on the maintainer, and
I though it might be similarly useful wide community.

One use case was:

IGNORE_TYPES="FILE_PATH_CHANGES,LINE_SPACING,GIT_COMMIT_ID,SPLIT_STRING,PREFER_PRINTF"
checkpatch.pl \
    --no-tree --no-signoff --show-types --emacs \
    --max-changed-lines=500 \
    --ignore "$IGNORE_TYPES" \
    --git  "$(git merge-base "$SOURCE" "$TARGET")".."$SOURCE"

Thanks,
Nick

> 
> > Thanks,
> > Nick
> 
> cheers, Joe
> 
> > 
> > On Tue, 2018-01-30 at 20:26 +0000, Nicholas Brown wrote:
> > > Changed lines is the total of inserted and deleted lines.
> > > By default there is no limit, --max-changed-lines may be used to
> > > set a
> > > value. Some users may wish to encourage that patches are split
> > > into
> > > smaller parts using this.
> > > See Documentation/process/submitting-patches.rst#split-changes
> > > 
> > > Signed-off-by: Nicholas Brown <nick.brown@att.com>
> > > ---
> > >  scripts/checkpatch.pl | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 31031f10fe56..a71bc4f15ce7 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -49,6 +49,7 @@ my @ignore = ();
> > >  my $help = 0;
> > >  my $configuration_file = ".checkpatch.conf";
> > >  my $max_line_length = 80;
> > > +my $max_changed_lines; # undef = no max
> > >  my $ignore_perl_version = 0;
> > >  my $minimum_perl_version = 5.10.0;
> > >  my $min_conf_desc_length = 4;
> > > @@ -92,6 +93,8 @@ Options:
> > >    --ignore TYPE(,TYPE2...)   ignore various comma separated
> > > message types
> > >    --show-types               show the specific message type in
> > > the output
> > >    --max-line-length=n        set the maximum line length, if
> > > exceeded, warn
> > > +  --max-changed-lines=n      set the maximum number of changed
> > > lines allowed,
> > > +                             if exceeded, warn. (insertions +
> > > deletions)
> > >    --min-conf-desc-length=n   set the min description length, if
> > > shorter, warn
> > >    --root=PATH                PATH to the kernel tree root
> > >    --no-summary               suppress the per-file summary
> > > @@ -209,6 +212,7 @@ GetOptions(
> > >  	'show-types!'	=> \$show_types,
> > >  	'list-types!'	=> \$list_types,
> > >  	'max-line-length=i' => \$max_line_length,
> > > +	'max-changed-lines=i' => \$max_changed_lines,
> > >  	'min-conf-desc-length=i' => \$min_conf_desc_length,
> > >  	'root=s'	=> \$root,
> > >  	'summary!'	=> \$summary,
> > > @@ -2165,6 +2169,8 @@ sub process {
> > >  	my $filename = shift;
> > >  
> > >  	my $linenr=0;
> > > +	my $inserted_lines_total=0;
> > > +	my $deleted_lines_total=0;
> > >  	my $prevline="";
> > >  	my $prevrawline="";
> > >  	my $stashline="";
> > > @@ -2233,6 +2239,14 @@ sub process {
> > >  
> > >  		push(@fixed, $rawline) if ($fix);
> > >  
> > > +		if ($rawline=~/^\+/ && $rawline!~/^\+\+\+/) {
> > > +			$inserted_lines_total++
> > > +		}
> > > +
> > > +		if ($rawline=~/^-/ && $rawline!~/^---/) {
> > > +			$deleted_lines_total++;
> > > +		}
> > > +
> > >  		if ($rawline=~/^\+\+\+\s+(\S+)/) {
> > >  			$setup_docs = 0;
> > >  			if ($1 =~ m@Documentation/admin-
> > > guide/kernel-parameters.rst$@) {
> > > @@ -2306,6 +2320,13 @@ sub process {
> > >  
> > >  	$prefix = '';
> > >  
> > > +	#print "inserted: $inserted_lines_total\n";
> > > +	#print "deleted: $deleted_lines_total\n";
> 
> Not much utility in the debugging code either.
> 
> > > +	if (defined $max_changed_lines &&
> > > +	    ($inserted_lines_total+$deleted_lines_total >
> > > $max_changed_lines)) {
> > > +		WARN("MAX_CHANGED_LINES", "please split the
> > > change into smaller parts\n");
> > > +	}
> > > +
> > >  	$realcnt = 0;
> > >  	$linenr = 0;
> > >  	$fixlinenr = -1;

Re: [PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Jonathan Corbet]

On Thu, 1 Feb 2018 15:42:35 +0000
"Brown, Nicholas" <nb930b@intl.att.com> wrote:

> > I think the metric is too simplistic and
> > not particularly useful.  
> 
> I'm not sure it's any more simplistic than than the character line
> length limit, which is there to prompt thought on code nesting levels,
> etc. And as it has to be explicitly configured it allows developers the
> discretion to determine a code change size that meaningful in a given
> situation.

The line-length limit relates directly to code readability and
non-infringement of a developer's sacred right to work unimpeded on an
80x24 xterm (the last vt100 died, unfortunately).

A line-count limit lacks even that justification.  The rule on splitting
patches is entirely about logical changes that can be reviewed
independently.  Some of those changes involve a lot of lines, others do
not.  If the correct splits do not come to you when you're writing the
changelogs for your patches (or before), a tool nagging about line counts
really isn't going to help.  I would expect it to miss most patches
actually in need of splitting while complaining about many patches that
are just fine.

Thanks for working to improve the tools - they certainly can afford a lot
of improvement!  But my own suggestion would be to look a bit further for
improvements that will be truly helpful to the development community.

Thanks,

jon

[PATCH] checkpatch: warn if changed lines exceeds a maximum size

[Brown, Nicholas]

Changed lines is the total of inserted and deleted lines.
By default there is no limit, --max-changed-lines may be used to set a
value. Some users may wish to encourage that patches are split into
smaller parts using this.
See Documentation/process/submitting-patches.rst#split-changes

Signed-off-by: Nicholas Brown <nick.brown@att.com>
---
 scripts/checkpatch.pl | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 31031f10fe56..1217d782b6bb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -49,6 +49,7 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
+my $max_changed_lines; # undef = no max
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
 my $min_conf_desc_length = 4;
@@ -209,6 +210,7 @@ GetOptions(
 	'show-types!'	=> \$show_types,
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
+	'max-changed-lines=i' => \$max_changed_lines,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
@@ -2165,6 +2167,8 @@ sub process {
 	my $filename = shift;
 
 	my $linenr=0;
+	my $inserted_lines_total=0;
+	my $deleted_lines_total=0;
 	my $prevline="";
 	my $prevrawline="";
 	my $stashline="";
@@ -2233,6 +2237,14 @@ sub process {
 
 		push(@fixed, $rawline) if ($fix);
 
+		if ($rawline=~/^\+/) {
+		  $inserted_lines_total++
+		}
+
+		if ($rawline=~/^-/) {
+		  $deleted_lines_total++
+		}
+
 		if ($rawline=~/^\+\+\+\s+(\S+)/) {
 			$setup_docs = 0;
 			if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
@@ -2306,6 +2318,11 @@ sub process {
 
 	$prefix = '';
 
+	if (defined $max_changed_lines &&
+	    ($inserted_lines_total+$deleted_lines_total > $max_changed_lines)) {
+	  WARN("MAX_CHANGED_LINES", "please split the change into smaller parts\n");
+	}
+
 	$realcnt = 0;
 	$linenr = 0;
 	$fixlinenr = -1;
-- 
2.14.1