From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A50D83858297 for ; Tue, 27 Sep 2022 08:30:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A50D83858297 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1664267442; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OE7bCW/oJvDOM+dX2HSRBwV+Jy1fej5fPYEznqpYXdY=; b=fdEdqiY3egLnHeFEsH3W9FV901SNttOyJL47lsrJc6e0LXBekACzHDC68hta5mhSQqxwb7 6gBWnj+R3fHx1Fsh39biFrAa0k2GwLuTX5d6t2TgklsbXClFWlX1YAOdbRIfY0rmdfurPN F66TsNh4T7pyMRCUuzyUTlXB5u0LNNo= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-490--5Arsmz7N8qX4moz8L0kvw-1; Tue, 27 Sep 2022 04:30:41 -0400 X-MC-Unique: -5Arsmz7N8qX4moz8L0kvw-1 Received: by mail-wr1-f70.google.com with SMTP id r22-20020adfa156000000b0022cc3018fbaso18881wrr.2 for ; Tue, 27 Sep 2022 01:30:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=OE7bCW/oJvDOM+dX2HSRBwV+Jy1fej5fPYEznqpYXdY=; b=ZGKaQRJ45jj23S6R7XbwZmXWavTHlncmMO3G1elB2LtjkoqJzQGCW2l1QGsdT2yIwi ReCDDikVL0m/rxvEg0MCaV6XkwZIfvev4ZWInM69ZaUC+PCrFvDlE8/02FRnz5hhS+aw +J5vcH9wM4yAyyncmHDw/gRyMsPzgBoIhwvTLTGpyCYS0PtdmiDuxpxS+M9QZx6/IBpC u2T8oZ2kfvh+i1rDvvuXa/EedMN+5vFm4Va1EEiRGESzhc3ibxDEkA2XD0II7uoSuuT/ To14l3MsLTG+w480oMQ+AbjoQCx/xO45lZK0zPY4zK7nTLoHHKMyMD9ss2KrLNE/Jmgz gFuQ== X-Gm-Message-State: ACrzQf0gunQJUq/wH1nF5wnzQpjfr2lhiUBblnHewsIFVkQ9UHTMJB8W q0P5vsfQNUmCSy1agYFTaZIZS7zhBoZMG0UCqxptCnHMWc2v9F5jesa08YOu1cTYjBHWgjdVI+F Gjv8Nucs3WZ0= X-Received: by 2002:a05:600c:154a:b0:3b4:c9ec:41df with SMTP id f10-20020a05600c154a00b003b4c9ec41dfmr1630770wmg.109.1664267439769; Tue, 27 Sep 2022 01:30:39 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6WnFNjZeIfDw7v7DheXmbq4Ft3Q+vmbcc6SL43ygk5FPmnyoSY3MYD0ZxA846E23OkefbYEQ== X-Received: by 2002:a05:600c:154a:b0:3b4:c9ec:41df with SMTP id f10-20020a05600c154a00b003b4c9ec41dfmr1630753wmg.109.1664267439454; Tue, 27 Sep 2022 01:30:39 -0700 (PDT) Received: from [192.168.0.45] (ip-62-245-66-121.bb.vodafone.cz. [62.245.66.121]) by smtp.gmail.com with ESMTPSA id z3-20020a5d6403000000b00228c792aaaasm1102105wru.100.2022.09.27.01.30.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Sep 2022 01:30:38 -0700 (PDT) Message-ID: Date: Tue, 27 Sep 2022 10:30:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: Proposal: Add review tags to patch review workflow. To: Elena Zannoni , Luis Machado , gdb@sourceware.org References: From: Bruno Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,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 Elena, Thanks for the link, I took a look at it. From what I could understand, the Signed-off-by tags solve a problem that we don't really have in GDB, as the kernel seems to have multiple layers of pushing between the submitter and it going upstream, whereas here we manually push the patch, so there is little need to track submissions so thoroughly. I am indifferent to reported-by and sugested-by tags, as we tend to link the bugs already in submissions. Also, I don't think this proposal should include those tags, simply because other contributor's opinions may change based on it, delaying acceptance of R-b, T-b and A-b tags. I will, however, use that text to inspire what I will write in the wiki about the suggested tags. Thanks a lot! Cheers, Bruno On 26/09/2022 18:32, Elena Zannoni wrote: > One thing to do possibly is to take a look at what the kernel does in > this area, with the tags for inspiration: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html > (middle of the page). > > The key-signed patch workflow is also worth looking at to implement. > > elena > > > On 9/26/22 9:59 AM, Luis Machado via Gdb wrote: >> Hi Bruno, >> >> Thanks for putting this together. I think this is an improvement to the >> current review workflow, and I >> think it would be a positive update. >> >> On 9/21/22 12:04, 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! >>>