* [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line @ 2020-02-24 23:58 John Stultz 2020-02-25 2:12 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2020-02-24 23:58 UTC (permalink / raw) To: lkml; +Cc: John Stultz, Andy Whitcroft, Joe Perches Quite often, the Change-Id may be between Signed-off-by: lines or at the end of them. Unfortunately checkpatch won't catch these cases as it disables in_commit_log when it catches the first Signed-off-by line. This has bitten me many many times. I suspect this patch will break other use cases, so it probably shouldn't be merged, but I wanted to share it just to help illustrate the problem. Cc: Andy Whitcroft <apw@canonical.com> Cc: Joe Perches <joe@perches.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- scripts/checkpatch.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a63380c6b0d2..a55340a9e3ea 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2609,7 +2609,8 @@ sub process { # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:/i) { $signoff++; - $in_commit_log = 0; + #Disabling in_commit_log here breaks Change-Id checking in some cases + #$in_commit_log = 0; if ($author ne '') { my $l = $line; $l =~ s/"//g; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-24 23:58 [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line John Stultz @ 2020-02-25 2:12 ` Joe Perches 2020-02-25 4:48 ` John Stultz 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2020-02-25 2:12 UTC (permalink / raw) To: John Stultz, lkml; +Cc: Andy Whitcroft On Mon, 2020-02-24 at 23:58 +0000, John Stultz wrote: > Quite often, the Change-Id may be between Signed-off-by: lines or > at the end of them. Unfortunately checkpatch won't catch these > cases as it disables in_commit_log when it catches the first > Signed-off-by line. > > This has bitten me many many times. Hmm. When is change-id used in your workflow? > I suspect this patch will break other use cases, so it probably > shouldn't be merged, but I wanted to share it just to help > illustrate the problem. > > Cc: Andy Whitcroft <apw@canonical.com> > Cc: Joe Perches <joe@perches.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> Yes, I expect this will break things. And it's probably better to not add a Signed-off-by: when you intend this not to be merged. > --- > scripts/checkpatch.pl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index a63380c6b0d2..a55340a9e3ea 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2609,7 +2609,8 @@ sub process { > # Check the patch for a signoff: > if ($line =~ /^\s*signed-off-by:/i) { > $signoff++; > - $in_commit_log = 0; > + #Disabling in_commit_log here breaks Change-Id checking in some cases > + #$in_commit_log = 0; > if ($author ne '') { > my $l = $line; > $l =~ s/"//g; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 2:12 ` Joe Perches @ 2020-02-25 4:48 ` John Stultz 2020-02-25 6:49 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: John Stultz @ 2020-02-25 4:48 UTC (permalink / raw) To: Joe Perches; +Cc: lkml, Andy Whitcroft On Mon, Feb 24, 2020 at 6:13 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-02-24 at 23:58 +0000, John Stultz wrote: > > Quite often, the Change-Id may be between Signed-off-by: lines or > > at the end of them. Unfortunately checkpatch won't catch these > > cases as it disables in_commit_log when it catches the first > > Signed-off-by line. > > > > This has bitten me many many times. > > Hmm. When is change-id used in your workflow? Since I have a few kernel repos that I use for both upstream work and work targeting AOSP trees, I usually have the gerrit commit hook enabled in my tree (its easier to strip with sed then it is to re-add after submitting to gerrit), and at least the commit-msg hook I have will usually append a Change-Id: line at the end of the commit message, usually after the signed-off-by line. Even in the example in the README from: https://android.googlesource.com/kernel/common/+/android-mainline shows how one might have the change-id and other AOSP tags added after the existing sob-chain. So it doesn't seem to be that rare. Some other examples from the android-mainline tree: https://android.googlesource.com/kernel/common/+/5fba1b18cfc72e264e5f3ce49020ed322aa6ac9f https://android.googlesource.com/kernel/common/+/6ea0a439a15ba42b6c5f81618e53d5c61f89e4ac https://android.googlesource.com/kernel/common/+/99f4553ab4a6b008b37e878f7046a2202cdb2ec4 > > I suspect this patch will break other use cases, so it probably > > shouldn't be merged, but I wanted to share it just to help > > illustrate the problem. > > > > Cc: Andy Whitcroft <apw@canonical.com> > > Cc: Joe Perches <joe@perches.com> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > Yes, I expect this will break things. Suggestions for a better approach? I can't say I'm very familiar with checkpatch's code. > And it's probably better to not add a Signed-off-by: when > you intend this not to be merged. So, I try to add Sign-off-by to all the patches I send as it certifies that I wrote it or otherwise have the right to pass it on as an open-source patch - not as "ok to merge" criteria. More vendor code then I'd like is usually not intended to be merged upstream, but it's still important that folks sign off their patches. :) thanks -john ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 4:48 ` John Stultz @ 2020-02-25 6:49 ` Joe Perches 2020-02-25 17:45 ` John Stultz 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2020-02-25 6:49 UTC (permalink / raw) To: John Stultz; +Cc: lkml, Andy Whitcroft On Mon, 2020-02-24 at 20:48 -0800, John Stultz wrote: > On Mon, Feb 24, 2020 at 6:13 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-02-24 at 23:58 +0000, John Stultz wrote: > > > Quite often, the Change-Id may be between Signed-off-by: lines or > > > at the end of them. Unfortunately checkpatch won't catch these > > > cases as it disables in_commit_log when it catches the first > > > Signed-off-by line. > > > > > > This has bitten me many many times. > > > > Hmm. When is change-id used in your workflow? > > Since I have a few kernel repos that I use for both upstream work and > work targeting AOSP trees, I usually have the gerrit commit hook > enabled in my tree (its easier to strip with sed then it is to re-add > after submitting to gerrit), and at least the commit-msg hook I have > will usually append a Change-Id: line at the end of the commit > message, usually after the signed-off-by line. Perhaps this is better: --- scripts/checkpatch.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a63380..698c7c8 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2721,9 +2721,9 @@ sub process { } # Check for unwanted Gerrit info - if ($in_commit_log && $line =~ /^\s*change-id:/i) { + if ($realfile eq '' && $line =~ /^\s*change-id:/i) { ERROR("GERRIT_CHANGE_ID", - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); } # Check if the commit log is in a possible stack dump ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 6:49 ` Joe Perches @ 2020-02-25 17:45 ` John Stultz 2020-02-25 18:02 ` Joe Perches ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: John Stultz @ 2020-02-25 17:45 UTC (permalink / raw) To: Joe Perches; +Cc: lkml, Andy Whitcroft On Mon, Feb 24, 2020 at 10:50 PM Joe Perches <joe@perches.com> wrote: > > Since I have a few kernel repos that I use for both upstream work and > > work targeting AOSP trees, I usually have the gerrit commit hook > > enabled in my tree (its easier to strip with sed then it is to re-add > > after submitting to gerrit), and at least the commit-msg hook I have > > will usually append a Change-Id: line at the end of the commit > > message, usually after the signed-off-by line. > > Perhaps this is better: > --- > scripts/checkpatch.pl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index a63380..698c7c8 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2721,9 +2721,9 @@ sub process { > } > > # Check for unwanted Gerrit info > - if ($in_commit_log && $line =~ /^\s*change-id:/i) { > + if ($realfile eq '' && $line =~ /^\s*change-id:/i) { > ERROR("GERRIT_CHANGE_ID", > - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); > + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); > } > > # Check if the commit log is in a possible stack dump > Yep. This works well for me, catching Change-Id lines that are missed by the current code. Tested-by: John Stultz <john.stultz@linaro.org> Thanks so much! -john ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 17:45 ` John Stultz @ 2020-02-25 18:02 ` Joe Perches 2020-02-25 20:14 ` Joe Perches 2020-02-25 22:16 ` [PATCH] checkpatch: Improve Gerrit Change-Id: test Joe Perches 2 siblings, 0 replies; 9+ messages in thread From: Joe Perches @ 2020-02-25 18:02 UTC (permalink / raw) To: John Stultz; +Cc: lkml, Andy Whitcroft On Tue, 2020-02-25 at 09:45 -0800, John Stultz wrote: > On Mon, Feb 24, 2020 at 10:50 PM Joe Perches <joe@perches.com> wrote: > > > Since I have a few kernel repos that I use for both upstream work and > > > work targeting AOSP trees, I usually have the gerrit commit hook > > > enabled in my tree (its easier to strip with sed then it is to re-add > > > after submitting to gerrit), and at least the commit-msg hook I have > > > will usually append a Change-Id: line at the end of the commit > > > message, usually after the signed-off-by line. > > > > Perhaps this is better: > > --- > > scripts/checkpatch.pl | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > @@ -2721,9 +2721,9 @@ sub process { > > } > > > > # Check for unwanted Gerrit info > > - if ($in_commit_log && $line =~ /^\s*change-id:/i) { > > + if ($realfile eq '' && $line =~ /^\s*change-id:/i) { > > ERROR("GERRIT_CHANGE_ID", > > - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); > > + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); > > } > > > > # Check if the commit log is in a possible stack dump > > > > Yep. This works well for me, catching Change-Id lines that are missed > by the current code. > > Tested-by: John Stultz <john.stultz@linaro.org> A negative approach to this is if the change-id is after the "---" separator line but before any file diff, then the "Remove Gerrit Change-Id" message will still be emitted. Dunno if that's an issue. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 17:45 ` John Stultz 2020-02-25 18:02 ` Joe Perches @ 2020-02-25 20:14 ` Joe Perches 2020-02-25 21:33 ` John Stultz 2020-02-25 22:16 ` [PATCH] checkpatch: Improve Gerrit Change-Id: test Joe Perches 2 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2020-02-25 20:14 UTC (permalink / raw) To: John Stultz; +Cc: lkml, Andy Whitcroft, Andrew Morton On Tue, 2020-02-25 at 09:45 -0800, John Stultz wrote: > On Mon, Feb 24, 2020 at 10:50 PM Joe Perches <joe@perches.com> wrote: > > > Since I have a few kernel repos that I use for both upstream work and > > > work targeting AOSP trees, I usually have the gerrit commit hook > > > enabled in my tree (its easier to strip with sed then it is to re-add > > > after submitting to gerrit), and at least the commit-msg hook I have > > > will usually append a Change-Id: line at the end of the commit > > > message, usually after the signed-off-by line. I think this better still: --- scripts/checkpatch.pl | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 3d55d8a2a..d0f850e 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2343,6 +2343,7 @@ sub process { my $is_binding_patch = -1; my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch + my $has_patch_separator = 0; #Found a --- line my $has_commit_log = 0; #Encountered lines before patch my $commit_log_lines = 0; #Number of commit log lines my $commit_log_possible_stack_dump = 0; @@ -2668,6 +2669,12 @@ sub process { } } +# Check for patch separator + if ($line =~ /^---$/) { + $has_patch_separator = 1; + $in_commit_log = 0; + } + # Check if MAINTAINERS is being updated. If so, there's probably no need to # emit the "does MAINTAINERS need updating?" message on file add/move/delete if ($line =~ /^\s*MAINTAINERS\s*\|/) { @@ -2767,10 +2774,10 @@ sub process { "A patch subject line should describe the change not the tool that found it\n" . $herecurr); } -# Check for unwanted Gerrit info - if ($in_commit_log && $line =~ /^\s*change-id:/i) { +# Check for Gerrit Change-Ids not in any patch context + if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) { ERROR("GERRIT_CHANGE_ID", - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); } # Check if the commit log is in a possible stack dump ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line 2020-02-25 20:14 ` Joe Perches @ 2020-02-25 21:33 ` John Stultz 0 siblings, 0 replies; 9+ messages in thread From: John Stultz @ 2020-02-25 21:33 UTC (permalink / raw) To: Joe Perches; +Cc: lkml, Andy Whitcroft, Andrew Morton On Tue, Feb 25, 2020 at 12:16 PM Joe Perches <joe@perches.com> wrote: > On Tue, 2020-02-25 at 09:45 -0800, John Stultz wrote: > > On Mon, Feb 24, 2020 at 10:50 PM Joe Perches <joe@perches.com> wrote: > > > > Since I have a few kernel repos that I use for both upstream work and > > > > work targeting AOSP trees, I usually have the gerrit commit hook > > > > enabled in my tree (its easier to strip with sed then it is to re-add > > > > after submitting to gerrit), and at least the commit-msg hook I have > > > > will usually append a Change-Id: line at the end of the commit > > > > message, usually after the signed-off-by line. > > I think this better still: > > --- > scripts/checkpatch.pl | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 3d55d8a2a..d0f850e 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2343,6 +2343,7 @@ sub process { > my $is_binding_patch = -1; > my $in_header_lines = $file ? 0 : 1; > my $in_commit_log = 0; #Scanning lines before patch > + my $has_patch_separator = 0; #Found a --- line > my $has_commit_log = 0; #Encountered lines before patch > my $commit_log_lines = 0; #Number of commit log lines > my $commit_log_possible_stack_dump = 0; > @@ -2668,6 +2669,12 @@ sub process { > } > } > > +# Check for patch separator > + if ($line =~ /^---$/) { > + $has_patch_separator = 1; > + $in_commit_log = 0; > + } > + > # Check if MAINTAINERS is being updated. If so, there's probably no need to > # emit the "does MAINTAINERS need updating?" message on file add/move/delete > if ($line =~ /^\s*MAINTAINERS\s*\|/) { > @@ -2767,10 +2774,10 @@ sub process { > "A patch subject line should describe the change not the tool that found it\n" . $herecurr); > } > > -# Check for unwanted Gerrit info > - if ($in_commit_log && $line =~ /^\s*change-id:/i) { > +# Check for Gerrit Change-Ids not in any patch context > + if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) { > ERROR("GERRIT_CHANGE_ID", > - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); > + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); > } > > # Check if the commit log is in a possible stack dump This one works well for me too. Tested-by: John Stultz <john.stultz@linaro.org> Thanks so much! -john ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] checkpatch: Improve Gerrit Change-Id: test 2020-02-25 17:45 ` John Stultz 2020-02-25 18:02 ` Joe Perches 2020-02-25 20:14 ` Joe Perches @ 2020-02-25 22:16 ` Joe Perches 2 siblings, 0 replies; 9+ messages in thread From: Joe Perches @ 2020-02-25 22:16 UTC (permalink / raw) To: Andrew Morton; +Cc: lkml, Andy Whitcroft, John Stultz The Gerrit Change-Id: entry is sometimes placed after a Signed-off-by: line. When this occurs, the Gerrit warning is not currently emitted as the first Signed-off-by: signature sets a flag to stop looking. Change the test to add a test for the --- patch separator and emit the warning before any before the --- and also before any diff file name. Signed-off-by: Joe Perches <joe@perches.com> Tested-by: John Stultz <john.stultz@linaro.org> --- scripts/checkpatch.pl | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 3d55d8a2a..d0f850e 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2343,6 +2343,7 @@ sub process { my $is_binding_patch = -1; my $in_header_lines = $file ? 0 : 1; my $in_commit_log = 0; #Scanning lines before patch + my $has_patch_separator = 0; #Found a --- line my $has_commit_log = 0; #Encountered lines before patch my $commit_log_lines = 0; #Number of commit log lines my $commit_log_possible_stack_dump = 0; @@ -2668,6 +2669,12 @@ sub process { } } +# Check for patch separator + if ($line =~ /^---$/) { + $has_patch_separator = 1; + $in_commit_log = 0; + } + # Check if MAINTAINERS is being updated. If so, there's probably no need to # emit the "does MAINTAINERS need updating?" message on file add/move/delete if ($line =~ /^\s*MAINTAINERS\s*\|/) { @@ -2767,10 +2774,10 @@ sub process { "A patch subject line should describe the change not the tool that found it\n" . $herecurr); } -# Check for unwanted Gerrit info - if ($in_commit_log && $line =~ /^\s*change-id:/i) { +# Check for Gerrit Change-Ids not in any patch context + if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) { ERROR("GERRIT_CHANGE_ID", - "Remove Gerrit Change-Id's before submitting upstream.\n" . $herecurr); + "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr); } # Check if the commit log is in a possible stack dump ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-25 22:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-24 23:58 [RFC][PATCH] checkpatch: Properly warn if Change-Id comes after first Signed-off-by line John Stultz 2020-02-25 2:12 ` Joe Perches 2020-02-25 4:48 ` John Stultz 2020-02-25 6:49 ` Joe Perches 2020-02-25 17:45 ` John Stultz 2020-02-25 18:02 ` Joe Perches 2020-02-25 20:14 ` Joe Perches 2020-02-25 21:33 ` John Stultz 2020-02-25 22:16 ` [PATCH] checkpatch: Improve Gerrit Change-Id: test Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).