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@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:
>>
>> [...]
>>
>>> 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