From: "niklas.soderlund@corigine.com" <niklas.soderlund@corigine.com>
To: Philippe Schenker <philippe.schenker@toradex.com>
Cc: "corbet@lwn.net" <corbet@lwn.net>,
"dwaipayanray1@gmail.com" <dwaipayanray1@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"joe@perches.com" <joe@perches.com>,
"lukas.bulwahn@gmail.com" <lukas.bulwahn@gmail.com>,
"apw@canonical.com" <apw@canonical.com>,
"sfr@canb.auug.org.au" <sfr@canb.auug.org.au>,
"louis.peens@corigine.com" <louis.peens@corigine.com>,
"simon.horman@corigine.com" <simon.horman@corigine.com>,
"oss-drivers@corigine.com" <oss-drivers@corigine.com>
Subject: Re: [PATCH v4] checkpatch: warn for non-standard fixes tag style
Date: Fri, 9 Sep 2022 09:40:05 +0200 [thread overview]
Message-ID: <Yxrt1aa60xY0H7j0@oden.dyn.berto.se> (raw)
In-Reply-To: <3275d886491dc934e3277cde9cc766b0ce0886ea.camel@toradex.com>
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
next prev parent reply other threads:[~2022-09-09 7:47 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 16:44 [PATCH v4] checkpatch: warn for non-standard fixes tag style Niklas Söderlund
2022-09-08 17:49 ` Philippe Schenker
2022-09-09 7:40 ` niklas.soderlund [this message]
2022-09-09 14:39 ` Philippe Schenker
2022-09-09 17:57 ` Joe Perches
2022-09-10 8:48 ` niklas.soderlund
2022-09-11 2:47 ` Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yxrt1aa60xY0H7j0@oden.dyn.berto.se \
--to=niklas.soderlund@corigine.com \
--cc=apw@canonical.com \
--cc=corbet@lwn.net \
--cc=dwaipayanray1@gmail.com \
--cc=joe@perches.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=louis.peens@corigine.com \
--cc=lukas.bulwahn@gmail.com \
--cc=oss-drivers@corigine.com \
--cc=philippe.schenker@toradex.com \
--cc=sfr@canb.auug.org.au \
--cc=simon.horman@corigine.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).