[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for MISSING_SIGN_OFF

[Lukas Bulwahn]

On Tue, Nov 10, 2020 at 7:43 AM Aditya <yashsri421 at gmail.com> wrote:
>
> On 9/11/20 11:20 pm, Lukas Bulwahn wrote:
> > On Mon, Nov 9, 2020 at 5:35 PM Aditya <yashsri421 at gmail.com> wrote:
> >>
> >> On 9/11/20 8:15 pm, Lukas Bulwahn wrote:
> >>> On Mon, Nov 9, 2020 at 3:33 PM Aditya <yashsri421 at gmail.com> wrote:
> >>>>
> >>>> On 9/11/20 7:10 pm, Lukas Bulwahn wrote:
> >>>>> On Mo., 9. Nov. 2020 at 14:10, 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 f68e7927212f ("Revert
> >>>>>> "powerpc/book3s32: Reorder _PAGE_XXX flags to simplify TLB handling"")
> >>>>>> 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 start of diff(s)
> >>>>>>
> >>>>>
> >>>>> Can you please provide an evaluation first?
> >>>>>
> >>>>> As far as remember, the fix is often not to add a Signed-off-by: tag but it
> >>>>> requires something completely different.
> >>>>>
> >>>>
> >>>> I have generated the 'git logs' of the commits causing
> >>>> MISSING_SIGN_OFF at:
> >>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task7/summary_log.txt
> >>>>
> >>>> Actually this fix was suggested by Joe at:
> >>>> https://lore.kernel.org/linux-kernel-mentees/e8fef5fbd71331b010988769b249af6a79048f48.camel@perches.com/
> >>>>
> >>>
> >>> Yes I know. Now look at the reports.
> >>>
> >>> The rc and release commit tags do not get a Signed-off-by
> >>> intentionally. Other than that, reverts (for reverts is probably best
> >>> to explain how to do a revert... so that a sign-off-by is included)
> >>> and typos.
> >>>
> >>>
> >>> Lukas
> >>>
> >>
> >> Yes, I agree. But for the typos, the user must get BAD_SIGN_OFF
> >> warning for "Non-standard signature". So user will have to fix that
> >> manually. If so, this error is already avoided.
> >>
> >
> > But then the fix should not be applied if there is a BAD_SIGN_OFF
> > warning, right?
> >
> > Typos can also be fixed automatically by a fuzzy match. That is a
> > different feature, though and a different (next) patch.
> >
> Okay.
>
> >> For the release commits, I think the user(s) may not use checkpatch
> >>
> >
> > Agree. You can filter those out of your evaluation.
> >
> >> For the rest (ie reverts) with no sign-offs, I think that if we just
> >> add the sign off line, it serves our purpose.
> >> Of course if we explain them to revert with sign-off, it will be
> >> probably the same outcome.
> >>
> >
> > Then do both. Provide a fix in checkpatch.pl and provide documentation.
> >
> Okay.
>
> > So, how many are because of reverts and how many are because of typos?
> >
>
> Reverts: 4,
> Typos: 5
> True positives: 2 (3c0edea9b29f9be6, 9ac060a708e05423)
>

So for Reverts and the true positives, your suggested fix works.

For the typos cases, your suggested fix does not work.

You can add this fix but maybe you can also work on how to fix the typo case.
You might consider that the fix is not suggested when there is still
an BAD_SIGNOFF warning (because then it is probably a typo).

I think for the typos in tags, it will emit the BAD_SIGNOFF warning.
You can add a fix option to correct the string by some fuzzy match and
then correct the Signed-off-by: tag.

> > Also, how many reverts do currently include a proper sign-off?
> >> When the majority does not include a proper sign-off on revert commit,
> > maybe the convention is to not have a sign-off on reverts.
> > When we have some data on that, we can discuss this with some central
> > maintainers in the kernel community.
> >
> > If that is the case, then the fix is to change checkpatch.pl not to
> > complain on revert commits because of a missing sign-off.
> >
> So, there are 993 commits which are reverts. Out of these only 4 are
> there which have missing sign offs.(over 4.13..5.8)
>
> I have a doubt, How can I provide documentation? Couldn't find any doc
> file for checkpatch
>

Add an additional hint can be added here:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

E.g., that also reverts should include a Signed-off-by: and that git
revert <some option> does that for you.


Lukas