[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for MISSING_SIGN_OFF
Lukas Bulwahn
lukas.bulwahn at gmail.com
Tue Nov 10 18:43:34 UTC 2020
On Di., 10. Nov. 2020 at 19:13, Aditya <yashsri421 at gmail.com> wrote:
> On 10/11/20 8:52 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 10, 2020 at 4:13 PM Aditya <yashsri421 at gmail.com> wrote:
> >>
> >> On 10/11/20 7:30 pm, Lukas Bulwahn wrote:
> >>> On Tue, Nov 10, 2020 at 2:06 PM Aditya Srivastava <
> yashsri421 at 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 at 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 at perches.com>
> >>>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> >>>> ---
> >>>> This patch was made after applying
> https://lore.kernel.org/linux-kernel-mentees/20201108100632.75340-1-dwaipayanray1@gmail.com/
> >>>> and my last changes at
> https://lore.kernel.org/linux-kernel-mentees/20201108134317.25400-1-yashsri421@gmail.com/
> >>>>
> >>>> scripts/checkpatch.pl | 18 +++++++++++++++---
> >>>> 1 file changed, 15 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>>> index cb46288127ac..2deffd0c091b 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;
> >>>
> >>> Well are you sure linenr is correct? (As I wrote, I do not know, but
> >>> you should know.)
> >>>
> >>> I think you would need to create a patch for testing your feature
> >>> where multiple things are fixed within the same patch and then check
> >>> if the fix is added at the right place.
> >>>
> >>
> >> I checked the fix over following patch.
> >>
> >> I have modified the patch such that it has co-developed-by as the
> >> author(nominal patch author warning, which requires this line to be
> >> deleted)
> >>
> >> Also 'occuring', which is a typo(where the typo needs to be replaced)
> >>
> >> It works as expected.
> >>
> >
> > Well to really check if it works as expected, you need to have a test
> > patch where a fix adds multiple lines into the fixed patch before the
> > line you try to record.
> >
> > Then, fixlinenr and linenr are probably more than just 1 line apart,
> > but really multiple lines apart and only if you record the right
> > variable it really works.
> >
>
> Actually we currently don't have any new line insertion fixes for
> lines above patch separator.
> So, I modified checkpatch a bit to insert (instead of delete) for
> co-developed-by.
> ie here:
>
> if (WARN("BAD_SIGN_OFF",
> "Co-developed-by: should not be used to attribute nominal patch
> author '$author'\n" . "$here\n" . $rawline) &&
> $fix) {
> - fix_delete_line($fixlinenr, $rawline);
> + fix_insert_line($fixlinenr, $rawline);
> }
>
>
> When repeated a few times and removing signed-off-by, this provided an
> experience of inserting multiple lines before signed-off-by (as for
> each co-developed-by, it is inserting a new co-developed-by)
>
> With this as well, the fix was working as expected, ie, got this after
> some iterations of removing signed-off-by and keeping co-developed-by:
>
> ...
> It is causing boot failures with qemu mac99 in at least some
> configurations.occurring
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Co-developed-by: Michael Ellerman <mpe at ellerman.id.au>
> Signed-off-by: Michael Ellerman <mpe at ellerman.id.au>
> ---
> arch/powerpc/include/asm/book3s/32/hash.h | 8 ++++----
>
>
I did not look into the details. If you are 100% sure that your patch is
right, let us send it to Joe and the mailing list.
I think next a fix for bad sign-off that corrects those obvious typos would
be nice as well.
Lukas
> Aditya
>
> > Other than that, it is all good.
> >
> > Lukas
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/linux-kernel-mentees/attachments/20201110/f85da970/attachment.html>
More information about the Linux-kernel-mentees
mailing list