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

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

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
  

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

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
  

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

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
  

> 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