From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id C06CA3858C2F for ; Tue, 27 Sep 2022 20:50:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C06CA3858C2F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.93,350,1654588800"; d="scan'208";a="86522236" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 27 Sep 2022 12:50:15 -0800 IronPort-SDR: grb7iElWFztL1hHoJG8IYDMtcCy0U4GrRx0XaYKP/ptSYnSyFu+gFpmndEIdqqDfQf7fZlEjCZ HK1aTrbmotFY7pBm4rGmDTjJTIhMaqbNkgy6kNpzygzYtjI1ZFkE6COzJSaTOWuaJMLgT/sD6B 1b2LBy+64W7QFbPHOthykWKYgGKpmDTr2XpgMG/96UueD0pTm92kSRdFpecFU/UM71D4m2qBwI HkGdd4glYx4sFeHpHJq35Vk6te6JAGEmTZkYMwGdZzrMmdQWoajEO/xt2b7UpOaFsoyTfZ9XFS yY4= From: Thomas Schwinge To: Bruno Larsen CC: Subject: Re: Proposal: Add review tags to patch review workflow. In-Reply-To: References: User-Agent: Notmuch/0.29.1+93~g67ed7df (https://notmuchmail.org) Emacs/27.1 (i686-pc-linux-gnu) Date: Tue, 27 Sep 2022 22:50:05 +0200 Message-ID: <87v8p8sfsy.fsf@dirichlet.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: SVR-IES-MBX-07.mgc.mentorg.com (139.181.222.7) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! On 2022-09-21T13:04:47+0200, Bruno Larsen via Gdb wrot= e: > 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: '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 . 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=C3=BC=C3=9Fe 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: newcontributorcom > Subject: [PATCH] Fix GDB's behavior > > I fixed bug number PR/XXXXX by making GDB kick and scream instead of > failing silently. > > --- > > From: newreviewercom > 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 reviewercom> > > --- > > From: otherreviewercom > Subject: Re: [PATCH] Fix GDB's behavior > > The solution looks good, but I have some style nits, see below. > > --- > > From: newcontributorcom > 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 reviewercom> > > --- > > From: otherreviewercom > Subject: Re: [PATCHv2] Fix GDB's behavior > > Thanks, this looks good now! > > Reviewed-by: Other Contributor contributorcom> > > --- > > From: approvergdbcom > 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: newcontributorcom > 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: othermaintainercom > Subject: Re: [PATCHv3] Fix GDB's behavior > > Hi! This patch still looks good. > R-b: Other Maintainer maintainercom> > > --- > > From: approvergdbcom > Subject: Re: [PATCHv3] Fix GDB's behavior > > I like this solution much better, thank you! > Approved-by: Approver gdbcom> > > --- > > From: newcontributorcom > Subject: Re: Re: [PATCHv3] Fix GDB's behavior > > Thank you, I pushed it! > > -- > Cheers, > Bruno ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955