Re: [PATCH v4] checkpatch: warn for non-standard fixes tag style

[niklas.soderlund@xxxxxxxxxxxx]

Hi Philippe,

On 2022-09-08 17:49:14 +0000, Philippe Schenker wrote:
> Hi Niklas, 
> 
> Thanks for adding me to cc. I will also add Stephen, as he also sent
> some comments on my submission the exact same problem. I'm supportive of
> your code as it has the nice advantage of suggesting the right format of
> the tag if it might be wrong. However it seems lot of stuff is slightly
> duplicated and lots of lines could be left away simplifying it greatly.
> I don't want to hold anything up anyway so I'm fine with it, but will
> stillleave some comments of things I think should be improved.

I agree the LoC could be reduced, I try to mimic the style from the 
"Check for git id commit length and improperly formed commit 
descriptions" check. As there is some overlap maybe one day someone 
cleverer then me can figure out how to share code between them.

> > +# Check Fixes: styles is correct
> > +               if (!$in_header_lines && $line =~ /^fixes:?/i) {
> 
> I would check all lines that start with fixes, even if there is
> whitespace in front (and then failing later on...)
> 
> if (!$in_header_lines && $line =~ /^\s*fixes:?/i) {

Good point, will do so in v5.

> If we check also the fixes: lines that begin with whitespace this 
> would be a good space to check that we do not want any whitespace in 
> front of Fixes: tag.
> 
> /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {

Ditto.

> > +                               $id_length = 0 if ($orig_commit =~
> > /^[0-9a-f]{12}$/i);
> 
> I suggest we borrow the patter that is also used in "Check for git id
> commit length and improperly formed commit descriptions". This has the
> reason as checking strictly for 12 chars is at the moment right but as
> linux grows 13 chars will eventually come.
> 
> $id_length = 0 if ($orig_commit =~ /^[0-9a-f]{12,40}$/i);

This one bothers me a bit. I did do that before I sent out v1 but 
changed my mind. The reason being that the documentation asks explicitly 
for 12 chars [1]. I have no preference on keeping it strictly 12 or 
allowing anything in the 12 to 40 range, but i do think the check should 
reflect whats in the documentation. If we change this maybe we also need 
to update the documentation?

One argument to keep it strict is that when Linux the need 13 or more 
characters the documentation will need to be updated and it is natural 
that the script to check that the style documented is followed is 
updated at the same time.

1.  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

-- 
Kind Regards,
Niklas Söderlund