Re: Proposal: Add review tags to patch review workflow.

Hi!

On 2022-09-21T13:04:47+0200, Bruno Larsen via Gdb <[email protected]> wrote:
> TL;DR: I want to introduce the usage of 3 new review tags to the GDB
> patch review workflow. They are: Reviewed-by, Approved-by and Tested-by.

Nice!  (I haven't been contributing to GDB in many years, so my opinion
here only means so much.)


Five years ago, I had started a similar effort:
<https://inbox.sourceware.org/gdb/[email protected]>
'GNU Tools Cauldron 2017 follow up: "Reviewed-by" etc.'.

As that's where I spend most of my time, I then tried establishing that
in GCC land, see <https://gcc.gnu.org/wiki/Reviewed-by>.  This didn't
catch on back then, so I also again seem to have stopped in mid-2020.
(But, I'm planning to be here for a lot more years, and maybe
eventually...)  ;-)

This isn't meant to sound pessimistic or discouraging -- rather the
inverse: I'm happy to see that after glibc, where it's now an established
practice, another GNU Tools project, GDB, is considering this again.


(..., and I also do sympathize with those who are suggesting to use tools
more elaborate than email for doing reviews, but that seems even more far
out.  But we'll get there eventually!)  :-)


Grüße
 Thomas


> * Reviewed-by would be used by reviewers such as myself, who may look
> over the code but don't have the authority to approve it for pushing,
> essentially working as a +1.
>
> * Approved-by would be used by maintainers who have the authority to
> approve code for pushing when they want to do so. Referenced as a +2
> later in this email.
>
> * Tested-by would be used by new contributors or reviewers who aren't
> familiar with the code touched by a given patch, but who are able to
> verify that the test doesn't introduce regressions.
>
> I will now dive into motivation and specifics of the proposal.
>
> ----
>
> Motivation: As a new contributor, I see 4 main issues with the current
> patch review workflow:
>
> 1. It is sometimes hard to be sure that the reviewer has approved your
> patch, exemplified by a few recent patches which were already approved
> and were still pinged by their authors, such as
> https://sourceware.org/pipermail/gdb-patches/2022-August/191474.html
> 2. Patch review as it is now looks rather thankless. Sometimes a patch
> may go through various rounds of review, taking a lot of time from the
> contributor and reviewer, but if the latter never suggested enough code
> to warrant a Co-Authored-By tag, the only person mentioned at the final
> commit will be the original writer. I have personally wanted to thank
> reviewers who were especially helpful in understanding issues.
> 3. As a new contributor, it is not always obvious when a LGTM means the
> patch can be pushed. While one can always check the maintainers file, it
> would be nice if this information was baked into a review.
> 4. On the other hand, it's not always obvious to new reviewers
> (non-approvers) that their LGTM makes any difference, possibly
> discouraging them from giving positive feedback and making the community
> feel less lively than it is.
>
> Adding Reviewed-by (R-b) tags by itself would solve the first 2 issues,
> since it only is given once the review is finished and the patch is good
> to go from the reviewer's perspective. Approved-by (A-b) tags were
> mentioned during the GDB BoF at Cauldron 2022, as a way to fix issues 3
> and 4, along with allowing for maintainers to give only +1, instead of
> +2, when they are not sure about a certain change. Tested-by (T-b) were
> also mentioned at the BoF as a way to give another option for new
> reviewers, especially if they have different hardware or setups.
>
>
> The workflow: The basic usage is as simple as I explained above, if all
> a reviewer is comfortable with doing is confirming that the error is
> fixed and testing for regression, they can reply with a T-b tag; if they
> are comfortable sharing a positive opinion on the proposed code, but
> can't approve or are not sure if the patch should be approved, they can
> reply with an R-b tag; and if they wish to approve a patch, they reply
> with A-p.
>
> Questions arose when thinking of new versions of a given patch. The
> workflow used by Glibc, which inspired great part of this proposal,
> makes it so the tags are dropped only when a patch has been
> "sufficiently changed". Cosmetic things like fixing typos or moving a
> proposed function from one place to another would not invalidate R-b
> tags, while reworking the solution would. The submitter has some space
> to decide that they think the patch has changed enough to warrant a new
> review (and invalidate the previous R-b), or the reviewer may ask their
> tag to be removed if they disagree with a change.
>
> In case the explanation is not clear, the follow example shows how these
> would be used in a fictional patch that requires 3 versions before it is
> ready.
>
> ---
>
> From: new<at>contributor<dot>com
> Subject: [PATCH] Fix GDB's behavior
>
> I fixed bug number PR/XXXXX by making GDB kick and scream instead of
> failing silently.
>
> ---
>
> From: new<at>reviewer<dot>com
> Subject: Re: [PATCH] Fix GDB's behavior
>
> I'm not sure how good this solution is, but I verified that in my setup
> this doesn't regress anything and fixes the issue.
> Tested-by: New Reviewer <new<at>reviewer<dot>com>
>
> ---
>
> From: other<at>reviewer<dot>com
> Subject: Re: [PATCH] Fix GDB's behavior
>
> The solution looks good, but I have some style nits, see below.
>
> ---
>
> From: new<at>contributor<dot>com
> Subject: [PATCHv2] Fix GDB's behavior
>
> I fixed bug number PR/XXXXX by making GDB kick and scream instead of
> failing silently.
> Tested-by: New Reviewer <new<at>reviewer<dot>com>
>
> ---
>
> From: other<at>reviewer<dot>com
> Subject: Re: [PATCHv2] Fix GDB's behavior
>
> Thanks, this looks good now!
>
> Reviewed-by: Other Contributor <other<at>contributor<dot>com>
>
> ---
>
> From: approver<at>gdb<dot>com
> Subject: [PATCHv2] Fix GDB's behavior
>
> I don't think this is a good solution, because you haven't fixed the
> problem, just complained about it. Please ensure that the patch makes
> GDB behave correctly with this input, though I'm not opposed to keeping
> the warning for other unexpected errors.
>
> ---
>
> From: new<at>contributor<dot>com
> Subject: [PATCHv3] Fix GDB's behavior
>
> I fixed bug number PR/XXXXX by making GDB work as expected, and also
> added a warning for unexpected inputs.
>
> ---
>
> From: other<at>maintainer<dot>com
> Subject: Re: [PATCHv3] Fix GDB's behavior
>
> Hi! This patch still looks good.
> R-b: Other Maintainer <other<at>maintainer<dot>com>
>
> ---
>
> From: approver<at>gdb<dot>com
> Subject: Re: [PATCHv3] Fix GDB's behavior
>
> I like this solution much better, thank you!
> Approved-by: Approver <approver<at>gdb<dot>com>
>
> ---
>
> From: new<at>contributor<dot>com
> Subject: Re: Re: [PATCHv3] Fix GDB's behavior
>
> Thank you, I pushed it!
>
> --
> Cheers,
> Bruno
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955