[v3,0/1] update MAINTAINERS file with git trailers

  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 (and second) 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.

Thoughts? Comments? Suggestions?

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

 gdb/MAINTAINERS | 72 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 8 deletions(-)
  

On Thu, 13 Jul 2023 12:56:51 +0200
Bruno Larsen <blarsen@redhat.com> wrote:

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

I don't think we need a tag for this.  Since we review and/or approve
patches via email, I think some additional text stating which portions
were reviewed or approved is sufficient.

Suppose I'm an area maintainer or a global maintainer who has confident
knowledge of a particular area.  I might then do something like this:

    For the mn10300 architecture portions:
    Approved-by: Kevin Buettner <kevinb@redhat.com>

Only the Approved-by tag would be added to the git trailer, but it's
clear to anyone involved in the approval process that I haven't
approved the patch in its entirety, only certain parts.  If I were to
review the rest of the patch, but not approve it, I see nothing wrong
with also saying:

    For everything else:
    Reviewed-by: Kevin Buettner <kevinb@redhat.com>

I also see nothing wrong with qualifying the 'Reviewed-by' or
'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
but we might also get more people involved with the review process,
which I think would be a good thing.

If we really want to include the portions reviewed in the trailer, then
I suggest extending the format of the trailer, perhaps like this:

    Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)

Kevin
  

> Date: Thu, 13 Jul 2023 14:50:47 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> Cc: gdb-patches@sourceware.org, pedro@palves.net, aburgess@redhat.com,
>  brobecker@adacore.com, simon.marchi@polymtl.ca, tom@tromey.com,
>  tdevries@suse.de, ulrich.weigand@de.ibm.com, eliz@gnu.org
> 
> On Thu, 13 Jul 2023 12:56:51 +0200
> Bruno Larsen <blarsen@redhat.com> wrote:
> 
> > 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.
> 
> I don't think we need a tag for this.  Since we review and/or approve
> patches via email, I think some additional text stating which portions
> were reviewed or approved is sufficient.
> 
> Suppose I'm an area maintainer or a global maintainer who has confident
> knowledge of a particular area.  I might then do something like this:
> 
>     For the mn10300 architecture portions:
>     Approved-by: Kevin Buettner <kevinb@redhat.com>
> 
> Only the Approved-by tag would be added to the git trailer, but it's
> clear to anyone involved in the approval process that I haven't
> approved the patch in its entirety, only certain parts.  If I were to
> review the rest of the patch, but not approve it, I see nothing wrong
> with also saying:
> 
>     For everything else:
>     Reviewed-by: Kevin Buettner <kevinb@redhat.com>
> 
> I also see nothing wrong with qualifying the 'Reviewed-by' or
> 'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
> but we might also get more people involved with the review process,
> which I think would be a good thing.
> 
> If we really want to include the portions reviewed in the trailer, then
> I suggest extending the format of the trailer, perhaps like this:
> 
>     Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)

The above will only work if everyone pays attention to those
qualifications.  Moreover, in Real Life, the response doesn't include
just two such lines, it in many cases includes more text, and those
qualifications can easily "drown" in that.

Also, we used to have a way of saying "Approved, if those few nits are
fixed", and the above either removes that possibility entirely, or
will make it harder to determine whether and which parts were
approved, and on what conditions.

I still don't understand why we need the "partial-approved" facility
that uses Approved-by.  How is it different from Reviewed-by? the
submitter still needs to figure out whether all the parts were okayed
or not, so the only aspect this changes is making it more complicated
for area maintainers to write these tags, because instead of just a
single Reviewed-by they need to choose among two tags.

So my vote is for reserving Approved-by only to the cases where the
entire patch is approved.  Alternatively, we could introduce an
additional tag, like Partially-approved-by or something.

I guess my point is that this should be simple and ideally include
only fixed text, not some free-form text that could lead to
misunderstandings and misinterpretations.
  

On 14/07/2023 07:50, Eli Zaretskii wrote:
>> Date: Thu, 13 Jul 2023 14:50:47 -0700
>> From: Kevin Buettner <kevinb@redhat.com>
>> Cc: gdb-patches@sourceware.org, pedro@palves.net, aburgess@redhat.com,
>>   brobecker@adacore.com, simon.marchi@polymtl.ca, tom@tromey.com,
>>   tdevries@suse.de, ulrich.weigand@de.ibm.com, eliz@gnu.org
>>
>> On Thu, 13 Jul 2023 12:56:51 +0200
>> Bruno Larsen <blarsen@redhat.com> wrote:
>>
>>> 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.
>> I don't think we need a tag for this.  Since we review and/or approve
>> patches via email, I think some additional text stating which portions
>> were reviewed or approved is sufficient.
>>
>> Suppose I'm an area maintainer or a global maintainer who has confident
>> knowledge of a particular area.  I might then do something like this:
>>
>>      For the mn10300 architecture portions:
>>      Approved-by: Kevin Buettner <kevinb@redhat.com>
>>
>> Only the Approved-by tag would be added to the git trailer, but it's
>> clear to anyone involved in the approval process that I haven't
>> approved the patch in its entirety, only certain parts.  If I were to
>> review the rest of the patch, but not approve it, I see nothing wrong
>> with also saying:
>>
>>      For everything else:
>>      Reviewed-by: Kevin Buettner <kevinb@redhat.com>
>>
>> I also see nothing wrong with qualifying the 'Reviewed-by' or
>> 'Acked-by' tags.  Yes, we might end up with a patchwork of reviews,
>> but we might also get more people involved with the review process,
>> which I think would be a good thing.
>>
>> If we really want to include the portions reviewed in the trailer, then
>> I suggest extending the format of the trailer, perhaps like this:
>>
>>      Approved-by: Kevin Buettner <kevinb@redhat.com> (mn10300 only)
> The above will only work if everyone pays attention to those
> qualifications.  Moreover, in Real Life, the response doesn't include
> just two such lines, it in many cases includes more text, and those
> qualifications can easily "drown" in that.
>
> Also, we used to have a way of saying "Approved, if those few nits are
> fixed", and the above either removes that possibility entirely, or
> will make it harder to determine whether and which parts were
> approved, and on what conditions.
This is not something that could ever be dealt with in automation, 
unless we also wrote the nits in machine friendly ways. If we want to 
continue using this style, we need to have context dependent tags. Maybe 
I can add a line about it in the file?
>
> I still don't understand why we need the "partial-approved" facility
> that uses Approved-by.  How is it different from Reviewed-by? the
> submitter still needs to figure out whether all the parts were okayed
> or not, so the only aspect this changes is making it more complicated
> for area maintainers to write these tags, because instead of just a
> single Reviewed-by they need to choose among two tags.

There are a few reasons I don't like using reviewed-by. Most 
importantly, if a patch touches 2 separate areas, both area maintainers 
might send an rb tag and the contributor will keep waiting for an ab tag 
that is unnecessary. Another reason is that it could confuse things 
further when someone who doesn't have any approval rights looks at a 
patch and says things look ok, they might assume this rb has more power 
than it has.

The difference between the ambiguous review and the ambiguous approval 
is that the former relies on knowing who has what level of authority, 
while the latter relies on interpreting the email context.

I do like Kevin's idea of extending the tag to be "Approved-By: name 
<email> [(areas)]", where (areas) is used if the approval is partial. I 
think this could be explained easily enough in the file to avoid 
confusion. It would not be general speech context, it would be explicit 
in the tag itself. Do you think that this enough to not make the partial 
approval be missed? If not, I'm fine with us introducing 
Partially-Approved-By.