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

[Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org>]

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