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@xxxxxxxxxxxxxxxxxxxxxx>
>> To: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>, Thorsten Leemhuis <linux@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Kai Wasserbäch <kai@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx
> [...]
> 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@xxxxxxxxxxxxxx/

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@xxxxxxxxxxxxxx/

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@xxxxxxxxxxxxxx/

```
>> 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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx/

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

Ciao, Thorsten