[PATCH] checkpatch.pl: Add check for Change-Id

[PATCH] checkpatch.pl: Add check for Change-Id

[Christopher Covington]

A commit hook for the Gerrit code review server inserts change
identifiers so Gerrit can track patches through multiple revisions.
These identifiers are noise in the context of the upstream kernel.
(Many Gerrit servers are private. Even given a public instance,
given only a Change-Id, one must guess which server a change was
tracked on. Patches submitted to the Linux kernel mailing lists
should be able to stand on their own. If it's truly useful to
reference code review on a Gerrit server, a URL is a much clearer
way to do so.) Thus, issue an error when a Change-Id: line is
encountered.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 464dcef..afd6dde 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1891,6 +1891,12 @@ sub process {
 			}
 		}
 
+# Check for unwanted Gerrit info
+		if ($line =~ /^\s*change-id:/i) {
+			ERROR("BAD_SIGN_OFF",
+			      "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr);
+		}
+
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("CORRUPTED_PATCH",
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by the Linux Foundation.

[PATCH] checkpatch.pl: Add check for Change-Id

[Joe Perches]

On Wed, 2014-03-19 at 14:54 -0400, Christopher Covington wrote:
> A commit hook for the Gerrit code review server inserts change
> identifiers so Gerrit can track patches through multiple revisions.
> These identifiers are noise in the context of the upstream kernel.
> (Many Gerrit servers are private. Even given a public instance,
> given only a Change-Id, one must guess which server a change was
> tracked on. Patches submitted to the Linux kernel mailing lists
> should be able to stand on their own. If it's truly useful to
> reference code review on a Gerrit server, a URL is a much clearer
> way to do so.) Thus, issue an error when a Change-Id: line is
> encountered.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1891,6 +1891,12 @@ sub process {
>  			}
>  		}
>  
> +# Check for unwanted Gerrit info
> +		if ($line =~ /^\s*change-id:/i) {
> +			ERROR("BAD_SIGN_OFF",
> +			      "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr);
> +		}
> +

I think this needs an "$in_commit_log" test added

		if ($in_commit_log &&
		    $line =~ /^\s*change-id:/i) {

And maybe use a separate "TYPE", not "BAD_SIGN_OFF".
Maybe "GERRIT_CHANGE_ID"

[PATCH] checkpatch.pl: Add check for Change-Id

[Christopher Covington]

Hi Joe,

On 03/20/2014 06:08 AM, Joe Perches wrote:
> On Wed, 2014-03-19 at 14:54 -0400, Christopher Covington wrote:
>> A commit hook for the Gerrit code review server inserts change
>> identifiers so Gerrit can track patches through multiple revisions.
>> These identifiers are noise in the context of the upstream kernel.
>> (Many Gerrit servers are private. Even given a public instance,
>> given only a Change-Id, one must guess which server a change was
>> tracked on. Patches submitted to the Linux kernel mailing lists
>> should be able to stand on their own. If it's truly useful to
>> reference code review on a Gerrit server, a URL is a much clearer
>> way to do so.) Thus, issue an error when a Change-Id: line is
>> encountered.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -1891,6 +1891,12 @@ sub process {
>>  			}
>>  		}
>>  
>> +# Check for unwanted Gerrit info
>> +		if ($line =~ /^\s*change-id:/i) {
>> +			ERROR("BAD_SIGN_OFF",
>> +			      "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr);
>> +		}
>> +
> 
> I think this needs an "$in_commit_log" test added
> 
> 		if ($in_commit_log &&
> 		    $line =~ /^\s*change-id:/i) {

This would mean that Change-Id lines that follow a Signed-off-by line will not
get caught. I expect that the common case is to use the commit hook shipped
with Gerrit [1], which inserts the Change-Id above the Signed-off-by, so the
proposed change is fine by me.

1.
https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg

> And maybe use a separate "TYPE", not "BAD_SIGN_OFF".
> Maybe "GERRIT_CHANGE_ID"
> 

Will do.

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.