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

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

[Bruno Larsen via Gdb-patches]

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.

There was also an update to the contribution checklist[1], which was the
only way the trailers were officially documented. I'm not sure if that
addition was properly reviewed, so more eyes are welcome, especially
after this change goes through.

Finally, I also have some suggested git commands to help with managing
the git trailers that I'd like to add to a relevant place, where would I
add it? For those curious, it's a git alias[2] to add a trailer to the
most recent commit.

[1] https://sourceware.org/gdb/wiki/ContributionChecklist#Receiving_positive_reviews
[2] gdb-trailer = commit --amend --no-edit --trailer

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

 gdb/MAINTAINERS | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

-- 
2.40.1

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

[Bruno Larsen via Gdb-patches]

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.
---
 gdb/MAINTAINERS | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/gdb/MAINTAINERS b/gdb/MAINTAINERS
index 175595d5f17..ba4fac2f0d1 100644
--- a/gdb/MAINTAINERS
+++ b/gdb/MAINTAINERS
@@ -43,15 +43,6 @@ 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!
-
 There's also a couple of other people who play special roles in the GDB
 community, separately from the patch process:
 
@@ -78,6 +69,45 @@ 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.  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!
+
+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 git 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:
+
+ - 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 contributor has looked at code and agrees with the changes,
+   but either does not have the authority or doesn't feel comfortable
+   approving the patch (usually due to unfamiliarity with a certain
+   part of the code).
+
+ - Approved-by:
+
+   Used by responsible mainainers 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

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

[Eli Zaretskii via Gdb-patches]

> Cc: gdb@sourceware.org,
> 	Bruno Larsen <blarsen@redhat.com>
> Date: Tue, 16 May 2023 16:38:27 +0200
> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
> 
> + - 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 contributor has looked at code and agrees with the changes,
> +   but either does not have the authority or doesn't feel comfortable
> +   approving the patch (usually due to unfamiliarity with a certain
> +   part of the code).

Reviewed-by is used by responsible maintainers as well.

> +
> + - Approved-by:
> +
> +   Used by responsible mainainers or global maintainers when
                          ^^^^^^^^^^
Typo.

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

I think the above list is incomplete, because there appears to be no
"git trailer" (why do we have to call it "git" trailer, btw?  will
that change if we ever switch to a different VCS?) for the situation
where the responsible maintainer does approve some part of the patch,
but not all of it (e.g., because the other parts are not in the
expertise domain of that maintainer).  I thought Reviewed-by is such a
trailer, but based on the above I'm beginning to think I was confused.

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

[Bruno Larsen via Gdb-patches]

On 16/05/2023 18:04, Eli Zaretskii wrote:
>> Cc: gdb@sourceware.org,
>> 	Bruno Larsen <blarsen@redhat.com>
>> Date: Tue, 16 May 2023 16:38:27 +0200
>> From: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
>>
>> + - 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 contributor has looked at code and agrees with the changes,
>> +   but either does not have the authority or doesn't feel comfortable
>> +   approving the patch (usually due to unfamiliarity with a certain
>> +   part of the code).
> Reviewed-by is used by responsible maintainers as well.
I think I need clearer wording then. I tried using "contributor" to mean 
anyone who decided to look at a patch, so it would include responsible 
maintainers when they dont have authority (or desire) to approve a 
patch. How do you think it could be rephrased to make it more clear?
>
>> +
>> + - Approved-by:
>> +
>> +   Used by responsible mainainers or global maintainers when
>                            ^^^^^^^^^^
> Typo.
oops sorry
>
>> +   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.
>> +
> I think the above list is incomplete, because there appears to be no
> "git trailer" (why do we have to call it "git" trailer, btw?  will
> that change if we ever switch to a different VCS?) for the situation
> where the responsible maintainer does approve some part of the patch,
> but not all of it (e.g., because the other parts are not in the
> expertise domain of that maintainer).  I thought Reviewed-by is such a
> trailer, but based on the above I'm beginning to think I was confused.
>
I wrote the proposal based on how I think the use of trailers works on 
the QEMU project (I wasn't in it long enough to be sure that I am 
correct, though). My thinking was that you'd send something like 
"documentation changes are approved, but someone needs to look at the 
code, Approved-By ..." or something similar. That said, I just 
remembered that they also use Ack-By in those situations and the 
maintainer of the subsystem most affected by a change is the only one to 
approve the patch, and other relevant maintainers use Ack-By (they have 
a very different development workflow, with each subsystem maintainer 
having their own tree and them only being merged into the master tree 
periodically). I'm pretty open to suggestions, if you think using 
Acked-By or some other trailer is better. That is the reason I'm doing 
this :-)

As to why call them "git trailers", I copied over from the checklist[1], 
that was already there describing Co-Authored-By and Bug trailers. We 
can change the name to just trailers or tags or something else if you 
prefer.

[1] 
https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_formatted_commit_messages

-- 
Cheers,
Bruno

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

[Eli Zaretskii via Gdb-patches]

> Date: Tue, 16 May 2023 18:41:55 +0200
> Cc: gdb-patches@sourceware.org, gdb@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
> 
> On 16/05/2023 18:04, Eli Zaretskii wrote:
> >> +   Used when a contributor has looked at code and agrees with the changes,
> >> +   but either does not have the authority or doesn't feel comfortable
> >> +   approving the patch (usually due to unfamiliarity with a certain
> >> +   part of the code).
> > Reviewed-by is used by responsible maintainers as well.
> I think I need clearer wording then.

I think "both contributors and maintainers" is good enough.

> > I think the above list is incomplete, because there appears to be no
> > "git trailer" (why do we have to call it "git" trailer, btw?  will
> > that change if we ever switch to a different VCS?) for the situation
> > where the responsible maintainer does approve some part of the patch,
> > but not all of it (e.g., because the other parts are not in the
> > expertise domain of that maintainer).  I thought Reviewed-by is such a
> > trailer, but based on the above I'm beginning to think I was confused.
> >
> I wrote the proposal based on how I think the use of trailers works on 
> the QEMU project (I wasn't in it long enough to be sure that I am 
> correct, though). My thinking was that you'd send something like 
> "documentation changes are approved, but someone needs to look at the 
> code, Approved-By ..." or something similar. That said, I just 
> remembered that they also use Ack-By in those situations and the 
> maintainer of the subsystem most affected by a change is the only one to 
> approve the patch, and other relevant maintainers use Ack-By (they have 
> a very different development workflow, with each subsystem maintainer 
> having their own tree and them only being merged into the master tree 
> periodically). I'm pretty open to suggestions, if you think using 
> Acked-By or some other trailer is better. That is the reason I'm doing 
> this :-)

I don't think I'm in a position to put forward suggestions, since I'm
not sure I have a good understanding of the process.  I only use
Approved-By when I can approve the entire patch, not just parts of it.
But maybe I'm wrong in that.

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

[Simon Marchi via Gdb-patches]

On 5/16/23 13:48, Eli Zaretskii via Gdb-patches wrote:
>> Date: Tue, 16 May 2023 18:41:55 +0200
>> Cc: gdb-patches@sourceware.org, gdb@sourceware.org
>> From: Bruno Larsen <blarsen@redhat.com>
>>
>> On 16/05/2023 18:04, Eli Zaretskii wrote:
>>>> +   Used when a contributor has looked at code and agrees with the changes,
>>>> +   but either does not have the authority or doesn't feel comfortable
>>>> +   approving the patch (usually due to unfamiliarity with a certain
>>>> +   part of the code).
>>> Reviewed-by is used by responsible maintainers as well.
>> I think I need clearer wording then.
> 
> I think "both contributors and maintainers" is good enough.
> 
>>> I think the above list is incomplete, because there appears to be no
>>> "git trailer" (why do we have to call it "git" trailer, btw?  will
>>> that change if we ever switch to a different VCS?) for the situation
>>> where the responsible maintainer does approve some part of the patch,
>>> but not all of it (e.g., because the other parts are not in the
>>> expertise domain of that maintainer).  I thought Reviewed-by is such a
>>> trailer, but based on the above I'm beginning to think I was confused.
>>>
>> I wrote the proposal based on how I think the use of trailers works on 
>> the QEMU project (I wasn't in it long enough to be sure that I am 
>> correct, though). My thinking was that you'd send something like 
>> "documentation changes are approved, but someone needs to look at the 
>> code, Approved-By ..." or something similar. That said, I just 
>> remembered that they also use Ack-By in those situations and the 
>> maintainer of the subsystem most affected by a change is the only one to 
>> approve the patch, and other relevant maintainers use Ack-By (they have 
>> a very different development workflow, with each subsystem maintainer 
>> having their own tree and them only being merged into the master tree 
>> periodically). I'm pretty open to suggestions, if you think using 
>> Acked-By or some other trailer is better. That is the reason I'm doing 
>> this :-)
> 
> I don't think I'm in a position to put forward suggestions, since I'm
> not sure I have a good understanding of the process.  I only use
> Approved-By when I can approve the entire patch, not just parts of it.
> But maybe I'm wrong in that.

If this happens, I think it's fine to say "the documentation parts are
approved" and following with your Approved-By.  If you want to be
extra-clear, add "but the rest needs to be approved by someone else".
The patch will end up with multiple Approved-Bys.

Speaking of Acked-By, I felt the need to use it recently, where I just
read the commit message, agreed with it, but didn't have time to review
the code itself.  I wanted to show that I agreed with the intent of the
patch.  I think that's what Acked-By is for.  I think we could add it to
that list.

Simon

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

[Eli Zaretskii via Gdb-patches]

> Date: Tue, 16 May 2023 15:40:38 -0400
> Cc: gdb-patches@sourceware.org, gdb@sourceware.org
> From: Simon Marchi <simark@simark.ca>
> 
> > I don't think I'm in a position to put forward suggestions, since I'm
> > not sure I have a good understanding of the process.  I only use
> > Approved-By when I can approve the entire patch, not just parts of it.
> > But maybe I'm wrong in that.
> 
> If this happens, I think it's fine to say "the documentation parts are
> approved" and following with your Approved-By.  If you want to be
> extra-clear, add "but the rest needs to be approved by someone else".
> The patch will end up with multiple Approved-Bys.

I'd like to hear from more maintainers that this is how they see that
tag.  My fear is that someone mechanically scans the discussion thread
for the tags, in which case human-readable qualifications will go
unnoticed.

All in all, I feel that this aspect of our process is not well
defined.

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

[Bruno Larsen via Gdb-patches]

On 17/05/2023 04:28, Eli Zaretskii wrote:
>> Date: Tue, 16 May 2023 15:40:38 -0400
>> Cc: gdb-patches@sourceware.org, gdb@sourceware.org
>> From: Simon Marchi <simark@simark.ca>
>>
>>> I don't think I'm in a position to put forward suggestions, since I'm
>>> not sure I have a good understanding of the process.  I only use
>>> Approved-By when I can approve the entire patch, not just parts of it.
>>> But maybe I'm wrong in that.
You are affected by the process, you are absolutely in a position to put 
forward suggestions, especially for making it more straightforward!
>> If this happens, I think it's fine to say "the documentation parts are
>> approved" and following with your Approved-By.  If you want to be
>> extra-clear, add "but the rest needs to be approved by someone else".
>> The patch will end up with multiple Approved-Bys.
> I'd like to hear from more maintainers that this is how they see that
> tag.  My fear is that someone mechanically scans the discussion thread
> for the tags, in which case human-readable qualifications will go
> unnoticed.

That's a fair point. My fear with using rb for partial approval was that 
if a patch needed 2 responsible maintainers to approve it, it might get 
stalled because it never got  an ab tag.

We could have an extra tag like Partially-Approved-By (or 
Partial-Approval), but we would be the first project using it as far as 
I can see and that could make things confusing for people that already 
know the workflow.

>
> All in all, I feel that this aspect of our process is not well
> defined.
>

 > > Speaking of Acked-By, I felt the need to use it recently, where I just
 > > read the commit message, agreed with it, but didn't have time to review
 > > the code itself.  I wanted to show that I agreed with the intent of the
 > > patch.  I think that's what Acked-By is for.  I think we could add 
it to
 > > that list.

My reading of the kernel's documentation of the tag[1] makes it sound 
like it is a partial approval, especially the lines:

|

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

And the only time I saw it used in QEMU seems to corroborate that 
reading. That said, I can be misunderstanding here (english is not my 
native language after all).

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=HEAD#n441

-- 
Cheers,
Bruno

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

[Simon Marchi via Gdb-patches]

> My reading of the kernel's documentation of the tag[1] makes it sound like it is a partial approval, especially the lines:
> 
> |
> 
> 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.|
> 
> And the only time I saw it used in QEMU seems to corroborate that reading. That said, I can be misunderstanding here (english is not my native language after all).
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=HEAD#n441

Thanks for the pointers.  It looks like Acked-By can fit a range of
situations, as long as it fits in "I didn't look at all of it or didn't
do a thoroug review, but what I have seen LGTM".  I think it fits my use
case of just reading the commit message and agreeing with that.

Simon

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

[Bruno Larsen via Gdb-patches]

ping! :)

There has been some chat about this, but I'd like some more maintainers 
chiming in to ensure that everyone is following the same process. 
Sending an ack/LGTM if everything seems obvious is much appreciated :)

-- 
Cheers,
Bruno

On 16/05/2023 16:38, Bruno Larsen wrote:
> 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.
>
> There was also an update to the contribution checklist[1], which was the
> only way the trailers were officially documented. I'm not sure if that
> addition was properly reviewed, so more eyes are welcome, especially
> after this change goes through.
>
> Finally, I also have some suggested git commands to help with managing
> the git trailers that I'd like to add to a relevant place, where would I
> add it? For those curious, it's a git alias[2] to add a trailer to the
> most recent commit.
>
> [1] https://sourceware.org/gdb/wiki/ContributionChecklist#Receiving_positive_reviews
> [2] gdb-trailer = commit --amend --no-edit --trailer
>
> Bruno Larsen (1):
>    [gdb]: add git trailer information on gdb/MAINTAINERS
>
>   gdb/MAINTAINERS | 48 +++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 39 insertions(+), 9 deletions(-)
>