On Fri, 2022-09-09 at 09:40 +0200, niklas.soderlund@xxxxxxxxxxxx wrote:
> 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.
In the end I'm fine with both variants. Up to you, I just would not have
made the whole check too strict as until now there was no check at all.
But the fact we are now even checking one space character it is also
fine for me that we limit this to strictly 12 characters.
>
> 1.
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
