Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
> Apparently MAINTAINERS is hard to read.  Please review?
> 
> Thanks.
> 
> Begin forwarded message:
> 
> Date: Sun,  4 Dec 2022 12:33:37 +0100
> From: Kai Wasserbäch <kai@dev.carbon-project.org>
> To: linux-kernel@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>, Thorsten Leemhuis <linux@leemhuis.info>
> Subject: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:
> 
> 
> Hey,
> Thorsten Leemhuis suggested the following two changes to checkpatch, which
> I hereby humbly submit for review. Please let me know if any changes
> would be required for acceptance.
> 
> NOTES:
> - checkpatch is rather long, so I might have chosen the wrong section to
>   add these two checks. Any better placement suggestion is welcome.
> - checkpatch implements custum versions of Perl core modules, that might
>   be an future area for clean-ups? Eg. right off the bat there is a
>   `uniq` implementation. List::Util (core module since 5.8.0 (5.7.3 to
>   be pedantic)) has a far better performing version in XS.

Maybe.  I don't know there are that many generic functions
that could be used.  You are welcome find some.

> 
> Cheers,
> Kai
> 
> Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
> Signed-off-by: Kai Wasserbäch <kai@dev.carbon-project.org>
> 
> 
> Kai Wasserbäch (2):
>   feat: checkpatch: error on usage of a Buglink tag in the commit log

Why, what's wrong with a buglink reference?

>   feat: checkpatch: Warn about Reported-by: not being followed by a
>     Link:

I think this is unnecessary.

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Mon, 2022-12-05 at 17:02 -0800, Joe Perches wrote:
> On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
> > Date: Sun,  4 Dec 2022 12:33:37 +0100
> > From: Kai Wasserbäch <kai@dev.carbon-project.org>
[]
> > - checkpatch implements custum versions of Perl core modules, that might
> >   be an future area for clean-ups? Eg. right off the bat there is a
> >   `uniq` implementation. List::Util (core module since 5.8.0 (5.7.3 to
> >   be pedantic)) has a far better performing version in XS.

> Maybe.  I don't know there are that many generic functions
> that could be used.  You are welcome find some.

uniq isn't used in checkpatch since

commit 52178ce01335d9d76611c3a5198b8778cb9b03f5
Author: Dwaipayan Ray <dwaipayanray1@gmail.com>
Date:   Fri Feb 26 15:08:26 2021 +0530

    checkpatch: add verbose mode
 
and could be removed.

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

Hi Joe! Many thx for looking into this.

On 06.12.22 02:02, Joe Perches wrote:
> On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
>> Begin forwarded message:
>>
>> Date: Sun,  4 Dec 2022 12:33:37 +0100
>> From: Kai Wasserbäch <kai@dev.carbon-project.org>
>> To: linux-kernel@vger.kernel.org
>> Cc: Andrew Morton <akpm@linux-foundation.org>, Thorsten Leemhuis <linux@leemhuis.info>
>> Subject: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:
>>
>> Thorsten Leemhuis suggested the following two changes to checkpatch, which
>> I hereby humbly submit for review. Please let me know if any changes
>> would be required for acceptance.
> [...]
>> Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
>> Signed-off-by: Kai Wasserbäch <kai@dev.carbon-project.org>
>>
>> Kai Wasserbäch (2):
>>   feat: checkpatch: error on usage of a Buglink tag in the commit log
> 
> Why, what's wrong with a buglink reference?

Long story short: Linus doesn't like it:

```
>> BugLink: https://lore.kernel.org/r/20220610205038.GA3050413@paulmck-ThinkPad-P17-Gen-1
>> BugLink: https://lore.kernel.org/r/CAMdYzYpF4FNTBPZsEFeWRuEwSies36QM_As8osPWZSr2q-viEA@mail.gmail.com
> [...]
> please stop making up random tags that make no sense.
> 
> Just use "Link:"
> [...]
> 
> It's extra context to the commit, in case somebody wants to know the
> history. The "bug" part is (and always should be) already explained in
> the commit message, there's absolutely no point in adding soem extra
> noise to the "Link:" tag.>
```
That's a quote from
https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/

In that message he also links to another mail from him, where he wrote:
```
> I think "BugLink:" is silly, because that's just a regular link.
> What's the upside of saying "Bug" in there? If you're fixing a bug,
> and are linking to reports and discussions by people, what does that
> "bug" buy you apart from another ugly CamelCase thing?
> 
> In other words, in "BugLink:", neither "Bug" nor "Link" is actually meaningful.
> 
> The current "Link:" tag exists AS A GENERIC WAY TO SAY "look, here's
> more information". That's the point. The word "Link" has no other
> meaning, and trying to then combine it with other things is worthless.
```
https://lore.kernel.org/all/CAHk-=wgzRUT1fBpuz3xcN+YdsX0SxqOzHWRtj0ReHpUBb5TKbA@mail.gmail.com/

Our docs say to use Link in this case, too; see
Documentation/process/submitting-patches.rst
(http://docs.kernel.org/process/submitting-patches.html) and
Documentation/process/5.Posting.rst
(https://docs.kernel.org/process/5.Posting.html)

And using various tags for the same thing makes it also harder for
external scripts that look at commits to take further action -- like the
regression tracking bot I write and use for my work.

All of that obviously should have been in patch description. Let me
resubmit that patch with all of that in there, or are you dead against
this idea now?

>>   feat: checkpatch: Warn about Reported-by: not being followed by a
>>     Link:
> 
> I think this is unnecessary.

I expect this to cause more discussion, which is why this should be in a
separate submission. But in the end the reasons are similar as above:
(1) Linus really want to see those links to make things easier for
future code archeologists. (2) My regression tracking efforts heavily
rely on those Links, as they allow to automatically connect reports with
patches and commits to fix the reported regression; without those the
tracking is a PITA and doesn't scale.

Together that IMHO is strong enough reason to warn in this case.

Two quotes from Linus to show that he really wants those links:

```
> The current "Link:" tag exists AS A GENERIC WAY TO SAY "look, here's
> more information". That's the point. The word "Link" has no other
> meaning, and trying to then combine it with other things is worthless.
> 
> And a bug report is exactly that kind of "look, here's more
> information".
>
> [...]>
> The commit message should have enough of an explanation on its own
> ("Reported-by:" and the regular "Link:" to the report) that there's
> *no* excuse to try to make a bug report link special.
```
https://lore.kernel.org/all/CAHk-=wgzRUT1fBpuz3xcN+YdsX0SxqOzHWRtj0ReHpUBb5TKbA@mail.gmail.com/

```
>> The flag was dropped because it was causing drivers that requested their
>> memory resource with pci_request_region() to fail with -EBUSY (e.g: the
>> vmwgfx driver):
>>
>> https://www.spinics.net/lists/dri-devel/msg329672.html
> 
> See, *that* link would have been useful in the commit.
> 
> Again, the commit has a link to the patch *submission*, which is
> almost entirely useless. There's no link to the actual problem the
> patch fixes.
>> [...]>
> I _really_ wish the -tip tree had more links to the actual problem
> reports and explanations, rather than links to the patch submission.
>
> [...]>
> Put another way: I can see that
> 
>     Reported-by: Zhangfei Gao <zhangfei.gao@foxmail.com>
> 
> in the commit, but I don't have a clue what the actual report was, and
> there really isn't enough information in the commit itself, except for
> a fairly handwavy "Device drivers might, for instance, still need to
> flush operations.."
```
https://lore.kernel.org/amd-gfx/CAHk-=wjxzafG-=J8oT30s7upn4RhBs6TX-uVFZ5rME+L5_DoJA@mail.gmail.com/

This also obviously would need to be in the patch description in some
form. I can take care of that.

Ciao, Thorsten

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Tue, 2022-12-06 at 05:55 +0100, Thorsten Leemhuis wrote:
> Hi Joe! Many thx for looking into this.
> 
> On 06.12.22 02:02, Joe Perches wrote:
> > On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
> > > Begin forwarded message:
> > > 
> > > Date: Sun,  4 Dec 2022 12:33:37 +0100
> > > From: Kai Wasserbäch <kai@dev.carbon-project.org>
> > > To: linux-kernel@vger.kernel.org
> > > Cc: Andrew Morton <akpm@linux-foundation.org>, Thorsten Leemhuis <linux@leemhuis.info>
> > > Subject: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:
> > > 
> > > Thorsten Leemhuis suggested the following two changes to checkpatch, which
> > > I hereby humbly submit for review. Please let me know if any changes
> > > would be required for acceptance.
> > [...]
> > > Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
> > > Signed-off-by: Kai Wasserbäch <kai@dev.carbon-project.org>
> > > 
> > > Kai Wasserbäch (2):
> > >   feat: checkpatch: error on usage of a Buglink tag in the commit log
> > 
> > Why, what's wrong with a buglink reference?
> 
> Long story short: Linus doesn't like it:
> 
> ```
> > > BugLink: https://lore.kernel.org/r/20220610205038.GA3050413@paulmck-ThinkPad-P17-Gen-1
> > > BugLink: https://lore.kernel.org/r/CAMdYzYpF4FNTBPZsEFeWRuEwSies36QM_As8osPWZSr2q-viEA@mail.gmail.com
> > [...]
> > please stop making up random tags that make no sense.
> > 
> > Just use "Link:"
> > [...]
> > 
> > It's extra context to the commit, in case somebody wants to know the
> > history. The "bug" part is (and always should be) already explained in
> > the commit message, there's absolutely no point in adding soem extra
> > noise to the "Link:" tag.>
> ```
> That's a quote from
> https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/
> 
> In that message he also links to another mail from him, where he wrote:
> ```
> > I think "BugLink:" is silly, because that's just a regular link.
> > What's the upside of saying "Bug" in there? If you're fixing a bug,
> > and are linking to reports and discussions by people, what does that
> > "bug" buy you apart from another ugly CamelCase thing?
> > 
> > In other words, in "BugLink:", neither "Bug" nor "Link" is actually meaningful.
> > 
> > The current "Link:" tag exists AS A GENERIC WAY TO SAY "look, here's
> > more information". That's the point. The word "Link" has no other
> > meaning, and trying to then combine it with other things is worthless.
> ```
> https://lore.kernel.org/all/CAHk-=wgzRUT1fBpuz3xcN+YdsX0SxqOzHWRtj0ReHpUBb5TKbA@mail.gmail.com/
> 
> Our docs say to use Link in this case, too; see
> Documentation/process/submitting-patches.rst
> (http://docs.kernel.org/process/submitting-patches.html) and
> Documentation/process/5.Posting.rst
> (https://docs.kernel.org/process/5.Posting.html)
> 
> And using various tags for the same thing makes it also harder for
> external scripts that look at commits to take further action -- like the
> regression tracking bot I write and use for my work.
> 
> All of that obviously should have been in patch description. Let me
> resubmit that patch with all of that in there, or are you dead against
> this idea now?

No, better commit description would be useful and perhaps a more
generic, "is the thing in front of a URI/URL" a known/supported entry,
instead of using an known invalid test would be a better mechanism.

> > >   feat: checkpatch: Warn about Reported-by: not being followed by a
> > >     Link:
> > 
> > I think this is unnecessary.
> 
> I expect this to cause more discussion, which is why this should be in a
> separate submission. But in the end the reasons are similar as above:
> (1) Linus really want to see those links to make things easier for
> future code archeologists. (2) My regression tracking efforts heavily
> rely on those Links, as they allow to automatically connect reports with
> patches and commits to fix the reported regression; without those the
> tracking is a PITA and doesn't scale.
> 
> Together that IMHO is strong enough reason to warn in this case.

Maybe, I think there might be some value in not intermixing signature
tags and other tags though.

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

On 06.12.22 06:54, Joe Perches wrote:
> On Tue, 2022-12-06 at 05:55 +0100, Thorsten Leemhuis wrote:
>> On 06.12.22 02:02, Joe Perches wrote:
>>> On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
>>>> Begin forwarded message:
>>>>
>>>> Date: Sun,  4 Dec 2022 12:33:37 +0100
>>>> From: Kai Wasserbäch <kai@dev.carbon-project.org>
>>>> To: linux-kernel@vger.kernel.org
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>, Thorsten Leemhuis <linux@leemhuis.info>
>>>> Subject: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:
>>>>
>>>> Thorsten Leemhuis suggested the following two changes to checkpatch, which
>>>> I hereby humbly submit for review. Please let me know if any changes
>>>> would be required for acceptance.
>>> [...]
>>>> Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
>>>> Signed-off-by: Kai Wasserbäch <kai@dev.carbon-project.org>
>>>>
>>>> Kai Wasserbäch (2):
>>>>   feat: checkpatch: error on usage of a Buglink tag in the commit log
>>>
>>> Why, what's wrong with a buglink reference?
>>
>> Long story short: Linus doesn't like it:
>
> [...]
>
>> All of that obviously should have been in patch description. Let me
>> resubmit that patch with all of that in there, or are you dead against
>> this idea now?
> 
> No, better commit description would be useful

I'll prepare something.

> and perhaps a more
> generic, "is the thing in front of a URI/URL" a known/supported entry,
> instead of using an known invalid test would be a better mechanism.

Are you sure about that? It's not that I disagree completely, but it
sounds overly restrictive to me and makes it harder for new tags to
evolve in case we might want them.

And what tags would be on this allow-list? Anything else then "Link" and
"Patchwork"? Those are the ones that looked common and valid to me when
I ran

git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less

and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
sure.

But I found a few others that likely should be on the disallow list:
"Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
"RHBZ:", and "link", as "Link" should be used instead in all of these
cases afaics.

>>>>   feat: checkpatch: Warn about Reported-by: not being followed by a
>>>>     Link:
>>>
>>> I think this is unnecessary.
>>
>> I expect this to cause more discussion, which is why this should be in a
>> separate submission. But in the end the reasons are similar as above:
>> (1) Linus really want to see those links to make things easier for
>> future code archeologists. (2) My regression tracking efforts heavily
>> rely on those Links, as they allow to automatically connect reports with
>> patches and commits to fix the reported regression; without those the
>> tracking is a PITA and doesn't scale.
>>
>> Together that IMHO is strong enough reason to warn in this case.
> 
> Maybe, I think there might be some value in not intermixing signature
> tags and other tags though.

Hmm, sorry, not sure if I understood you here. Why do you consider
"Reported-by:" a signature tag? Isn't it more of an "appreciation" tag,
IOW, a kind of 'thank you' hat tip to the reporter?

Ciao, Thorsten

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

On 06.12.22 07:27, Thorsten Leemhuis wrote:
> On 06.12.22 06:54, Joe Perches wrote:
>> On Tue, 2022-12-06 at 05:55 +0100, Thorsten Leemhuis wrote:
>>> On 06.12.22 02:02, Joe Perches wrote:
>>>> On Mon, 2022-12-05 at 13:14 -0800, Andrew Morton wrote:
>>>>> Begin forwarded message:
>>>>>
>>>>> Date: Sun,  4 Dec 2022 12:33:37 +0100
>>>>> From: Kai Wasserbäch <kai@dev.carbon-project.org>
>>>>> To: linux-kernel@vger.kernel.org
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>, Thorsten Leemhuis <linux@leemhuis.info>
>>>>> Subject: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:
>>>>>
>>>>> Thorsten Leemhuis suggested the following two changes to checkpatch, which
>>>>> I hereby humbly submit for review. Please let me know if any changes
>>>>> would be required for acceptance.
>>>> [...]
>>>>> Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
>>>>> Signed-off-by: Kai Wasserbäch <kai@dev.carbon-project.org>
>>>>>
>>>>> Kai Wasserbäch (2):
>>>>>   feat: checkpatch: error on usage of a Buglink tag in the commit log
>>>>
>>>> Why, what's wrong with a buglink reference?
>>>
>>> Long story short: Linus doesn't like it:
>>
>> [...]
>>
>>> All of that obviously should have been in patch description. Let me
>>> resubmit that patch with all of that in there, or are you dead against
>>> this idea now?
>>
>> No, better commit description would be useful
> 
> I'll prepare something.
> 
>> and perhaps a more
>> generic, "is the thing in front of a URI/URL" a known/supported entry,
>> instead of using an known invalid test would be a better mechanism.
> 
> Are you sure about that? It's not that I disagree completely, but it
> sounds overly restrictive to me and makes it harder for new tags to
> evolve in case we might want them.
> 
> And what tags would be on this allow-list? Anything else then "Link" and
> "Patchwork"? Those are the ones that looked common and valid to me when
> I ran
> 
> git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less
> 
> and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
> sure.
> 
> But I found a few others that likely should be on the disallow list:
> "Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
> "RHBZ:", and "link", as "Link" should be used instead in all of these
> cases afaics.
> 
>>>>>   feat: checkpatch: Warn about Reported-by: not being followed by a
>>>>>     Link:
>>>>
>>>> I think this is unnecessary.
>>>
>>> I expect this to cause more discussion, which is why this should be in a
>>> separate submission. But in the end the reasons are similar as above:
>>> (1) Linus really want to see those links to make things easier for
>>> future code archeologists. (2) My regression tracking efforts heavily
>>> rely on those Links, as they allow to automatically connect reports with
>>> patches and commits to fix the reported regression; without those the
>>> tracking is a PITA and doesn't scale.
>>>
>>> Together that IMHO is strong enough reason to warn in this case.
>>
>> Maybe, I think there might be some value in not intermixing signature
>> tags and other tags though.
> 
> Hmm, sorry, not sure if I understood you here. Why do you consider
> "Reported-by:" a signature tag? Isn't it more of an "appreciation" tag,
> IOW, a kind of 'thank you' hat tip to the reporter?

Sorry, now I understand[1]: you consider "Reported-by:" a signature tag
as it's used in the area where the Signed-off-by resist.

Well, yeah, maybe it shouldn't be like that[2]. But it's quite normal
afaics to use Reported-by: (with and without Link:) in that area
currently. Changing that would break habits of many kernel developers.
And having the Reported-by: and the Link: next to each other IMHO is
important, as it makes it obvious what the Link is about; that's afaics
also why Linus wants it that way, as can be seen from the quotes I
provided earlier. See also commit 0ba09b173387 from a few days ago[3],
where he afaics added the Link tags to reports of that regression himself.

Ciao, Thorsten

[1] Kai (many thx!) explained and pointed me to the area in the
checkpatch.pl code that made it obvious:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl#n611

[2] in an ideal world things like that and tags like Tested-by: would
resist in git notes or something, as then they could be extended after a
change was committed...

[3] https://git.kernel.org/torvalds/c/0ba09b173387

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Tue, 2022-12-06 at 08:17 +0100, Thorsten Leemhuis wrote:
> On 06.12.22 07:27, Thorsten Leemhuis wrote:
> > On 06.12.22 06:54, Joe Perches wrote:
[]
> > > and perhaps a more
> > > generic, "is the thing in front of a URI/URL" a known/supported entry,
> > > instead of using an known invalid test would be a better mechanism.
> > 
> > Are you sure about that? It's not that I disagree completely, but it
> > sounds overly restrictive to me and makes it harder for new tags to
> > evolve in case we might want them.

It's easy to add newly supported values to a list.

> > And what tags would be on this allow-list? Anything else then "Link" and
> > "Patchwork"? Those are the ones that looked common and valid to me when
> > I ran
> > 
> > git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less
> > 
> > and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
> > sure.
[]
> > But I found a few others that likely should be on the disallow list:
> > "Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
> > "RHBZ:", and "link", as "Link" should be used instead in all of these
> > cases afaics.

Do understand please that checkpatch will never be perfect.
At best, it's just a guidance tool.

To me most of these are in the noise level, but perhaps all should just
use Link:

$ git log -100000 --format=email -P --grep='^\w+:[ \t]*http' | \
  grep -Poh '^\w+:[ \t]*http' | \
  sort | uniq -c | sort -rn
 103889 Link: http
    415 BugLink: http
    372 Patchwork: http
    270 Closes: http
    221 Bug: http
    121 References: http
    101 v1: http
     77 Bugzilla: http
     60 URL: http
     59 v2: http
     37 Datasheet: http
     35 v3: http
     19 v4: http
     12 v5: http
      9 Ref: http
      9 Fixes: http
      9 Buglink: http
      8 v6: http
      8 Reference: http
      7 V1: http
      7 See: http
      6 1: http
      5 link: http
      4 v7: http
      4 v0: http
      4 0: http
      3 LINK: http
      3 Link:http
      3 IGT: http
      2 V3: http
      2 Schematics: http
      2 RHBZ: http
      2 RFC: http
      2 Reported: http
      2 MR: http
      2 Bugs: http
      2 BUG: http
      2 2: http
      1 Website: http
      1 V9: http
      1 v9: http
      1 V8: http
      1 v8: http
      1 V7: http
      1 V6: http
      1 V5: http
      1 V4: http
      1 V2: http
      1 v1:http
      1 v10: http
      1 Twitter: http
      1 tree: http
      1 tool: http
      1 tests: http
      1 tasks: http
      1 SPI: http
      1 Source: http
      1 SoM: http
      1 Reproducer: http
      1 reliable: http
      1 Related: http
      1 Reference:http
      1 Mesa: http
      1 Lore: http
      1 Links:http
      1 Links: http
      1 Link:  http
      1 ink: http
      1 in: http
      1 here: http
      1 bz: http
      1 Bug:http
      1 AlsaInfo: http

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

On 06.12.22 08:44, Joe Perches wrote:
> On Tue, 2022-12-06 at 08:17 +0100, Thorsten Leemhuis wrote:
>> On 06.12.22 07:27, Thorsten Leemhuis wrote:
>>> On 06.12.22 06:54, Joe Perches wrote:
> []
>>>> and perhaps a more
>>>> generic, "is the thing in front of a URI/URL" a known/supported entry,
>>>> instead of using an known invalid test would be a better mechanism.
>>>
>>> Are you sure about that? It's not that I disagree completely, but it
>>> sounds overly restrictive to me and makes it harder for new tags to
>>> evolve in case we might want them.
> 
> It's easy to add newly supported values to a list.
> 
>>> And what tags would be on this allow-list? Anything else then "Link" and
>>> "Patchwork"? Those are the ones that looked common and valid to me when
>>> I ran
>>>
>>> git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less
>>>
>>> and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
>>> sure.
> []
>>> But I found a few others that likely should be on the disallow list:
>>> "Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
>>> "RHBZ:", and "link", as "Link" should be used instead in all of these
>>> cases afaics.
> 
> Do understand please that checkpatch will never be perfect.
> At best, it's just a guidance tool.

Of course -- and that's actually a reason why I prefer a disallow list
over an allow list, as that gives guidance in the way of "don't use this
tag, use Link instead" instead of enforcing "always use Link: when
linking somewhere" (now that I've written it like that it feels even
more odd, because it's obvious that it's a link, so why bother with a
tag; but whatever).

I also think the approach with a disallow list will not bother
developers much, while the other forces them a bit to much into a scheme.

> To me most of these are in the noise level, but perhaps all should just
> use Link:
> 
> $ git log -100000 --format=email -P --grep='^\w+:[ \t]*http' | \
>   grep -Poh '^\w+:[ \t]*http' | \
>   sort | uniq -c | sort -rn
>  103889 Link: http
>     415 BugLink: http
>     372 Patchwork: http
>     270 Closes: http
>     221 Bug: http
>     121 References: http
> [...]

Ha, I considered doing something like that when I wrote my earlier mail,
but was to lazy. :-D thx!

Yeah, they are not that often, but I grew tired arguing about that,
that's why I think checkpatch is the better place and in the better
position to handle that.

Anyway, so how to move forward now? Do you insist on a allow list (IOW:
a Link: or Patchwork: before every http...)? Or is a disallow list with
the most common unwanted tags for links (that you thankfully compiled)
fine for you as well?

Ciao, Thorsten

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Tue, 2022-12-06 at 09:50 +0100, Thorsten Leemhuis wrote:
> On 06.12.22 08:44, Joe Perches wrote:
> > On Tue, 2022-12-06 at 08:17 +0100, Thorsten Leemhuis wrote:
> > > On 06.12.22 07:27, Thorsten Leemhuis wrote:
> > > > On 06.12.22 06:54, Joe Perches wrote:
> > []
> > > > > and perhaps a more
> > > > > generic, "is the thing in front of a URI/URL" a known/supported entry,
> > > > > instead of using an known invalid test would be a better mechanism.
> > > > 
> > > > Are you sure about that? It's not that I disagree completely, but it
> > > > sounds overly restrictive to me and makes it harder for new tags to
> > > > evolve in case we might want them.
> > 
> > It's easy to add newly supported values to a list.
> > 
> > > > And what tags would be on this allow-list? Anything else then "Link" and
> > > > "Patchwork"? Those are the ones that looked common and valid to me when
> > > > I ran
> > > > 
> > > > git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less
> > > > 
> > > > and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
> > > > sure.
> > []
> > > > But I found a few others that likely should be on the disallow list:
> > > > "Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
> > > > "RHBZ:", and "link", as "Link" should be used instead in all of these
> > > > cases afaics.
> > 
> > Do understand please that checkpatch will never be perfect.
> > At best, it's just a guidance tool.
> 
> Of course -- and that's actually a reason why I prefer a disallow list
> over an allow list, as that gives guidance in the way of "don't use this
> tag, use Link instead" instead of enforcing "always use Link: when
> linking somewhere" (now that I've written it like that it feels even
> more odd, because it's obvious that it's a link, so why bother with a
> tag; but whatever).
> 
> I also think the approach with a disallow list will not bother
> developers much, while the other forces them a bit to much into a scheme.
> 
> > To me most of these are in the noise level, but perhaps all should just
> > use Link:
> > 
> > $ git log -100000 --format=email -P --grep='^\w+:[ \t]*http' | \
> >   grep -Poh '^\w+:[ \t]*http' | \
> >   sort | uniq -c | sort -rn
> >  103889 Link: http
> >     415 BugLink: http
> >     372 Patchwork: http
> >     270 Closes: http
> >     221 Bug: http
> >     121 References: http
> > [...]
> 
> Ha, I considered doing something like that when I wrote my earlier mail,
> but was to lazy. :-D thx!
> 
> Yeah, they are not that often, but I grew tired arguing about that,
> that's why I think checkpatch is the better place and in the better
> position to handle that.

I'm not sure that "Patchwork:" is a reasonable prefix.
Is that documented anywhere?

> Anyway, so how to move forward now? Do you insist on a allow list (IOW:
> a Link: or Patchwork: before every http...)? Or is a disallow list with
> the most common unwanted tags for links (that you thankfully compiled)
> fine for you as well?

Maybe
---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1c3d13e65c2d0..a526a354cdfbc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3250,6 +3250,13 @@ sub process {
 			$commit_log_possible_stack_dump = 0;
 		}
 
+# Check for odd prefixes before a URI/URL
+		if ($in_commit_log &&
+		    $line =~ /^\s*(\w+):\s*http/ && $1 !~ /^(?:Link|Patchwork)/) {
+			WARN("PREFER_LINK",
+			     "Unusual link reference '$1:', prefer 'Link:'\n" . $herecurr);
+		}
+
 # Check for lines starting with a #
 		if ($in_commit_log && $line =~ /^#/) {
 			if (WARN("COMMIT_COMMENT_SYMBOL",

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

On 06.12.22 10:21, Joe Perches wrote:
> On Tue, 2022-12-06 at 09:50 +0100, Thorsten Leemhuis wrote:
>> On 06.12.22 08:44, Joe Perches wrote:
>>> On Tue, 2022-12-06 at 08:17 +0100, Thorsten Leemhuis wrote:
>>>> On 06.12.22 07:27, Thorsten Leemhuis wrote:
>>>>> On 06.12.22 06:54, Joe Perches wrote:
> [...]
>> Ha, I considered doing something like that when I wrote my earlier mail,
>> but was to lazy. :-D thx!
>>
>> Yeah, they are not that often, but I grew tired arguing about that,
>> that's why I think checkpatch is the better place and in the better
>> position to handle that.
> 
> I'm not sure that "Patchwork:" is a reasonable prefix.

/me neither

> Is that documented anywhere?

Couldn't find anything.

>> Anyway, so how to move forward now? Do you insist on a allow list (IOW:
>> a Link: or Patchwork: before every http...)? Or is a disallow list with
>> the most common unwanted tags for links (that you thankfully compiled)
>> fine for you as well?
> 
> Maybe
> ---
>  scripts/checkpatch.pl | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1c3d13e65c2d0..a526a354cdfbc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3250,6 +3250,13 @@ sub process {
>  			$commit_log_possible_stack_dump = 0;
>  		}
>  
> +# Check for odd prefixes before a URI/URL
> +		if ($in_commit_log &&
> +		    $line =~ /^\s*(\w+):\s*http/ && $1 !~ /^(?:Link|Patchwork)/) {
> +			WARN("PREFER_LINK",
> +			     "Unusual link reference '$1:', prefer 'Link:'\n" . $herecurr);
> +		}
> +

LGTM: I did some tests and it seem to do the right thing. Can we have
your Signed-off-by: for that snippet?

Ciao, Thorsten

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Tue, 2022-12-06 at 11:06 +0100, Thorsten Leemhuis wrote:
> On 06.12.22 10:21, Joe Perches wrote:

> > I'm not sure that "Patchwork:" is a reasonable prefix.
> 
> /me neither
> 
> > Is that documented anywhere?
> 
> Couldn't find anything.

I knew that.

btw:

there are a _lot_ more uses of Link: with patchwork content than
Patchwork: prefix uses, so maybe just "Link:" should be accepted.

$ git log --format=email -i --grep "patchwork" | grep -i "patchwork" | \
  cut -f1-3 -d'/' | sort | uniq -c | sort -rn | head -10
  25789 Link: https://patchwork.freedesktop.org
   7160 Link: http://patchwork.freedesktop.org
   4109 Patchwork: https://patchwork.linux-mips.org
    777 Patchwork: http://patchwork.linux-mips.org
    372 Patchwork: https://patchwork.freedesktop.org
    200 https://patchwork.kernel.org
    154 Link: https://patchwork.kernel.org
    116 [1] https://patchwork.kernel.org
     76 https://patchwork.ozlabs.org
     33 http://patchwork.ozlabs.org

> LGTM: I did some tests and it seem to do the right thing. Can we have
> your Signed-off-by: for that snippet?

It's your patch, I'm just suggesting...

cheers, Joe

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Thorsten Leemhuis]

On 06.12.22 10:21, Joe Perches wrote:
> On Tue, 2022-12-06 at 09:50 +0100, Thorsten Leemhuis wrote:
>> On 06.12.22 08:44, Joe Perches wrote:
>>> On Tue, 2022-12-06 at 08:17 +0100, Thorsten Leemhuis wrote:
>>>> On 06.12.22 07:27, Thorsten Leemhuis wrote:
>>>>> On 06.12.22 06:54, Joe Perches wrote:
>>> []
>>>>>> and perhaps a more
>>>>>> generic, "is the thing in front of a URI/URL" a known/supported entry,
>>>>>> instead of using an known invalid test would be a better mechanism.
>>>>>
>>>>> Are you sure about that? It's not that I disagree completely, but it
>>>>> sounds overly restrictive to me and makes it harder for new tags to
>>>>> evolve in case we might want them.
>>>
>>> It's easy to add newly supported values to a list.
>>>
>>>>> And what tags would be on this allow-list? Anything else then "Link" and
>>>>> "Patchwork"? Those are the ones that looked common and valid to me when
>>>>> I ran
>>>>>
>>>>> git log --grep='http' v4.0.. | grep http | grep -v '    Link: ' | less
>>>>>
>>>>> and skimmed the output. Maybe "Datasheet" should be allowed, too -- not
>>>>> sure.
>>> []
>>>>> But I found a few others that likely should be on the disallow list:
>>>>> "Closes:", "Bug:", "Gitlab issue:", "References:", "Ref:", "Bugzilla:",
>>>>> "RHBZ:", and "link", as "Link" should be used instead in all of these
>>>>> cases afaics.
>>>
>>> Do understand please that checkpatch will never be perfect.
>>> At best, it's just a guidance tool.
>>
>> Of course -- and that's actually a reason why I prefer a disallow list
>> over an allow list, as that gives guidance in the way of "don't use this
>> tag, use Link instead" instead of enforcing "always use Link: when
>> linking somewhere" (now that I've written it like that it feels even
>> more odd, because it's obvious that it's a link, so why bother with a
>> tag; but whatever).
>>
>> I also think the approach with a disallow list will not bother
>> developers much, while the other forces them a bit to much into a scheme.
>>
>>> To me most of these are in the noise level, but perhaps all should just
>>> use Link:
>>>
>>> $ git log -100000 --format=email -P --grep='^\w+:[ \t]*http' | \
>>>   grep -Poh '^\w+:[ \t]*http' | \
>>>   sort | uniq -c | sort -rn
>>>  103889 Link: http
>>>     415 BugLink: http
>>>     372 Patchwork: http
>>>     270 Closes: http
>>>     221 Bug: http
>>>     121 References: http
>>>     101 v1: http
>>>      77 Bugzilla: http
>>>      60 URL: http
>>>      59 v2: http
>>>      37 Datasheet: http
>>>      35 v3: http
>>>      19 v4: http
>>>      12 v5: http
>
>> Ha, I considered doing something like that when I wrote my earlier mail,
>> but was to lazy. :-D thx!
>>
>> Yeah, they are not that often, but I grew tired arguing about that,
>> that's why I think checkpatch is the better place and in the better
>> position to handle that.
> 
> I'm not sure that "Patchwork:" is a reasonable prefix.
> Is that documented anywhere?
> 
>> Anyway, so how to move forward now? Do you insist on a allow list (IOW:
>> a Link: or Patchwork: before every http...)? Or is a disallow list with
>> the most common unwanted tags for links (that you thankfully compiled)
>> fine for you as well?
> 
> Maybe
> ---
>  scripts/checkpatch.pl | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 1c3d13e65c2d0..a526a354cdfbc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3250,6 +3250,13 @@ sub process {
>  			$commit_log_possible_stack_dump = 0;
>  		}
>  
> +# Check for odd prefixes before a URI/URL
> +		if ($in_commit_log &&
> +		    $line =~ /^\s*(\w+):\s*http/ && $1 !~ /^(?:Link|Patchwork)/) {
> +			WARN("PREFER_LINK",
> +			     "Unusual link reference '$1:', prefer 'Link:'\n" . $herecurr);
> +		}
> +

One more thing: That afaics would result in a warning when people use
things like "v1: https://example.com/somewhere", which some people
apparently like. Those imho are not considered tags, hence I'd say we
allow them, unless you disagree.

Ciao, Thorsten

Re: Fw: [PATCH 0/2] feat: checkpatch: prohibit Buglink: and warn about missing Link:

[Joe Perches]

On Thu, 2022-12-08 at 14:18 +0100, Thorsten Leemhuis wrote:
> On 06.12.22 10:21, Joe Perches wrote:
> > On Tue, 2022-12-06 at 09:50 +0100, Thorsten Leemhuis wrote:
> > > On 06.12.22 08:44, Joe Perches wrote:
[]
> > > > To me most of these are in the noise level, but perhaps all should just
> > > > use Link:
> > > > 
> > > > $ git log -100000 --format=email -P --grep='^\w+:[ \t]*http' | \
> > > >   grep -Poh '^\w+:[ \t]*http' | \
> > > >   sort | uniq -c | sort -rn
> > > >  103889 Link: http
> > > >     415 BugLink: http
> > > >     372 Patchwork: http
> > > >     270 Closes: http
> > > >     221 Bug: http
> > > >     121 References: http
> > > >     101 v1: http
> > > >      77 Bugzilla: http
> > > >      60 URL: http
> > > >      59 v2: http
> > > >      37 Datasheet: http
> > > >      35 v3: http
> > > >      19 v4: http
> > > >      12 v5: http
> > 
> > > Ha, I considered doing something like that when I wrote my earlier mail,
> > > but was to lazy. :-D thx!
[]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -3250,6 +3250,13 @@ sub process {
> >  			$commit_log_possible_stack_dump = 0;
> >  		}
> >  
> > +# Check for odd prefixes before a URI/URL
> > +		if ($in_commit_log &&
> > +		    $line =~ /^\s*(\w+):\s*http/ && $1 !~ /^(?:Link|Patchwork)/) {
> > +			WARN("PREFER_LINK",
> > +			     "Unusual link reference '$1:', prefer 'Link:'\n" . $herecurr);
> > +		}
> > +
> 
> One more thing: That afaics would result in a warning when people use
> things like "v1: https://example.com/somewhere", which some people
> apparently like. Those imho are not considered tags, hence I'd say we
> allow them, unless you disagree.

I'd say no as almost all of those are when patches contain
references to previous patch submissions that should instead
be below the --- line.  Perhaps there should be a separate
warning for those v<n>: uses saying to move them below the ---
but there really aren't many uses.

Most of the v<n>: style prefixes are from git pulls/merges.
Redoing the git log grep with --no-merges shows that fairly well.

$ git log -100000 --no-merges --format=email -P --grep='^\w+:[ \t]*http' | \
  grep -Poh '^\w+:[ \t]*http' | \
  sort | uniq -c | sort -rn
 103958 Link: http
    418 BugLink: http
    372 Patchwork: http
    280 Closes: http
    224 Bug: http
    123 References: http
     84 Bugzilla: http
     61 URL: http
     42 v1: http
     38 Datasheet: http
     20 v2: http
      9 Ref: http
      9 Fixes: http
      9 Buglink: http
      8 v3: http
      8 Reference: http
      7 See: http
      6 1: http
      5 link: http
      3 Link:http
      3 IGT: http
      3 0: http
      2 Website: http
      2 Schematics: http
      2 RHBZ: http
      2 Reported: http
      2 MR: http
      2 Links: http
      2 LINK: http
      2 Link:  http
      2 Bugs: http
      2 BUG: http
      2 2: http
      1 v5: http
      1 v4: http
      1 V1: http
      1 v1:http
      1 Twitter: http
      1 tree: http
      1 tool: http
      1 tests: http
      1 tasks: http
      1 Source: http
      1 SoM: http
      1 scctc: http
      1 Reproducer: http
      1 reliable: http
      1 Related: http
      1 Reference:http
      1 oscca: http
      1 Mesa: http
      1 Lore: http
      1 Links:http
      1 ink: http
      1 in: http
      1 IETF: http
      1 here: http
      1 Examples: http
      1 bz: http
      1 Bug:http
      1 AlsaInfo: http