[PATCH v3 0/1] update MAINTAINERS file with git trailers

[PATCH v3 0/1] update MAINTAINERS file with git trailers

[Bruno Larsen]

Some private chats highlighted that the currently in use git trailers
aren't explained well enough. This patch aims to fix that by adding the
information in a more verbose way to guarantee that everyone is on the
same page about them and refine anything that might still need work.

The first (and second) version of this update didn't get much traction, so I decided
to add every active global maintainer as CC, since everyone is affected
by this and I want some consensus before comitting.

Right now there is one big unanswered question: Should we have a
specific tag to explicitly signal when a patch has been partially
approved? Eli asked for it to avoid people mechanically reading tags
from thinking that a patch has been fully approved when it was only
partial. If we agree that there should be one, what would it look like?
I suggested using Acked-By, but Simon uses it differently, and I opted
to keep his usage of it.

Thoughts? Comments? Suggestions?

Bruno Larsen (1):
  [gdb]: add git trailer information on gdb/MAINTAINERS

 gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH v4 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS

[Bruno Larsen]

The project has been using Tested-By (tb), Reviewed-By (rb) and
Approved-By (ab) for some time, but there has been no information to be
found in the actual repository. This commit changes that by adding
information about all git trailers to the MAINTAINERS file, so that it
can be easily double-checked.

The upstream discussion also brought up the use of Acked-by, which is
better defined in this commit.  Finally, for completeness sake, the
trailers Co-Authored-By and Bug were added, even though they have been
in use for some time already
---
 gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
index 7fa608fd82c..609f740f21d 100644
--- a/gdb/MAINTAINERS
+++ b/gdb/MAINTAINERS
@@ -43,14 +43,9 @@ patch without review from another maintainer.  This especially includes
 patches which change internal interfaces (e.g. global functions, data
 structures) or external interfaces (e.g. user, remote, MI, et cetera).
 
-The term "review" is used in this file to describe several kinds of feedback
-from a maintainer: approval, rejection, and requests for changes or
-clarification with the intention of approving a revised version.  Review is
-a privilege and/or responsibility of various positions among the GDB
-Maintainers.  Of course, anyone - whether they hold a position but not the
-relevant one for a particular patch, or are just following along on the
-mailing lists for fun, or anything in between - may suggest changes or
-ask questions about a patch!
+The word "contributor" is used in this document to refer to any GDB
+developer listed above as well as folks who may have suggested some
+patches but aren't part of one of those categories for any reason.
 
 There's also a couple of other people who play special roles in the GDB
 community, separately from the patch process:
@@ -78,6 +73,67 @@ consensus among the global maintainers and any other involved parties.
 In cases where consensus can not be reached, the global maintainers may
 ask the official FSF-appointed GDB maintainers for a final decision.
 
+The term "review" is used in this file to describe several kinds of
+feedback from a maintainer: approval, rejection, and requests for changes
+or clarification with the intention of approving a revised version.
+Approval is a privilege and/or responsibility of various positions among
+the GDB Maintainers.  Of course, anyone - whether they hold a position, but
+not the relevant one for a particular patch, or are just following along on
+the mailing lists for fun, or anything in between - may suggest changes, ask
+questions about a patch or say if they believe a patch is fit for upstreaming!
+
+To ensure that patches are only pushed when approved, and to properly credit
+the contributors who take the time to improve this project, the following
+trailers are used to identify who contributed and how.  All patches pushed
+upstream should have at least one Approved-By patches (with the exception of
+obvious patches, see below).  The trailers (or tags) currently in use are:
+
+ - Acked-By:
+
+   Used when a contributor has taken a quick glance at a patch and agrees
+   with the direction outlined in the commit message, but hasn't evaluated
+   the code for correctness or regressions.
+   Usage: "Acked-By: Your Name <your@email>"
+
+ - Tested-by:
+
+   Used when a contributor has tested the patch and finds that it
+   fixes the claimed problem.  It may also be used to indicate that
+   the contributor has performed regression testing.  By itself, this
+   tag says nothing about the quality of the fix implemented by the
+   patch, nor the amount of testing that was actually performed.
+   Usage: "Tested-By: Your Name <your@email>"
+
+ - Reviewed-by:
+
+   Used when a contributor has looked at the code and agrees with
+   the changes, but either doesn't have the authority or doesn't
+   feel comfortable approving the patch.
+   Usage: "Reviewed-By: Your Name <your@email>"
+
+ - Approved-by:
+
+   Used by responsible maintainers or global maintainers when a patch is
+   ready to be upstreamed.  Some patches may touch multiple areas and
+   require multiple approvals before landing (such as a maintainer only
+   approving documentation), it is up to the maintainer giving the approval
+   tag to make it clear when that a tag is not sufficient. Responsible,
+   Global and Official FSF-appointed maintainers may approve their own
+   patches, but it is recommended that they seek external approval before
+   doing so.
+   Usage: "Approved-By: Your Name <your@email>"
+
+ - Co-Authored-By:
+
+   Used when the commit includes meaningful conrtibutions from multiple people.
+   Usage: "Co-Authored-By: Contributor's Name <their@email>"
+
+ - Bug:
+
+   This trailer is added with a link to the GDB bug tracker bug for
+   added context on relevant commits.
+   Usage: "Bug: <link>"
+
 
 			The Obvious Fix Rule
 			--------------------
-- 
2.41.0

Re: [PATCH v4 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS

[Kevin Buettner]

Hi,

This version looks good to me.

Reviewed-by: Kevin Buettner <kevinb@redhat.com>

P.S. I was tempted to use "Approved-by", but I want to give others an
opportunity to comment.  So, if you get a bunch of positive reviews
with no requests for changes, and no one else approves it, consider
the above to be an "Approved-by".

Kevin

On Thu, 13 Jul 2023 12:56:53 +0200
Bruno Larsen <blarsen@redhat.com> wrote:

> The project has been using Tested-By (tb), Reviewed-By (rb) and
> Approved-By (ab) for some time, but there has been no information to be
> found in the actual repository. This commit changes that by adding
> information about all git trailers to the MAINTAINERS file, so that it
> can be easily double-checked.
> 
> The upstream discussion also brought up the use of Acked-by, which is
> better defined in this commit.  Finally, for completeness sake, the
> trailers Co-Authored-By and Bug were added, even though they have been
> in use for some time already
> ---
>  gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
> index 7fa608fd82c..609f740f21d 100644
> --- a/gdb/MAINTAINERS
> +++ b/gdb/MAINTAINERS
> @@ -43,14 +43,9 @@ patch without review from another maintainer.  This especially includes
>  patches which change internal interfaces (e.g. global functions, data
>  structures) or external interfaces (e.g. user, remote, MI, et cetera).
>  
> -The term "review" is used in this file to describe several kinds of feedback
> -from a maintainer: approval, rejection, and requests for changes or
> -clarification with the intention of approving a revised version.  Review is
> -a privilege and/or responsibility of various positions among the GDB
> -Maintainers.  Of course, anyone - whether they hold a position but not the
> -relevant one for a particular patch, or are just following along on the
> -mailing lists for fun, or anything in between - may suggest changes or
> -ask questions about a patch!
> +The word "contributor" is used in this document to refer to any GDB
> +developer listed above as well as folks who may have suggested some
> +patches but aren't part of one of those categories for any reason.
>  
>  There's also a couple of other people who play special roles in the GDB
>  community, separately from the patch process:
> @@ -78,6 +73,67 @@ consensus among the global maintainers and any other involved parties.
>  In cases where consensus can not be reached, the global maintainers may
>  ask the official FSF-appointed GDB maintainers for a final decision.
>  
> +The term "review" is used in this file to describe several kinds of
> +feedback from a maintainer: approval, rejection, and requests for changes
> +or clarification with the intention of approving a revised version.
> +Approval is a privilege and/or responsibility of various positions among
> +the GDB Maintainers.  Of course, anyone - whether they hold a position, but
> +not the relevant one for a particular patch, or are just following along on
> +the mailing lists for fun, or anything in between - may suggest changes, ask
> +questions about a patch or say if they believe a patch is fit for upstreaming!
> +
> +To ensure that patches are only pushed when approved, and to properly credit
> +the contributors who take the time to improve this project, the following
> +trailers are used to identify who contributed and how.  All patches pushed
> +upstream should have at least one Approved-By patches (with the exception of
> +obvious patches, see below).  The trailers (or tags) currently in use are:
> +
> + - Acked-By:
> +
> +   Used when a contributor has taken a quick glance at a patch and agrees
> +   with the direction outlined in the commit message, but hasn't evaluated
> +   the code for correctness or regressions.
> +   Usage: "Acked-By: Your Name <your@email>"
> +
> + - Tested-by:
> +
> +   Used when a contributor has tested the patch and finds that it
> +   fixes the claimed problem.  It may also be used to indicate that
> +   the contributor has performed regression testing.  By itself, this
> +   tag says nothing about the quality of the fix implemented by the
> +   patch, nor the amount of testing that was actually performed.
> +   Usage: "Tested-By: Your Name <your@email>"
> +
> + - Reviewed-by:
> +
> +   Used when a contributor has looked at the code and agrees with
> +   the changes, but either doesn't have the authority or doesn't
> +   feel comfortable approving the patch.
> +   Usage: "Reviewed-By: Your Name <your@email>"
> +
> + - Approved-by:
> +
> +   Used by responsible maintainers or global maintainers when a patch is
> +   ready to be upstreamed.  Some patches may touch multiple areas and
> +   require multiple approvals before landing (such as a maintainer only
> +   approving documentation), it is up to the maintainer giving the approval
> +   tag to make it clear when that a tag is not sufficient. Responsible,
> +   Global and Official FSF-appointed maintainers may approve their own
> +   patches, but it is recommended that they seek external approval before
> +   doing so.
> +   Usage: "Approved-By: Your Name <your@email>"
> +
> + - Co-Authored-By:
> +
> +   Used when the commit includes meaningful conrtibutions from multiple people.
> +   Usage: "Co-Authored-By: Contributor's Name <their@email>"
> +
> + - Bug:
> +
> +   This trailer is added with a link to the GDB bug tracker bug for
> +   added context on relevant commits.
> +   Usage: "Bug: <link>"
> +
>  
>  			The Obvious Fix Rule
>  			--------------------
> -- 
> 2.41.0
> 

Re: [PATCH v3 0/1] update MAINTAINERS file with git trailers

[Kevin Buettner]

On Thu, 13 Jul 2023 12:56:51 +0200
Bruno Larsen <blarsen@redhat.com> wrote:

> Right now there is one big unanswered question: Should we have a
> specific tag to explicitly signal when a patch has been partially
> approved? Eli asked for it to avoid people mechanically reading tags
> from thinking that a patch has been fully approved when it was only
> partial.

I don't think we need a tag for this.  Since we review and/or approve
patches via email, I think some additional text stating which portions
were reviewed or approved is sufficient.

Suppose I'm an area maintainer or a global maintainer who has confident
knowledge of a particular area.  I might then do something like this:

    For the mn10300 architecture portions:
    Approved-by: Kevin Buettner <kevinb@redhat.com>

Only the Approved-by tag would be added to the git trailer, but it's
clear to anyone involved in the approval process that I haven't
approved the patch in its entirety, only certain parts.  If I were to
review the rest of the patch, but not approve it, I see nothing wrong
with also saying:

    For everything else:
    Reviewed-by: Kevin Buettner <kevinb@redhat.com>

I also see nothing wrong with qualifying the 'Reviewed-by' or
'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
but we might also get more people involved with the review process,
which I think would be a good thing.

If we really want to include the portions reviewed in the trailer, then
I suggest extending the format of the trailer, perhaps like this:

    Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)

Kevin

Re: [PATCH v3 0/1] update MAINTAINERS file with git trailers

[Eli Zaretskii]

> Date: Thu, 13 Jul 2023 14:50:47 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> Cc: gdb-patches@sourceware.org, pedro@palves.net, aburgess@redhat.com,
>  brobecker@adacore.com, simon.marchi@polymtl.ca, tom@tromey.com,
>  tdevries@suse.de, ulrich.weigand@de.ibm.com, eliz@gnu.org
> 
> On Thu, 13 Jul 2023 12:56:51 +0200
> Bruno Larsen <blarsen@redhat.com> wrote:
> 
> > Right now there is one big unanswered question: Should we have a
> > specific tag to explicitly signal when a patch has been partially
> > approved? Eli asked for it to avoid people mechanically reading tags
> > from thinking that a patch has been fully approved when it was only
> > partial.
> 
> I don't think we need a tag for this.  Since we review and/or approve
> patches via email, I think some additional text stating which portions
> were reviewed or approved is sufficient.
> 
> Suppose I'm an area maintainer or a global maintainer who has confident
> knowledge of a particular area.  I might then do something like this:
> 
>     For the mn10300 architecture portions:
>     Approved-by: Kevin Buettner <kevinb@redhat.com>
> 
> Only the Approved-by tag would be added to the git trailer, but it's
> clear to anyone involved in the approval process that I haven't
> approved the patch in its entirety, only certain parts.  If I were to
> review the rest of the patch, but not approve it, I see nothing wrong
> with also saying:
> 
>     For everything else:
>     Reviewed-by: Kevin Buettner <kevinb@redhat.com>
> 
> I also see nothing wrong with qualifying the 'Reviewed-by' or
> 'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
> but we might also get more people involved with the review process,
> which I think would be a good thing.
> 
> If we really want to include the portions reviewed in the trailer, then
> I suggest extending the format of the trailer, perhaps like this:
> 
>     Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)

The above will only work if everyone pays attention to those
qualifications.  Moreover, in Real Life, the response doesn't include
just two such lines, it in many cases includes more text, and those
qualifications can easily "drown" in that.

Also, we used to have a way of saying "Approved, if those few nits are
fixed", and the above either removes that possibility entirely, or
will make it harder to determine whether and which parts were
approved, and on what conditions.

I still don't understand why we need the "partial-approved" facility
that uses Approved-by.  How is it different from Reviewed-by? the
submitter still needs to figure out whether all the parts were okayed
or not, so the only aspect this changes is making it more complicated
for area maintainers to write these tags, because instead of just a
single Reviewed-by they need to choose among two tags.

So my vote is for reserving Approved-by only to the cases where the
entire patch is approved.  Alternatively, we could introduce an
additional tag, like Partially-approved-by or something.

I guess my point is that this should be simple and ideally include
only fixed text, not some free-form text that could lead to
misunderstandings and misinterpretations.

Re: [PATCH v3 0/1] update MAINTAINERS file with git trailers

[Bruno Larsen]

On 14/07/2023 07:50, Eli Zaretskii wrote:
>> Date: Thu, 13 Jul 2023 14:50:47 -0700
>> From: Kevin Buettner <kevinb@redhat.com>
>> Cc: gdb-patches@sourceware.org, pedro@palves.net, aburgess@redhat.com,
>>   brobecker@adacore.com, simon.marchi@polymtl.ca, tom@tromey.com,
>>   tdevries@suse.de, ulrich.weigand@de.ibm.com, eliz@gnu.org
>>
>> On Thu, 13 Jul 2023 12:56:51 +0200
>> Bruno Larsen <blarsen@redhat.com> wrote:
>>
>>> Right now there is one big unanswered question: Should we have a
>>> specific tag to explicitly signal when a patch has been partially
>>> approved? Eli asked for it to avoid people mechanically reading tags
>>> from thinking that a patch has been fully approved when it was only
>>> partial.
>> I don't think we need a tag for this.  Since we review and/or approve
>> patches via email, I think some additional text stating which portions
>> were reviewed or approved is sufficient.
>>
>> Suppose I'm an area maintainer or a global maintainer who has confident
>> knowledge of a particular area.  I might then do something like this:
>>
>>      For the mn10300 architecture portions:
>>      Approved-by: Kevin Buettner <kevinb@redhat.com>
>>
>> Only the Approved-by tag would be added to the git trailer, but it's
>> clear to anyone involved in the approval process that I haven't
>> approved the patch in its entirety, only certain parts.  If I were to
>> review the rest of the patch, but not approve it, I see nothing wrong
>> with also saying:
>>
>>      For everything else:
>>      Reviewed-by: Kevin Buettner <kevinb@redhat.com>
>>
>> I also see nothing wrong with qualifying the 'Reviewed-by' or
>> 'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
>> but we might also get more people involved with the review process,
>> which I think would be a good thing.
>>
>> If we really want to include the portions reviewed in the trailer, then
>> I suggest extending the format of the trailer, perhaps like this:
>>
>>      Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)
> The above will only work if everyone pays attention to those
> qualifications.  Moreover, in Real Life, the response doesn't include
> just two such lines, it in many cases includes more text, and those
> qualifications can easily "drown" in that.
>
> Also, we used to have a way of saying "Approved, if those few nits are
> fixed", and the above either removes that possibility entirely, or
> will make it harder to determine whether and which parts were
> approved, and on what conditions.
This is not something that could ever be dealt with in automation, 
unless we also wrote the nits in machine friendly ways. If we want to 
continue using this style, we need to have context dependent tags. Maybe 
I can add a line about it in the file?
>
> I still don't understand why we need the "partial-approved" facility
> that uses Approved-by.  How is it different from Reviewed-by? the
> submitter still needs to figure out whether all the parts were okayed
> or not, so the only aspect this changes is making it more complicated
> for area maintainers to write these tags, because instead of just a
> single Reviewed-by they need to choose among two tags.

There are a few reasons I don't like using reviewed-by. Most 
importantly, if a patch touches 2 separate areas, both area maintainers 
might send an rb tag and the contributor will keep waiting for an ab tag 
that is unnecessary. Another reason is that it could confuse things 
further when someone who doesn't have any approval rights looks at a 
patch and says things look ok, they might assume this rb has more power 
than it has.

The difference between the ambiguous review and the ambiguous approval 
is that the former relies on knowing who has what level of authority, 
while the latter relies on interpreting the email context.

I do like Kevin's idea of extending the tag to be "Approved-By: name 
<email> [(areas)]", where (areas) is used if the approval is partial. I 
think this could be explained easily enough in the file to avoid 
confusion. It would not be general speech context, it would be explicit 
in the tag itself. Do you think that this enough to not make the partial 
approval be missed? If not, I'm fine with us introducing 
Partially-Approved-By.

-- 
Cheers,
Bruno

>
> So my vote is for reserving Approved-by only to the cases where the
> entire patch is approved.  Alternatively, we could introduce an
> additional tag, like Partially-approved-by or something.
>
> I guess my point is that this should be simple and ideally include
> only fixed text, not some free-form text that could lead to
> misunderstandings and misinterpretations.
>