[v6] : add git trailer information on gdb/MAINTAINERS

  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. Simply put, the trailers in use work as
follows:

* Tested-by: The person tested the patch and it fixes the problem, or
  introduces no regressions (or both).
* Acked-by: The general outline looks good, but the maintainer hasn't
  looked at the code
* Reviewed-by: The code looks good, but the reviewer has not approved
  the patch to go upstream
* Approved-by: The patch is ready to be pushed to master

These last 3 trailers can also be restricted to one or more areas of GDB
by adding the areas in a comma separated list in parenthesis after the
trailers.

Finally, for completeness sake, the trailers Co-Authored-By and Bug
were added, even though they have been in use for a long time already

---
 gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 8 deletions(-)
  

Ping!
  

Ping!
  

On 11/2/23 13:54, Guinevere Larsen 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. Simply put, the trailers in use work as
> follows:
> 
> * Tested-by: The person tested the patch and it fixes the problem, or
>   introduces no regressions (or both).
> * Acked-by: The general outline looks good, but the maintainer hasn't
>   looked at the code
> * Reviewed-by: The code looks good, but the reviewer has not approved
>   the patch to go upstream
> * Approved-by: The patch is ready to be pushed to master
> 
> These last 3 trailers can also be restricted to one or more areas of GDB
> by adding the areas in a comma separated list in parenthesis after the
> trailers.
> 
> Finally, for completeness sake, the trailers Co-Authored-By and Bug
> were added, even though they have been in use for a long time already
> 
> ---
>  gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 85 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the exception of
> +obvious patches, see below).  The trailers (or tags) currently in use are:
> +
> + - 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>"
> +
> + - Acked-By:
> +
> +   Used when a responsible or global maintaner has taken a superficial
> +   look at a patch and agree with its direction, but has not done further
> +   review on the subject.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.
> +   Usage: "Acked-By: Your Name <your@email> (area1, area2)"
> +
> + - 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.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.
> +   Usage: "Reviewed-By: Your Name <your@email> (area1, area2)"
> +
> + - Approved-by:
> +
> +   Used by responsible maintainers or global maintainers when a patch is
> +   ready to be upstreamed.  If a patch requires multiple approvals, only
> +   the last reviewer should use this tag, making it obvious to the
> +   contributor that the patch is ready to be pushed.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.  Patches must have all areas
> +   approved before being pushed.  If a patch has had some areas approved,
> +   it is recommended that the final approver makes it explicit that the
> +   patch is ready for pushing.
> +   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>"
> +
> +Sometimes, contributors may request small changes, such as fixing typos, before
> +granting the review or approval trailer. When the contributor thinks that
> +these changes are so small that it isn't necessary to send a new version, they
> +may add some text like "with these changes, I'm ok with the patch", followed by
> +their trailer.  In those situations, the trailer is only valid after the
> +changes are made.
> +
>  
>  			The Obvious Fix Rule
>  			--------------------

This looks OK to me. The trailers make sense and are a good indication, for
the contributor, about how the patch/series is progressing.

I'm happy with this version, but I'd like to see what other maintainers think.

Reviewed-by: Luis Machado <luis.machado@arm.com>
  

On 11/2/23 6:54 AM, Guinevere Larsen 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. Simply put, the trailers in use work as
> follows:
> 
> * Tested-by: The person tested the patch and it fixes the problem, or
>    introduces no regressions (or both).
> * Acked-by: The general outline looks good, but the maintainer hasn't
>    looked at the code
> * Reviewed-by: The code looks good, but the reviewer has not approved
>    the patch to go upstream
> * Approved-by: The patch is ready to be pushed to master
> 
> These last 3 trailers can also be restricted to one or more areas of GDB
> by adding the areas in a comma separated list in parenthesis after the
> trailers.
> 
> Finally, for completeness sake, the trailers Co-Authored-By and Bug
> were added, even though they have been in use for a long time already
> 
> ---
>   gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 85 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the exception of
> +obvious patches, see below).  The trailers (or tags) currently in use are:

This next to last sentence doesn't match current practice where a maintainer
pushes a self-approved change.  That is, to date we haven't used any
'Approved-by: <self>' trailers and this would seem to mandate that?

(Also, s/trailers/trailer/ in that sentence)

Also, there is some inconsistency with "by" vs "By" in this change.  For example,
this paragraph uses 'Approved-By' but the description below uses 'Approved-by'.
The manpages for git(1) (e.g. git-interpret-trailer(1)) tend to only capitalize
the first word, e.g. 'Signed-off-by'.

Our existing log messages are a bit all over the place currently, e.g. for
Co-authored-by we have the following in master to date:

  127 Co-Authored-By:
    9 Co-Authored-by:
   35 Co-authored-by:

For both Approved-by and Reviewed-by the uppercase 'By' variants outnumber
lowercase 'by' by a fair margin.

> + - 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>"

I would find it slightly easier to read with a blank line before each Usage line.

> +
> + - Acked-By:
> +
> +   Used when a responsible or global maintaner has taken a superficial

s/maintaner/maintainer/

> +   look at a patch and agree with its direction, but has not done further

s/agree/agrees/

> +   review on the subject.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.

I think you want s/comma separated/comma-separated/ here and elsewhere.

> +   Usage: "Acked-By: Your Name <your@email> (area1, area2)"
> +
> + - 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.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.
> +   Usage: "Reviewed-By: Your Name <your@email> (area1, area2)"
> +
> + - Approved-by:
> +
> +   Used by responsible maintainers or global maintainers when a patch is
> +   ready to be upstreamed.  If a patch requires multiple approvals, only
> +   the last reviewer should use this tag, making it obvious to the
> +   contributor that the patch is ready to be pushed.
> +   This trailer can be specific to one or more areas of the project, as
> +   defined by the "Responsible maintainers" section of this file.  If
> +   that is the case, the area(s) should be added at the end of the tag in
> +   parenthesis in a comma separated list.  Patches must have all areas
> +   approved before being pushed.  If a patch has had some areas approved,
> +   it is recommended that the final approver makes it explicit that the
> +   patch is ready for pushing.
> +   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.

s/conrtibutions/contributions/

> +   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>"
> +
> +Sometimes, contributors may request small changes, such as fixing typos, before
> +granting the review or approval trailer. When the contributor thinks that
> +these changes are so small that it isn't necessary to send a new version, they
> +may add some text like "with these changes, I'm ok with the patch", followed by
> +their trailer.  In those situations, the trailer is only valid after the
> +changes are made.
> +
>   
>   			The Obvious Fix Rule
>   			--------------------
  

On 28/11/2023 17:39, John Baldwin wrote:
> On 11/2/23 6:54 AM, Guinevere Larsen 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. Simply put, the trailers in use work as
>> follows:
>>
>> * Tested-by: The person tested the patch and it fixes the problem, or
>>    introduces no regressions (or both).
>> * Acked-by: The general outline looks good, but the maintainer hasn't
>>    looked at the code
>> * Reviewed-by: The code looks good, but the reviewer has not approved
>>    the patch to go upstream
>> * Approved-by: The patch is ready to be pushed to master
>>
>> These last 3 trailers can also be restricted to one or more areas of GDB
>> by adding the areas in a comma separated list in parenthesis after the
>> trailers.
>>
>> Finally, for completeness sake, the trailers Co-Authored-By and Bug
>> were added, even though they have been in use for a long time already
>>
>> ---
>>   gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 85 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
>> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the 
>> exception of
>> +obvious patches, see below).  The trailers (or tags) currently in 
>> use are:
>
> This next to last sentence doesn't match current practice where a 
> maintainer
> pushes a self-approved change.  That is, to date we haven't used any
> 'Approved-by: <self>' trailers and this would seem to mandate that?

It would certainly encourage that. My reasoning to want to encourage it  
is that it makes things simpler to automate. For example, if we ever 
decide to have a pre-receive hook on the remote, we could check for an 
approval tag (and maybe add an explicit call-out to the obvious fix 
rule), but I didn't want to bog down this first discussion with that 
automation idea.

>
> (Also, s/trailers/trailer/ in that sentence)
>
> Also, there is some inconsistency with "by" vs "By" in this change.  
> For example,
> this paragraph uses 'Approved-By' but the description below uses 
> 'Approved-by'.
> The manpages for git(1) (e.g. git-interpret-trailer(1)) tend to only 
> capitalize
> the first word, e.g. 'Signed-off-by'.
>
> Our existing log messages are a bit all over the place currently, e.g. 
> for
> Co-authored-by we have the following in master to date:
>
>  127 Co-Authored-By:
>    9 Co-Authored-by:
>   35 Co-authored-by:
>
> For both Approved-by and Reviewed-by the uppercase 'By' variants 
> outnumber
> lowercase 'by' by a fair margin.

This would seem to imply that we want to standardize capitalizing all 
words, right? It's different to the manual, but is the most common in 
our logs

I will fix everything else you mentioned locally, but I don't think it's 
worth sending a new version with these small changes. Let me know if you 
disagree, though
  

On 11/28/23 9:15 AM, Guinevere Larsen wrote:
> On 28/11/2023 17:39, John Baldwin wrote:
>> On 11/2/23 6:54 AM, Guinevere Larsen 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. Simply put, the trailers in use work as
>>> follows:
>>>
>>> * Tested-by: The person tested the patch and it fixes the problem, or
>>>     introduces no regressions (or both).
>>> * Acked-by: The general outline looks good, but the maintainer hasn't
>>>     looked at the code
>>> * Reviewed-by: The code looks good, but the reviewer has not approved
>>>     the patch to go upstream
>>> * Approved-by: The patch is ready to be pushed to master
>>>
>>> These last 3 trailers can also be restricted to one or more areas of GDB
>>> by adding the areas in a comma separated list in parenthesis after the
>>> trailers.
>>>
>>> Finally, for completeness sake, the trailers Co-Authored-By and Bug
>>> were added, even though they have been in use for a long time already
>>>
>>> ---
>>>    gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
>>>    1 file changed, 85 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
>>> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the
>>> exception of
>>> +obvious patches, see below).  The trailers (or tags) currently in
>>> use are:
>>
>> This next to last sentence doesn't match current practice where a
>> maintainer
>> pushes a self-approved change.  That is, to date we haven't used any
>> 'Approved-by: <self>' trailers and this would seem to mandate that?
> 
> It would certainly encourage that. My reasoning to want to encourage it
> is that it makes things simpler to automate. For example, if we ever
> decide to have a pre-receive hook on the remote, we could check for an
> approval tag (and maybe add an explicit call-out to the obvious fix
> rule), but I didn't want to bog down this first discussion with that
> automation idea.

I think it's probably useful to have some balance that doesn't necessarily
require automated checking.  In particular, my worry is that adding self
approval trailers will dilute the meaning of the trailers somewhat.  I
think it is useful for approval to always mean someone else besides the
committer approved the change so that self-approved commits do stand out.

>> (Also, s/trailers/trailer/ in that sentence)
>>
>> Also, there is some inconsistency with "by" vs "By" in this change.
>> For example,
>> this paragraph uses 'Approved-By' but the description below uses
>> 'Approved-by'.
>> The manpages for git(1) (e.g. git-interpret-trailer(1)) tend to only
>> capitalize
>> the first word, e.g. 'Signed-off-by'.
>>
>> Our existing log messages are a bit all over the place currently, e.g.
>> for
>> Co-authored-by we have the following in master to date:
>>
>>   127 Co-Authored-By:
>>     9 Co-Authored-by:
>>    35 Co-authored-by:
>>
>> For both Approved-by and Reviewed-by the uppercase 'By' variants
>> outnumber
>> lowercase 'by' by a fair margin.
> 
> This would seem to imply that we want to standardize capitalizing all
> words, right? It's different to the manual, but is the most common in
> our logs

I will defer to the consensus of other folks here on which one to pick.
I just think there's value in picking one and then being consistent. :)

> I will fix everything else you mentioned locally, but I don't think it's
> worth sending a new version with these small changes. Let me know if you
> disagree, though

That's fine, no need to re-send.
  

On 28/11/2023 20:36, John Baldwin wrote:
> On 11/28/23 9:15 AM, Guinevere Larsen wrote:
>> On 28/11/2023 17:39, John Baldwin wrote:
>>> On 11/2/23 6:54 AM, Guinevere Larsen 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. Simply put, the trailers in use work as
>>>> follows:
>>>>
>>>> * Tested-by: The person tested the patch and it fixes the problem, or
>>>>     introduces no regressions (or both).
>>>> * Acked-by: The general outline looks good, but the maintainer hasn't
>>>>     looked at the code
>>>> * Reviewed-by: The code looks good, but the reviewer has not approved
>>>>     the patch to go upstream
>>>> * Approved-by: The patch is ready to be pushed to master
>>>>
>>>> These last 3 trailers can also be restricted to one or more areas 
>>>> of GDB
>>>> by adding the areas in a comma separated list in parenthesis after the
>>>> trailers.
>>>>
>>>> Finally, for completeness sake, the trailers Co-Authored-By and Bug
>>>> were added, even though they have been in use for a long time already
>>>>
>>>> ---
>>>>    gdb/MAINTAINERS | 93 
>>>> ++++++++++++++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 85 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
>>>> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the
>>>> exception of
>>>> +obvious patches, see below).  The trailers (or tags) currently in
>>>> use are:
>>>
>>> This next to last sentence doesn't match current practice where a
>>> maintainer
>>> pushes a self-approved change.  That is, to date we haven't used any
>>> 'Approved-by: <self>' trailers and this would seem to mandate that?
>>
>> It would certainly encourage that. My reasoning to want to encourage it
>> is that it makes things simpler to automate. For example, if we ever
>> decide to have a pre-receive hook on the remote, we could check for an
>> approval tag (and maybe add an explicit call-out to the obvious fix
>> rule), but I didn't want to bog down this first discussion with that
>> automation idea.
>
> I think it's probably useful to have some balance that doesn't 
> necessarily
> require automated checking.  In particular, my worry is that adding self
> approval trailers will dilute the meaning of the trailers somewhat.  I
> think it is useful for approval to always mean someone else besides the
> committer approved the change so that self-approved commits do stand out.

This makes sense, I hadn't considered that self approval wouldn't be as 
obvious, you make a good point. That and the fact that exceptions would 
already be needed because of the obvious fix rule, I think I'll remove 
this sentence from the update.

The new version will look like this:

    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.  The trailers 
(or tags)
    currently in use are:

>
>>> (Also, s/trailers/trailer/ in that sentence)
>>>
>>> Also, there is some inconsistency with "by" vs "By" in this change.
>>> For example,
>>> this paragraph uses 'Approved-By' but the description below uses
>>> 'Approved-by'.
>>> The manpages for git(1) (e.g. git-interpret-trailer(1)) tend to only
>>> capitalize
>>> the first word, e.g. 'Signed-off-by'.
>>>
>>> Our existing log messages are a bit all over the place currently, e.g.
>>> for
>>> Co-authored-by we have the following in master to date:
>>>
>>>   127 Co-Authored-By:
>>>     9 Co-Authored-by:
>>>    35 Co-authored-by:
>>>
>>> For both Approved-by and Reviewed-by the uppercase 'By' variants
>>> outnumber
>>> lowercase 'by' by a fair margin.
>>
>> This would seem to imply that we want to standardize capitalizing all
>> words, right? It's different to the manual, but is the most common in
>> our logs
>
> I will defer to the consensus of other folks here on which one to pick.
> I just think there's value in picking one and then being consistent. :)
That's fair. I have standardized on capitalizing all words, but I'll 
wait for other opinions before pushing.
  

On 11/29/23 10:47, Guinevere Larsen wrote:
> On 28/11/2023 20:36, John Baldwin wrote:
>> On 11/28/23 9:15 AM, Guinevere Larsen wrote:
>>> On 28/11/2023 17:39, John Baldwin wrote:
>>>> On 11/2/23 6:54 AM, Guinevere Larsen 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. Simply put, the trailers in use work as
>>>>> follows:
>>>>>
>>>>> * Tested-by: The person tested the patch and it fixes the problem, or
>>>>>     introduces no regressions (or both).
>>>>> * Acked-by: The general outline looks good, but the maintainer hasn't
>>>>>     looked at the code
>>>>> * Reviewed-by: The code looks good, but the reviewer has not approved
>>>>>     the patch to go upstream
>>>>> * Approved-by: The patch is ready to be pushed to master
>>>>>
>>>>> These last 3 trailers can also be restricted to one or more areas of GDB
>>>>> by adding the areas in a comma separated list in parenthesis after the
>>>>> trailers.
>>>>>
>>>>> Finally, for completeness sake, the trailers Co-Authored-By and Bug
>>>>> were added, even though they have been in use for a long time already
>>>>>
>>>>> ---
>>>>>    gdb/MAINTAINERS | 93 ++++++++++++++++++++++++++++++++++++++++++++-----
>>>>>    1 file changed, 85 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
>>>>> index 9989956065e..e1bb437a675 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,88 @@ 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 trailers (with the
>>>>> exception of
>>>>> +obvious patches, see below).  The trailers (or tags) currently in
>>>>> use are:
>>>>
>>>> This next to last sentence doesn't match current practice where a
>>>> maintainer
>>>> pushes a self-approved change.  That is, to date we haven't used any
>>>> 'Approved-by: <self>' trailers and this would seem to mandate that?
>>>
>>> It would certainly encourage that. My reasoning to want to encourage it
>>> is that it makes things simpler to automate. For example, if we ever
>>> decide to have a pre-receive hook on the remote, we could check for an
>>> approval tag (and maybe add an explicit call-out to the obvious fix
>>> rule), but I didn't want to bog down this first discussion with that
>>> automation idea.
>>
>> I think it's probably useful to have some balance that doesn't necessarily
>> require automated checking.  In particular, my worry is that adding self
>> approval trailers will dilute the meaning of the trailers somewhat.  I
>> think it is useful for approval to always mean someone else besides the
>> committer approved the change so that self-approved commits do stand out.
> 
> This makes sense, I hadn't considered that self approval wouldn't be as obvious, you make a good point. That and the fact that exceptions would already be needed because of the obvious fix rule, I think I'll remove this sentence from the update.
> 
> The new version will look like this:
> 
>    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.  The trailers (or tags)
>    currently in use are:
> 
>>
>>>> (Also, s/trailers/trailer/ in that sentence)
>>>>
>>>> Also, there is some inconsistency with "by" vs "By" in this change.
>>>> For example,
>>>> this paragraph uses 'Approved-By' but the description below uses
>>>> 'Approved-by'.
>>>> The manpages for git(1) (e.g. git-interpret-trailer(1)) tend to only
>>>> capitalize
>>>> the first word, e.g. 'Signed-off-by'.
>>>>
>>>> Our existing log messages are a bit all over the place currently, e.g.
>>>> for
>>>> Co-authored-by we have the following in master to date:
>>>>
>>>>   127 Co-Authored-By:
>>>>     9 Co-Authored-by:
>>>>    35 Co-authored-by:
>>>>
>>>> For both Approved-by and Reviewed-by the uppercase 'By' variants
>>>> outnumber
>>>> lowercase 'by' by a fair margin.
>>>
>>> This would seem to imply that we want to standardize capitalizing all
>>> words, right? It's different to the manual, but is the most common in
>>> our logs
>>
>> I will defer to the consensus of other folks here on which one to pick.
>> I just think there's value in picking one and then being consistent. :)
> That's fair. I have standardized on capitalizing all words, but I'll wait for other opinions before pushing.
> 

I'm happy with John's suggestions for what's worth.