* [PATCH v2] checkpatch.pl: warn on invalid commit id
@ 2019-07-11 0:16 Matteo Croce
2019-07-11 9:04 ` Joe Perches
2019-07-25 3:07 ` Andrew Morton
0 siblings, 2 replies; 7+ messages in thread
From: Matteo Croce @ 2019-07-11 0:16 UTC (permalink / raw)
To: Joe Perches, LKML; +Cc: Andrew Morton, Andy Whitcroft
It can happen that a commit message refers to an invalid commit id, because
the referenced hash changed following a rebase, or simply by mistake.
Add a check in checkpatch.pl which checks that an hash referenced by
a Fixes tag, or just cited in the commit message, is a valid commit id.
$ scripts/checkpatch.pl <<'EOF'
Subject: [PATCH] test commit
Sample test commit to test checkpatch.pl
Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") really exists,
commit 0bba044c4ce7 ("tree") is valid but not a commit,
while commit b4cc0b1c0cca ("unknown") is invalid.
Fixes: f0cacc14cade ("unknown")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
EOF
WARNING: Unknown commit id '0bba044c4ce7', maybe rebased or not pulled?
#8:
commit 0bba044c4ce7 ("tree") is valid but not a commit,
WARNING: Unknown commit id 'b4cc0b1c0cca', maybe rebased or not pulled?
#9:
while commit b4cc0b1c0cca ("unknown") is invalid.
WARNING: Unknown commit id 'f0cacc14cade', maybe rebased or not pulled?
#11:
Fixes: f0cacc14cade ("unknown")
total: 0 errors, 3 warnings, 4 lines checked
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
scripts/checkpatch.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a6d436809bf5..3b77279df13b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2898,6 +2898,17 @@ sub process {
}
}
+# check for invalid commit id
+ if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) {
+ my $id;
+ my $description;
+ ($id, $description) = git_commit_info($2, undef, undef);
+ if (!defined($id)) {
+ WARN("UNKNOWN_COMMIT_ID",
+ "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr);
+ }
+ }
+
# ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-11 0:16 [PATCH v2] checkpatch.pl: warn on invalid commit id Matteo Croce
@ 2019-07-11 9:04 ` Joe Perches
2019-07-25 3:07 ` Andrew Morton
1 sibling, 0 replies; 7+ messages in thread
From: Joe Perches @ 2019-07-11 9:04 UTC (permalink / raw)
To: Matteo Croce, LKML; +Cc: Andrew Morton, Andy Whitcroft
On Thu, 2019-07-11 at 02:16 +0200, Matteo Croce wrote:
> It can happen that a commit message refers to an invalid commit id, because
> the referenced hash changed following a rebase, or simply by mistake.
> Add a check in checkpatch.pl which checks that an hash referenced by
> a Fixes tag, or just cited in the commit message, is a valid commit id.
Thanks Matteo, this seems sensible.
> $ scripts/checkpatch.pl <<'EOF'
> Subject: [PATCH] test commit
>
> Sample test commit to test checkpatch.pl
> Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") really exists,
> commit 0bba044c4ce7 ("tree") is valid but not a commit,
> while commit b4cc0b1c0cca ("unknown") is invalid.
>
> Fixes: f0cacc14cade ("unknown")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> EOF
> WARNING: Unknown commit id '0bba044c4ce7', maybe rebased or not pulled?
> #8:
> commit 0bba044c4ce7 ("tree") is valid but not a commit,
>
> WARNING: Unknown commit id 'b4cc0b1c0cca', maybe rebased or not pulled?
> #9:
> while commit b4cc0b1c0cca ("unknown") is invalid.
>
> WARNING: Unknown commit id 'f0cacc14cade', maybe rebased or not pulled?
> #11:
> Fixes: f0cacc14cade ("unknown")
>
> total: 0 errors, 3 warnings, 4 lines checked
>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
> scripts/checkpatch.pl | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a6d436809bf5..3b77279df13b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2898,6 +2898,17 @@ sub process {
> }
> }
>
> +# check for invalid commit id
> + if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) {
> + my $id;
> + my $description;
> + ($id, $description) = git_commit_info($2, undef, undef);
> + if (!defined($id)) {
> + WARN("UNKNOWN_COMMIT_ID",
> + "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr);
> + }
> + }
> +
> # ignore non-hunk lines and lines being removed
> next if (!$hunk_line || $line =~ /^-/);
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-11 0:16 [PATCH v2] checkpatch.pl: warn on invalid commit id Matteo Croce
2019-07-11 9:04 ` Joe Perches
@ 2019-07-25 3:07 ` Andrew Morton
2019-07-25 9:26 ` Matteo Croce
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2019-07-25 3:07 UTC (permalink / raw)
To: Matteo Croce; +Cc: Joe Perches, LKML, Andy Whitcroft
On Thu, 11 Jul 2019 02:16:40 +0200 Matteo Croce <mcroce@redhat.com> wrote:
> It can happen that a commit message refers to an invalid commit id, because
> the referenced hash changed following a rebase, or simply by mistake.
> Add a check in checkpatch.pl which checks that an hash referenced by
> a Fixes tag, or just cited in the commit message, is a valid commit id.
>
> $ scripts/checkpatch.pl <<'EOF'
> Subject: [PATCH] test commit
>
> Sample test commit to test checkpatch.pl
> Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") really exists,
> commit 0bba044c4ce7 ("tree") is valid but not a commit,
> while commit b4cc0b1c0cca ("unknown") is invalid.
>
> Fixes: f0cacc14cade ("unknown")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> EOF
> WARNING: Unknown commit id '0bba044c4ce7', maybe rebased or not pulled?
> #8:
> commit 0bba044c4ce7 ("tree") is valid but not a commit,
>
> WARNING: Unknown commit id 'b4cc0b1c0cca', maybe rebased or not pulled?
> #9:
> while commit b4cc0b1c0cca ("unknown") is invalid.
>
> WARNING: Unknown commit id 'f0cacc14cade', maybe rebased or not pulled?
> #11:
> Fixes: f0cacc14cade ("unknown")
>
> total: 0 errors, 3 warnings, 4 lines checked
>
> ...
>
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2898,6 +2898,17 @@ sub process {
> }
> }
>
> +# check for invalid commit id
> + if ($in_commit_log && $line =~ /(^fixes:|\bcommit)\s+([0-9a-f]{6,40})\b/i) {
> + my $id;
> + my $description;
> + ($id, $description) = git_commit_info($2, undef, undef);
> + if (!defined($id)) {
> + WARN("UNKNOWN_COMMIT_ID",
> + "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr);
> + }
> + }
> +
What does it do if we're not operating in a git directory? For example,
I work in /usr/src/25 and my git repo is in ../git26.
Also, what happens relatively often is that someone quotes a linux-next
or long-term-stable hash. If the user has those trees in the git repo,
I assume they won't be informed of the inappropriate hash?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-25 3:07 ` Andrew Morton
@ 2019-07-25 9:26 ` Matteo Croce
2019-07-26 0:22 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Matteo Croce @ 2019-07-25 9:26 UTC (permalink / raw)
To: Andrew Morton; +Cc: Joe Perches, LKML, Andy Whitcroft
On Thu, Jul 25, 2019 at 5:07 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> What does it do if we're not operating in a git directory? For example,
> I work in /usr/src/25 and my git repo is in ../git26.
>
If .git is not found, the check is disabled
> Also, what happens relatively often is that someone quotes a linux-next
> or long-term-stable hash. If the user has those trees in the git repo,
> I assume they won't be informed of the inappropriate hash?
>
In this case it won't warn, but this should not be a problem, as the
hash doesn't change following a merge.
The problem is just if the other tree gets rebased, or if the other
tree gets never merged, e.g. stable/linux-*
Cheers,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-25 9:26 ` Matteo Croce
@ 2019-07-26 0:22 ` Andrew Morton
2019-07-26 1:17 ` Matteo Croce
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2019-07-26 0:22 UTC (permalink / raw)
To: Matteo Croce; +Cc: Joe Perches, LKML, Andy Whitcroft
On Thu, 25 Jul 2019 11:26:04 +0200 Matteo Croce <mcroce@redhat.com> wrote:
> On Thu, Jul 25, 2019 at 5:07 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > What does it do if we're not operating in a git directory? For example,
> > I work in /usr/src/25 and my git repo is in ../git26.
> >
>
> If .git is not found, the check is disabled
We could permit user to set an environment variable to tell checkpatch
where the kernel git tree resides.
> > Also, what happens relatively often is that someone quotes a linux-next
> > or long-term-stable hash. If the user has those trees in the git repo,
> > I assume they won't be informed of the inappropriate hash?
> >
>
> In this case it won't warn, but this should not be a problem, as the
> hash doesn't change following a merge.
> The problem is just if the other tree gets rebased, or if the other
> tree gets never merged, e.g. stable/linux-*
linux-next patches get rebased quite often. I guess this is acceptable
- failing to warn about an error is better than warning about
not-an-error.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-26 0:22 ` Andrew Morton
@ 2019-07-26 1:17 ` Matteo Croce
2019-07-26 1:25 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Matteo Croce @ 2019-07-26 1:17 UTC (permalink / raw)
To: Andrew Morton; +Cc: Joe Perches, LKML, Andy Whitcroft
On July 26, 2019 2:22:05 AM GMT+02:00, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 25 Jul 2019 11:26:04 +0200 Matteo Croce <mcroce@redhat.com>
> wrote:
>
> > On Thu, Jul 25, 2019 at 5:07 AM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > > What does it do if we're not operating in a git directory? For
> example,
> > > I work in /usr/src/25 and my git repo is in ../git26.
> > >
> >
> > If .git is not found, the check is disabled
>
> We could permit user to set an environment variable to tell checkpatch
> where the kernel git tree resides.
>
Maybe GIT_DIR already does it.
> > > Also, what happens relatively often is that someone quotes a
> linux-next
> > > or long-term-stable hash. If the user has those trees in the git
> repo,
> > > I assume they won't be informed of the inappropriate hash?
> > >
> >
> > In this case it won't warn, but this should not be a problem, as the
> > hash doesn't change following a merge.
> > The problem is just if the other tree gets rebased, or if the other
> > tree gets never merged, e.g. stable/linux-*
>
> linux-next patches get rebased quite often.
I see :)
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] checkpatch.pl: warn on invalid commit id
2019-07-26 1:17 ` Matteo Croce
@ 2019-07-26 1:25 ` Andrew Morton
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2019-07-26 1:25 UTC (permalink / raw)
To: Matteo Croce; +Cc: Joe Perches, LKML, Andy Whitcroft
On Fri, 26 Jul 2019 03:17:32 +0200 Matteo Croce <mcroce@redhat.com> wrote:
> > > If .git is not found, the check is disabled
> >
> > We could permit user to set an environment variable to tell checkpatch
> > where the kernel git tree resides.
> >
>
> Maybe GIT_DIR already does it.
Yes, that works. GIT_DIR=<wherever>/.git
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-26 1:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 0:16 [PATCH v2] checkpatch.pl: warn on invalid commit id Matteo Croce
2019-07-11 9:04 ` Joe Perches
2019-07-25 3:07 ` Andrew Morton
2019-07-25 9:26 ` Matteo Croce
2019-07-26 0:22 ` Andrew Morton
2019-07-26 1:17 ` Matteo Croce
2019-07-26 1:25 ` Andrew Morton
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).