Messages in this thread | | | Subject | Re: [PATCH v2] checkpatch: add fix option for MISSING_SIGN_OFF | From | Aditya <> | Date | Wed, 11 Nov 2020 16:39:26 +0530 |
| |
On 11/11/20 4:00 pm, Lukas Bulwahn wrote: > On Wed, Nov 11, 2020 at 10:01 AM Aditya Srivastava <yashsri421@gmail.com> wrote: >> >> Currently checkpatch warns us if there is no 'Signed-off-by' line >> for the patch. >> >> E.g., running checkpatch on commit 9ac060a708e0 ("leaking_addresses: >> Completely remove --version flag") reports this error: >> >> ERROR: Missing Signed-off-by: line(s) >> >> Provide a fix by adding a Signed-off-by line corresponding to the author >> of the patch before the patch separator line. Also avoid this error for >> the commits where some typo is present in the sign off. >> >> E.g. for commit 8cde5d5f7361 ("bus: ti-sysc: Detect omap4 type timers >> for quirk") we get missing sign off as well as bad sign off for: >> >> Siganed-off-by: Tony Lindgren <tony@atomide.com> >> >> Here it is probably best to give BAD_SIGN_OFF warning for Non-standard >> signature and avoid MISSING_SIGN_OFF >> >> Suggested-by: Joe Perches <joe@perches.com> >> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com> >> --- >> Changes in v2: >> Add space after 'if' >> Add check for $patch_separator_linenr to be greater than 0 >> >> scripts/checkpatch.pl | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index cb46288127ac..ac7e5ac80b58 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -2404,6 +2404,8 @@ sub process { >> >> my $last_blank_line = 0; >> my $last_coalesced_string_linenr = -1; >> + my $patch_separator_linenr = 0; >> + my $non_standard_signature = 0; >> >> our @report = (); >> our $cnt_lines = 0; >> @@ -2755,6 +2757,10 @@ sub process { >> if ($line =~ /^---$/) { >> $has_patch_separator = 1; >> $in_commit_log = 0; >> + # to add missing sign off line before diff(s) >> + if ($patch_separator_linenr == 0) { >> + $patch_separator_linenr = $linenr; >> + } >> } >> >> # Check if MAINTAINERS is being updated. If so, there's probably no need to >> @@ -2775,6 +2781,9 @@ sub process { >> if ($sign_off !~ /$signature_tags/) { >> WARN("BAD_SIGN_OFF", >> "Non-standard signature: $sign_off\n" . $herecurr); >> + >> + # to avoid missing_sign_off error as it most probably is just a typo >> + $non_standard_signature = 1; >> } >> if (defined $space_before && $space_before ne "") { >> if (WARN("BAD_SIGN_OFF", >> @@ -7118,9 +7127,12 @@ sub process { >> "Does not appear to be a unified-diff format patch\n"); >> } >> if ($is_patch && $has_commit_log && $chk_signoff) { >> - if ($signoff == 0) { >> - ERROR("MISSING_SIGN_OFF", >> - "Missing Signed-off-by: line(s)\n"); >> + if ($signoff == 0 && !$non_standard_signature) { >> + if (ERROR("MISSING_SIGN_OFF", >> + "Missing Signed-off-by: line(s)\n") && >> + $fix && $patch_separator_linenr > 0) { >> + fix_insert_line($patch_separator_linenr - 1, "Signed-off-by: $author"); >> + } > > Maybe I am already digging too much in the details... however: > > I think it should still warn about a Missing Signed-off-by: even when > we know there is a $non_standard_signature. So, checkpatch simply > emits two warnings; that is okay in that case. > > It is just that our evaluation shows that the provided fix option > should not be suggested when there is a $non_standard_signature > because we actually would predict that there is typo in the intended > Signed-off-by tag and the fix that checkpatch would suggest would not > be adequate. > > Joe, what is your opinion? > > Aditya, it should not be too difficult to implement the rule that way, right? >
No, I'd probably just have to add the check with $fix, instead of with $signoff
Thanks Aditya
> >> } elsif ($authorsignoff != 1) { >> # authorsignoff values: >> # 0 -> missing sign off >> -- >> 2.17.1 >>
| |