From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 7912D3858C53 for ; Sun, 25 Sep 2022 22:38:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7912D3858C53 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 3D2858A511; Sun, 25 Sep 2022 22:38:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1664145523; bh=O0g3pMkn2PJsF66OsMwJRsk8v/jE3zKbhAgogmruWVU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rJwFd6Abmf+8h+QZ08ifJ3bXrx/nT9QKEuUiWabBj3ZoXC6LMbV4G+esbrqz+5geW QMjg68XZ40IV/Ba2fvXeFdjeeR1YUb2xMow+p7OQ19cG4MAmCDyuh6s4EnHr7o5TCh RGBqjCi79Zr18qNyfsi2/PXxUehNpKd276vHSrrCqFgWahLRn+zCQ5hNgydOv0PXHm HRluT1AnslJ7NxqNW2UkjTvsxObkJAYSP9VX9txfUY9b74i9i2ANMOZOl8OGhe1m6j Yqbg9r3BNQahoZ+62JUEgVh+ILYYt4lG7o7cAM+Oa0RgR+84nFdN+plQFMH6cSyOsZ iqNiiC4K7vm+g== Date: Sun, 25 Sep 2022 22:38:38 +0000 From: Lancelot SIX To: Bruno Larsen Cc: gdb@sourceware.org Subject: Re: Proposal: Add review tags to patch review workflow. Message-ID: <20220925223838.hs5pjdkjoavtsnaq@ubuntu.lan> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Sun, 25 Sep 2022 22:38:43 +0000 (UTC) X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham 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 Bruno Thanks for putting this together. As far as I am concerned, I find this reasonable, and am willing to give it a try. Also, considering that the patchwork got a fresh start, I am curious to see if this workflow can accommodate GDB. Of course, I am no maintainer so I leave the decision to their wisdom. To comment on your motivations, as a still relatively new contributor and occasional reviewer, I know that I tend to stay away from any language that might imply that I give any sort of approval to patches. This is not for me to do, and unless I am sure the patch’s author is familiar with who can approve patches, I want to avoid any possible confusion. As a consequence, I only comment on patches if I find issues or have questions, not when I find a patch OK to me. The corollary is that from the outside a patch I have not read, a patch I skimmed through or a patch I spend time testing and am convinced is good all look the same. This give little help to maintainers in their task. The system you propose to adopt could help improve on this. Best, Lancelot. On Wed, Sep 21, 2022 at 01:04:47PM +0200, Bruno Larsen via Gdb 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. > > * 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 >