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@xxxxxxx>
> > > ---
> > >  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;