Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

[Jonathan Corbet <corbet@lwn.net>]

Thorsten Leemhuis <linux@leemhuis.info> writes:

> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
> as the latter is overloaded and hence doesn't indicate what the provided
> URL is about. Documenting these also provides clarity, as a few
> developers have used 'References:' to point to problem reports;
> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
> matches up with the 'Reported-by:' tag commonly used already and needed
> in this situation already.
>
> Signed-off-by: Thorsten Leemhuis <linux@leemhuis.info>
> To: workflows@vger.kernel.org

Thanks for flooding my inbox during a holiday week :)  Just looking at
this now.

> v1/RFC:
> - first, *rough version* to see how this idea is received in the
>   community
> ---
>  Documentation/maintainer/configure-git.rst   |  6 +--
>  Documentation/process/5.Posting.rst          | 54 ++++++++++++++------
>  Documentation/process/submitting-patches.rst | 22 ++++----
>  3 files changed, 53 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
> index 80ae5030a590..8429d45d661c 100644
> --- a/Documentation/maintainer/configure-git.rst
> +++ b/Documentation/maintainer/configure-git.rst
> @@ -40,12 +40,12 @@ Creating commit links to lore.kernel.org
>  The web site http://lore.kernel.org is meant as a grand archive of all mail
>  list traffic concerning or influencing the kernel development. Storing archives
>  of patches here is a recommended practice, and when a maintainer applies a
> -patch to a subsystem tree, it is a good idea to provide a Link: tag with a
> +patch to a subsystem tree, it is a good idea to provide a Reviewed: tag with a
>  reference back to the lore archive so that people that browse the commit
>  history can find related discussions and rationale behind a certain change.
>  The link tag will look like this:
>  
> -    Link: https://lore.kernel.org/r/<message-id>
> +    Reviewed: https://lore.kernel.org/r/<message-id>

The *link* tag will look like that?

[...]

> +The tags in common use are:
> +
> + - ``Reported:`` points to a report of a problem fixed by this patch. The
> +   provided URL thus might point to a entry in a bug tracker or a mail in a
> +   mailing list archive. Typically this tag is followed by a "Reported-by:"
> +   tag (see below).
> +
> + - ``Link:`` points to websites providing additional backgrounds or details,
> +   for example a document with a specification implemented by the patch.

So this is a serious change from how Link: is used now, and runs counter
to the scripts used by a lot of maintainers.  I suspect that this thread
is only as short as it is because a lot of people haven't seen this yet;
it could be a hard change to sell.

Also, I think that documents like specs should be called out separately
in the changelog, with text saying what they actually are.

> + - ``Reviewed:`` ignore this, as maintainers add it when applying a patch, to
> +   make the commit point to the latest public review of the patch.

Another question would be: what's the interplay between the (quite
similar) "Reviewed" and "Reviewed-by" tags (and the same for the report
tags).  If there's a "Reviewed" do we still need "Reviewed-by"?  That
should be spelled out, whichever way is wanted.

I do worry that the similarity is going to lead to a certain amount of
confusion and use of the wrong tag.  People have a hard time getting all
the tags we have now right; adding more that look almost like the
existing ones seems like a recipe for trouble.

For these reasons, I would be more inclined toward Konstantin's
suggestion of adding notes to the existing Link: tags.

> +A third kind of tags are used to document which developers were involved in
> +the development of the patch. Each of these uses this format::
>  
>  	tag: Full Name <email address>  optional-other-stuff
>  
>  The tags in common use are:
>  
> - - Signed-off-by: this is a developer's certification that he or she has
> + - ``Signed-off-by:`` is a developer's certification that he or she has

So this markup addition is a separate change that would belong in its
own patch.  Do we really need it, though?  It clutters the text and
irritates the anti-RST minority (which has been mercifully quiet
recently) without really adding any benefit.

Thanks,

jon