According to submitting-patches.rst, 'Fixes:' tag has a little
stricter condition about the one line summary:
...
Do not split the tag across multiple
lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
parsing scripts
...
And there is no 'Fixes:' tag format checker in checkpatch to check
the commit id length too, so let's add dedicated checker to check
these conditions for 'Fixes:' tag.
Signed-off-by: Wang YanQing <[email protected]>
---
scripts/checkpatch.pl | 41 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
v2-v3
1:No modification to GIT_COMMIT_ID checker.
I make a mistake previously, GIT_COMMIT_ID doesn't check 'Fixes:' tag in any way,
it isn't designed to do it, so let's don't touch it.
2:Check for too long commit id too.
3:Check for title line mismatch too.
4:Move invalid commit id check for 'Fixes:' tag from UNKNOWN_COMMIT_ID to FIXES_TAG checker.
5:Reword the error message (Markus Elfring).
6:Reword the commit log (Markus Elfring).
v1-v2:
1: Reword commit log (Markus Elfring).
2: Allow more than 12 characters of SHA-1 id (Markus Elfring).
3: Update the error message according to reflect the second update.
4: Add missing (?:...).
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 23a001a..4de05b5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2969,7 +2969,7 @@ sub process {
}
# check for invalid commit id
- if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) {
+ if ($in_commit_log && $line =~ /(\bcommit)\s+([0-9a-f]{6,40})\b/i) {
my $id;
my $description;
($id, $description) = git_commit_info($2, undef, undef);
@@ -2979,6 +2979,45 @@ sub process {
}
}
+ if ($in_commit_log && $line =~ /^fixes:\s*[0-9a-f]{6,40}\b/i) {
+ my $short = 1;
+ my $long = 0;
+ my $lines = 1;
+ my $orig_commit = "";
+ my $id = '0123456789ab';
+ my $orig_desc = "commit description";
+ my $description;
+
+ $short = 0 if ($line =~ /\bfixes:\s+[0-9a-f]{12,40}/i);
+ $long = 0 if ($line =~ /\bfixes:\s+[0-9a-f]{41,}/i);
+
+ if ($line =~ /^\s*fixes:\s*[0-9a-f]{6,40}\s*(.*)/i) {
+ $lines = 0 if ($1 =~ /^\(\"(?:.*)\"\)$/i);
+ }
+
+ if ($line =~ /^\s*fixes:\s*([0-9a-f]{6,40})\s+\("([^"]+)"\)/i) {
+ $orig_commit = lc($1);
+ $orig_desc = $2
+ }
+
+ ($id, $description) = git_commit_info($orig_commit,
+ $id, $orig_desc);
+
+ if (!defined($id)) {
+ WARN("FIXES_TAG",
+ "Unknown commit id '$orig_commit', maybe rebased or not pulled?\n" . $herecurr);
+ } elsif ($orig_desc ne $description) {
+ WARN("FIXES_TAG",
+ "Provided title line doesn't match the original title line of commit '$id', maybe misspelled?\n" . $herecurr);
+ }
+
+ if ($short || $long || $lines) {
+ my $fixes_tag_fmt = "Fixes: 54a4f0239f2e (\"KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed\")";
+ ERROR("FIXES_TAG",
+ "Please use 'Fixes:' tag with commit description style '<12+ chars of sha1> (\"<title line>\")', and the title line doesn't across multiple lines - ie: '$fixes_tag_fmt'\n" . $herecurr);
+ }
+ }
+
# ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
--
1.8.5.6.2.g3d8a54e.dirty
> ...
> Do not split the tag across multiple
> lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
> parsing scripts
> ...
I suggest to reformat the quotation.
“…
Do not split the tag across multiple lines, tags are exempt from
the "wrap at 75 columns" rule in order to simplify parsing scripts.
…”
> And there is no 'Fixes:' tag format checker in checkpatch to check
> the commit id length too, so let's add dedicated checker to check
> these conditions for 'Fixes:' tag.
It seems that there are further challenges to consider for an imperative wording
in such a change description.
How do you think about the following wording variant?
The script “checkpatch.pl” did not provide a check for the commit
identification length. Thus add a dedicated check.
> v2-v3
I would find a shorter version identification (without the arrow)
also sufficient.
> 1:No modification to GIT_COMMIT_ID checker.
Would you like to add a space character for the item enumeration?
> I make a mistake previously, …
Would you like to use the word “made” here?
> + my $id = '0123456789ab';
> + my $orig_desc = "commit description";
How much do these variable initialisations matter?
> + $lines = 0 if ($1 =~ /^\(\"(?:.*)\"\)$/i);
I wonder why you see a need to use a non-capturing group in such
a regular expression (when no alternatives were specified there).
> + ERROR("FIXES_TAG",
> + "Please use 'Fixes:' tag with commit description style '<12+ chars of sha1> (\"<title line>\")', and the title line doesn't across multiple lines - ie: '$fixes_tag_fmt'\n" . $herecurr);
* Would we like to support any other quotation characters around
the commit summary?
* I propose to split the error message.
May it become multi-line?
* How do you think about another wording variant?
The title must be specified as a single line (without line breaks).
* Would you like to point questionable commit titles out?
Regards,
Markus
> And there is no 'Fixes:' tag format checker in checkpatch
I have taken another look at corresponding implementation details.
Will programming challenges get any more attention?
> to check the commit id length too,
The mentioned script contains the following information.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/checkpatch.pl?id=b240f960afb900e59112ebcfa5a759bb0a85a14e#n2818
# Check for git id commit length and improperly formed commit descriptions
> so let's add dedicated checker to check these conditions for 'Fixes:' tag.
How do you think about to reconsider the usage of the word “checker”
at specific places?
> + my $id = '0123456789ab';
> + my $orig_desc = "commit description";
* Do you try to extend the existing software analysis approach “GIT_COMMIT_ID”?
* Would you like to avoid the development of duplicate Perl code?
Regards,
Markus
On Tue, Apr 28, 2020 at 08:21:37AM +0200, Markus Elfring wrote:
> > ...
> > Do not split the tag across multiple
> > lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
> > parsing scripts
> > ...
>
> I suggest to reformat the quotation.
>
> “…
> Do not split the tag across multiple lines, tags are exempt from
> the "wrap at 75 columns" rule in order to simplify parsing scripts.
> …”
>
>
> > And there is no 'Fixes:' tag format checker in checkpatch to check
> > the commit id length too, so let's add dedicated checker to check
> > these conditions for 'Fixes:' tag.
>
> It seems that there are further challenges to consider for an imperative wording
> in such a change description.
>
> How do you think about the following wording variant?
>
> The script “checkpatch.pl” did not provide a check for the commit
> identification length. Thus add a dedicated check.
>
>
> > v2-v3
>
> I would find a shorter version identification (without the arrow)
> also sufficient.
>
>
> > 1:No modification to GIT_COMMIT_ID checker.
>
> Would you like to add a space character for the item enumeration?
>
>
> > I make a mistake previously, …
>
> Would you like to use the word “made” here?
>
>
> > + my $id = '0123456789ab';
> > + my $orig_desc = "commit description";
>
> How much do these variable initialisations matter?
>
Well! Thanks and you are right, I will fix them in next version.
>
> > + $lines = 0 if ($1 =~ /^\(\"(?:.*)\"\)$/i);
>
> I wonder why you see a need to use a non-capturing group in such
> a regular expression (when no alternatives were specified there).
I guess it is a better perl programming practice that use non-capturing group always when
you don't need to use the '$' to access it, it will make code a little faster I guess.
>
>
> > + ERROR("FIXES_TAG",
> > + "Please use 'Fixes:' tag with commit description style '<12+ chars of sha1> (\"<title line>\")', and the title line doesn't across multiple lines - ie: '$fixes_tag_fmt'\n" . $herecurr);
>
> * Would we like to support any other quotation characters around
> the commit summary?
>
I think we don't need now and people could patch it easily when
we need others in future:)
> * I propose to split the error message.
> May it become multi-line?
Yes
>
> * How do you think about another wording variant?
>
> The title must be specified as a single line (without line breaks).
>
Ok
> * Would you like to point questionable commit titles out?
>
> Regards,
> Markus
No, I think your previous wording variant is clear enough.
Thanks again!
> I guess it is a better perl programming practice that use non-capturing group always
I suggest to reconsider the information “always”.
> when you don't need to use the '$' to access it,
Such an expectation can be fine if alternatives would be specified in
the notation “(?:…)”.
> it will make code a little faster I guess.
I hope this also when such a specification is actually needed.
>> * Would we like to support any other quotation characters around
>> the commit summary?
>
> I think we don't need now
I propose to reconsider also this view.
How should titles be safely handled if they contain the mentioned
quotation characters?
Can it be that you are used to the application of a specific language
where you should choose between two quotes for specification
of attributes (or text)?
> and people could patch it easily when we need others in future:)
I guess that there are further software development challenges to consider.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/checkpatch.pl?id=b240f960afb900e59112ebcfa5a759bb0a85a14e#n2872
Regards,
Markus
On Tue, 2020-04-28 at 10:02 +0800, Wang YanQing wrote:
> According to submitting-patches.rst, 'Fixes:' tag has a little
> stricter condition about the one line summary:
> ...
> Do not split the tag across multiple
> lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
> parsing scripts
> ...
>
> And there is no 'Fixes:' tag format checker in checkpatch to check
> the commit id length too, so let's add dedicated checker to check
> these conditions for 'Fixes:' tag.
There's no need to duplicate functionality like this.
Put this additional Fixes: logic into the existing block.
(and don't take advise from Markus too seriously when it
comes to English grammar or wording. He is not an arbiter
of taste for this code)
On Tue, 2020-04-28 at 09:10 -0700, Joe Perches wrote:
> On Tue, 2020-04-28 at 10:02 +0800, Wang YanQing wrote:
> > According to submitting-patches.rst, 'Fixes:' tag has a little
> > stricter condition about the one line summary:
> > ...
> > Do not split the tag across multiple
> > lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
> > parsing scripts
> > ...
> >
> > And there is no 'Fixes:' tag format checker in checkpatch to check
> > the commit id length too, so let's add dedicated checker to check
> > these conditions for 'Fixes:' tag.
>
> There's no need to duplicate functionality like this.
> Put this additional Fixes: logic into the existing block.
>
> (and don't take advise from Markus too seriously when it
> comes to English grammar or wording. He is not an arbiter
> of taste for this code)
>
btw: I suggested this patch last year.
https://lore.kernel.org/lkml/[email protected]/
> btw: I suggested this patch last year.
>
> https://lore.kernel.org/lkml/[email protected]/
Thanks for this link to the previous discussion topic “linux-next:
Fixes tag needs some work in the tip tree”.
https://lkml.org/lkml/2019/1/17/966
With which script did you determine the presented statistic for variations
in the usage of details for such tags?
Will another update become interesting how often shorter commit identifiers
are still published for Linux patches?
Regards,
Markus
On Tue, Apr 28, 2020 at 12:52:59PM +0200, Markus Elfring wrote:
> > And there is no 'Fixes:' tag format checker in checkpatch
>
> I have taken another look at corresponding implementation details.
> Will programming challenges get any more attention?
>
>
> > to check the commit id length too,
>
> The mentioned script contains the following information.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/checkpatch.pl?id=b240f960afb900e59112ebcfa5a759bb0a85a14e#n2818
>
> # Check for git id commit length and improperly formed commit descriptions
>
>
> > so let's add dedicated checker to check these conditions for 'Fixes:' tag.
>
> How do you think about to reconsider the usage of the word “checker”
> at specific places?
Yes, I will use the word "check" only in later version.
>
>
> > + my $id = '0123456789ab';
> > + my $orig_desc = "commit description";
>
> * Do you try to extend the existing software analysis approach “GIT_COMMIT_ID”?
>
> * Would you like to avoid the development of duplicate Perl code?
Fixes: lines don't need to have a "commit" prefix before the commit id, the description
in normal commit id could across multiple lines, and we don't need to consider the
$commit_log_possible_stack_dump for 'Fixes:' tag line. I mean it will make the GIT_COMMIT_ID
code become harder to read and maintain.
>
> Regards,
> Markus
>> * Do you try to extend the existing software analysis approach “GIT_COMMIT_ID”?
>>
>> * Would you like to avoid the development of duplicate Perl code?
>
> Fixes: lines don't need to have a "commit" prefix before the commit id, the description
> in normal commit id could across multiple lines, and we don't need to consider the
> $commit_log_possible_stack_dump for 'Fixes:' tag line.
It can be helpful to know such differences.
> I mean it will make the GIT_COMMIT_ID code become harder to read and maintain.
This view depends on some factors.
* How many data processing can be shared for your software extension?
* Do you get any further development ideas from a previous suggestion
by Joe Perches according to the discussion topic “linux-next:
Fixes tag needs some work in the tip tree”?
https://lkml.org/lkml/2019/1/17/966
https://lore.kernel.org/lkml/[email protected]/
Regards,
Markus