checkpatch: warn if changed lines exceeds a maximum size
PATCH 1/1 checkpatch: warn if changed lines exceeds a maximum size
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
--- scripts/checkpatch.pl | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
scripts/checkpatch.pl +17 -0
@@ -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;
Signature · 1 line
2.14.1
Joe Perches
maintainerQuoted 5 lines · 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
(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.
Quoted 63 lines
Signed-off-by Nicholas Brown <[email protected]>--- scripts/checkpatch.pl | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)scripts/checkpatch.pl +17
-0@@ -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;Signature · 1 line
2.14.1
Quoted 8 lines · Joe Perches
Quoted 6 lines · 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
Quoted 5 lines
(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.
Quoted 9 lines
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
Quoted 67 lines
Quoted 67 lines
Signed-off-by Nicholas Brown <[email protected]>--- scripts/checkpatch.pl | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)scripts/checkpatch.pl +12
-0@@ -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$@) {
scripts/checkpatch.pl +5
-0@@ -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;Signature · 1 line
2.14.1
PATCH 1/1 checkpatch: warn if changed lines exceeds a maximum size
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
--- scripts/checkpatch.pl | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
scripts/checkpatch.pl +19 -0
@@ -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;
Signature · 1 line
2.14.1
Joe Perches
maintainerQuoted 5 lines · 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
Still whitespace damaged.
Indentations use tabs not 2 spaces.
and another issue below
Quoted 44 lines
Signed-off-by Nicholas Brown <[email protected]>--- scripts/checkpatch.pl | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)scripts/checkpatch.pl +6
-0@@ -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="";
scripts/checkpatch.pl sub process { · line 2243
Quoted 5 lines
scripts/checkpatch.pl +2
-0@@ -2233,6 +2239,14 @@ sub process { push(@fixed, $rawline) if ($fix); + if ($rawline=~/^\+/) { + $inserted_lines_total++
This counts patch header blocks with \+\+\+
scripts/checkpatch.pl sub process { · line 2248
Quoted 5 lines
scripts/checkpatch.pl +5
-0+ } + + if ($rawline=~/^-/) { + $deleted_lines_total++ + }
and \-\-\-
Quoted 16 lines
scripts/checkpatch.pl +6
-0+ 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;
Quoted 10 lines · Joe Perches
Quoted 6 lines · 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-changesStill whitespace damaged.
Indentations use tabs not 2 spaces.
Hopefully editor set correctly now, so follow-up should fix this.
Quoted 4 lines
and another issue below
Quoted 1 line
Quoted 8 lines
scripts/checkpatch.pl sub process { · line 2243
Quoted 6 lines
scripts/checkpatch.pl +2
-0@@ -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.
Quoted 8 lines
scripts/checkpatch.pl sub process { · line 2248
Quoted 6 lines
scripts/checkpatch.pl +5
-0+ } + + if ($rawline=~/^-/) { + $deleted_lines_total++ + }and \-\-\-
Ditto.
Thanks, Nick
PATCH 1/1 checkpatch: warn if changed lines exceeds a maximum size
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
--- scripts/checkpatch.pl | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
scripts/checkpatch.pl +21 -0
@@ -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;
Signature · 1 line
2.14.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Joe Perches
maintainerQuoted 3 lines · Nicholas
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.
Quoted 2 lines
Thanks,
Nick
cheers, Joe
Quoted 68 lines · Nicholas Brown
Quoted 66 lines
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-changesSigned-off-by Nicholas Brown <[email protected]>--- scripts/checkpatch.pl | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)scripts/checkpatch.pl +14
-0@@ -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 {
Quoted 4 lines
scripts/checkpatch.pl sub process { · line 2324
Quoted 4 lines
scripts/checkpatch.pl +2
-0$prefix = ''; + #print "inserted: $inserted_lines_total\n"; + #print "deleted: $deleted_lines_total\n";
Not much utility in the debugging code either.
Quoted 8 lines
Quoted 8 lines
scripts/checkpatch.pl +5
-0+ 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;
Quoted 10 lines · Joe Perches
Quoted 4 lines · Nicholas
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.
Quoted 5 lines
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
Quoted 100 lines
Quoted 3 lines
Thanks,
Nickcheers, Joe
Quoted 77 lines · Nicholas Brown
Quoted 75 lines
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-changesSigned-off-by Nicholas Brown <[email protected]>--- scripts/checkpatch.pl | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)scripts/checkpatch.pl +1
-0@@ -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 separatedmessage 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 summaryscripts/checkpatch.pl +11
-0@@ -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$@) {
scripts/checkpatch.pl +0
-0@@ -2306,6 +2320,13 @@ sub process {Quoted 4 lines
scripts/checkpatch.pl sub process { · line 2324
Quoted 4 lines
scripts/checkpatch.pl +2
-0$prefix = ''; + #print "inserted: $inserted_lines_total\n"; + #print "deleted: $deleted_lines_total\n";Not much utility in the debugging code either.
Quoted 10 lines
Quoted 10 lines
scripts/checkpatch.pl +5
-0+ 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;scripts/checkpatch.pl +0
-0$linenr = 0; $fixlinenr = -1;
Quoted 8 lines · Nicholas
Quoted 2 lines
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