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

[Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>]

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.

-- 
Cheers,
Bruno

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