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

[Bruno Larsen <blarsen@redhat.com>]

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