* [PATCH v2 0/1] update MAINTAINERS file with git trailers @ 2023-06-12 10:01 Bruno Larsen 2023-06-12 10:01 ` [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS Bruno Larsen 0 siblings, 1 reply; 4+ messages in thread From: Bruno Larsen @ 2023-06-12 10:01 UTC (permalink / raw) To: gdb-patches Cc: pedro, aburgess, kevinb, brobecker, simon.marchi, tom, tdevries, ulrich.weigand, eliz, 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 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. There is a secondary question for me, which is to add a hard requirement for approval tags. If we add it, the sourceware repository could have a pre-receive hook (or update hook) that automatically reject pushes if they dont have an approval tag. This would be especially useful to avoid accidentally pushing the wrong commit, but is more annoying for global maintainers (need to approve their own patches) and we'd need to see what to do about the obvious fix rule. My first answer for that is adding a special "Approved-By: Obvious fix" or similar. Thoughts? Comments? Suggestions? Version 1: https://inbox.sourceware.org/gdb-patches/20230516143826.3431583-1-blarsen@redhat.com/ Bruno Larsen (1): [gdb]: add git trailer information on gdb/MAINTAINERS gdb/MAINTAINERS | 56 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 8 deletions(-) -- 2.40.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS 2023-06-12 10:01 [PATCH v2 0/1] update MAINTAINERS file with git trailers Bruno Larsen @ 2023-06-12 10:01 ` Bruno Larsen 2023-06-12 23:59 ` Kevin Buettner 0 siblings, 1 reply; 4+ messages in thread From: Bruno Larsen @ 2023-06-12 10:01 UTC (permalink / raw) To: gdb-patches Cc: pedro, aburgess, kevinb, brobecker, simon.marchi, tom, tdevries, ulrich.weigand, eliz, 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. --- gdb/MAINTAINERS | 56 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS index 175595d5f17..22f7ecbd471 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,51 @@ 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 thank the +contributors who take the time to review incoming patches, the GDB project +uses trailers 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. + + - Tested-by: + + Used when a contributor does not want to comment on the quality + of the code in the patch, but has tested and sees no regressions on their + hardware. + + - Reviewed-by: + + Used when a contributors have looked at code and agrees with the changes, + but either don't have the authority or don't feel comfortable approving + the patch (usually due to unfamiliarity with the relevant part of the code). + + - 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. + The Obvious Fix Rule -------------------- -- 2.40.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS 2023-06-12 10:01 ` [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS Bruno Larsen @ 2023-06-12 23:59 ` Kevin Buettner 2023-06-13 7:43 ` Bruno Larsen 0 siblings, 1 reply; 4+ messages in thread From: Kevin Buettner @ 2023-06-12 23:59 UTC (permalink / raw) To: Bruno Larsen Cc: gdb-patches, pedro, aburgess, brobecker, simon.marchi, tom, tdevries, ulrich.weigand, eliz Hi, Thanks for taking this on. See my thoughts inline below... On Mon, 12 Jun 2023 12:01:40 +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 If you wish to document *all* git trailers, then 'Bug:' and 'Co-Authored-By:' should be documented too. > can be easily double-checked. > > The upstream discussion also brought up the use of Acked-by, which is > better defined in this commit. > --- > gdb/MAINTAINERS | 56 ++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 48 insertions(+), 8 deletions(-) > > diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS > index 175595d5f17..22f7ecbd471 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,51 @@ 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 In the line above, you've replaced the word "Review" with "Approval". I think I like "Review" better. (But there may be an even better word or phrase to use here.) > +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 thank the > +contributors who take the time to review incoming patches, the GDB project > +uses trailers 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. > + > + - Tested-by: > + > + Used when a contributor does not want to comment on the quality > + of the code in the patch, but has tested and sees no regressions on their > + hardware. For "Tested-by", I suggest this instead: 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. > + > + - Reviewed-by: > + > + Used when a contributors have looked at code and agrees with the changes, s/contributors have/contributor has/ s/code/the code/ > + but either don't have the authority or don't feel comfortable approving s/don't/doesn't/ ?? > + the patch (usually due to unfamiliarity with the relevant part of the code). I think I'd remove the parenthetical remark. There could be a variety of reasons for feeling uncomfortable with outright approving the change. One of the reasons that I sometimes don't approve a patch with which I agree is that I want to give other maintainers a chance to look at it first, especially when I know that they were involved with review of earlier iterations of the patch. I sometimes ask that a contributor wait a few days or a week for others to comment prior to pushing it. I'm not sure how that would work when using these tags. Could I give conditional 'Approved-by:' tag? Maybe something like this?: Approved-by: (name & email here) (But please wait a week for others to comment first. If there are any objections or concerns, please address those first.) > + > + - 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. > + In the above paragraph (and perhaps in a few other placess too), it seems that the formatting needs some adjustment. (Some lines are quite short while others seem a bit long.) > > The Obvious Fix Rule > -------------------- > -- > 2.40.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS 2023-06-12 23:59 ` Kevin Buettner @ 2023-06-13 7:43 ` Bruno Larsen 0 siblings, 0 replies; 4+ messages in thread From: Bruno Larsen @ 2023-06-13 7:43 UTC (permalink / raw) To: Kevin Buettner Cc: gdb-patches, pedro, aburgess, brobecker, simon.marchi, tom, tdevries, ulrich.weigand, eliz On 13/06/2023 01:59, Kevin Buettner wrote: > Hi, > > Thanks for taking this on. See my thoughts inline below... > > On Mon, 12 Jun 2023 12:01:40 +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 > If you wish to document *all* git trailers, then 'Bug:' and > 'Co-Authored-By:' should be documented too. Good point, I will add them to v3. I will just wait a bit for other people's input before sending a v3. >> can be easily double-checked. >> >> The upstream discussion also brought up the use of Acked-by, which is >> better defined in this commit. >> --- >> gdb/MAINTAINERS | 56 ++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 48 insertions(+), 8 deletions(-) >> >> diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS >> index 175595d5f17..22f7ecbd471 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,51 @@ 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 > In the line above, you've replaced the word "Review" with "Approval". > I think I like "Review" better. (But there may be an even better word > or phrase to use here.) I replaced it on purpose, because the full original wording is "Review is a privilege and/or responsibility (...) [of] GDB maintainers." This seems to imply that a new contributor may not review a patch if they aren't a maintainer yet, when the feeling I had of the patch and community as a whole was that anyone can review, what is a privilege that needs to be earned is approving patches. If you think my reasoning is wrong, do let me know! > >> +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 thank the >> +contributors who take the time to review incoming patches, the GDB project >> +uses trailers 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. >> + >> + - Tested-by: >> + >> + Used when a contributor does not want to comment on the quality >> + of the code in the patch, but has tested and sees no regressions on their >> + hardware. > For "Tested-by", I suggest this instead: > > 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. I like this better! I will use this on v3. > >> + >> + - Reviewed-by: >> + >> + Used when a contributors have looked at code and agrees with the changes, > s/contributors have/contributor has/ > s/code/the code/ oops, sorry, will change. > >> + but either don't have the authority or don't feel comfortable approving > s/don't/doesn't/ ?? ah, yes, leftover from a previous draft. > >> + the patch (usually due to unfamiliarity with the relevant part of the code). > I think I'd remove the parenthetical remark. There could be a variety of > reasons for feeling uncomfortable with outright approving the change. > One of the reasons that I sometimes don't approve a patch with which I > agree is that I want to give other maintainers a chance to look at it > first, especially when I know that they were involved with review of > earlier iterations of the patch. That's fair. I guess I don't need to give hints to the maintainers themselves here. > > I sometimes ask that a contributor wait a few days or a week for > others to comment prior to pushing it. I'm not sure how that would work > when using these tags. Could I give conditional 'Approved-by:' tag? Maybe > something like this?: > > Approved-by: (name & email here) > (But please wait a week for others to comment first. If there are any > objections or concerns, please address those first.) This is how I would recommend you go about things, yes. With the rb tag, I would have to wait an unknown amount of time before pushing, until some other maintainer gives me the ab tag or until you decide to look at a patch you've already reviewed, which is extra work for you. Conditional approvals, on the other hand, are already how we deal with partial approval. >> + >> + - 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. >> + > In the above paragraph (and perhaps in a few other placess too), it > seems that the formatting needs some adjustment. (Some lines are > quite short while others seem a bit long.) Ah... the wonders of multiple drafts in an editor that doesn't format automatically, hahaha. Thanks for pointing this out! -- Cheers, Bruno > >> >> The Obvious Fix Rule >> -------------------- >> -- >> 2.40.1 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-13 7:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-12 10:01 [PATCH v2 0/1] update MAINTAINERS file with git trailers Bruno Larsen 2023-06-12 10:01 ` [PATCH v2 1/1] [gdb]: add git trailer information on gdb/MAINTAINERS Bruno Larsen 2023-06-12 23:59 ` Kevin Buettner 2023-06-13 7:43 ` Bruno Larsen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).