Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8edb82ed-10e8-c236-fb08-ed14ed3e4634@tessares.net>
Date:   Tue, 4 Apr 2023 10:42:29 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH v4 1/5] docs: process: allow Closes tags with links
To:     Thorsten Leemhuis <linux@leemhuis.info>,
        Jonathan Corbet <corbet@lwn.net>,
        Andy Whitcroft <apw@canonical.com>,
        Joe Perches <joe@perches.com>,
        Dwaipayan Ray <dwaipayanray1@gmail.com>,
        Lukas Bulwahn <lukas.bulwahn@gmail.com>,
        =?UTF-8?Q?Kai_Wasserb=c3=a4ch?= <kai@dev.carbon-project.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Konstantin Ryabitsev <konstantin@linuxfoundation.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc:     linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, mptcp@lists.linux.dev
References: <20230314-doc-checkpatch-closes-tag-v4-0-d26d1fa66f9f@tessares.net>
 <20230314-doc-checkpatch-closes-tag-v4-1-d26d1fa66f9f@tessares.net>
 <e671189c-086e-87bd-68db-fb80678d4666@leemhuis.info>
Content-Language: en-GB
From:   Matthieu Baerts <matthieu.baerts@tessares.net>
In-Reply-To: <e671189c-086e-87bd-68db-fb80678d4666@leemhuis.info>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Thorsten,

Thank you for this review.

On 04/04/2023 10:09, Thorsten Leemhuis wrote:
> 
> On 03.04.23 18:23, Matthieu Baerts wrote:
>> [...]
>> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
>> index 828997bc9ff9..12d58ddc2b8a 100644
>> --- a/Documentation/process/submitting-patches.rst
>> +++ b/Documentation/process/submitting-patches.rst
>> @@ -113,11 +113,9 @@ there is no collision with your six-character ID now, that condition may
>>  change five years from now.
>>  
>>  If related discussions or any other background information behind the change
>> -can be found on the web, add 'Link:' tags pointing to it. In case your patch
>> -fixes a bug, for example, add a tag with a URL referencing the report in the
>> -mailing list archives or a bug tracker; if the patch is a result of some
>> -earlier mailing list discussion or something documented on the web, point to
>> -it.
>> +can be found on the web, add 'Link:' tags pointing to it. If the patch is a
>> +result of some earlier mailing list discussions or something documented on the
>> +web, point to it.
>>  
>>  When linking to mailing list archives, preferably use the lore.kernel.org
>>  message archiver service. To create the link URL, use the contents of the
>> @@ -134,6 +132,16 @@ resources. In addition to giving a URL to a mailing list archive or bug,
>>  summarize the relevant points of the discussion that led to the
>>  patch as submitted.
>>  
>> +In case your patch fixes a bug, use the 'Closes:' tag with a URL referencing
>> +the report in the mailing list archives or a public bug tracker. For example::
>> +
>> +	Closes: https://example.com/issues/1234
> 
> YMMV, but is this...
> 
>> +Some bug trackers have the ability to close issues automatically when a
>> +commit with such a tag is applied. Some bots monitoring mailing lists can
>> +also track such tags and take certain actions. Private bug trackers and
>> +invalid URLs are forbidden.
>> +
> 
> ...section (and a similar one in the other document) really worth it
> and/or does it have to be that long? A simple "Some bug trackers then
> will automatically close the issue when the commit is merged" IMHO would
> suffice, but OTOH it might be considered common knowledge. And the
> "found on the web", "a public bug tracker" (both quoted above) and
> "available on the web" (quoted below) already make it pretty clear that
> links to private bug trackers are now desired. And there is also a
> "Please check the link to make sure that it is actually working and
> points to the relevant message." in submitting-patches.rst already, so
> invalid URLs are obviously not wanted either.

This paragraph seems worth it to me: the two first sentences explain how
this tag can be used by external tools and the last one clearly explain
what is not allowed. I agree that it makes sense and it is somehow
already described around with the "positive form" but it is very common
to use the "Closes:" tag with just the ticket ID, not the full URL. It
might then be important to clearly mention that it has to be used with a
valid URL and not a short version. While at it, I think it is fine to
add that private bug trackers are forbidden too because it can be very
tempting for devs to use them if automations are in place. And also
because checkpatch.pl is not going to verify if URLs are public.

But I'm clearly not an expert in writing docs, it is just my point of
view as a developer :)
I don't mind changing the text.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net