scripts: let checkpatch.pl process an entire GIT branch

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

For example:

    $ ./scripts/checkpatch.pl --branch
    total: 0 errors, 0 warnings, 297 lines checked

b886d352a2bf58f0996471fb3991a138373a2957 has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 182 lines checked

2a731f9a9ce145e0e0df6d42dd2a3ce4dfc543fa has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 102 lines checked

11844169bcc0c8ed4449eb3744a69877ed329dd7 has no obvious style problems and is ready for submission.

By default it checks every patch identified by 'master..', however, an alternative origin can be given if desired, if the current branch is rebased to another non-master branch:

    $ ./scripts/checkpatch.pl --branch somebranch..

Signed-off-by Daniel P. Berrange <[email protected]>

---
 scripts/checkpatch.pl | 97 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 26 deletions(-)

scripts/checkpatch.pl +71 -26

index fa478074b8..f8d080441f 100755
@@ -19,6 +19,8 @@ my $quiet = 0;
 my $tree = 1;
 my $chk_signoff = 1;
 my $chk_patch = 1;
+my $chk_branch = 0;
+my $revlist = "master..";
 my $tst_only;
 my $emacs = 0;
 my $terse = 0;

@@ -43,6 +45,7 @@ Options:
   --no-tree                  run without a kernel tree
   --no-signoff               do not check for 'Signed-off-by' line
   --patch                    treat FILE as patchfile (default)
+  --branch                   check all patches on branch since master
   --emacs                    emacs compile window format
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file

@@ -69,6 +72,7 @@ GetOptions(
 	'tree!'		=> \$tree,
 	'signoff!'	=> \$chk_signoff,
 	'patch!'	=> \$chk_patch,
+	'branch'	=> \$chk_branch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
 	'f|file!'	=> \$file,

@@ -88,9 +92,19 @@ help(0) if ($help);
 
 my $exit = 0;
 
-if ($#ARGV < 0) {
-	print "$P: no input files\n";
-	exit(1);
+if ($chk_branch) {
+	if ($#ARGV > 0) {
+		print "$P: expected zero or one origni revisions\n";
+		exit(1);
+	}
+	if ($#ARGV == 0) {
+		$revlist = shift @ARGV;
+	}
+} else {
+	if ($#ARGV < 0) {
+		print "$P: no input files\n";
+		exit(1);
+	}
 }
 
 my $dbg_values = 0;

@@ -251,32 +265,63 @@ $chk_signoff = 0 if ($file);
 my @rawlines = ();
 my @lines = ();
 my $vname;
-for my $filename (@ARGV) {
-	my $FILE;
-	if ($file) {
-		open($FILE, '-|', "diff -u /dev/null $filename") ||
-			die "$P: $filename: diff failed - $!\n";
-	} elsif ($filename eq '-') {
-		open($FILE, '<&STDIN');
-	} else {
-		open($FILE, '<', "$filename") ||
-			die "$P: $filename: open failed - $!\n";
-	}
-	if ($filename eq '-') {
-		$vname = 'Your patch';
-	} else {
-		$vname = $filename;
-	}
-	while (<$FILE>) {
+if ($chk_branch) {
+	my @patches;
+	my $HASH;
+	open($HASH, "-|", "git", "log", "--format=%H", $revlist) ||
+		die "$P: git log --format=%H $revlist failed - $!\n";
+
+	while (<$HASH>) {
 		chomp;
-		push(@rawlines, $_);
+		push @patches, $_;
 	}
-	close($FILE);
-	if (!process($filename)) {
-		$exit = 1;
+
+	close $HASH;
+
+	for my $hash (@patches) {
+		my $FILE;
+		open($FILE, '-|', "git", "show", $hash) ||
+			die "$P: git show $hash - $!\n";
+		$vname = $hash;
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($hash)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
+	}
+} else {
+	for my $filename (@ARGV) {
+		my $FILE;
+		if ($file) {
+			open($FILE, '-|', "diff -u /dev/null $filename") ||
+				die "$P: $filename: diff failed - $!\n";
+		} elsif ($filename eq '-') {
+			open($FILE, '<&STDIN');
+		} else {
+			open($FILE, '<', "$filename") ||
+				die "$P: $filename: open failed - $!\n";
+		}
+		if ($filename eq '-') {
+			$vname = 'Your patch';
+		} else {
+			$vname = $filename;
+		}
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($filename)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
 	}
-	@rawlines = ();
-	@lines = ();
 }
 
 exit($exit);

Quoted 6 lines · Daniel P. Berrange

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

Quoted 3 lines

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

Great idea, though I'm not sure about having a default. And to keep it easy to invoke, having a sole argument that ends with ".." might DWIM and enable --branch too...

Paolo

Quoted 162 lines

For example:

    $ ./scripts/checkpatch.pl --branch
    total: 0 errors, 0 warnings, 297 lines checked

b886d352a2bf58f0996471fb3991a138373a2957 has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 182 lines checked

2a731f9a9ce145e0e0df6d42dd2a3ce4dfc543fa has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 102 lines checked

11844169bcc0c8ed4449eb3744a69877ed329dd7 has no obvious style problems and is ready for submission.

By default it checks every patch identified by 'master..', however, an alternative origin can be given if desired, if the current branch is rebased to another non-master branch:

    $ ./scripts/checkpatch.pl --branch somebranch..

Signed-off-by Daniel P. Berrange <[email protected]>

---
 scripts/checkpatch.pl | 97 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 26 deletions(-)

scripts/checkpatch.pl +71 -26

index fa478074b8..f8d080441f 100755
@@ -19,6 +19,8 @@ my $quiet = 0;
 my $tree = 1;
 my $chk_signoff = 1;
 my $chk_patch = 1;
+my $chk_branch = 0;
+my $revlist = "master..";
 my $tst_only;
 my $emacs = 0;
 my $terse = 0;

@@ -43,6 +45,7 @@ Options:
   --no-tree                  run without a kernel tree
   --no-signoff               do not check for 'Signed-off-by' line
   --patch                    treat FILE as patchfile (default)
+  --branch                   check all patches on branch since master
   --emacs                    emacs compile window format
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file

@@ -69,6 +72,7 @@ GetOptions(
 	'tree!'		=> \$tree,
 	'signoff!'	=> \$chk_signoff,
 	'patch!'	=> \$chk_patch,
+	'branch'	=> \$chk_branch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
 	'f|file!'	=> \$file,

@@ -88,9 +92,19 @@ help(0) if ($help);
 
 my $exit = 0;
 
-if ($#ARGV < 0) {
-	print "$P: no input files\n";
-	exit(1);
+if ($chk_branch) {
+	if ($#ARGV > 0) {
+		print "$P: expected zero or one origni revisions\n";
+		exit(1);
+	}
+	if ($#ARGV == 0) {
+		$revlist = shift @ARGV;
+	}
+} else {
+	if ($#ARGV < 0) {
+		print "$P: no input files\n";
+		exit(1);
+	}
 }
 
 my $dbg_values = 0;

@@ -251,32 +265,63 @@ $chk_signoff = 0 if ($file);
 my @rawlines = ();
 my @lines = ();
 my $vname;
-for my $filename (@ARGV) {
-	my $FILE;
-	if ($file) {
-		open($FILE, '-|', "diff -u /dev/null $filename") ||
-			die "$P: $filename: diff failed - $!\n";
-	} elsif ($filename eq '-') {
-		open($FILE, '<&STDIN');
-	} else {
-		open($FILE, '<', "$filename") ||
-			die "$P: $filename: open failed - $!\n";
-	}
-	if ($filename eq '-') {
-		$vname = 'Your patch';
-	} else {
-		$vname = $filename;
-	}
-	while (<$FILE>) {
+if ($chk_branch) {
+	my @patches;
+	my $HASH;
+	open($HASH, "-|", "git", "log", "--format=%H", $revlist) ||
+		die "$P: git log --format=%H $revlist failed - $!\n";
+
+	while (<$HASH>) {
 		chomp;
-		push(@rawlines, $_);
+		push @patches, $_;
 	}
-	close($FILE);
-	if (!process($filename)) {
-		$exit = 1;
+
+	close $HASH;
+
+	for my $hash (@patches) {
+		my $FILE;
+		open($FILE, '-|', "git", "show", $hash) ||
+			die "$P: git show $hash - $!\n";
+		$vname = $hash;
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($hash)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
+	}
+} else {
+	for my $filename (@ARGV) {
+		my $FILE;
+		if ($file) {
+			open($FILE, '-|', "diff -u /dev/null $filename") ||
+				die "$P: $filename: diff failed - $!\n";
+		} elsif ($filename eq '-') {
+			open($FILE, '<&STDIN');
+		} else {
+			open($FILE, '<', "$filename") ||
+				die "$P: $filename: open failed - $!\n";
+		}
+		if ($filename eq '-') {
+			$vname = 'Your patch';
+		} else {
+			$vname = $filename;
+		}
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($filename)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
 	}
-	@rawlines = ();
-	@lines = ();
 }
 

scripts/checkpatch.pl $chk_signoff = 0 if ($file); · line 327

Quoted 2 lines

scripts/checkpatch.pl +0 -0

 exit($exit);

Quoted 11 lines · Paolo Bonzini

Quoted 9 lines · Daniel P. Berrange

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

Quoted 3 lines

Great idea, though I'm not sure about having a default. And to keep it easy to invoke, having a sole argument that ends with ".." might DWIM and enable --branch too...

I think it is beneficial to have a default, as I figure the majority of contributors are working on a branch that's rebased against master.. Half as many characters to type in the common case :-)

Sometimes people might write patches against a particular subsystem staging branch (eg kevin/block), but I don't think there's downside in assuming 'master..' by default.

Quoted 165 lines

Paolo

Quoted 162 lines

For example:

    $ ./scripts/checkpatch.pl --branch
    total: 0 errors, 0 warnings, 297 lines checked

b886d352a2bf58f0996471fb3991a138373a2957 has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 182 lines checked

2a731f9a9ce145e0e0df6d42dd2a3ce4dfc543fa has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 102 lines checked

11844169bcc0c8ed4449eb3744a69877ed329dd7 has no obvious style problems and is ready for submission.

By default it checks every patch identified by 'master..', however, an alternative origin can be given if desired, if the current branch is rebased to another non-master branch:

    $ ./scripts/checkpatch.pl --branch somebranch..

Signed-off-by Daniel P. Berrange <[email protected]>

---
 scripts/checkpatch.pl | 97 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 26 deletions(-)

scripts/checkpatch.pl +71 -26

index fa478074b8..f8d080441f 100755
@@ -19,6 +19,8 @@ my $quiet = 0;
 my $tree = 1;
 my $chk_signoff = 1;
 my $chk_patch = 1;
+my $chk_branch = 0;
+my $revlist = "master..";
 my $tst_only;
 my $emacs = 0;
 my $terse = 0;

@@ -43,6 +45,7 @@ Options:
   --no-tree                  run without a kernel tree
   --no-signoff               do not check for 'Signed-off-by' line
   --patch                    treat FILE as patchfile (default)
+  --branch                   check all patches on branch since master
   --emacs                    emacs compile window format
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file

@@ -69,6 +72,7 @@ GetOptions(
 	'tree!'		=> \$tree,
 	'signoff!'	=> \$chk_signoff,
 	'patch!'	=> \$chk_patch,
+	'branch'	=> \$chk_branch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
 	'f|file!'	=> \$file,

@@ -88,9 +92,19 @@ help(0) if ($help);
 
 my $exit = 0;
 
-if ($#ARGV < 0) {
-	print "$P: no input files\n";
-	exit(1);
+if ($chk_branch) {
+	if ($#ARGV > 0) {
+		print "$P: expected zero or one origni revisions\n";
+		exit(1);
+	}
+	if ($#ARGV == 0) {
+		$revlist = shift @ARGV;
+	}
+} else {
+	if ($#ARGV < 0) {
+		print "$P: no input files\n";
+		exit(1);
+	}
 }
 
 my $dbg_values = 0;

@@ -251,32 +265,63 @@ $chk_signoff = 0 if ($file);
 my @rawlines = ();
 my @lines = ();
 my $vname;
-for my $filename (@ARGV) {
-	my $FILE;
-	if ($file) {
-		open($FILE, '-|', "diff -u /dev/null $filename") ||
-			die "$P: $filename: diff failed - $!\n";
-	} elsif ($filename eq '-') {
-		open($FILE, '<&STDIN');
-	} else {
-		open($FILE, '<', "$filename") ||
-			die "$P: $filename: open failed - $!\n";
-	}
-	if ($filename eq '-') {
-		$vname = 'Your patch';
-	} else {
-		$vname = $filename;
-	}
-	while (<$FILE>) {
+if ($chk_branch) {
+	my @patches;
+	my $HASH;
+	open($HASH, "-|", "git", "log", "--format=%H", $revlist) ||
+		die "$P: git log --format=%H $revlist failed - $!\n";
+
+	while (<$HASH>) {
 		chomp;
-		push(@rawlines, $_);
+		push @patches, $_;
 	}
-	close($FILE);
-	if (!process($filename)) {
-		$exit = 1;
+
+	close $HASH;
+
+	for my $hash (@patches) {
+		my $FILE;
+		open($FILE, '-|', "git", "show", $hash) ||
+			die "$P: git show $hash - $!\n";
+		$vname = $hash;
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($hash)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
+	}
+} else {
+	for my $filename (@ARGV) {
+		my $FILE;
+		if ($file) {
+			open($FILE, '-|', "diff -u /dev/null $filename") ||
+				die "$P: $filename: diff failed - $!\n";
+		} elsif ($filename eq '-') {
+			open($FILE, '<&STDIN');
+		} else {
+			open($FILE, '<', "$filename") ||
+				die "$P: $filename: open failed - $!\n";
+		}
+		if ($filename eq '-') {
+			$vname = 'Your patch';
+		} else {
+			$vname = $filename;
+		}
+		while (<$FILE>) {
+			chomp;
+			push(@rawlines, $_);
+		}
+		close($FILE);
+		if (!process($filename)) {
+			$exit = 1;
+		}
+		@rawlines = ();
+		@lines = ();
 	}
-	@rawlines = ();
-	@lines = ();
 }
 

Quoted 3 lines

scripts/checkpatch.pl $chk_signoff = 0 if ($file); · line 327

Quoted 2 lines

scripts/checkpatch.pl +0 -0

 exit($exit);

Regards,
Daniel

Quoted 16 lines · Daniel P. Berrange

Quoted 14 lines · Paolo Bonzini

Quoted 9 lines · Daniel P. Berrange

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

Great idea, though I'm not sure about having a default. And to keep it easy to invoke, having a sole argument that ends with ".." might DWIM and enable --branch too...

Quoted 3 lines

I think it is beneficial to have a default, as I figure the majority of contributors are working on a branch that's rebased against master.. Half as many characters to type in the common case :-)

With the DWIM option "--branch" and "master.." are exactly the same length. :)

Quoted 21 lines · Paolo Bonzini

Quoted 19 lines · Daniel P. Berrange

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines · Daniel P. Berrange

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

Quoted 7 lines

Quoted 3 lines

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

Great idea, though I'm not sure about having a default. And to keep it easy to invoke, having a sole argument that ends with ".." might DWIM and enable --branch too...

I think it is beneficial to have a default, as I figure the majority of contributors are working on a branch that's rebased against master.. Half as many characters to type in the common case :-)

Quoted 2 lines

With the DWIM option "--branch" and "master.." are exactly the same length. :)

Oh hang on. I think I misunderstood what you suggested. I thought you meant 'checkpatch.pl --branch master..', but IIUC you actually mean 'checkpatch.pl master..' with no flag. That would work with me.

Quoted 22 lines · Daniel P. Berrange

Quoted 21 lines · Paolo Bonzini

Quoted 19 lines · Daniel P. Berrange

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines · Daniel P. Berrange

Currently before submitting a series, devs should run checkpatch.pl across each patch to be submitted. This can be automated using a command such as:

git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'

Quoted 7 lines

Quoted 3 lines

This is rather long winded to type, so this patch introduces a new flag '--branch' to checkpatch.pl which instructs it to check every patch on the current GIT branch.

Great idea, though I'm not sure about having a default. And to keep it easy to invoke, having a sole argument that ends with ".." might DWIM and enable --branch too...

I think it is beneficial to have a default, as I figure the majority of contributors are working on a branch that's rebased against master.. Half as many characters to type in the common case :-)

Quoted 6 lines

Quoted 2 lines

With the DWIM option "--branch" and "master.." are exactly the same length. :)

Oh hang on. I think I misunderstood what you suggested. I thought you meant 'checkpatch.pl --branch master..', but IIUC you actually mean 'checkpatch.pl master..' with no flag. That would work with me.

Yes, basically if length(argv) == 1 and argv[0] ends with ".." then enable branch. The default for --branch with no ARGV could be "origin/master.."---or it could ask git-config for the upstream tracking branch but maybe that's too much to ask.

Paolo