[v5,1/1] : 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.

In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed
that Acked-by is already in use to represent partial approvals for
projects like the Linux Kernel and QEMU, so it makes sense to use it
similarly on this project.

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

Reviewed-by: Kevin Buettner <kevinb@redhat.com>
---
 gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)
  

On 10/5/23 07:35, Guinevere Larsen via Gdb-patches 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.
> 
> In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed
> that Acked-by is already in use to represent partial approvals for
> projects like the Linux Kernel and QEMU, so it makes sense to use it
> similarly on this project.
> 
> Finally, for completeness sake, the trailers Co-Authored-By and Bug
> were added, even though they have been in use for some time already
> 
> Reviewed-by: Kevin Buettner <kevinb@redhat.com>
> ---
>  gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
> index 9989956065e..e8243005531 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:
> +
> + - 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>"
> +
> + - Acked-By:
> +
> +   Used by a responsible or global maintainer when the patch touches multiple
> +   areas of GDB, and the maintainer in question is only approving some of
> +   those areas.  When using this tag, add the area(s) at the end of the text.
> +   This tag is also often described as "partial approval"
> +   Usage: "Acked-By: Your Name <your@email> (area)"
> +
> + - 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.
> +   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>"

An extremely nitty nit: maybe I'm overthinking this, but in the examples
that use emails, the < > characters are meant to appear in the real
trailer, around the email address.  For the bug link, you used them as a
placeholder, and they are not meant to appear on the actual Bug: line.
Maybe something like this?

  Usage: "Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=..."

I don't know, either way your change looks good to me.  However, I was
not really part of those discussions, so I'd rather let someone else
approve the change, if possible.

Simon
  

> From: Guinevere Larsen <blarsen@redhat.com>
> Cc: eliz@gnu.org,
> 	Guinevere Larsen <blarsen@redhat.com>,
> 	Kevin Buettner <kevinb@redhat.com>
> Date: Thu,  5 Oct 2023 13:35:34 +0200
> 
> + - Acked-By:
> +
> +   Used by a responsible or global maintainer when the patch touches multiple
> +   areas of GDB, and the maintainer in question is only approving some of
> +   those areas.  When using this tag, add the area(s) at the end of the text.
> +   This tag is also often described as "partial approval"
> +   Usage: "Acked-By: Your Name <your@email> (area)"

What are the possible "area"s?  And how to indicate more than a single
area?

And a more general question: when the review comments are minor, we
are used to say something like "okay with those nits fixed", meaning
that there's no need for posting another version of the patch before
committing it "with those nits fixed".  Is this an
Acked-By/Approved-By or Reviewed-By?

Thanks.
  

On Thu,  5 Oct 2023 13:35:34 +0200
Guinevere Larsen <blarsen@redhat.com> wrote:

> In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed
> that Acked-by is already in use to represent partial approvals for
> projects like the Linux Kernel and QEMU, so it makes sense to use it
> similarly on this project.
[...]
> + - Acked-By:
> +
> +   Used by a responsible or global maintainer when the patch touches multiple
> +   areas of GDB, and the maintainer in question is only approving some of
> +   those areas.  When using this tag, add the area(s) at the end of the text.
> +   This tag is also often described as "partial approval"
> +   Usage: "Acked-By: Your Name <your@email> (area)"

To indicate partial approval or partial review or even partial testing,
I recommend that we simply add "(area)" after the tag.

E.g.  if a reviewer has partially reviewed a patch and finds the parts
that change the gdbserver portions of it to be acceptable, they might
say:

Reviewed-by: Random J Developer <random@developer.example.org> (gdbserver)

The same could be done with the "Approved-by" tag.  If this is done,
then multiple partial approvals may be needed.

I think that an "Acked-by" tag could be useful when someone else has
done a detailed analysis and an area or global maintainer wishes to
indicate that they've read or skimmed the review, but have not done
any detailed analysis themselves.  It might also be used to indicate
agreement with direction of the patch after reading the commit log. 
But I don't think that it should be considered as strong as
"Reviewed-by" and definitely not as strong as "Approved-by".

I reached those conclusions after looking at how two other projects,
the git and kernel projects, use "Acked-by" and "Reviewed-by".  The
links that I looked at are:

https://git-scm.com/docs/SubmittingPatches
https://docs.kernel.org/process/submitting-patches.html

The git project has this to say about "Acked-by":

    Acked-by: says that the person who is more familiar with the area
    the patch attempts to modify liked the patch.

For the git project, "Reviewed-by" is much stronger:

    Reviewed-by: unlike the other tags, can only be offered by the
    reviewers themselves when they are completely satisfied with the
    patch after a detailed analysis.

Here's how the kernel project uses Acked-by:

    Aked-by:  is often used by the maintainer of the affected code
    when that maintainer neither contributed to nor forwarded the
    patch.

    Acked-by:  is not as formal as Signed-off-by:.  It is a record
    that the acker has at least reviewed the patch and has indicated
    acceptance.  Hence patch mergers will sometimes manually convert
    an acker's "yep, looks good to me" into an Acked-by:  (but note
    that it is usually better to ask for an explicit ack).

    Acked-by:  does not necessarily indicate acknowledgement of the
    entire patch.  For example, if a patch affects multiple subsystems
    and has an Acked-by:  from one subsystem maintainer then this
    usually indicates acknowledgement of just the part which affects
    that maintainer's code.  Judgement should be used here.  When in
    doubt people should refer to the original discussion in the
    mailing list archives.

However, for the kernel project, "Acked-by" is definitely not as
strong as "Reviewed-by":

    Reviewed-by:, instead, indicates that the patch has been reviewed
    and found acceptable according to the Reviewer's Statement:

    Reviewer's statement of oversight

    By offering my Reviewed-by:  tag, I state that:

	a. I have carried out a technical review of this patch to
	   evaluate its appropriateness and readiness for inclusion
	   into the mainline kernel.

	b. Any problems, concerns, or questions relating to the patch
	   have been communicated back to the submitter.  I am
	   satisfied with the submitter's response to my comments.

	c. While there may be things that could be improved with this
	   submission, I believe that it is, at this time, (1) a
	   worthwhile modification to the kernel, and (2) free of
	   known issues which would argue against its inclusion.

	d. While I have reviewed the patch and believe it to be
	   sound, I do not (unless explicitly stated elsewhere) make
	   any warranties or guarantees that it will achieve its
	   stated purpose or function properly in any given situation.

    A Reviewed-by tag is a statement of opinion that the patch is an
    appropriate modification of the kernel without any remaining
    serious technical issues.  Any interested reviewer (who has done
    the work) can offer a Reviewed-by tag for a patch.  This tag
    serves to give credit to reviewers and to inform maintainers of
    the degree of review which has been done on the patch. 
    Reviewed-by:  tags, when supplied by reviewers known to understand
    the subject area and to perform thorough reviews, will normally
    increase the likelihood of your patch getting into the kernel.

For the GDB project, I have been thinking of "Approved-by" as meaning
"Reviewed-by"-with-approval.  But I don't think that should
necessarily be the case.  I think it makes sense for a global
maintainer to see that there are "Reviewed-by" and possibly "Acked-by"
tags and then make a decision to approve the patch (giving it an
"Approved-by" tag) without necessarily having done a detailed analysis
(i.e.  review) themselves.

Kevin
  

On 05/10/2023 18:01, Eli Zaretskii wrote:
>> From: Guinevere Larsen <blarsen@redhat.com>
>> Cc: eliz@gnu.org,
>> 	Guinevere Larsen <blarsen@redhat.com>,
>> 	Kevin Buettner <kevinb@redhat.com>
>> Date: Thu,  5 Oct 2023 13:35:34 +0200
>>
>> + - Acked-By:
>> +
>> +   Used by a responsible or global maintainer when the patch touches multiple
>> +   areas of GDB, and the maintainer in question is only approving some of
>> +   those areas.  When using this tag, add the area(s) at the end of the text.
>> +   This tag is also often described as "partial approval"
>> +   Usage: "Acked-By: Your Name <your@email> (area)"
> What are the possible "area"s?  And how to indicate more than a single
> area?
"area of GDB" is used throughout the maintainers file without being 
specified anywhere, such as when explaining "Authorized Comitters":
   - The Authorized Committers.

     These are developers who are trusted to make changes within a specific
     area of GDB without additional oversight.

And all throughout the "Responsible maintainers" section. So I think it 
is understood well enough, especially since it is expected that the 
reviewers are the ones who should know if the patch touches multiple 
areas and so on.

As for how to indicate more than one area in the tag itself, I thought 
of a comma separated list. I'll update the example to say the following:

     Usage: "Acked-By: Your Name <your@email> (area1[, area2, ...])"

>
> And a more general question: when the review comments are minor, we
> are used to say something like "okay with those nits fixed", meaning
> that there's no need for posting another version of the patch before
> committing it "with those nits fixed".  Is this an
> Acked-By/Approved-By or Reviewed-By?

The tags are just ways to make your intent obvious. If you were 
approving the patch "with the nits fixed", you use Approved-By, if you 
were approving only parts of the patch, use Acked-by, and if you think 
the patch is ok but can't/won't approve, use Reviewed-By.
  

> Date: Fri, 6 Oct 2023 09:39:01 +0200
> Cc: gdb-patches@sourceware.org, kevinb@redhat.com
> From: Guinevere Larsen <blarsen@redhat.com>
> 
> On 05/10/2023 18:01, Eli Zaretskii wrote:
> >> +   This tag is also often described as "partial approval"
> >> +   Usage: "Acked-By: Your Name <your@email> (area)"
> > What are the possible "area"s?  And how to indicate more than a single
> > area?
> "area of GDB" is used throughout the maintainers file without being 
> specified anywhere, such as when explaining "Authorized Comitters":
>    - The Authorized Committers.
> 
>      These are developers who are trusted to make changes within a specific
>      area of GDB without additional oversight.
> 
> And all throughout the "Responsible maintainers" section. So I think it 
> is understood well enough, especially since it is expected that the 
> reviewers are the ones who should know if the patch touches multiple 
> areas and so on.

Fine by me, but then I think the text describing the tags should say
explicitly that the areas are those mentioned elsewhere in the
document.

> > And a more general question: when the review comments are minor, we
> > are used to say something like "okay with those nits fixed", meaning
> > that there's no need for posting another version of the patch before
> > committing it "with those nits fixed".  Is this an
> > Acked-By/Approved-By or Reviewed-By?
> 
> The tags are just ways to make your intent obvious. If you were 
> approving the patch "with the nits fixed", you use Approved-By, if you 
> were approving only parts of the patch, use Acked-by, and if you think 
> the patch is ok but can't/won't approve, use Reviewed-By.

I think this should also be in the document explicitly.

Thanks.
  

Top posting to ensure that both Eli and Pedro check your response. It is 
opposite to what was discussed on Cauldron, and I would like this worked 
out before sending a new version.

On 05/10/2023 19:55, Kevin Buettner wrote:
> On Thu,  5 Oct 2023 13:35:34 +0200
> Guinevere Larsen <blarsen@redhat.com> wrote:
>
>> In the GDB BoF in 2023's GNU tools cauldron it was discussed and agreed
>> that Acked-by is already in use to represent partial approvals for
>> projects like the Linux Kernel and QEMU, so it makes sense to use it
>> similarly on this project.
> [...]
>> + - Acked-By:
>> +
>> +   Used by a responsible or global maintainer when the patch touches multiple
>> +   areas of GDB, and the maintainer in question is only approving some of
>> +   those areas.  When using this tag, add the area(s) at the end of the text.
>> +   This tag is also often described as "partial approval"
>> +   Usage: "Acked-By: Your Name <your@email> (area)"
> To indicate partial approval or partial review or even partial testing,
> I recommend that we simply add "(area)" after the tag.
>
> E.g.  if a reviewer has partially reviewed a patch and finds the parts
> that change the gdbserver portions of it to be acceptable, they might
> say:
>
> Reviewed-by: Random J Developer <random@developer.example.org> (gdbserver)
>
> The same could be done with the "Approved-by" tag.  If this is done,
> then multiple partial approvals may be needed.
>
> I think that an "Acked-by" tag could be useful when someone else has
> done a detailed analysis and an area or global maintainer wishes to
> indicate that they've read or skimmed the review, but have not done
> any detailed analysis themselves.  It might also be used to indicate
> agreement with direction of the patch after reading the commit log.
> But I don't think that it should be considered as strong as
> "Reviewed-by" and definitely not as strong as "Approved-by".
>
> I reached those conclusions after looking at how two other projects,
> the git and kernel projects, use "Acked-by" and "Reviewed-by".  The
> links that I looked at are:
>
> https://git-scm.com/docs/SubmittingPatches
> https://docs.kernel.org/process/submitting-patches.html
>
> The git project has this to say about "Acked-by":
>
>      Acked-by: says that the person who is more familiar with the area
>      the patch attempts to modify liked the patch.
>
> For the git project, "Reviewed-by" is much stronger:
>
>      Reviewed-by: unlike the other tags, can only be offered by the
>      reviewers themselves when they are completely satisfied with the
>      patch after a detailed analysis.
>
> Here's how the kernel project uses Acked-by:
>
>      Aked-by:  is often used by the maintainer of the affected code
>      when that maintainer neither contributed to nor forwarded the
>      patch.
>
>      Acked-by:  is not as formal as Signed-off-by:.  It is a record
>      that the acker has at least reviewed the patch and has indicated
>      acceptance.  Hence patch mergers will sometimes manually convert
>      an acker's "yep, looks good to me" into an Acked-by:  (but note
>      that it is usually better to ask for an explicit ack).
>
>      Acked-by:  does not necessarily indicate acknowledgement of the
>      entire patch.  For example, if a patch affects multiple subsystems
>      and has an Acked-by:  from one subsystem maintainer then this
>      usually indicates acknowledgement of just the part which affects
>      that maintainer's code.  Judgement should be used here.  When in
>      doubt people should refer to the original discussion in the
>      mailing list archives.
>
> However, for the kernel project, "Acked-by" is definitely not as
> strong as "Reviewed-by":
>
>      Reviewed-by:, instead, indicates that the patch has been reviewed
>      and found acceptable according to the Reviewer's Statement:
>
>      Reviewer's statement of oversight
>
>      By offering my Reviewed-by:  tag, I state that:
>
> 	a. I have carried out a technical review of this patch to
> 	   evaluate its appropriateness and readiness for inclusion
> 	   into the mainline kernel.
>
> 	b. Any problems, concerns, or questions relating to the patch
> 	   have been communicated back to the submitter.  I am
> 	   satisfied with the submitter's response to my comments.
>
> 	c. While there may be things that could be improved with this
> 	   submission, I believe that it is, at this time, (1) a
> 	   worthwhile modification to the kernel, and (2) free of
> 	   known issues which would argue against its inclusion.
>
> 	d. While I have reviewed the patch and believe it to be
> 	   sound, I do not (unless explicitly stated elsewhere) make
> 	   any warranties or guarantees that it will achieve its
> 	   stated purpose or function properly in any given situation.
>
>      A Reviewed-by tag is a statement of opinion that the patch is an
>      appropriate modification of the kernel without any remaining
>      serious technical issues.  Any interested reviewer (who has done
>      the work) can offer a Reviewed-by tag for a patch.  This tag
>      serves to give credit to reviewers and to inform maintainers of
>      the degree of review which has been done on the patch.
>      Reviewed-by:  tags, when supplied by reviewers known to understand
>      the subject area and to perform thorough reviews, will normally
>      increase the likelihood of your patch getting into the kernel.

I understand what you mean, and I see where I may have misunderstood 
when I said that "Acked-By" worked as partial review in QEMU.

I'm fine with this being the default if everyone agrees with it. To 
summarize informally, the tags would be like this:

* Acked-By: A maintainer of a certain area looked at the patch 
description and is fine with its direction. this says nothing about the 
quality of the code. May be restricted to some areas of the code
* Reviewed-By: A contributor has looked at the code and thinks it is 
good, but is not approving it for any reason. May be restricted to some 
areas of the code.
* Approved-By: A maintainer has looked at the code and thinks that it is 
ready for upstreaming. May be restricted to some areas, and may be 
conditional on receiving a review or ack for some area(s).

> For the GDB project, I have been thinking of "Approved-by" as meaning
> "Reviewed-by"-with-approval.  But I don't think that should
> necessarily be the case.  I think it makes sense for a global
> maintainer to see that there are "Reviewed-by" and possibly "Acked-by"
> tags and then make a decision to approve the patch (giving it an
> "Approved-by" tag) without necessarily having done a detailed analysis
> (i.e.  review) themselves.
>
This makes sense, but it feels more like some informal thing to be 
decided between maintainers.
  

On 10/9/23 05:59, Guinevere Larsen wrote:
> I understand what you mean, and I see where I may have misunderstood when I said that "Acked-By" worked as partial review in QEMU.
> 
> I'm fine with this being the default if everyone agrees with it. To summarize informally, the tags would be like this:
> 
> * Acked-By: A maintainer of a certain area looked at the patch description and is fine with its direction. this says nothing about the quality of the code. May be restricted to some areas of the code
> * Reviewed-By: A contributor has looked at the code and thinks it is good, but is not approving it for any reason. May be restricted to some areas of the code.
> * Approved-By: A maintainer has looked at the code and thinks that it is ready for upstreaming. May be restricted to some areas, and may be conditional on receiving a review or ack for some area(s).

I like this use of Acked-By, it matches the meaning I thought it had,
based on what other projects do.  It happens that we say something like
"I gave it a quick look and it looks fine to me", indicating that we did
not do a thorough review, in which case Acked-By is appropriate.

Simon
  

On Tue, Oct 10, 2023 at 11:14:26AM -0400, Simon Marchi wrote:
> On 10/9/23 05:59, Guinevere Larsen wrote:
> > I understand what you mean, and I see where I may have misunderstood when I said that "Acked-By" worked as partial review in QEMU.
> > 
> > I'm fine with this being the default if everyone agrees with it. To summarize informally, the tags would be like this:
> > 
> > * Acked-By: A maintainer of a certain area looked at the patch description and is fine with its direction. this says nothing about the quality of the code. May be restricted to some areas of the code
> > * Reviewed-By: A contributor has looked at the code and thinks it is good, but is not approving it for any reason. May be restricted to some areas of the code.
> > * Approved-By: A maintainer has looked at the code and thinks that it is ready for upstreaming. May be restricted to some areas, and may be conditional on receiving a review or ack for some area(s).
> 
> I like this use of Acked-By, it matches the meaning I thought it had,
> based on what other projects do.  It happens that we say something like
> "I gave it a quick look and it looks fine to me", indicating that we did
> not do a thorough review, in which case Acked-By is appropriate.
> 
> Simon

Hi,

For the record, I am fine with this direction.

Best,
Lancelot.